[mfp] do not vote on potentially GC'ed blocks

Author: mwtian

consensus/core/src/commit_finalizer.rs

@@ -339,9 +339,9 @@ impl CommitFinalizer {
                 }
             }
             // Initialize the block state.
-            blocks_map
-                .entry(block_ref)
-                .or_insert_with(|| RwLock::new(BlockState::new(block)));
+            blocks_map.entry(block_ref).or_insert_with(|| {
+                RwLock::new(BlockState::new(block, commit_state.commit.commit_ref.index))
+            });
         }
     }
 
@@ -477,6 +477,19 @@ impl CommitFinalizer {
             .map(|(k, v)| (*k, v.clone()))
             .collect();
 
+        let gc_rounds = self
+            .pending_commits
+            .iter()
+            .map(|c| {
+                (
+                    c.commit.commit_ref.index,
+                    self.dag_state
+                        .read()
+                        .calculate_gc_round(c.commit.leader.round),
+                )
+            })
+            .collect::<Vec<_>>();
+
         // Number of blocks to process in each task.
         const BLOCKS_PER_INDIRECT_COMMIT_TASK: usize = 8;
 
@@ -488,6 +501,7 @@ impl CommitFinalizer {
         for chunk in pending_blocks.chunks(BLOCKS_PER_INDIRECT_COMMIT_TASK) {
             let context = self.context.clone();
             let blocks = self.blocks.clone();
+            let gc_rounds = gc_rounds.clone();
             let chunk: Vec<(BlockRef, BTreeSet<TransactionIndex>)> = chunk.to_vec();
 
             join_set.spawn(tokio::task::spawn_blocking(move || {
@@ -497,6 +511,7 @@ impl CommitFinalizer {
                     let finalized = Self::try_indirect_finalize_pending_transactions_in_block(
                         &context,
                         &blocks,
+                        &gc_rounds,
                         block_ref,
                         pending_transactions,
                     );
@@ -578,6 +593,7 @@ impl CommitFinalizer {
     fn try_indirect_finalize_pending_transactions_in_block(
         context: &Arc<Context>,
         blocks: &Arc<RwLock<BTreeMap<BlockRef, RwLock<BlockState>>>>,
+        gc_rounds: &[(CommitIndex, Round)],
         pending_block_ref: BlockRef,
         pending_transactions: BTreeSet<TransactionIndex>,
     ) -> Vec<TransactionIndex> {
@@ -591,13 +607,11 @@ impl CommitFinalizer {
                 .collect();
         let mut finalized_transactions = vec![];
         let blocks_map = blocks.read();
-        // Use BTreeSet to ensure always visit blocks in the earliest round.
-        let mut to_visit_blocks = blocks_map
-            .get(&pending_block_ref)
-            .unwrap()
-            .read()
-            .children
-            .clone();
+        // Use BTreeSet for to_visit_blocks, to visit blocks in the earliest round first.
+        let (pending_commit_index, mut to_visit_blocks) = {
+            let block_state = blocks_map.get(&pending_block_ref).unwrap().read();
+            (block_state.commit_index, block_state.children.clone())
+        };
         // Blocks that have been visited.
         let mut visited = BTreeSet::new();
         // Blocks where votes and origin descendants should be ignored for processing.
@@ -608,15 +622,15 @@ impl CommitFinalizer {
                 continue;
             }
             let curr_block_state = blocks_map.get(&curr_block_ref).unwrap_or_else(|| panic!("Block {curr_block_ref} is either incorrectly gc'ed or failed to be recovered after crash.")).read();
-            // The first ancestor of current block should have the same origin / author as the current block.
-            // If it is not found in the blocks map but have round higher than the pending block, it might have
-            // voted on the pending block but have been GC'ed.
-            // Because the GC'ed block might have voted on the pending block and rejected some of the pending transactions,
-            // we cannot assume current block is voting to accept transactions from the pending block.
-            let curr_origin_ancestor_ref = curr_block_state.block.ancestors().first().unwrap();
-            let skip_votes = curr_block_ref.author == curr_origin_ancestor_ref.author
-                && pending_block_ref.round < curr_origin_ancestor_ref.round
-                && !blocks_map.contains_key(curr_origin_ancestor_ref);
+            // When proposing the current block, if it is possible that the pending block has already been GC'ed,
+            // then it is possible that the current block does not carry votes for the pending block.
+            // In this case, skip counting votes from the current block.
+            let skip_votes = Self::check_gc_at_current_commit(
+                gc_rounds,
+                pending_block_ref.round,
+                pending_commit_index,
+                curr_block_state.commit_index,
+            );
             // Skip counting votes from the block if it has been marked to be ignored.
             if ignored.insert(curr_block_ref) {
                 // Skip collecting votes from origin descendants of current block.
@@ -683,6 +697,33 @@ impl CommitFinalizer {
         finalized_transactions
     }
 
+    // Returns true if the pending block round can be GC'ed when proposing blocks in current commit.
+    fn check_gc_at_current_commit(
+        gc_rounds: &[(CommitIndex, Round)],
+        pending_block_round: Round,
+        pending_commit_index: CommitIndex,
+        current_commit_index: CommitIndex,
+    ) -> bool {
+        assert!(
+            pending_commit_index <= current_commit_index,
+            "Pending {pending_commit_index} should be <= current {current_commit_index}"
+        );
+        if pending_commit_index == current_commit_index {
+            return false;
+        }
+        // Since GC round only advances after a commit, when proposing blocks for the current commit,
+        // the GC round is determined by the commit previous to the current commit.
+        let (commit_index, gc_round) = *gc_rounds
+            .get((current_commit_index - 1 - pending_commit_index) as usize)
+            .unwrap();
+        assert_eq!(
+            commit_index,
+            current_commit_index - 1,
+            "Commit index mismatch {commit_index} != {current_commit_index}"
+        );
+        pending_block_round <= gc_round
+    }
+
     fn pop_finalized_commits(&mut self) -> Vec<CommittedSubDag> {
         let mut finalized_commits = vec![];
 
@@ -800,10 +841,12 @@ struct BlockState {
     // Other committed blocks that are origin descendants of this block.
     // See the comment above append_origin_descendants_from_last_commit() for more details.
     origin_descendants: Vec<BlockRef>,
+    // Commit which contains this block.
+    commit_index: CommitIndex,
 }
 
 impl BlockState {
-    fn new(block: VerifiedBlock) -> Self {
+    fn new(block: VerifiedBlock, commit_index: CommitIndex) -> Self {
         let reject_votes: BTreeMap<_, _> = block
             .transaction_votes()
             .iter()
@@ -817,6 +860,7 @@ impl BlockState {
             children: BTreeSet::new(),
             reject_votes,
             origin_descendants,
+            commit_index,
         }
     }
 }

consensus/core/src/dag_state.rs

@@ -799,7 +799,8 @@ impl DagState {
         let mut targets = VecDeque::new();
         targets.push_back(root_block);
         while let Some(block_ref) = targets.pop_front() {
-            // This is only correct with GC enabled.
+            // No need to collect and mark blocks at or below GC round. These blocks will not be included in new commits
+            // and do not need their transactions to be voted on.
             if block_ref.round <= gc_round {
                 continue;
             }

crates/sui-protocol-config/src/lib.rs

@@ -4287,7 +4287,10 @@ impl ProtocolConfig {
 
                     cfg.poseidon_bn254_cost_base = Some(260);
 
-                    cfg.feature_flags.consensus_skip_gced_accept_votes = true;
+                    if chain != Chain::Mainnet && chain != Chain::Testnet {
+                        cfg.feature_flags.consensus_skip_gced_accept_votes = true;
+                    }
+
                     if chain != Chain::Mainnet {
                         cfg.feature_flags
                             .enable_nitro_attestation_all_nonzero_pcrs_parsing = true;

crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__Mainnet_version_104.snap

@@ -126,7 +126,6 @@ feature_flags:
   generate_df_type_layouts: true
   private_generics_verifier_v2: true
   deprecate_global_storage_ops: true
-  consensus_skip_gced_accept_votes: true
 max_tx_size_bytes: 131072
 max_input_objects: 2048
 max_size_written_objects: 5000000

crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__Testnet_version_104.snap

@@ -1,7 +1,6 @@
 ---
 source: crates/sui-protocol-config/src/lib.rs
 expression: "ProtocolConfig::get_for_version(cur, *chain_id)"
-snapshot_kind: text
 ---
 version: 104
 feature_flags:
@@ -129,7 +128,6 @@ feature_flags:
   generate_df_type_layouts: true
   private_generics_verifier_v2: true
   deprecate_global_storage_ops: true
-  consensus_skip_gced_accept_votes: true
 max_tx_size_bytes: 131072
 max_input_objects: 2048
 max_size_written_objects: 5000000