feat: on timeout, re-propose the same block

When a miner times out waiting for signatures, instead of proposing a
new block, it should only re-propose the same block. Proposing a new
block is guaranteed to fail because signers that approved the original
block will reject any new block at the same height.

This implements the miner side of #5856. A change is still needed on the
signer side to allow a signer to accept a block that it previously
rejected.

Author: obycode

testnet/stacks-node/src/nakamoto_node.rs

@@ -143,6 +143,8 @@ pub enum Error {
     /// NetError wrapper
     #[error("NetError: {0}")]
     NetError(#[from] NetError),
+    #[error("Timed out waiting for signatures")]
+    SignatureTimeout,
 }
 
 impl StacksNode {

testnet/stacks-node/src/nakamoto_node/signer_coordinator.rs

@@ -254,46 +254,54 @@ impl SignerCoordinator {
         };
 
         let block_proposal_message = SignerMessageV0::BlockProposal(block_proposal);
-        debug!("Sending block proposal message to signers";
-            "signer_signature_hash" => %block.header.signer_signature_hash(),
-        );
-        Self::send_miners_message::<SignerMessageV0>(
-            &self.message_key,
-            sortdb,
-            election_sortition,
-            stackerdbs,
-            block_proposal_message,
-            MinerSlotID::BlockProposal,
-            self.is_mainnet,
-            &mut self.miners_session,
-            &election_sortition.consensus_hash,
-        )?;
-        counters.bump_naka_proposed_blocks();
 
-        #[cfg(test)]
-        {
-            info!(
-                "SignerCoordinator: sent block proposal to .miners, waiting for test signing channel"
+        loop {
+            debug!("Sending block proposal message to signers";
+                "signer_signature_hash" => %block.header.signer_signature_hash(),
             );
-            // In test mode, short-circuit waiting for the signers if the TEST_SIGNING
-            //  channel has been created. This allows integration tests for the stacks-node
-            //  independent of the stacks-signer.
-            if let Some(signatures) =
-                crate::tests::nakamoto_integrations::TestSigningChannel::get_signature()
+            Self::send_miners_message::<SignerMessageV0>(
+                &self.message_key,
+                sortdb,
+                election_sortition,
+                stackerdbs,
+                block_proposal_message.clone(),
+                MinerSlotID::BlockProposal,
+                self.is_mainnet,
+                &mut self.miners_session,
+                &election_sortition.consensus_hash,
+            )?;
+            counters.bump_naka_proposed_blocks();
+
+            #[cfg(test)]
             {
-                debug!("Short-circuiting waiting for signers, using test signature");
-                return Ok(signatures);
+                info!(
+                "SignerCoordinator: sent block proposal to .miners, waiting for test signing channel"
+            );
+                // In test mode, short-circuit waiting for the signers if the TEST_SIGNING
+                //  channel has been created. This allows integration tests for the stacks-node
+                //  independent of the stacks-signer.
+                if let Some(signatures) =
+                    crate::tests::nakamoto_integrations::TestSigningChannel::get_signature()
+                {
+                    debug!("Short-circuiting waiting for signers, using test signature");
+                    return Ok(signatures);
+                }
             }
-        }
 
-        self.get_block_status(
-            &block.header.signer_signature_hash(),
-            &block.block_id(),
-            block.header.parent_block_id,
-            chain_state,
-            sortdb,
-            counters,
-        )
+            let res = self.get_block_status(
+                &block.header.signer_signature_hash(),
+                &block.block_id(),
+                block.header.parent_block_id,
+                chain_state,
+                sortdb,
+                counters,
+            );
+
+            match res {
+                Err(NakamotoNodeError::SignatureTimeout) => continue,
+                _ => return res,
+            }
+        }
     }
 
     /// Get the block status for a given block hash.
@@ -340,7 +348,7 @@ impl SignerCoordinator {
                     if rejections_timer.elapsed() > *rejections_timeout {
                         return false;
                     }
-                    // number or rejections changed?
+                    // number of rejections changed?
                     if status.total_weight_rejected != rejections {
                         return false;
                     }
@@ -353,7 +361,7 @@ impl SignerCoordinator {
                     // If we just received a timeout, we should check if the burnchain
                     // tip has changed or if we received this signed block already in
                     // the staging db.
-                    debug!("SignerCoordinator: Timeout waiting for block signatures");
+                    debug!("SignerCoordinator: Intermediate timeout waiting for block status");
 
                     // Look in the nakamoto staging db -- a block can only get stored there
                     // if it has enough signing weight to clear the threshold.
@@ -380,15 +388,17 @@ impl SignerCoordinator {
                     }
 
                     if rejections_timer.elapsed() > *rejections_timeout {
-                        warn!("Timed out while waiting for responses from signers";
-                                  "elapsed" => rejections_timer.elapsed().as_secs(),
-                                  "rejections_timeout" => rejections_timeout.as_secs(),
-                                  "rejections" => rejections,
-                                  "rejections_threshold" => self.total_weight.saturating_sub(self.weight_threshold)
+                        warn!("Timed out while waiting for responses from signers, resending proposal";
+                            "elapsed" => rejections_timer.elapsed().as_secs(),
+                            "rejections_timeout" => rejections_timeout.as_secs(),
+                            "rejections" => rejections,
+                            "rejections_threshold" => self.total_weight.saturating_sub(self.weight_threshold)
                         );
-                        return Err(NakamotoNodeError::SigningCoordinatorFailure(
-                            "Timed out while waiting for signatures".into(),
-                        ));
+
+                        // Reset the rejections in the stackerdb listener
+                        self.stackerdb_comms.reset_rejections(block_signer_sighash);
+
+                        return Err(NakamotoNodeError::SignatureTimeout);
                     }
 
                     // Check if a new Stacks block has arrived in the parent tenure
@@ -399,7 +409,7 @@ impl SignerCoordinator {
                         )?
                         .ok_or(NakamotoNodeError::UnexpectedChainState)?;
                     if highest_in_tenure.index_block_hash() != parent_block_id {
-                        debug!("SignCoordinator: Exiting due to new stacks tip");
+                        info!("SignCoordinator: Exiting due to new stacks tip");
                         return Err(NakamotoNodeError::StacksTipChanged);
                     }
 
@@ -448,14 +458,16 @@ impl SignerCoordinator {
                 return Ok(block_status.gathered_signatures.values().cloned().collect());
             } else if rejections_timer.elapsed() > *rejections_timeout {
                 warn!("Timed out while waiting for responses from signers";
-                          "elapsed" => rejections_timer.elapsed().as_secs(),
-                          "rejections_timeout" => rejections_timeout.as_secs(),
-                          "rejections" => rejections,
-                          "rejections_threshold" => self.total_weight.saturating_sub(self.weight_threshold)
+                    "elapsed" => rejections_timer.elapsed().as_secs(),
+                    "rejections_timeout" => rejections_timeout.as_secs(),
+                    "rejections" => rejections,
+                    "rejections_threshold" => self.total_weight.saturating_sub(self.weight_threshold)
                 );
-                return Err(NakamotoNodeError::SigningCoordinatorFailure(
-                    "Timed out while waiting for signatures".into(),
-                ));
+
+                // Reset the rejections in the stackerdb listener
+                self.stackerdb_comms.reset_rejections(block_signer_sighash);
+
+                return Err(NakamotoNodeError::SignatureTimeout);
             } else {
                 continue;
             }

testnet/stacks-node/src/nakamoto_node/stackerdb_listener.rs

@@ -52,9 +52,13 @@ pub static EVENT_RECEIVER_POLL: Duration = Duration::from_millis(500);
 
 #[derive(Debug, Clone)]
 pub struct BlockStatus {
-    pub responded_signers: HashSet<StacksPublicKey>,
+    /// Set of the slot ids of signers who have responded
+    pub responded_signers: HashSet<u32>,
+    /// Map of the slot id of signers who have signed the block and their signature
     pub gathered_signatures: BTreeMap<u32, MessageSignature>,
+    /// Total weight of signers who have signed the block
     pub total_weight_approved: u32,
+    /// Total weight of signers who have rejected the block
     pub total_weight_rejected: u32,
 }
 
@@ -342,7 +346,7 @@ impl StackerDBListener {
                             "server_version" => metadata.server_version,
                         );
                         block.gathered_signatures.insert(slot_id, signature);
-                        block.responded_signers.insert(signer_pubkey);
+                        block.responded_signers.insert(slot_id);
 
                         if block.total_weight_approved >= self.weight_threshold {
                             // Signal to anyone waiting on this block that we have enough signatures
@@ -385,7 +389,7 @@ impl StackerDBListener {
                             }
                         };
 
-                        if block.responded_signers.insert(rejected_pubkey) {
+                        if block.responded_signers.insert(slot_id) {
                             block.total_weight_rejected = block
                                 .total_weight_rejected
                                 .checked_add(signer_entry.weight)
@@ -498,6 +502,25 @@ impl StackerDBListenerComms {
         blocks.insert(block.signer_signature_hash(), block_status);
     }
 
+    /// Reset rejections for a block proposal.
+    /// This is used when a block proposal times out and we need to retry it by
+    /// clearing the block's rejections. Block approvals cannot be cleared
+    /// because an old approval could always be used to make a block reach
+    /// the approval threshold.
+    pub fn reset_rejections(&self, signer_sighash: &Sha512Trunc256Sum) {
+        let (lock, _cvar) = &*self.blocks;
+        let mut blocks = lock.lock().expect("FATAL: failed to lock block status");
+        if let Some(block) = blocks.get_mut(signer_sighash) {
+            block.responded_signers.clear();
+            block.total_weight_rejected = 0;
+
+            // Add approving signers back to the responded signers set
+            for (slot_id, _) in block.gathered_signatures.iter() {
+                block.responded_signers.insert(*slot_id);
+            }
+        }
+    }
+
     /// Get the status for `block` from the Stacker DB listener.
     /// If the block is not found in the map, return an error.
     /// If the block is found, call `condition` to check if the block status

testnet/stacks-node/src/tests/signer/v0.rs

@@ -12170,3 +12170,130 @@ fn repeated_rejection() {
 
     signer_test.shutdown();
 }
+
+#[test]
+#[ignore]
+/// This test verifies that a miner will re-propose the same block if it times
+/// out waiting for signers to reach consensus on the block.
+fn retry_proposal() {
+    if env::var("BITCOIND_TEST") != Ok("1".into()) {
+        return;
+    }
+    tracing_subscriber::registry()
+        .with(fmt::layer())
+        .with(EnvFilter::from_default_env())
+        .init();
+
+    info!("------------------------- Test Setup -------------------------");
+    let num_signers = 5;
+    let sender_sk = Secp256k1PrivateKey::random();
+    let sender_addr = tests::to_addr(&sender_sk);
+    let send_amt = 100;
+    let send_fee = 180;
+    let recipient = PrincipalData::from(StacksAddress::burn_address(false));
+    let mut signer_test: SignerTest<SpawnedSigner> = SignerTest::new_with_config_modifications(
+        num_signers,
+        vec![(sender_addr, (send_amt + send_fee) * 3)],
+        |_| {},
+        |config| {
+            config.miner.block_rejection_timeout_steps.clear();
+            config
+                .miner
+                .block_rejection_timeout_steps
+                .insert(0, Duration::from_secs(123));
+            config
+                .miner
+                .block_rejection_timeout_steps
+                .insert(10, Duration::from_secs(20));
+            config
+                .miner
+                .block_rejection_timeout_steps
+                .insert(15, Duration::from_secs(10));
+            config
+                .miner
+                .block_rejection_timeout_steps
+                .insert(20, Duration::from_secs(30));
+        },
+        None,
+        None,
+    );
+    let http_origin = format!("http://{}", &signer_test.running_nodes.conf.node.rpc_bind);
+    signer_test.boot_to_epoch_3();
+
+    let proposed_blocks = signer_test
+        .running_nodes
+        .counters
+        .naka_proposed_blocks
+        .clone();
+
+    signer_test.mine_nakamoto_block(Duration::from_secs(60), true);
+
+    let info = get_chain_info(&signer_test.running_nodes.conf);
+    let block_height_before = info.stacks_tip_height;
+
+    // make signer[0] reject all proposals
+    let rejecting_signer =
+        StacksPublicKey::from_private(&signer_test.signer_stacks_private_keys[0]);
+    TEST_REJECT_ALL_BLOCK_PROPOSAL.set(vec![rejecting_signer]);
+
+    // make signer[1] ignore all proposals
+    let ignoring_signer = StacksPublicKey::from_private(&signer_test.signer_stacks_private_keys[1]);
+    TEST_IGNORE_ALL_BLOCK_PROPOSALS.set(vec![ignoring_signer]);
+
+    let proposals_before = proposed_blocks.load(Ordering::SeqCst);
+
+    // submit a tx so that the miner will mine a block
+    let transfer_tx = make_stacks_transfer(
+        &sender_sk,
+        0,
+        send_fee,
+        signer_test.running_nodes.conf.burnchain.chain_id,
+        &recipient,
+        send_amt,
+    );
+    submit_tx(&http_origin, &transfer_tx);
+
+    info!("Submitted transfer tx and waiting for block proposal");
+    wait_for(60, || {
+        if proposed_blocks.load(Ordering::SeqCst) > proposals_before {
+            return Ok(true);
+        }
+        Ok(false)
+    })
+    .expect("Timed out waiting for block proposal");
+
+    info!(
+        "Block proposed, submitting another transaction that should not get included in the block"
+    );
+    let transfer_tx = make_stacks_transfer(
+        &sender_sk,
+        1,
+        send_fee,
+        signer_test.running_nodes.conf.burnchain.chain_id,
+        &recipient,
+        send_amt,
+    );
+    submit_tx(&http_origin, &transfer_tx);
+
+    info!("Disable signer 1 from ignoring proposals");
+    test_observer::clear();
+    TEST_IGNORE_ALL_BLOCK_PROPOSALS.set(vec![]);
+
+    info!("Waiting for the block to be approved");
+    wait_for(60, || {
+        let info = get_chain_info(&signer_test.running_nodes.conf);
+        if info.stacks_tip_height > block_height_before {
+            return Ok(true);
+        }
+        Ok(false)
+    })
+    .expect("Timed out waiting for block");
+
+    // Ensure that the block was the original block with just 1 transfer
+    let blocks = test_observer::get_blocks();
+    let block = blocks.first().expect("No blocks found");
+    let transactions = block["transactions"].as_array().unwrap();
+    assert_eq!(transactions.len(), 1);
+
+    signer_test.shutdown();
+}