Skip to content

Commit 85aeacd

Browse files
committed
fix(storage): scope BlockSignatures synthesis to slot-0 anchor only
Reviewers (Claude, Greptile, Codex) on PR #371 flagged that the new synthesize-on-read fallback was too broad: it covered *any* block whose `BlockSignatures` row was missing, not just the slot-0 anchor. The documented invariant is that non-genesis blocks have entries in all three tables (CLAUDE.md), so a missing signature row for slot > 0 is storage corruption, not a valid placeholder case — but `get_signed_block` was hiding it by fabricating a `SignedBlock` with empty `attestation_signatures`, regardless of what the body actually carried. Guard the synthesis on `header.slot == 0`. For any other slot, fall back to returning `None` (the pre-existing "block not found" semantics). Update the doc on `get_signed_block` to make the scoping explicit. Also add direct coverage for the synthesis path: - `attestation::tests::blank_xmss_signature_has_expected_ssz_offsets` reads the three SSZ offsets back out of `blank_xmss_signature()` and asserts every non-offset byte is zero — catches a one-off in any of the three constants that would silently break inner-`Signature` decoding without changing the outer length. - `storage::store::tests::get_signed_block_synthesizes_blank_signatures_for_genesis_anchor` exercises the synthesis path directly against a `from_anchor_state` store. - `storage::store::tests::get_signed_block_returns_none_for_non_genesis_with_missing_signatures` hand-inserts a slot-1 header without its signature row and confirms the new guard returns `None`. - The RPC test for the genesis-finalized-block endpoint now asserts the response body equals the expected SSZ encoding and the Content-Type header, matching the adjacent `test_get_latest_finalized_block` (per Greptile Issue 2). Cosmetic: move `empty_block_signatures` below the `use` block in `storage/src/store.rs` to follow the file's existing layout.
1 parent 2cf0e45 commit 85aeacd

3 files changed

Lines changed: 139 additions & 22 deletions

File tree

crates/common/types/src/attestation.rs

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,4 +306,36 @@ mod tests {
306306
let b = bits(24, &[0, 1, 16]);
307307
assert!(!bits_is_subset(&a, &b));
308308
}
309+
310+
/// Guard the three SSZ offsets at fixed byte positions so a one-off in any
311+
/// constant doesn't silently produce a blob that still has the right outer
312+
/// length but decodes incorrectly at the inner `Signature` container level.
313+
#[test]
314+
fn blank_xmss_signature_has_expected_ssz_offsets() {
315+
let sig = blank_xmss_signature();
316+
let bytes: Vec<u8> = sig.into_iter().collect();
317+
318+
assert_eq!(bytes.len(), SIGNATURE_SIZE);
319+
assert_eq!(
320+
u32::from_le_bytes(bytes[0..4].try_into().unwrap()),
321+
SIGNATURE_PATH_OFFSET,
322+
);
323+
assert_eq!(
324+
u32::from_le_bytes(bytes[32..36].try_into().unwrap()),
325+
SIGNATURE_HASHES_OFFSET,
326+
);
327+
assert_eq!(
328+
u32::from_le_bytes(bytes[36..40].try_into().unwrap()),
329+
SIGNATURE_PATH_SIBLINGS_OFFSET,
330+
);
331+
332+
// Everything outside the three offset slots must be zero — the
333+
// placeholder is "all-zero hashes" once the offsets locate them.
334+
for (i, b) in bytes.iter().enumerate() {
335+
let in_offset_slot = matches!(i, 0..4 | 32..36 | 36..40);
336+
if !in_offset_slot {
337+
assert_eq!(*b, 0, "non-offset byte at index {i} should be zero");
338+
}
339+
}
340+
}
309341
}

crates/net/rpc/src/lib.rs

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -559,6 +559,12 @@ mod tests {
559559

560560
#[tokio::test]
561561
async fn test_get_latest_finalized_block_serves_genesis_with_placeholder_signature() {
562+
use ethlambda_types::{
563+
attestation::blank_xmss_signature,
564+
block::{BlockSignatures, SignedBlock},
565+
};
566+
use libssz::SszEncode;
567+
562568
// Genesis-anchored store: `init_store` writes the header + state but no
563569
// `BlockSignatures` row. `get_signed_block` synthesizes an empty
564570
// `BlockSignatures` so peers can still receive the genesis block on
@@ -568,6 +574,21 @@ mod tests {
568574
let backend = Arc::new(InMemoryBackend::new());
569575
let store = Store::from_anchor_state(backend, state);
570576

577+
// The body the endpoint serves must round-trip to a `SignedBlock`
578+
// matching the genesis header paired with the synthetic blank
579+
// signatures — same shape `get_signed_block` builds in storage.
580+
let genesis_block = store
581+
.get_signed_block(&store.latest_finalized().root)
582+
.expect("genesis served via get_signed_block");
583+
let expected = SignedBlock {
584+
message: genesis_block.message.clone(),
585+
signature: BlockSignatures {
586+
attestation_signatures: Default::default(),
587+
proposer_signature: blank_xmss_signature(),
588+
},
589+
};
590+
let expected_ssz = expected.to_ssz();
591+
571592
let app = build_api_router(store);
572593

573594
let response = app
@@ -581,5 +602,12 @@ mod tests {
581602
.unwrap();
582603

583604
assert_eq!(response.status(), StatusCode::OK);
605+
assert_eq!(
606+
response.headers().get(header::CONTENT_TYPE).unwrap(),
607+
SSZ_CONTENT_TYPE
608+
);
609+
610+
let body = response.into_body().collect().await.unwrap().to_bytes();
611+
assert_eq!(body.as_ref(), expected_ssz.as_slice());
584612
}
585613
}

crates/storage/src/store.rs

Lines changed: 79 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,22 @@
11
use std::collections::{BTreeMap, HashMap, HashSet, VecDeque};
22
use std::sync::{Arc, LazyLock, Mutex};
33

4+
use crate::api::{StorageBackend, StorageWriteBatch, Table};
5+
6+
use ethlambda_types::{
7+
attestation::{AttestationData, HashedAttestationData, bits_is_subset, blank_xmss_signature},
8+
block::{
9+
AggregatedSignatureProof, AttestationSignatures, Block, BlockBody, BlockHeader,
10+
BlockSignatures, SignedBlock,
11+
},
12+
checkpoint::Checkpoint,
13+
primitives::{H256, HashTreeRoot as _},
14+
signature::ValidatorSignature,
15+
state::{ChainConfig, State, anchor_pair_is_consistent},
16+
};
17+
use libssz::{SszDecode, SszEncode};
18+
use tracing::info;
19+
420
/// The tree hash root of an empty block body.
521
///
622
/// Used to detect genesis/anchor blocks that have no attestations,
@@ -19,22 +35,6 @@ fn empty_block_signatures() -> BlockSignatures {
1935
}
2036
}
2137

22-
use crate::api::{StorageBackend, StorageWriteBatch, Table};
23-
24-
use ethlambda_types::{
25-
attestation::{AttestationData, HashedAttestationData, bits_is_subset, blank_xmss_signature},
26-
block::{
27-
AggregatedSignatureProof, AttestationSignatures, Block, BlockBody, BlockHeader,
28-
BlockSignatures, SignedBlock,
29-
},
30-
checkpoint::Checkpoint,
31-
primitives::{H256, HashTreeRoot as _},
32-
signature::ValidatorSignature,
33-
state::{ChainConfig, State, anchor_pair_is_consistent},
34-
};
35-
use libssz::{SszDecode, SszEncode};
36-
use tracing::info;
37-
3838
/// Checkpoints to update in the forkchoice store.
3939
///
4040
/// Used with `Store::update_checkpoints` to update head and optionally
@@ -981,12 +981,16 @@ impl Store {
981981

982982
/// Get a signed block by combining header, body, and signatures.
983983
///
984-
/// Returns None if the header or body (for non-empty bodies) is missing.
984+
/// Returns None if the header or body (for non-empty bodies) is missing,
985+
/// or if the signature row is missing for any block other than the
986+
/// slot-0 anchor.
985987
///
986-
/// Signatures are absent for genesis-style anchor blocks (no proposer ever
987-
/// signed them). To keep BlocksByRoot symmetric with the fork-choice view
988-
/// for peers, synthesize empty `BlockSignatures` when the header exists
989-
/// but no signature row was written.
988+
/// Signatures are absent for genesis-style anchor blocks (no proposer
989+
/// ever signed them). To keep BlocksByRoot symmetric with the
990+
/// fork-choice view for peers, synthesize empty `BlockSignatures` for
991+
/// the slot-0 case only; for any other slot the missing-signature
992+
/// state is treated as storage corruption and surfaces as `None`
993+
/// rather than as a fabricated block.
990994
pub fn get_signed_block(&self, root: &H256) -> Option<SignedBlock> {
991995
let view = self.backend.begin_read().expect("read view");
992996
let key = root.to_ssz();
@@ -1006,7 +1010,12 @@ impl Store {
10061010
Some(sig_bytes) => {
10071011
BlockSignatures::from_ssz_bytes(&sig_bytes).expect("valid signatures")
10081012
}
1009-
None => empty_block_signatures(),
1013+
// Synthesis only covers the genesis-style anchor (slot 0). Any other
1014+
// missing-signature case is a storage corruption that should surface
1015+
// as `None` rather than fabricating a block whose `attestation_signatures`
1016+
// list is empty regardless of what the body actually carries.
1017+
None if header.slot == 0 => empty_block_signatures(),
1018+
None => return None,
10101019
};
10111020

10121021
let block = Block::from_header_and_body(header, body);
@@ -2312,4 +2321,52 @@ mod tests {
23122321
assert_eq!(buf.total_signatures(), 2); // slot 2 (1) + slot 3 (1)
23132322
assert_eq!(buf.len(), 2);
23142323
}
2324+
2325+
/// `Store::from_anchor_state` writes the header but no `BlockSignatures`
2326+
/// row for the slot-0 anchor. `get_signed_block` must synthesize an empty
2327+
/// `BlockSignatures` so the genesis block can still be served on
2328+
/// BlocksByRoot / `/lean/v0/blocks/finalized`.
2329+
#[test]
2330+
fn get_signed_block_synthesizes_blank_signatures_for_genesis_anchor() {
2331+
let backend: Arc<dyn StorageBackend> = Arc::new(InMemoryBackend::new());
2332+
let store = Store::from_anchor_state(backend, State::from_genesis(0, vec![]));
2333+
2334+
let head_root = store.head();
2335+
let signed = store
2336+
.get_signed_block(&head_root)
2337+
.expect("genesis block must be retrievable with synthetic signatures");
2338+
2339+
assert_eq!(signed.message.slot, 0);
2340+
assert_eq!(signed.signature.proposer_signature, blank_xmss_signature());
2341+
assert_eq!(signed.signature.attestation_signatures.len(), 0);
2342+
}
2343+
2344+
/// The synthesis branch must be confined to the slot-0 anchor: a
2345+
/// non-genesis block whose `BlockSignatures` row is missing is treated
2346+
/// as storage corruption and surfaces as `None`, not a fabricated block.
2347+
#[test]
2348+
fn get_signed_block_returns_none_for_non_genesis_with_missing_signatures() {
2349+
let backend: Arc<dyn StorageBackend> = Arc::new(InMemoryBackend::new());
2350+
2351+
// Hand-insert a slot-1 header (and empty body, via `EMPTY_BODY_ROOT`)
2352+
// but skip the `BlockSignatures` row. This mimics the corruption case
2353+
// the guard is meant to catch, without going through the normal
2354+
// `insert_signed_block` write path which always writes all three rows.
2355+
let header = BlockHeader {
2356+
slot: 1,
2357+
proposer_index: 0,
2358+
parent_root: H256::ZERO,
2359+
state_root: H256::ZERO,
2360+
body_root: *EMPTY_BODY_ROOT,
2361+
};
2362+
let root = header.hash_tree_root();
2363+
let mut batch = backend.begin_write().expect("write batch");
2364+
batch
2365+
.put_batch(Table::BlockHeaders, vec![(root.to_ssz(), header.to_ssz())])
2366+
.expect("put header");
2367+
batch.commit().expect("commit");
2368+
2369+
let store = Store::from_anchor_state(backend, State::from_genesis(0, vec![]));
2370+
assert!(store.get_signed_block(&root).is_none());
2371+
}
23152372
}

0 commit comments

Comments
 (0)