Merge branch 'develop' into fix/reload-config-flaky-test

Author: hstove

.github/workflows/github-release.yml

@@ -142,3 +142,21 @@ jobs:
           DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }}
           DOCKERHUB_PASSWORD: ${{ secrets.DOCKERHUB_PASSWORD }}
           dist: ${{ matrix.dist }}
+
+  ## Create the downstream PR for the release branch to master,develop
+  create-pr:
+    if: |
+      inputs.node_tag != '' ||
+      inputs.signer_tag != ''
+    name: Create Downstream PR (${{ github.ref_name }})
+    runs-on: ubuntu-latest
+    needs:
+      - build-binaries
+      - create-release
+      - docker-image
+    steps:
+      - name: Open Downstream PR
+        id: create-pr
+        uses: stacks-network/actions/stacks-core/release/downstream-pr@main
+        with:
+          token: ${{ secrets.GH_TOKEN }}

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/CHANGELOG.md

@@ -7,9 +7,13 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE
 
 ## [Unreleased]
 
+### Changed
+
+- For some rejection reasons, a signer will reconsider a block proposal that it previously rejected ([#5880](https://github.com/stacks-network/stacks-core/pull/5880))
+
 ## [3.1.0.0.7.0]
 
-## Changed
+### Changed
 
 - Add new reject codes to the signer response for better visibility into why a block was rejected.
 - When allowing a reorg within the `reorg_attempts_activity_timeout_ms`, the signer will now watch the responses from other signers and if >30% of them reject this reorg attempt, then the signer will mark the miner as invalid, reject further attempts to reorg and allow the previous miner to extend their tenure.
@@ -20,7 +24,7 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE
 
 ## [3.1.0.0.6.0]
 
-## Added
+### Added
 
 - Introduced the `reorg_attempts_activity_timeout_ms` configuration option for signers which is used to determine the length of time after the last block of a tenure is confirmed that an incoming miner's attempts to reorg it are considered valid miner activity.
 - Add signer configuration option `tenure_idle_timeout_buffer_secs` to specify the number of seconds of buffer the signer will add to its tenure extend time that it sends to miners. The idea is to allow for some clock skew between the miner and signers, preventing the case where the miner attempts to tenure extend too early.

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,
         }
     }
 }
@@ -2193,4 +2196,77 @@ mod tests {
             .unwrap()
             .is_none());
     }
+
+    /// BlockInfo without the `reject_reason` field for backwards compatibility testing
+    #[derive(Serialize, Deserialize, Debug, PartialEq)]
+    pub struct BlockInfoPrev {
+        /// The block we are considering
+        pub block: NakamotoBlock,
+        /// The burn block height at which the block was proposed
+        pub burn_block_height: u64,
+        /// The reward cycle the block belongs to
+        pub reward_cycle: u64,
+        /// Our vote on the block if we have one yet
+        pub vote: Option<NakamotoBlockVote>,
+        /// Whether the block contents are valid
+        pub valid: Option<bool>,
+        /// Whether this block is already being signed over
+        pub signed_over: bool,
+        /// Time at which the proposal was received by this signer (epoch time in seconds)
+        pub proposed_time: u64,
+        /// Time at which the proposal was signed by this signer (epoch time in seconds)
+        pub signed_self: Option<u64>,
+        /// Time at which the proposal was signed by a threshold in the signer set (epoch time in seconds)
+        pub signed_group: Option<u64>,
+        /// The block state relative to the signer's view of the stacks blockchain
+        pub state: BlockState,
+        /// Consumed processing time in milliseconds to validate this block
+        pub validation_time_ms: Option<u64>,
+        /// Extra data specific to v0, v1, etc.
+        pub ext: ExtraBlockInfo,
+    }
+
+    /// Verify that we can deserialize the old BlockInfo struct into the new version
+    #[test]
+    fn deserialize_old_block_info() {
+        let block_info_prev = BlockInfoPrev {
+            block: NakamotoBlock {
+                header: NakamotoBlockHeader::genesis(),
+                txs: vec![],
+            },
+            burn_block_height: 2,
+            reward_cycle: 3,
+            vote: None,
+            valid: None,
+            signed_over: true,
+            proposed_time: 4,
+            signed_self: None,
+            signed_group: None,
+            state: BlockState::Unprocessed,
+            validation_time_ms: Some(5),
+            ext: ExtraBlockInfo::default(),
+        };
+
+        let block_info: BlockInfo =
+            serde_json::from_value(serde_json::to_value(&block_info_prev).unwrap()).unwrap();
+        assert_eq!(block_info.block, block_info_prev.block);
+        assert_eq!(
+            block_info.burn_block_height,
+            block_info_prev.burn_block_height
+        );
+        assert_eq!(block_info.reward_cycle, block_info_prev.reward_cycle);
+        assert_eq!(block_info.vote, block_info_prev.vote);
+        assert_eq!(block_info.valid, block_info_prev.valid);
+        assert_eq!(block_info.signed_over, block_info_prev.signed_over);
+        assert_eq!(block_info.proposed_time, block_info_prev.proposed_time);
+        assert_eq!(block_info.signed_self, block_info_prev.signed_self);
+        assert_eq!(block_info.signed_group, block_info_prev.signed_group);
+        assert_eq!(block_info.state, block_info_prev.state);
+        assert_eq!(
+            block_info.validation_time_ms,
+            block_info_prev.validation_time_ms
+        );
+        assert_eq!(block_info.ext, block_info_prev.ext);
+        assert!(block_info.reject_reason.is_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(
@@ -556,33 +559,19 @@ impl Signer {
         // TODO: should add a check to ignore an old burn block height if we know its outdated. Would require us to store the burn block height we last saw on the side.
         //  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(_) => {
-                    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;
+        let prior_evaluation = self
+            .block_lookup_by_reward_cycle(&signer_signature_hash)
+            .and_then(|block_info| if should_reevaluate_block(&block_info) {
+                debug!("Received a proposal for this block before, but our rejection reason allows us to reconsider";
+                    "reject_reason" => ?block_info.reject_reason);
+                None
+            } else {
+                Some(block_info)
+            });
+
+        // we previously considered this proposal, handle the status here
+        if let Some(block_info) = prior_evaluation {
+            return self.handle_prior_proposal_eval(&block_info);
         }
 
         info!(
@@ -667,6 +656,32 @@ impl Signer {
         }
     }
 
+    fn handle_prior_proposal_eval(&mut self, block_info: &BlockInfo) {
+        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" => %block_info.signer_signature_hash(),
+                "block_id" => %block_info.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_info.block);
+        }
+    }
+
     /// Handle block response messages from a signer
     fn handle_block_response(
         &mut self,
@@ -886,6 +901,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));
@@ -1015,6 +1032,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:?}");
@@ -1035,6 +1053,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));
@@ -1128,6 +1147,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.
@@ -1408,3 +1428,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::InvalidTenureExtend
+            | 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 => {
+                // 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();