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

Introduce and use TimeoutCertificate #2845

Merged
merged 6 commits into from
Nov 11, 2024
Merged

Introduce and use TimeoutCertificate #2845

merged 6 commits into from
Nov 11, 2024

Conversation

deuszx
Copy link
Contributor

@deuszx deuszx commented Nov 7, 2024 •
edited
Loading

Motivation

Similarly to the previous PRs - add a new TimeoutCertificate type and use where appropriately.

Proposal

Test Plan

Release Plan

  • Nothing to do / These changes follow the usual release cycle.
  • These changes should be backported to the latest devnet branch, then
    • be released in a new SDK,
    • be released in a validator hotfix.
  • These changes should be backported to the latest testnet branch, then
    • be released in a new SDK,
    • be released in a validator hotfix.
  • Backporting is not possible but we may want to deploy a new devnet and release a new
    SDK soon.

Links

Closes #2843

ma2bd
ma2bd reviewed Nov 9, 2024
View reviewed changes
linera-chain/src/certificate.rs Outdated
@@ -30,6 +30,9 @@ pub type ValidatedBlockCertificate = GenericCertificate<ValidatedBlock>;
/// Certificate for a [`ConfirmedBlock`] instance.
pub type ConfirmedBlockCertificate = GenericCertificate<ConfirmedBlock>;

/// Certificate for a Timeout instance.
pub type TimeoutCerificate = GenericCertificate<Timeout>;
Copy link
Contributor

Choose a reason for hiding this comment

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

typo TimeoutCerificate -> TimeoutCertificate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

ma2bd
ma2bd reviewed Nov 9, 2024
View reviewed changes
linera-chain/src/certificate.rs Outdated
Comment on lines 106 to 107
let cert = Certificate::deserialize(deserializer)?;
TimeoutCerificate::try_from(cert).map_err(<D::Error as serde::de::Error>::custom)
Copy link
Contributor

@ma2bd ma2bd Nov 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok so this won't work because serde_reflection::trace_type must produce a value of type Certificate at line 106 but it has no way to know which variant of Certificate will work line 107.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I've implemented the (de)serializers manually.

@deuszx deuszx force-pushed the timeout-certificate branch 7 times, most recently from 486d6e9 to 7431f43 Compare November 11, 2024 13:17
@deuszx deuszx marked this pull request as ready for review November 11, 2024 13:20
afck
afck reviewed Nov 11, 2024
View reviewed changes
linera-chain/src/block.rs Outdated Show resolved Hide resolved
linera-chain/src/certificate.rs Show resolved Hide resolved
linera-chain/src/certificate.rs Show resolved Hide resolved
linera-chain/src/certificate.rs
state.serialize_field("value", &self.inner())?;
state.serialize_field("round", &self.round)?;
state.serialize_field("signatures", &self.signatures)?;
state.end()
Copy link
Contributor

Choose a reason for hiding this comment

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

Does that really BCS-serialize to the same bytes as a Certificate with Timeout value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it matter? I carry-on the hash and I map to/from the old ConfirmedValue types.

Copy link
Contributor

@afck afck Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I guess it will only matter in the future, when we use this directly to compute the hash.

Or at least it will matter then that ConfirmedBlockCertificate serializes to something different than ValidatedBlockCertificate (in BCS).

linera-chain/src/certificate.rs Show resolved Hide resolved
linera-core/src/chain_worker/state/mod.rs
@@ -130,7 +130,7 @@ where
pub(super) async fn read_certificate(
&mut self,
height: BlockHeight,
) -> Result<Option<Certificate>, WorkerError> {
) -> Result<Option<linera_chain::data_types::Certificate>, WorkerError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should double-check, but I believe this will always be a ConfirmedBlockCertificate read from storage?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's correct (and one of the benefits of this refactoring)

ma2bd
ma2bd reviewed Nov 11, 2024
View reviewed changes
linera-chain/src/certificate.rs Outdated
#[derive(Deserialize)]
#[serde(rename = "TimeoutCertificate")]
struct Inner {
timeout: Timeout,
Copy link
Contributor

@ma2bd ma2bd Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can we use value here? (and everywhere)

This code could be factorized using serde_name by the way, but I'm happy to do it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, it was value and then I changed it to timeout :) I'll change it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in fe66e51.

Base automatically changed from use-confirmed-block-type to main November 11, 2024 16:03
@deuszx deuszx requested a review from ma2bd November 11, 2024 17:01
@deuszx deuszx merged commit 8f4bd86 into main Nov 11, 2024
21 checks passed
@deuszx deuszx deleted the timeout-certificate branch November 11, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afck afck afck approved these changes

@ma2bd ma2bd Awaiting requested review from ma2bd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduce a separate type for TimeoutCertificate and use where CertificateValue::Timeout is used now.
3 participants
@deuszx @ma2bd @afck