Conversation
There was a problem hiding this comment.
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.
| 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) { |
There was a problem hiding this comment.
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.
| 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) { |
| state_private_key: <T::StateSignatureKey as StateSignatureKey>::StatePrivateKey, | ||
| stake_table_capacity: usize, | ||
| // TODO: persist state_certs | ||
| state_certs: BTreeMap<EpochNumber, LightClientStateUpdateCertificateV2<T>>, |
There was a problem hiding this comment.
|
Unable to build without Cargo.lock file. This means that after this change 3rd party projects may have For the first failing build see: https://github.com/EspressoSystems/espresso-network/actions/runs/25009641361 To reproduce locally run This PR can still be merged. |
This PR:
This PR does not:
Key places to review: