feat: allow signers to reconsider some rejected blocks

Allows a signer to reconsider a block that it previously rejected if it
was rejected for certain reasons that may resolve themselves, for
example a testing directive, the parent block was unknown, or there was
a communication failure with the stacks-node.

Resolves #5856

Author: obycode

libsigner/src/v0/messages.rs

@@ -51,7 +51,7 @@ use clarity::util::hash::Sha256Sum;
 use clarity::util::retry::BoundReader;
 use clarity::util::secp256k1::MessageSignature;
 use clarity::vm::types::serialization::SerializationError;
-use clarity::vm::types::{QualifiedContractIdentifier, TupleData};
+use clarity::vm::types::{QualifiedContractIdentifier, ResponseData, TupleData};
 use clarity::vm::Value;
 use hashbrown::{HashMap, HashSet};
 use serde::{Deserialize, Serialize};
@@ -875,11 +875,11 @@ impl BlockResponse {
         }
     }
 
-    /// The signer signature hash for the block response
-    pub fn signer_signature_hash(&self) -> Sha512Trunc256Sum {
+    /// Get the block response data from the block response
+    pub fn get_response_data(&self) -> &BlockResponseData {
         match self {
-            BlockResponse::Accepted(accepted) => accepted.signer_signature_hash,
-            BlockResponse::Rejected(rejection) => rejection.signer_signature_hash,
+            BlockResponse::Accepted(accepted) => &accepted.response_data,
+            BlockResponse::Rejected(rejection) => &rejection.response_data,
         }
     }
 

stacks-signer/src/signerdb.rs

@@ -167,6 +167,8 @@ pub struct BlockInfo {
     pub validation_time_ms: Option<u64>,
     /// Extra data specific to v0, v1, etc.
     pub ext: ExtraBlockInfo,
+    /// If this signer rejected this block, what was the reason
+    pub reject_reason: Option<RejectReason>,
 }
 
 impl From<BlockProposal> for BlockInfo {
@@ -184,6 +186,7 @@ impl From<BlockProposal> for BlockInfo {
             ext: ExtraBlockInfo::default(),
             state: BlockState::Unprocessed,
             validation_time_ms: None,
+            reject_reason: None,
         }
     }
 }

stacks-signer/src/v0/signer.rs

@@ -21,7 +21,8 @@ use std::time::{Duration, Instant};
 
 use blockstack_lib::chainstate::nakamoto::{NakamotoBlock, NakamotoBlockHeader};
 use blockstack_lib::net::api::postblock_proposal::{
-    BlockValidateOk, BlockValidateReject, BlockValidateResponse, TOO_MANY_REQUESTS_STATUS,
+    BlockValidateOk, BlockValidateReject, BlockValidateResponse, ValidateRejectCode,
+    TOO_MANY_REQUESTS_STATUS,
 };
 use blockstack_lib::util_lib::db::Error as DBError;
 use clarity::types::chainstate::StacksPrivateKey;
@@ -391,6 +392,7 @@ impl Signer {
             ),
         )
     }
+
     /// Create a block rejection response for a block with the given reject code
     pub fn create_block_rejection(
         &self,
@@ -411,6 +413,7 @@ impl Signer {
             ),
         )
     }
+
     /// Check if block should be rejected based on sortition state
     /// Will return a BlockResponse::Rejection if the block is invalid, none otherwise.
     fn check_block_against_sortition_state(
@@ -561,32 +564,37 @@ impl Signer {
         //  the signer needs to be able to determine whether or not the block they're about to sign would conflict with an already-signed Stacks block
         let signer_signature_hash = block_proposal.block.header.signer_signature_hash();
         if let Some(block_info) = self.block_lookup_by_reward_cycle(&signer_signature_hash) {
-            let Some(block_response) = self.determine_response(&block_info) else {
-                // We are still waiting for a response for this block. Do nothing.
-                debug!("{self}: Received a block proposal for a block we are already validating.";
-                    "signer_sighash" => %signer_signature_hash,
-                    "block_id" => %block_proposal.block.block_id()
-                );
-                return;
-            };
-            // Submit a proposal response to the .signers contract for miners
-            debug!("{self}: Broadcasting a block response to stacks node: {block_response:?}");
-            let accepted = matches!(block_response, BlockResponse::Accepted(..));
-            match self
-                .stackerdb
-                .send_message_with_retry::<SignerMessage>(block_response.into())
-            {
-                Ok(_) => {
+            if should_reevaluate_block(&block_info) {
+                // Treat this case the same as if no block info was found
+            } else {
+                let Some(block_response) = self.determine_response(&block_info) else {
+                    // We are still waiting for a response for this block. Do nothing.
+                    debug!(
+                        "{self}: Received a block proposal for a block we are already validating.";
+                        "signer_sighash" => %signer_signature_hash,
+                        "block_id" => %block_proposal.block.block_id()
+                    );
+                    return;
+                };
+
+                // Submit a proposal response to the .signers contract for miners
+                debug!("{self}: Broadcasting a block response to stacks node: {block_response:?}");
+
+                let accepted = matches!(block_response, BlockResponse::Accepted(..));
+                if let Err(e) = self
+                    .stackerdb
+                    .send_message_with_retry::<SignerMessage>(block_response.into())
+                {
+                    warn!("{self}: Failed to send block response to stacker-db: {e:?}");
+                } else {
                     crate::monitoring::actions::increment_block_responses_sent(accepted);
                     crate::monitoring::actions::record_block_response_latency(
                         &block_proposal.block,
                     );
                 }
-                Err(e) => {
-                    warn!("{self}: Failed to send block response to stacker-db: {e:?}",);
-                }
+
+                return;
             }
-            return;
         }
 
         info!(
@@ -890,6 +898,8 @@ impl Signer {
                 false,
             ),
         );
+
+        block_info.reject_reason = Some(block_rejection.response_data.reject_reason.clone());
         self.signer_db
             .insert_block(&block_info)
             .unwrap_or_else(|e| self.handle_insert_block_error(e));
@@ -1019,6 +1029,7 @@ impl Signer {
             ),
             &block_info.block,
         );
+        block_info.reject_reason = Some(rejection.get_response_data().reject_reason.clone());
         if let Err(e) = block_info.mark_locally_rejected() {
             if !block_info.has_reached_consensus() {
                 warn!("{self}: Failed to mark block as locally rejected: {e:?}");
@@ -1039,6 +1050,7 @@ impl Signer {
             ),
             Ok(_) => debug!("{self}: Block rejection accepted by stacker-db"),
         }
+
         self.signer_db
             .insert_block(&block_info)
             .unwrap_or_else(|e| self.handle_insert_block_error(e));
@@ -1132,6 +1144,7 @@ impl Signer {
         ) {
             warn!("{self}: Failed to save block rejection signature: {e:?}",);
         }
+        block_info.reject_reason = Some(rejection.response_data.reject_reason.clone());
 
         // do we have enough signatures to mark a block a globally rejected?
         // i.e. is (set-size) - (threshold) + 1 reached.
@@ -1412,3 +1425,33 @@ impl Signer {
         }
     }
 }
+
+/// Determine if a block should be re-evaluated based on its rejection reason˝
+fn should_reevaluate_block(block_info: &BlockInfo) -> bool {
+    if let Some(reject_reason) = &block_info.reject_reason {
+        match reject_reason {
+            RejectReason::ValidationFailed(ValidateRejectCode::UnknownParent)
+            | RejectReason::NoSortitionView
+            | RejectReason::ConnectivityIssues(_)
+            | RejectReason::TestingDirective
+            | RejectReason::NotRejected
+            | RejectReason::Unknown(_) => true,
+            RejectReason::ValidationFailed(_)
+            | RejectReason::RejectedInPriorRound
+            | RejectReason::SortitionViewMismatch
+            | RejectReason::ReorgNotAllowed
+            | RejectReason::InvalidBitvec
+            | RejectReason::PubkeyHashMismatch
+            | RejectReason::InvalidMiner
+            | RejectReason::NotLatestSortitionWinner
+            | RejectReason::InvalidParentBlock
+            | RejectReason::DuplicateBlockFound
+            | RejectReason::InvalidTenureExtend => {
+                // No need to re-validate these types of rejections.
+                false
+            }
+        }
+    } else {
+        false
+    }
+}

stacks-signer/src/v0/tests.rs

@@ -92,6 +92,9 @@ impl Signer {
                     warn!("{self}: Failed to mark block as locally rejected: {e:?}");
                 }
             };
+
+            block_info.reject_reason = Some(RejectReason::TestingDirective);
+
             // We must insert the block into the DB to prevent subsequent repeat proposals being accepted (should reject
             // as invalid since we rejected in a prior round if this crops up again)
             // in case this is the first time we saw this block. Safe to do since this is testing case only.

testnet/stacks-node/src/tests/nakamoto_integrations.rs

@@ -6642,6 +6642,7 @@ fn signer_chainstate() {
                 ext: ExtraBlockInfo::None,
                 state: BlockState::Unprocessed,
                 validation_time_ms: None,
+                reject_reason: None,
             })
             .unwrap();
 
@@ -6722,6 +6723,7 @@ fn signer_chainstate() {
                 ext: ExtraBlockInfo::None,
                 state: BlockState::GloballyAccepted,
                 validation_time_ms: Some(1000),
+                reject_reason: None,
             })
             .unwrap();
 

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

@@ -12305,3 +12305,133 @@ fn retry_proposal() {
 
     signer_test.shutdown();
 }
+
+#[test]
+#[ignore]
+/// This test verifies that a a signer will accept a rejected block if it is
+/// re-proposed and determined to be legitimate. This can happen if the block
+/// is initially rejected due to a test flag or because the stacks-node had
+/// not yet processed the block's parent.
+fn signer_can_accept_rejected_block() {
+    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 0 from rejecting proposals");
+    test_observer::clear();
+    TEST_REJECT_ALL_BLOCK_PROPOSAL.set(vec![]);
+
+    info!("Waiting for the block to be approved");
+    wait_for(60, || {
+        let blocks = test_observer::get_blocks();
+        let last_block = blocks.last().expect("No blocks found");
+        let height = last_block["block_height"].as_u64().unwrap();
+        if 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.last().expect("No blocks found");
+    assert_eq!(transfers_in_block(block), 1);
+
+    signer_test.shutdown();
+}