Merge pull request #3225 from ProvableHQ/fix/sync-invalid-subdag

[Fix] Check subDAG atomicity correctly for a chain of pending blocks

Author: kaimast

.circleci/config.yml

@@ -775,7 +775,7 @@ jobs:
 
   ledger-narwhal-data:
     executor: rust-docker
-    resource_class: << pipeline.parameters.small >>
+    resource_class: << pipeline.parameters.medium >>
     steps:
       - run_test:
           workspace_member: snarkvm-ledger-narwhal-data

ledger/src/check_next_block.rs

@@ -112,6 +112,9 @@ impl<N: Network, C: ConsensusStorage<N>> Ledger<N, C> {
     fn check_block_subdag_inner(&self, block: &Block<N>, prefix: &[PendingBlock<N>]) -> Result<(), CheckBlockError<N>> {
         // Grab a lock to the latest_block in the ledger, to prevent concurrent writes to the ledger,
         // and to ensure that this check is atomic.
+        //
+        // Note: The latest block in the ledger is not necessarily the direct predecessor of `block`.
+        // If `prefix` is non-empty the direct predecessor is the last entry in the prefix.
         let latest_block = self.current_block.read();
 
         // First check that the heights and hashes of the pending block sequence and of the new block are correct.
@@ -149,8 +152,10 @@ impl<N: Network, C: ConsensusStorage<N>> Ledger<N, C> {
         // Ensure the certificates in the block subdag have met quorum requirements.
         self.check_block_subdag_quorum(block)?;
 
-        // Determine if the block subdag is correctly constructed and is not a combination of multiple subdags.
-        self.check_block_subdag_atomicity(block, &latest_block)?;
+        // Check subDAG atomicity against the latest block in the prefix.
+        // Only if the prefix is empty, check against the latest block in the ledger.
+        let predecessor = prefix.last().map_or(&*latest_block, |b| &**b);
+        self.check_block_subdag_atomicity(block, predecessor)?;
 
         // Ensure that all leaves of the subdag point to valid batches in other subdags/blocks.
         self.check_block_subdag_leaves(block, prefix)?;

ledger/tests/helpers/mod.rs

@@ -76,9 +76,13 @@ impl TestChainBuilder {
         let genesis_rng = &mut TestRng::from_seed(seed);
         let genesis_block = VM::from(store).unwrap().genesis_beacon(&private_key, genesis_rng).unwrap();
 
-        // Extract the private keys from the genesis committee by using the same RNG to sample private keys.
+        // Reconstruct the private keys of the genesis committee.  genesis_beacon uses `private_key`
+        // as the first member, then samples (committee_size - 1) more from the seeded RNG.
         let genesis_rng = &mut TestRng::from_seed(seed);
-        let private_keys = (0..committee_size).map(|_| PrivateKey::new(genesis_rng).unwrap()).collect();
+        let mut private_keys = vec![private_key];
+        for _ in 1..committee_size {
+            private_keys.push(PrivateKey::new(genesis_rng).unwrap());
+        }
 
         Self::from_genesis(private_keys, genesis_block)
     }
@@ -297,4 +301,16 @@ impl TestChainBuilder {
     pub fn genesis_block(&self) -> &Block<CurrentNetwork> {
         &self.genesis_block
     }
+
+    /// Returns the index into `private_keys` of the elected leader for the given round,
+    /// or `None` if the round has no committee or the leader is not among the known keys.
+    pub fn get_leader_index(&self, round: u64) -> Option<usize> {
+        let committee = self.ledger.get_committee_lookback_for_round(round).ok()??;
+        let leader = committee.get_leader(round).ok()?;
+        self.private_keys
+            .iter()
+            .enumerate()
+            .find(|(_, key)| Address::try_from(*key).unwrap() == leader)
+            .map(|(idx, _)| idx)
+    }
 }

ledger/tests/pending_blocks.rs

@@ -132,6 +132,44 @@ fn test_prefix_with_duplicate_block_error() {
     assert!(matches!(*error, CheckBlockError::InvalidHeight { expected: 2, actual: 3 }));
 }
 
+/// Regression test for the bug where `check_block_subdag_atomicity` used the raw ledger
+/// tip round as `latest_round` instead of the last prefix block's round when a non-empty
+/// prefix was provided.
+#[test]
+fn test_atomicity_check_uses_prefix_latest_block() {
+    let rng = &mut TestRng::default();
+    let mut builder = TestChainBuilder::new(4, rng);
+
+    // External ledger starts at genesis and is never explicitly advanced in this test.
+    let ledger = Ledger::<CurrentNetwork, LedgerType<CurrentNetwork>>::load(
+        builder.genesis_block().clone(),
+        StorageMode::new_test(None),
+    )
+    .unwrap();
+
+    // Identify the elected leader at round 2 and exclude them from block 1.
+    // This guarantees their certificate at round 2 is absent from block 1's subdag,
+    // making it a late-arriving *leader* certificate that will appear in block 2's subdag.
+    let skip_idx = builder.get_leader_index(2).expect("Leader not found for round 2");
+    let block1 =
+        builder.generate_block_with_opts(&BlockOptions { skip_nodes: vec![skip_idx], ..Default::default() }, rng);
+
+    // Pre-process block 1 against the external ledger (still at genesis) so it can
+    // be used as a prefix when validating block 2.
+    let pending_block1 = ledger.check_block_subdag(block1, &[]).unwrap();
+
+    // Generate block 2 with all validators participating.  The previously skipped
+    // validator's certificates for rounds ≤ block1.anchor_round are now included in
+    // block 2's subdag as late-arriving entries.
+    let block2 = builder.generate_block(rng);
+
+    // Block 2 must be accepted with block 1 as the prefix while the external ledger
+    // is still at genesis.  This would fail before the fix because the atomicity check
+    // incorrectly used genesis's round (0) instead of block 1's anchor round, causing it
+    // to flag the late-arriving leader cert at round 2 as a protocol violation.
+    ledger.check_block_subdag(block2, &[pending_block1]).unwrap();
+}
+
 #[test]
 fn test_check_block_content_invalid_height() {
     let rng = &mut TestRng::default();