Separate recent hashed values cache into separate types #2902

deuszx · 2024-11-15T15:54:12Z

Motivation

Part of the epic to remove CertificateValue and be able to statically determine a type of value the certificate is for.

Proposal

Replace ChainManager::pending with two separate fields - confirmed_vote and validated_vote. When one of the votes is inserted, the other is cleared so that only one of the fields is ever populated.

Test Plan

CI should catch any regressions.

Release Plan

Nothing to do / These changes follow the usual release cycle.

Links

Closes #2897

reviewer checklist

deuszx · 2024-11-15T15:55:06Z

linera-chain/src/manager.rs

        }
+
+        // TODO: Is it guaranteed that if key_pair = None, then the vote is not signed ever?


I want to bring reviewers' attention to this - i checked but want to confirm

Yes, these should only ever contain votes we signed with key_pair, and only if that's Some. (~~That invariant should, in fact, be guaranteed just by this module alone.~~ Edit: It isn't, strictly speaking, since the fields are public.)

deuszx · 2024-11-15T16:05:27Z

linera-core/src/value_cache.rs

-    /// If the `value` is a [`HashedCertificateValue::ValidatedBlock`], its respective
-    /// [`HashedCertificateValue::ConfirmedBlock`] is also cached.
-    pub async fn insert<'a>(&self, value: Cow<'a, HashedCertificateValue>) -> bool {
+    pub async fn insert<'a>(&self, value: Cow<'a, Hashed<T>>) -> bool {


Want to emphasize that we no longer insert a validated variant of the hashed value if the confirmed one is being inserted.

(It was the other way round, I think?)
If I'm reading this correctly we are inserting the confirmed variant when we sign it; that should still take care of the most common case, so that most validators that receive a lite confirmed block certificate will already have its value in the cache.

Yes yes, it was the other way around.

deuszx · 2024-11-18T15:44:18Z

linera-chain/src/manager.rs

+        let pending = manager
+            .confirmed_vote
+            .as_ref()
+            .map(|vote| vote.lite())
+            .or_else(move || manager.validated_vote.as_ref().map(|vote| vote.lite()));


Also this - it shouldn't happen that both confirmed_vote and validated_vote are both set to Some(_) at the same time.

It looks like you made sure of that everywhere.

Alternatively, we could avoid explicitly setting them to None and always prefer the highest-round vote, with the confirmed one taking precedence.

deuszx · 2024-11-18T16:29:45Z

linera-core/src/unit_tests/value_cache_tests.rs

-/// Test that insertion of a validated block certificate also inserts its respective confirmed
-/// block certificate.
-#[tokio::test]
-async fn test_insertion_of_validated_also_inserts_confirmed() {


Tests were removed but there's an issue now to bring back similar functionality #2912

Makes sense to me; most of these seem to be related to the "validated vs. confirmed" logic. (cc @jvff)

afck

No blockers from my side; but the TODO question needs to be removed from the code, of course.

afck · 2024-11-19T09:49:11Z

linera-chain/src/certificate/lite.rs

@@ -74,12 +74,11 @@ impl<'a> LiteCertificate<'a> {
    }

    /// Returns the `Certificate` with the specified value, if it matches.


Suggested change

/// Returns the `Certificate` with the specified value, if it matches.

/// Returns the [`GenericCertificate`] with the specified value, if it matches.

Done in Fix comments

afck · 2024-11-19T09:51:34Z

linera-chain/src/manager.rs

        }
+
+        // TODO: Is it guaranteed that if key_pair = None, then the vote is not signed ever?


Yes, these should only ever contain votes we signed with key_pair, and only if that's Some. (~~That invariant should, in fact, be guaranteed just by this module alone.~~ Edit: It isn't, strictly speaking, since the fields are public.)

afck · 2024-11-19T09:52:25Z

linera-chain/src/manager.rs

    }

-    /// Verifies the safety of a proposed block with respect to voting rules.


You mean why I removed the comment? probably by mistake.

Done in 2cfad93

afck · 2024-11-19T10:00:04Z

linera-chain/src/manager.rs

-                }
+        if let Some(Vote { value, round, .. }) = &self.confirmed_vote {
+            if value.inner().inner().block == *new_block && *round == new_round {
+                return Ok(Outcome::Skip); // We already voted to confirm this block.


Now I'm wondering if we're missing a check in this function that, regardless of locked block or votes, the certificate is from at least the current round.

I created #2914 for it.

afck · 2024-11-19T10:02:12Z

linera-chain/src/manager.rs

+                    HashedCertificateValue::new_confirmed(executed_block)
+                        .try_into()
+                        .expect("ConfirmedBlock"),


We could have a more direct constructor for that now.

afck · 2024-11-19T10:09:57Z

linera-chain/src/unit_tests/chain_tests.rs

@@ -250,7 +250,7 @@ async fn test_application_permissions() {
    let value = HashedCertificateValue::new_confirmed(outcome.with(valid_block));


Here we could also start using the new type right away.

linera-core/src/chain_worker/actor.rs

afck · 2024-11-19T10:23:36Z

linera-core/src/value_cache.rs

-    /// If the `value` is a [`HashedCertificateValue::ValidatedBlock`], its respective
-    /// [`HashedCertificateValue::ConfirmedBlock`] is also cached.
-    pub async fn insert<'a>(&self, value: Cow<'a, HashedCertificateValue>) -> bool {
+    pub async fn insert<'a>(&self, value: Cow<'a, Hashed<T>>) -> bool {


(It was the other way round, I think?)
If I'm reading this correctly we are inserting the confirmed variant when we sign it; that should still take care of the most common case, so that most validators that receive a lite confirmed block certificate will already have its value in the cache.

afck · 2024-11-19T10:25:17Z

linera-core/src/unit_tests/value_cache_tests.rs

-/// Test that insertion of a validated block certificate also inserts its respective confirmed
-/// block certificate.
-#[tokio::test]
-async fn test_insertion_of_validated_also_inserts_confirmed() {


Makes sense to me; most of these seem to be related to the "validated vs. confirmed" logic. (cc @jvff)

afck · 2024-11-19T10:27:36Z

linera-core/src/unit_tests/worker_tests.rs

@@ -331,7 +333,7 @@ where
        }
        .with(block),
    );
-    make_certificate(committee, worker, value)
+    make_certificate(committee, worker, value.try_into().expect("Confirmed cert"))


Here we should also be able to avoid the expect, using the appropriate constructor.

Even if inside that constructor we still expect for now (but I hope we can remove that entirely soon) it would at least clean up all the call sites.
(But maybe that's something for another PR. 🤷)

Yes, let's leave it for a separate PR - I'm currently in the process of removing a lot of HashedCertificateValue usage and maybe it'll go away entirely soon.

deuszx · 2024-11-19T10:52:06Z

No blockers from my side; but the TODO question needs to be removed from the code, of course.

Done

Separate recent hashed values cache into separate types

e40c1a3

deuszx commented Nov 15, 2024

View reviewed changes

deuszx added 2 commits November 15, 2024 15:57

Get rid of unnecessary clones

4114648

Replace call to contains with pulling cert directly

b13db05

deuszx commented Nov 15, 2024

View reviewed changes

In create_final_vote, set validated_vote to None

32587e2

deuszx requested review from ma2bd, afck and bart-linera November 15, 2024 16:08

deuszx marked this pull request as ready for review November 15, 2024 16:09

deuszx added 2 commits November 15, 2024 16:50

make-X methods in tests work with new certificate types

3ac3326

Fix tests

a527744

deuszx commented Nov 18, 2024

View reviewed changes

deuszx mentioned this pull request Nov 18, 2024

Cache executed blocks separately from certificates #2912

Closed

deuszx commented Nov 18, 2024

View reviewed changes

deuszx mentioned this pull request Nov 18, 2024

Change test methods to return GenericCertificate<T> instead of old Certificate type. #2903

Closed

afck reviewed Nov 19, 2024

View reviewed changes

deuszx added 3 commits November 19, 2024 10:49

Fix comments

98202c9

RM todo comment

0e6baef

Bring back rm'd comment

2cfad93

afck approved these changes Nov 19, 2024

View reviewed changes

deuszx merged commit 12e99a5 into main Nov 19, 2024
21 checks passed

deuszx deleted the chain-manager-pending-split branch November 19, 2024 11:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate recent hashed values cache into separate types #2902

Separate recent hashed values cache into separate types #2902

deuszx commented Nov 15, 2024 •

edited

Loading

deuszx Nov 15, 2024

afck Nov 19, 2024

deuszx Nov 15, 2024

afck Nov 19, 2024

deuszx Nov 19, 2024

deuszx Nov 18, 2024

afck Nov 19, 2024

deuszx Nov 18, 2024

afck Nov 19, 2024

afck left a comment

afck Nov 19, 2024

deuszx Nov 19, 2024

afck Nov 19, 2024

afck Nov 19, 2024

deuszx Nov 19, 2024

deuszx Nov 19, 2024 •

edited

Loading

afck Nov 19, 2024

afck Nov 19, 2024

afck Nov 19, 2024

afck Nov 19, 2024

afck Nov 19, 2024

afck Nov 19, 2024

deuszx Nov 19, 2024

deuszx commented Nov 19, 2024

		}

		// TODO: Is it guaranteed that if key_pair = None, then the vote is not signed ever?

		@@ -74,12 +74,11 @@ impl<'a> LiteCertificate<'a> {
		}

		/// Returns the `Certificate` with the specified value, if it matches.

	/// Returns the `Certificate` with the specified value, if it matches.
	/// Returns the [`GenericCertificate`] with the specified value, if it matches.

		}

		/// Verifies the safety of a proposed block with respect to voting rules.

		@@ -250,7 +250,7 @@ async fn test_application_permissions() {
		let value = HashedCertificateValue::new_confirmed(outcome.with(valid_block));

Separate recent hashed values cache into separate types #2902

Separate recent hashed values cache into separate types #2902

Conversation

deuszx commented Nov 15, 2024 • edited Loading

Motivation

Proposal

Test Plan

Release Plan

Links

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deuszx Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deuszx commented Nov 19, 2024

deuszx commented Nov 15, 2024 •

edited

Loading

deuszx Nov 19, 2024 •

edited

Loading