diff --git a/crates/blockchain/src/store.rs b/crates/blockchain/src/store.rs index 010d8ff6..60711f8b 100644 --- a/crates/blockchain/src/store.rs +++ b/crates/blockchain/src/store.rs @@ -172,9 +172,15 @@ fn update_safe_target(store: &mut Store) { /// Return whether `ancestor` lies on `descendant`'s parent chain. /// /// `descendant_header` is the descendant's already-fetched header, so the walk -/// starts from its parent without re-reading it. Walks parent links down to the -/// ancestor's slot. Empty (skipped) slots on the path are traversed transparently -/// since they carry no block. A block missing from the store yields `false`. +/// never re-reads it. Empty (skipped) slots on the path are traversed +/// transparently since they carry no block. A block missing from the store +/// yields `false`. +/// +/// The walk stops as soon as it reaches a block on the canonical chain: below +/// that point the chain is a single path, so the canonical slot index answers +/// the rest with one lookup. That bounds the cost by how deep the descendant's +/// branch has forked rather than by the distance between the two checkpoints, +/// which is what keeps validation cheap when finality is far behind the head. fn checkpoint_is_ancestor( store: &Store, ancestor: &Checkpoint, @@ -187,22 +193,52 @@ fn checkpoint_is_ancestor( return ancestor.slot == descendant.slot && ancestor.root == descendant.root; } - // The descendant header is already in hand, so begin the walk at its parent. - let mut current_root = descendant_header.parent_root; - while let Some(current_header) = store - .get_block_header(¤t_root) - .expect("parent block exists") - { - if current_header.slot == ancestor.slot { + // Resolve the ancestor against the canonical index once. `None` means the + // index has nothing to say about that slot, so the parent walk stays the only + // sound answer: a miss must never be read as "not an ancestor". + let ancestor_is_canonical = store + .canonical_root_at_slot(ancestor.slot) + .expect("canonical block root") + .map(|canonical| canonical == ancestor.root); + + let mut current_root = descendant.root; + let mut current_slot = descendant.slot; + let mut current_parent = descendant_header.parent_root; + loop { + if current_slot == ancestor.slot { return current_root == ancestor.root; } - if current_header.slot < ancestor.slot { + if current_slot < ancestor.slot { + // Walked past the ancestor's slot without meeting it: this branch + // skips that slot, so the ancestor is not on it. return false; } - current_root = current_header.parent_root; - } - false + // Reaching a canonical block strictly above the ancestor's slot settles + // the rest: the chain below it is a single path, so the ancestor lies on + // it exactly when the index names the ancestor at its own slot. This must + // stay below the slot guards, since a canonical block at or below the + // ancestor's slot says nothing about whether this branch passes through + // the ancestor. + if let Some(is_canonical) = ancestor_is_canonical + && store + .canonical_root_at_slot(current_slot) + .expect("canonical block root") + == Some(current_root) + { + return is_canonical; + } + + current_root = current_parent; + let Some(current_header) = store + .get_block_header(¤t_root) + .expect("parent block exists") + else { + return false; + }; + current_slot = current_header.slot; + current_parent = current_header.parent_root; + } } /// Validate incoming attestation before processing. @@ -1511,6 +1547,165 @@ mod tests { ); } + /// Fetch a block's header for the ancestry helper, which takes the + /// descendant's header already in hand. + fn header_of(store: &Store, root: H256) -> BlockHeader { + store + .get_block_header(&root) + .expect("get_block_header should succeed") + .expect("test block header exists") + } + + /// A vote sitting entirely on the canonical chain validates through the + /// canonical index rather than the parent walk. + #[test] + fn validate_attestation_accepts_canonical_vote_via_index() { + let mut store = new_test_store(); + let genesis = store.head().expect("store head exists"); + + let b1 = H256([1u8; 32]); + let b2 = H256([2u8; 32]); + let b3 = H256([3u8; 32]); + insert_test_block(&mut store, b1, 1, genesis); + insert_test_block(&mut store, b2, 2, b1); + insert_test_block(&mut store, b3, 3, b2); + // Moving the head is what populates the canonical slot index. + store + .update_checkpoints(ForkCheckpoints::head_only(b3)) + .expect("update_checkpoints should succeed"); + store + .set_time(3 * INTERVALS_PER_SLOT) + .expect("set_time should succeed"); + + let data = AttestationData { + slot: 3, + source: Checkpoint { + root: genesis, + slot: 0, + }, + target: Checkpoint { root: b1, slot: 1 }, + head: Checkpoint { root: b3, slot: 3 }, + }; + + assert!( + validate_attestation_data(&store, &data).is_ok(), + "canonical vote must validate" + ); + } + + /// With the descendant on the canonical chain, an ancestor the index places + /// on a different branch is rejected without walking down to its slot. + #[test] + fn checkpoint_is_ancestor_rejects_off_chain_ancestor_via_index() { + let mut store = new_test_store(); + let genesis = store.head().expect("store head exists"); + + let b1 = H256([1u8; 32]); + let b2 = H256([2u8; 32]); + let sibling_1 = H256([3u8; 32]); + insert_test_block(&mut store, b1, 1, genesis); + insert_test_block(&mut store, b2, 2, b1); + insert_test_block(&mut store, sibling_1, 1, genesis); + store + .update_checkpoints(ForkCheckpoints::head_only(b2)) + .expect("update_checkpoints should succeed"); + + // The index names b1 at slot 1, so sibling_1 cannot be on b2's chain. + assert!(!checkpoint_is_ancestor( + &store, + &Checkpoint { + root: sibling_1, + slot: 1 + }, + &Checkpoint { root: b2, slot: 2 }, + &header_of(&store, b2), + )); + } + + /// A branch that skips over the ancestor's slot entirely does not contain + /// the ancestor, even when the branch rejoins the canonical chain below it. + /// Answering from the index at that rejoin point would wrongly accept the + /// vote, so the slot guards have to settle the walk first. + #[test] + fn checkpoint_is_ancestor_rejects_ancestor_skipped_by_fork_branch() { + let mut store = new_test_store(); + let genesis = store.head().expect("store head exists"); + + // Canonical: genesis(0) <- a(1) <- c(2). Fork: genesis(0) <- d(3), + // which jumps straight from slot 0 to slot 3 and never passes through a. + let a = H256([1u8; 32]); + let c = H256([2u8; 32]); + let d = H256([3u8; 32]); + insert_test_block(&mut store, a, 1, genesis); + insert_test_block(&mut store, c, 2, a); + insert_test_block(&mut store, d, 3, genesis); + store + .update_checkpoints(ForkCheckpoints::head_only(c)) + .expect("update_checkpoints should succeed"); + + assert!(!checkpoint_is_ancestor( + &store, + &Checkpoint { root: a, slot: 1 }, + &Checkpoint { root: d, slot: 3 }, + &header_of(&store, d), + )); + } + + /// A slot the index cannot speak for falls back to the parent walk. Reading + /// that miss as "not an ancestor" would reject a perfectly good vote. + #[test] + fn checkpoint_is_ancestor_walks_when_slot_has_no_index_entry() { + let mut store = new_test_store(); + let genesis = store.head().expect("store head exists"); + + let b1 = H256([1u8; 32]); + let b2 = H256([2u8; 32]); + let b3 = H256([3u8; 32]); + insert_test_block(&mut store, b1, 1, genesis); + insert_test_block(&mut store, b2, 2, b1); + insert_test_block(&mut store, b3, 3, b2); + // Without a checkpoint update the index holds nothing but the anchor. + assert_eq!( + store + .canonical_root_at_slot(1) + .expect("canonical block root"), + None + ); + + assert!(checkpoint_is_ancestor( + &store, + &Checkpoint { root: b1, slot: 1 }, + &Checkpoint { root: b3, slot: 3 }, + &header_of(&store, b3), + )); + } + + /// A block imported but not yet selected by fork choice has no index entry + /// of its own, so the walk takes one step to its canonical parent and stops. + #[test] + fn checkpoint_is_ancestor_resolves_block_not_yet_selected_as_head() { + let mut store = new_test_store(); + let genesis = store.head().expect("store head exists"); + + let b1 = H256([1u8; 32]); + let b2 = H256([2u8; 32]); + insert_test_block(&mut store, b1, 1, genesis); + store + .update_checkpoints(ForkCheckpoints::head_only(b1)) + .expect("update_checkpoints should succeed"); + insert_test_block(&mut store, b2, 2, b1); + + assert!(checkpoint_is_ancestor( + &store, + &Checkpoint { + root: genesis, + slot: 0 + }, + &Checkpoint { root: b2, slot: 2 }, + &header_of(&store, b2), + )); + } + /// leanSpec #833: a vote whose head sits on a sibling fork of the target /// must be rejected by gossip validation, even though every slot and /// availability check passes. diff --git a/crates/storage/src/store.rs b/crates/storage/src/store.rs index 2c775c1c..b92aaad2 100644 --- a/crates/storage/src/store.rs +++ b/crates/storage/src/store.rs @@ -1320,6 +1320,26 @@ impl Store { }) } + /// Return the canonical block root at `slot`, or `None` when the canonical + /// chain has no block there. + /// + /// The index is maintained atomically with the head in + /// [`update_checkpoints`](Self::update_checkpoints), so it always describes + /// the branch ending at the stored head. It can lag a freshly imported block + /// that fork choice has not selected yet, but it never runs ahead of the head. + /// + /// A `None` covers two cases the index cannot tell apart: a slot the + /// canonical chain skipped, and a slot below the anchor this store was + /// bootstrapped from. Callers that use this to *reject* something must treat + /// `None` as "unknown" rather than "not canonical". + pub fn canonical_root_at_slot(&self, slot: u64) -> Result, Error> { + let view = self.backend.begin_read().expect("read view"); + Ok(view + .get(Table::BlockRoots, &encode_block_root_key(slot)) + .expect("get block root") + .map(|bytes| H256::from_ssz_bytes(&bytes).expect("valid block root"))) + } + /// Return canonical signed blocks for the slot range `[start_slot, end_slot]`. /// /// Missing slots or blocks are skipped. This keeps the current request @@ -1333,6 +1353,10 @@ impl Store { let view = self.backend.begin_read().expect("read view"); let mut blocks = Vec::new(); for slot in start_slot..=end_slot { + // Read the index through this range's own view rather than via + // `canonical_root_at_slot`, which opens a fresh one per call: a + // range must be served from a single snapshot so a head change + // partway through cannot splice two branches into one response. let Some(root_bytes) = view .get(Table::BlockRoots, &encode_block_root_key(slot)) .expect("get block root") @@ -1912,12 +1936,11 @@ mod tests { .is_some() } - /// Return the canonical block root at `slot` for storage-index assertions. - fn block_root_by_slot(backend: &dyn StorageBackend, slot: u64) -> Option { - let view = backend.begin_read().expect("read view"); - view.get(Table::BlockRoots, &encode_block_root_key(slot)) - .expect("get block root") - .map(|bytes| H256::from_ssz_bytes(&bytes).expect("valid block root")) + /// Canonical block root at `slot`, for storage-index assertions. + fn canonical_root(store: &Store, slot: u64) -> Option { + store + .canonical_root_at_slot(slot) + .expect("canonical block root") } /// Generate a deterministic H256 root from an index. @@ -2020,13 +2043,10 @@ mod tests { .update_checkpoints(ForkCheckpoints::head_only(root_3)) .expect("update head to block 3"); - assert_eq!( - block_root_by_slot(store.backend.as_ref(), 0), - Some(anchor_root) - ); - assert_eq!(block_root_by_slot(store.backend.as_ref(), 1), Some(root_1)); - assert_eq!(block_root_by_slot(store.backend.as_ref(), 2), None); - assert_eq!(block_root_by_slot(store.backend.as_ref(), 3), Some(root_3)); + assert_eq!(canonical_root(&store, 0), Some(anchor_root)); + assert_eq!(canonical_root(&store, 1), Some(root_1)); + assert_eq!(canonical_root(&store, 2), None); + assert_eq!(canonical_root(&store, 3), Some(root_3)); let side_block_2 = signed_block(2, anchor_root); let side_root_2 = side_block_2.message.hash_tree_root(); @@ -2043,20 +2063,11 @@ mod tests { .update_checkpoints(ForkCheckpoints::head_only(side_root_4)) .expect("update head to side block 4"); - assert_eq!( - block_root_by_slot(store.backend.as_ref(), 0), - Some(anchor_root) - ); - assert_eq!(block_root_by_slot(store.backend.as_ref(), 1), None); - assert_eq!( - block_root_by_slot(store.backend.as_ref(), 2), - Some(side_root_2) - ); - assert_eq!(block_root_by_slot(store.backend.as_ref(), 3), None); - assert_eq!( - block_root_by_slot(store.backend.as_ref(), 4), - Some(side_root_4) - ); + assert_eq!(canonical_root(&store, 0), Some(anchor_root)); + assert_eq!(canonical_root(&store, 1), None); + assert_eq!(canonical_root(&store, 2), Some(side_root_2)); + assert_eq!(canonical_root(&store, 3), None); + assert_eq!(canonical_root(&store, 4), Some(side_root_4)); } #[test]