From d13419323f6794e4cd7add2011837aafee115a7b Mon Sep 17 00:00:00 2001
From: Brice Dobry <brice@hiro.so>
Date: Fri, 28 Jun 2024 14:01:46 -0400
Subject: [PATCH] feat: make miner retry when signature collection times out

---
 .../stacks-node/src/nakamoto_node/miner.rs    |  8 +-
 testnet/stacks-node/src/tests/signer/mod.rs   |  1 +
 testnet/stacks-node/src/tests/signer/v0.rs    | 90 ++++++++++++++++++-
 3 files changed, 92 insertions(+), 7 deletions(-)

diff --git a/testnet/stacks-node/src/nakamoto_node/miner.rs b/testnet/stacks-node/src/nakamoto_node/miner.rs
index 421702bcfb..edd26243d2 100644
--- a/testnet/stacks-node/src/nakamoto_node/miner.rs
+++ b/testnet/stacks-node/src/nakamoto_node/miner.rs
@@ -248,12 +248,8 @@ impl BlockMinerThread {
                 ) {
                     Ok(x) => x,
                     Err(e) => {
-                        error!(
-                            "Unrecoverable error while gathering signatures: {e:?}. Ending tenure."
-                        );
-                        return Err(NakamotoNodeError::MiningFailure(
-                            ChainstateError::MinerAborted,
-                        ));
+                        error!("Error while gathering signatures: {e:?}. Will try mining again.");
+                        continue;
                     }
                 };
 
diff --git a/testnet/stacks-node/src/tests/signer/mod.rs b/testnet/stacks-node/src/tests/signer/mod.rs
index 794c00f05a..0ac82f4365 100644
--- a/testnet/stacks-node/src/tests/signer/mod.rs
+++ b/testnet/stacks-node/src/tests/signer/mod.rs
@@ -118,6 +118,7 @@ impl<S: Signer<T> + Send + 'static, T: SignerEventTrait + 'static> SignerTest<Sp
         // That's the kind of thing an idiot would have on his luggage!
         let password = "12345";
         naka_conf.connection_options.block_proposal_token = Some(password.to_string());
+        naka_conf.miner.wait_on_signers = Duration::from_secs(10);
 
         let run_stamp = rand::random();
 
diff --git a/testnet/stacks-node/src/tests/signer/v0.rs b/testnet/stacks-node/src/tests/signer/v0.rs
index c1d6169f7d..6cef21be16 100644
--- a/testnet/stacks-node/src/tests/signer/v0.rs
+++ b/testnet/stacks-node/src/tests/signer/v0.rs
@@ -417,7 +417,6 @@ fn end_of_tenure() {
 
     signer_test.boot_to_epoch_3();
 
-    // signer_test.mine_and_verify_confirmed_naka_block(Duration::from_secs(30), num_signers);
     signer_test.mine_nakamoto_block(Duration::from_secs(30));
 
     TEST_VALIDATE_STALL.lock().unwrap().replace(true);
@@ -501,3 +500,92 @@ fn end_of_tenure() {
 
     signer_test.shutdown();
 }
+
+#[test]
+#[ignore]
+/// This test checks that the miner will retry when signature collection times out.
+fn retry_on_timeout() {
+    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::new();
+    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(
+        num_signers,
+        vec![(sender_addr.clone(), send_amt + send_fee)],
+    );
+    let http_origin = format!("http://{}", &signer_test.running_nodes.conf.node.rpc_bind);
+
+    signer_test.boot_to_epoch_3();
+
+    signer_test.mine_nakamoto_block(Duration::from_secs(30));
+
+    // Stall block validation so the signers will not be able to sign.
+    TEST_VALIDATE_STALL.lock().unwrap().replace(true);
+
+    let proposals_before = signer_test
+        .running_nodes
+        .nakamoto_blocks_proposed
+        .load(Ordering::SeqCst);
+    let blocks_before = signer_test
+        .running_nodes
+        .nakamoto_blocks_mined
+        .load(Ordering::SeqCst);
+
+    // submit a tx so that the miner will mine a block
+    let sender_nonce = 0;
+    let transfer_tx =
+        make_stacks_transfer(&sender_sk, sender_nonce, send_fee, &recipient, send_amt);
+    submit_tx(&http_origin, &transfer_tx);
+
+    info!("Submitted transfer tx and waiting for block proposal");
+    loop {
+        let blocks_proposed = signer_test
+            .running_nodes
+            .nakamoto_blocks_proposed
+            .load(Ordering::SeqCst);
+        if blocks_proposed > proposals_before {
+            break;
+        }
+        std::thread::sleep(Duration::from_millis(100));
+    }
+
+    info!("Block proposed, verifying that it is not processed");
+
+    // Wait 20 seconds to be sure that the timeout has occurred
+    std::thread::sleep(Duration::from_secs(20));
+    assert_eq!(
+        signer_test
+            .running_nodes
+            .nakamoto_blocks_mined
+            .load(Ordering::SeqCst),
+        blocks_before
+    );
+
+    // Disable the stall and wait for the block to be processed on retry
+    info!("Disable the stall and wait for the block to be processed");
+    TEST_VALIDATE_STALL.lock().unwrap().replace(false);
+    loop {
+        let blocks_mined = signer_test
+            .running_nodes
+            .nakamoto_blocks_mined
+            .load(Ordering::SeqCst);
+        if blocks_mined > blocks_before {
+            break;
+        }
+        std::thread::sleep(Duration::from_millis(100));
+    }
+
+    signer_test.shutdown();
+}