Generalize Vote and SignatureAggregator over type of value they hold. #2887

deuszx · 2024-11-13T19:38:17Z

Motivation

Vote and SignatureAggregator both work with hardcoded types (Certificate and HashedCertificateValue respectively) which is an obstacle to full removal of Certificate from the codebase.

Proposal

Make both generic over type they work with.

Test Plan

CI should catch regressions.

Release Plan

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

Links

Closes #2885

reviewer checklist

deuszx · 2024-11-13T19:41:02Z

linera-chain/src/manager.rs

@@ -125,13 +125,13 @@ pub struct ChainManager {
    pub timeout: Option<TimeoutCertificate>,
    /// Latest vote we have cast, to validate or confirm.
    #[debug(skip_if = Option::is_none)]
-    pub pending: Option<Vote>,
+    pub pending: Option<Vote<CertificateValue>>,


This will be tricky to get rid of as we set all three types of CertificateValue here.

The timeout_vote is already separate. We could split up the other two as well.

Agreed to do in a seperate PR

linera-chain/src/manager.rs

ma2bd · 2024-11-13T21:06:22Z

linera-chain/src/block.rs

@@ -109,3 +112,21 @@ impl From<Timeout> for HashedCertificateValue {
        HashedCertificateValue::new_timeout(value.chain_id, value.height, value.epoch)
    }
 }
+
+impl From<HashedCertificateValue> for Hashed<Timeout> {
+    fn from(hv: HashedCertificateValue) -> Hashed<Timeout> {


nit: (See naming guidelines) We don't use acronyms so it should be value: HashedCertificateValue or (hashed_)certificate_value: HashedCertificateValue.

Done in Replace From with TryFrom

afck · 2024-11-14T09:48:00Z

linera-chain/src/block.rs

+        let hash = hv.hash();
+        match hv.into_inner() {
+            CertificateValue::Timeout(timeout) => Hashed::unchecked_new(timeout, hash),
+            _ => panic!("Expected a Timeout value"),


I would feel more confident if we used TryFrom instead of From, and then expected or unwrapped at the call site if we're sure.

I'll change this to TryFrom and make a separate PR where I'll make a sweep over other ones

Done in Replace From with TryFrom

afck · 2024-11-14T09:50:37Z

linera-chain/src/manager.rs

@@ -125,13 +125,13 @@ pub struct ChainManager {
    pub timeout: Option<TimeoutCertificate>,
    /// Latest vote we have cast, to validate or confirm.
    #[debug(skip_if = Option::is_none)]
-    pub pending: Option<Vote>,
+    pub pending: Option<Vote<CertificateValue>>,


The timeout_vote is already separate. We could split up the other two as well.

afck · 2024-11-14T09:53:35Z

linera-chain/src/certificate/hashed.rs

+//TODO: Move
+pub trait Has<T> {
+    fn get(&self) -> &T;
+}


Is it useful to be that general? Would a HasChainId trait be more readable, since its method could simply be called chain_id?

Let's leave for now and if it turns out that there's only one version of it - I'll replace it.

Please remove the TODO or add a task number

Done in Replace TODO with a comment.

Feeling a little bit like @afck here: I would probably choose the very explicit HasChainId (which can use the fact that ChainId is Copy) and otherwise why not AsRef.

Soon we will need more instances for things like Block and Epoch (next PR will try to remove the CertificateValue which has all of these methods defined on the enum).

linera-chain/src/data_types.rs

afck

No blockers from my side; just still not sure about the Has<ChainId>.

afck · 2024-11-14T17:19:22Z

linera-chain/src/manager.rs

@@ -319,7 +319,11 @@ impl ChainManager {
            }
        }
        let value = HashedCertificateValue::new_timeout(chain_id, height, epoch);


Why not use a constructor for a Hashed<Timeout> directly? (Also below.)

~~b/c the hashes would change, and we still haven't done that.~~

actually let me check.

Oh, we are currently instantiating Hashed<T>s using new_unchecked that actually don't match what Hashed::new would do? 😬 Would be good to fix that as soon as possible and have only one way to hash things.

With this change I get a test failure (without passes fine):

failures: client::client_tests::test_request_leader_timeout::storage_service

Oh, we are currently instantiating Hashed<T>s using new_unchecked that actually don't match what Hashed::new would do? 😬 Would be good to fix that as soon as possible and have only one way to hash things.

The only way to fix this is to get rid of CertificateValue(and Certificate) entirely.

Exactly: We could serialize Timeout as if it were a CertificateValue::Timeout.

do the transformation in the Serialize (and Deserialize)? Not a bad idea, I'll give it a try.

And maybe not even do a transformation, but serialize a struct that only borrows the payload but is otherwise identical to CertificateValue?

Here we go again...

Running tests/format.rs (target/debug/deps/format-3b29b29e5fdad0f7) running 1 test test test_format ... FAILED failures: ---- test_format stdout ---- thread 'test_format' panicked at linera-rpc/tests/format.rs:61:64: called `Result::unwrap()` on an `Err` value: Custom("Failed to deserialize value: \"Expected a Timeout value\"")

This won't work. In the deserializer for Timeout:

impl<'de> Deserialize<de> for TImeout { // omit for brevity let timeout = CertificateValue::deserialize(deserializer)?

the last line will "hint" the serde-reflection to generate an instance of CertificateValue enum but it will generate the first enum variant - CertificateValue::ValidatedBlock and the format.yaml test will fail. I think we need to get rid of that enum ASAP to get any further - right now a mix of an enum and how serde-reflection behaves with enums is blocking us.

And maybe not even do a transformation, but serialize a struct that only borrows the payload but is otherwise identical to CertificateValue?

CertifivateValue is an enum so my "temporary struct" would have to reflect its structure b/c serde(-reflection) uses that to derive the binary format.

ma2bd · 2024-11-14T17:54:05Z

linera-chain/src/data_types.rs

+        #[derive(Debug, Deserialize)]
+        struct VoteHelper<T> {


This is going to make VoteHelper appear in the recorded formats, disregarding T. You need to use serde_name for this.

It's also a potential security vulnerability because we use the serde-provided name as a seed for the hashing, but here the seed won't take T into account here. It looks like the format is not recorded (because we're converting votes into legacy RPC messages?) so maybe it's not a blocker for this PR but you need to add TODO(#nnn) with a task and we need to start doing the right thing asap.

This is Deserialize instance, not Serialize (that one is still derived) so it doesn't appear in the logs?

because we're converting votes into legacy RPC messages?

We shouldn't 🤔 I haven't added any conversions for that IIRC.

Point taken for Deserialize-only. Curious what this is used for if there is no Serialize implementation. (Hopefully we're not using serde-serialization to recast types? :))

Deserializing a Hashed<T> adds a new trait bound T: BcsHashable – b/c Hashed<_> type does not serialize the hash, just the value, and we recreate the hash when deserializing. I didn't add this bound on the Vote<T> struct itself for two reasons:

trait bounds on structs are not good

compiler complains about it anyway (see here for details)

I realized that I actually can do this with derive but I need additional attributes - b4e8e64

ma2bd · 2024-11-14T21:22:52Z

linera-chain/src/certificate/hashed.rs

+    {
+        LiteValue {
+            value_hash: self.hash,
+            chain_id: *Has::<ChainId>::get(&self.value),


I believe this would simply be self.value.chain_id() with T: HasChainId

Simplified in Simplify call to extract ChainId

deuszx added 2 commits November 13, 2024 19:33

SignatureAggregator over GenericCertificate

99042b8

Vote is generic over value it contains

7ecabc9

deuszx requested review from ma2bd and afck and removed request for ma2bd November 13, 2024 19:40

deuszx commented Nov 13, 2024

View reviewed changes

linera-chain/src/manager.rs Outdated Show resolved Hide resolved

Use actual Timeout type in ChainManager instead

a1e3418

deuszx changed the title ~~Generalize sigs and votes over t~~ Generalize Vote and SignatureAggregator over type of value they hold. Nov 13, 2024

deuszx marked this pull request as ready for review November 13, 2024 19:52

ma2bd reviewed Nov 13, 2024

View reviewed changes

afck reviewed Nov 14, 2024

View reviewed changes

Replace From with TryFrom

bdbad2f

deuszx requested review from afck and ma2bd November 14, 2024 17:17

afck approved these changes Nov 14, 2024

View reviewed changes

ma2bd reviewed Nov 14, 2024

View reviewed changes

deuszx added 2 commits November 14, 2024 20:31

Replace TODO with a comment.

dcf0a66

Replace hand-written Deserialize for Vote with derive

b4e8e64

ma2bd reviewed Nov 14, 2024

View reviewed changes

Simplify call to extract ChainId

07bb76a

deuszx merged commit 92115c7 into main Nov 15, 2024
21 checks passed

deuszx deleted the generalize-sigs-and-votes-over-t branch November 15, 2024 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize Vote and SignatureAggregator over type of value they hold. #2887

Generalize Vote and SignatureAggregator over type of value they hold. #2887

deuszx commented Nov 13, 2024

deuszx Nov 13, 2024

afck Nov 14, 2024

deuszx Nov 14, 2024

ma2bd Nov 13, 2024

deuszx Nov 14, 2024

afck Nov 14, 2024

deuszx Nov 14, 2024

deuszx Nov 14, 2024

afck Nov 14, 2024

afck Nov 14, 2024

deuszx Nov 14, 2024

ma2bd Nov 14, 2024

deuszx Nov 14, 2024

ma2bd Nov 14, 2024

deuszx Nov 14, 2024

afck left a comment

afck Nov 14, 2024

deuszx Nov 14, 2024 •

edited

Loading

afck Nov 14, 2024

deuszx Nov 14, 2024 •

edited

Loading

deuszx Nov 14, 2024 •

edited

Loading

afck Nov 14, 2024

deuszx Nov 14, 2024

afck Nov 14, 2024

deuszx Nov 14, 2024 •

edited

Loading

deuszx Nov 14, 2024

ma2bd Nov 14, 2024

ma2bd Nov 14, 2024 •

edited

Loading

deuszx Nov 14, 2024

ma2bd Nov 14, 2024 •

edited

Loading

deuszx Nov 14, 2024

deuszx Nov 14, 2024

ma2bd Nov 14, 2024

deuszx Nov 14, 2024

Generalize Vote and SignatureAggregator over type of value they hold. #2887

Generalize Vote and SignatureAggregator over type of value they hold. #2887

Conversation

deuszx commented Nov 13, 2024

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

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

deuszx Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deuszx Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

deuszx Nov 14, 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

deuszx Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ma2bd Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ma2bd Nov 14, 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

deuszx Nov 14, 2024 •

edited

Loading

deuszx Nov 14, 2024 •

edited

Loading

deuszx Nov 14, 2024 •

edited

Loading

deuszx Nov 14, 2024 •

edited

Loading

ma2bd Nov 14, 2024 •

edited

Loading

ma2bd Nov 14, 2024 •

edited

Loading