-
Notifications
You must be signed in to change notification settings - Fork 26
fix: serve genesis block on BlocksByRoot with structurally-valid blank XMSS signature #371
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.
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
Changes from all commits
1cb3fc3
a3a1900
c21bc03
9b90520
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,18 +1,13 @@ | ||
| use std::collections::{BTreeMap, HashMap, HashSet, VecDeque}; | ||
| use std::sync::{Arc, LazyLock, Mutex}; | ||
|
|
||
| /// The tree hash root of an empty block body. | ||
| /// | ||
| /// Used to detect genesis/anchor blocks that have no attestations, | ||
| /// allowing us to skip storing empty bodies and reconstruct them on read. | ||
| static EMPTY_BODY_ROOT: LazyLock<H256> = LazyLock::new(|| BlockBody::default().hash_tree_root()); | ||
|
|
||
| use crate::api::{StorageBackend, StorageWriteBatch, Table}; | ||
|
|
||
| use ethlambda_types::{ | ||
| attestation::{AttestationData, HashedAttestationData, bits_is_subset}, | ||
| attestation::{AttestationData, HashedAttestationData, bits_is_subset, blank_xmss_signature}, | ||
| block::{ | ||
| AggregatedSignatureProof, Block, BlockBody, BlockHeader, BlockSignatures, SignedBlock, | ||
| AggregatedSignatureProof, AttestationSignatures, Block, BlockBody, BlockHeader, | ||
| BlockSignatures, SignedBlock, | ||
| }, | ||
| checkpoint::Checkpoint, | ||
| primitives::{H256, HashTreeRoot as _}, | ||
|
|
@@ -36,6 +31,24 @@ pub enum GetForkchoiceStoreError { | |
| }, | ||
| } | ||
|
|
||
| /// The tree hash root of an empty block body. | ||
| /// | ||
| /// Used to detect genesis/anchor blocks that have no attestations, | ||
| /// allowing us to skip storing empty bodies and reconstruct them on read. | ||
| static EMPTY_BODY_ROOT: LazyLock<H256> = LazyLock::new(|| BlockBody::default().hash_tree_root()); | ||
|
|
||
| /// Build a placeholder `BlockSignatures` for blocks that were never signed. | ||
| /// | ||
| /// Genesis-style anchor blocks have no proposer signature and no per-attestation | ||
| /// proofs (no attestations exist). `get_signed_block` returns this so peers can | ||
| /// still receive the block in BlocksByRoot responses. | ||
| fn empty_block_signatures() -> BlockSignatures { | ||
| BlockSignatures { | ||
| attestation_signatures: AttestationSignatures::default(), | ||
| proposer_signature: blank_xmss_signature(), | ||
| } | ||
| } | ||
|
|
||
| /// Checkpoints to update in the forkchoice store. | ||
| /// | ||
| /// Used with `Store::update_checkpoints` to update head and optionally | ||
|
|
@@ -990,15 +1003,21 @@ impl Store { | |
|
|
||
| /// Get a signed block by combining header, body, and signatures. | ||
| /// | ||
| /// Returns None if any of the components are not found. | ||
| /// Note: Genesis block has no entry in BlockSignatures table. | ||
| /// Returns None if the header or body (for non-empty bodies) is missing, | ||
| /// or if the signature row is missing for any block other than the | ||
| /// slot-0 anchor. | ||
| /// | ||
| /// Signatures are absent for genesis-style anchor blocks (no proposer | ||
| /// ever signed them). To keep BlocksByRoot symmetric with the | ||
| /// fork-choice view for peers, synthesize empty `BlockSignatures` for | ||
| /// the slot-0 case only; for any other slot the missing-signature | ||
| /// state is treated as storage corruption and surfaces as `None` | ||
| /// rather than as a fabricated block. | ||
| pub fn get_signed_block(&self, root: &H256) -> Option<SignedBlock> { | ||
| let view = self.backend.begin_read().expect("read view"); | ||
| let key = root.to_ssz(); | ||
|
|
||
| let header_bytes = view.get(Table::BlockHeaders, &key).expect("get")?; | ||
| let sig_bytes = view.get(Table::BlockSignatures, &key).expect("get")?; | ||
|
|
||
| let header = BlockHeader::from_ssz_bytes(&header_bytes).expect("valid header"); | ||
|
|
||
| // Use empty body if header indicates empty, otherwise fetch from DB | ||
|
|
@@ -1009,8 +1028,19 @@ impl Store { | |
| BlockBody::from_ssz_bytes(&body_bytes).expect("valid body") | ||
| }; | ||
|
|
||
| let signature = match view.get(Table::BlockSignatures, &key).expect("get") { | ||
| Some(sig_bytes) => { | ||
| BlockSignatures::from_ssz_bytes(&sig_bytes).expect("valid signatures") | ||
| } | ||
| // Synthesis only covers the genesis-style anchor (slot 0). Any other | ||
| // missing-signature case is a storage corruption that should surface | ||
| // as `None` rather than fabricating a block whose `attestation_signatures` | ||
| // list is empty regardless of what the body actually carries. | ||
| None if header.slot == 0 => empty_block_signatures(), | ||
| None => return None, | ||
| }; | ||
|
Comment on lines
+1031
to
+1041
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The synthesized-signature path triggers whenever Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/storage/src/store.rs
Line: 1005-1010
Comment:
**Fallback applies to any block missing signatures, not just genesis**
The synthesized-signature path triggers whenever `Table::BlockSignatures` has no row for a given root — including non-genesis blocks whose signature write was skipped or failed silently. The doc comment scopes the intent to "genesis-style anchor blocks," but the code has no guard (e.g., `header.slot == 0`) to enforce that. A non-genesis block with a missing signature row would now be served to peers with a blank XMSS placeholder rather than being treated as absent, making the gap invisible to callers. In the current write path this can't happen, but the invariant is implicit rather than structural.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| let block = Block::from_header_and_body(header, body); | ||
| let signature = BlockSignatures::from_ssz_bytes(&sig_bytes).expect("valid signatures"); | ||
|
|
||
| Some(SignedBlock { | ||
| message: block, | ||
|
|
@@ -2313,4 +2343,52 @@ mod tests { | |
| assert_eq!(buf.total_signatures(), 2); // slot 2 (1) + slot 3 (1) | ||
| assert_eq!(buf.len(), 2); | ||
| } | ||
|
|
||
| /// `Store::from_anchor_state` writes the header but no `BlockSignatures` | ||
| /// row for the slot-0 anchor. `get_signed_block` must synthesize an empty | ||
| /// `BlockSignatures` so the genesis block can still be served on | ||
| /// BlocksByRoot / `/lean/v0/blocks/finalized`. | ||
| #[test] | ||
| fn get_signed_block_synthesizes_blank_signatures_for_genesis_anchor() { | ||
| let backend: Arc<dyn StorageBackend> = Arc::new(InMemoryBackend::new()); | ||
| let store = Store::from_anchor_state(backend, State::from_genesis(0, vec![])); | ||
|
|
||
| let head_root = store.head(); | ||
| let signed = store | ||
| .get_signed_block(&head_root) | ||
| .expect("genesis block must be retrievable with synthetic signatures"); | ||
|
|
||
| assert_eq!(signed.message.slot, 0); | ||
| assert_eq!(signed.signature.proposer_signature, blank_xmss_signature()); | ||
| assert_eq!(signed.signature.attestation_signatures.len(), 0); | ||
| } | ||
|
|
||
| /// The synthesis branch must be confined to the slot-0 anchor: a | ||
| /// non-genesis block whose `BlockSignatures` row is missing is treated | ||
| /// as storage corruption and surfaces as `None`, not a fabricated block. | ||
| #[test] | ||
| fn get_signed_block_returns_none_for_non_genesis_with_missing_signatures() { | ||
| let backend: Arc<dyn StorageBackend> = Arc::new(InMemoryBackend::new()); | ||
|
|
||
| // Hand-insert a slot-1 header (and empty body, via `EMPTY_BODY_ROOT`) | ||
| // but skip the `BlockSignatures` row. This mimics the corruption case | ||
| // the guard is meant to catch, without going through the normal | ||
| // `insert_signed_block` write path which always writes all three rows. | ||
| let header = BlockHeader { | ||
| slot: 1, | ||
| proposer_index: 0, | ||
| parent_root: H256::ZERO, | ||
| state_root: H256::ZERO, | ||
| body_root: *EMPTY_BODY_ROOT, | ||
| }; | ||
| let root = header.hash_tree_root(); | ||
| let mut batch = backend.begin_write().expect("write batch"); | ||
| batch | ||
| .put_batch(Table::BlockHeaders, vec![(root.to_ssz(), header.to_ssz())]) | ||
| .expect("put header"); | ||
| batch.commit().expect("commit"); | ||
|
|
||
| let store = Store::from_anchor_state(backend, State::from_genesis(0, vec![])); | ||
| assert!(store.get_signed_block(&root).is_none()); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test_get_latest_finalized_block_serves_genesis_with_placeholder_signatureverifiesStatusCode::OKbut does not check the response body orContent-Typeheader. The adjacent test (test_get_latest_finalized_block) asserts both, and their symmetry is the best guard that the serialisation path actually works end-to-end for the genesis case. Without a body check, a regression that returns 200 with empty or malformed SSZ bytes would pass this test silently.Prompt To Fix With AI