Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 7 commits into from
Nov 15, 2024
+142 −64
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
99042b8
SignatureAggregator over GenericCertificate
deuszx Nov 13, 2024
7ecabc9
Vote is generic over value it contains
deuszx Nov 13, 2024
a1e3418
Use actual Timeout type in ChainManager instead
deuszx Nov 13, 2024
bdbad2f
Replace From with TryFrom
deuszx Nov 14, 2024
dcf0a66
Replace TODO with a comment.
deuszx Nov 14, 2024
b4e8e64
Replace hand-written Deserialize for Vote with derive
deuszx Nov 14, 2024
07bb76a
Simplify call to extract ChainId
deuszx Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions linera-chain/src/block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,12 +113,13 @@ impl From<Timeout> for HashedCertificateValue {
}
}

impl From<HashedCertificateValue> for Hashed<Timeout> {
fn from(hv: HashedCertificateValue) -> Hashed<Timeout> {
let hash = hv.hash();
match hv.into_inner() {
CertificateValue::Timeout(timeout) => Hashed::unchecked_new(timeout, hash),
_ => panic!("Expected a Timeout value"),
impl TryFrom<HashedCertificateValue> for Hashed<Timeout> {
type Error = &'static str;
fn try_from(value: HashedCertificateValue) -> Result<Hashed<Timeout>, Self::Error> {
let hash = value.hash();
match value.into_inner() {
CertificateValue::Timeout(timeout) => Ok(Hashed::unchecked_new(timeout, hash)),
_ => Err("Expected a Timeout value"),
}
}
}
Expand Down
12 changes: 10 additions & 2 deletions linera-chain/src/manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,11 @@ impl ChainManager {
}
}
let value = HashedCertificateValue::new_timeout(chain_id, height, epoch);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@deuszx deuszx Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

actually let me check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@deuszx deuszx Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

@deuszx deuszx Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@deuszx deuszx Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.timeout_vote = Some(Vote::new(value.into(), current_round, key_pair));
self.timeout_vote = Some(Vote::new(
value.try_into().expect("Timeout certificate"),
current_round,
key_pair,
));
true
}

Expand All @@ -342,7 +346,11 @@ impl ChainManager {
}
let value = HashedCertificateValue::new_timeout(chain_id, height, epoch);
let last_regular_round = Round::SingleLeader(u32::MAX);
self.fallback_vote = Some(Vote::new(value.into(), last_regular_round, key_pair));
self.fallback_vote = Some(Vote::new(
value.try_into().expect("Timeout certificate"),
last_regular_round,
key_pair,
));
true
}

Expand Down
Loading