Skip to content

[Fast Finality] State Certificate Voting + Proposing#4224

Open
bfish713 wants to merge 8 commits intomainfrom
bf/state
Open

[Fast Finality] State Certificate Voting + Proposing#4224
bfish713 wants to merge 8 commits intomainfrom
bf/state

Conversation

@bfish713
Copy link
Copy Markdown
Contributor

This PR:

  • Moves the light client state helper functions to a common place so they are shared between old and new protocol
  • Creates a vote collector for the state certificate.
  • Also collects regular vote 1s for the root certificate in new collector. This means either we form a Cert1 + State cert of nothing at all.
  • Adds the logic to propose the cert when extending the root
  • Adds logic to send state cert vote when a proposal is the epoch root
  • Adds logic to check for the state cert when the proposal extends the epoch root

This PR does not:

Key places to review:

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request implements light client state update certificates within the new protocol. Key changes include the introduction of an EpochRootVoteCollector to handle the atomic accumulation of quorum and state votes for epoch-root views, and the integration of state certificate validation into the proposal process. The Consensus and Coordinator structs were updated to manage state private keys and store state certificates, while several helper functions were moved to the hotshot-contract-adapter. Review feedback identifies a critical vulnerability where duplicate signatures could inflate accumulated stake, a compilation error caused by a typo in the vote collector, and a memory leak in the consensus state certificate storage.

Comment on lines +188 to +195
let mut accumulated_stake = U256::from(0);
let signed_state_digest = derive_signed_state_digest(
&state_cert.light_client_state,
&state_cert.next_stake_table_state,
&state_cert.auth_root,
);
for (key, sig, sig_v2) in state_cert.signatures.iter() {
if let Some(stake) = state_key_map.get(key) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

The certificate validation logic does not check for duplicate signatures from the same validator. A malicious certificate could include multiple copies of a valid signature to artificially inflate the accumulated stake and bypass the success threshold. Tracking seen keys using a HashSet and rejecting duplicates prevents this double-counting.

Suggested change
let mut accumulated_stake = U256::from(0);
let signed_state_digest = derive_signed_state_digest(
&state_cert.light_client_state,
&state_cert.next_stake_table_state,
&state_cert.auth_root,
);
for (key, sig, sig_v2) in state_cert.signatures.iter() {
if let Some(stake) = state_key_map.get(key) {
let mut accumulated_stake = U256::from(0);
let mut seen_keys = std::collections::HashSet::new();
let signed_state_digest = derive_signed_state_digest(
&state_cert.light_client_state,
&state_cert.next_stake_table_state,
&state_cert.auth_root,
);
for (key, sig, sig_v2) in state_cert.signatures.iter() {
if !seen_keys.insert(key) {
bail!("Duplicate signer in light client state update certificate");
}
if let Some(stake) = state_key_map.get(key) {

Comment thread crates/hotshot/new-protocol/src/epoch_root_vote_collector.rs
state_private_key: <T::StateSignatureKey as StateSignatureKey>::StatePrivateKey,
stake_table_capacity: usize,
// TODO: persist state_certs
state_certs: BTreeMap<EpochNumber, LightClientStateUpdateCertificateV2<T>>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The state_certs map is populated in handle_input but is never cleared. Since Consensus is a long-lived struct, this will lead to a memory leak as the number of epochs grows. Please implement a garbage collection mechanism for this map (e.g., in a gc method called by the Coordinator).

@github-actions
Copy link
Copy Markdown
Contributor

Unable to build without Cargo.lock file.

This means that after this change 3rd party projects may have
difficulties using crates in this repo as a dependency. If this
isn't easy to fix please open an issue so we can fix it later.

For the first failing build see: https://github.com/EspressoSystems/espresso-network/actions/runs/25009641361

To reproduce locally run

cargo generate-lockfile
cargo check --all-targets

This PR can still be merged.

@bfish713 bfish713 marked this pull request as ready for review April 27, 2026 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant