Merge remote-tracking branch 'upstream/develop' into chore/cleanup_contrib

* upstream/develop: (38 commits)
  Fix typo in function def
  CRC: pass block info as an optonal to send_block_response_impl
  Remove duplicate code when sending block responses
  fix: cargo check --tests warning
  address PR comments (6081)
  move CODEOWNERS and adjust requirements to automatically add multiple teams to reviews
  add index for processable_block in staging_blocks
  fix entry block-id/hash cache refactor
  perf: do not read whole snapshot for ancestor check in find_new_block_arrivals
  remove release build arg
  build all binaries in core build workflow
  crc: remove chrono dependency, #5918
  Add missing import for SUPPORTED_SIGNER_PROTOCOL_VERSION
  crc: remove system time utility functions, #5918
  crc: fix record_signer_agreement_capitulation_latency documentation, #5918
  fix: mock miner should continue mining throughout tenure
  Remove testing assert from debugging
  Remove use of macro
  Add deserializer for custom blockstack op fields and make sure backwards compatibility satisfied for TransactionEventPayload
  CRC: typo in unreachable message
  ...

Author: wileyj

.github/CODEOWNERS

@@ -0,0 +1,16 @@
+# These owners will be the default owners for everything in
+# the repo. Unless a later match takes precedence,
+#
+# require both blockchain-team-codeowners and blockchain-team to review all PR's not specifically matched below
+* @stacks-network/blockchain-team-codeowners @stacks-network/blockchain-team
+
+
+# Signer code
+# require both blockchain-team-codeowners and blockchain-team-signer to review PR's for the signer folder(s)
+libsigner/**/*.rs @stacks-network/blockchain-team-codeowners @stacks-network/blockchain-team-signer
+stacks-signer/**/*.rs @stacks-network/blockchain-team-codeowners @stacks-network/blockchain-team-signer
+
+# CI workflows
+#  require both blockchain-team and blockchain-team-ci teams to review PR's modifying CI workflows
+/.github/workflows/ @stacks-network/blockchain-team @stacks-network/blockchain-team-ci
+/.github/actions/ @stacks-network/blockchain-team @stacks-network/blockchain-team-ci

.github/workflows/core-build-tests.yml

@@ -24,7 +24,7 @@ jobs:
       - name: Build the binaries
         id: build
         run: |
-          cargo build --bin stacks-inspect
+          cargo build
       - name: Dump constants JSON
         id: consts-dump
         run: cargo run --bin stacks-inspect -- dump-consts | tee out.json

CHANGELOG.md

@@ -9,15 +9,21 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE
 
 ### Added
 
+- Added field `vm_error` to EventObserver transaction outputs
 - Added new `ValidateRejectCode` values to the `/v3/block_proposal` endpoint
 - Added `StateMachineUpdateContent::V1` to support a vector of `StacksTransaction` expected to be replayed in subsequent Stacks blocks
+- Include a reason string in the transaction receipt when a transaction is rolled back due to a post-condition. This should help users in understanding what went wrong.
 
 ### Changed
 
 - Reduce the default `block_rejection_timeout_steps` configuration so that miners will retry faster when blocks fail to reach 70% approved or 30% rejected.
 - Added index for `next_ready_nakamoto_block()` which improves block processing performance.
 - Added a new field, `parent_burn_block_hash`, to the payload that is included in the `/new_burn_block` event observer payload.
 
+### Fixed
+
+- Fix regression in mock-mining, allowing the mock miner to continue mining blocks throughout a tenure instead of failing after mining the tenure change block.
+
 ## [3.1.0.0.8]
 
 ### Added

CODEOWNERS

@@ -20,18 +20,30 @@ pub enum Error {
     Interpreter(InterpreterError),
     BadTransaction(String),
     CostError(ExecutionCost, ExecutionCost),
-    AbortedByCallback(Option<Value>, AssetMap, Vec<StacksTransactionEvent>),
+    AbortedByCallback {
+        /// What the output value of the transaction would have been.
+        /// This will be a Some for contract-calls, and None for contract initialization txs.
+        output: Option<Value>,
+        /// The asset map which was evaluated by the abort callback
+        assets_modified: AssetMap,
+        /// The events from the transaction processing
+        tx_events: Vec<StacksTransactionEvent>,
+        /// A human-readable explanation for aborting the transaction
+        reason: String,
+    },
 }
 
 impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match *self {
+        match self {
             Error::CostError(ref a, ref b) => {
-                write!(f, "Cost Error: {} cost exceeded budget of {} cost", a, b)
+                write!(f, "Cost Error: {a} cost exceeded budget of {b} cost")
             }
             Error::Analysis(ref e) => fmt::Display::fmt(e, f),
             Error::Parse(ref e) => fmt::Display::fmt(e, f),
-            Error::AbortedByCallback(..) => write!(f, "Post condition aborted transaction"),
+            Error::AbortedByCallback { reason, .. } => {
+                write!(f, "Post condition aborted transaction: {reason}")
+            }
             Error::Interpreter(ref e) => fmt::Display::fmt(e, f),
             Error::BadTransaction(ref s) => fmt::Display::fmt(s, f),
         }
@@ -42,7 +54,7 @@ impl std::error::Error for Error {
     fn cause(&self) -> Option<&dyn std::error::Error> {
         match *self {
             Error::CostError(ref _a, ref _b) => None,
-            Error::AbortedByCallback(..) => None,
+            Error::AbortedByCallback { .. } => None,
             Error::Analysis(ref e) => Some(e),
             Error::Parse(ref e) => Some(e),
             Error::Interpreter(ref e) => Some(e),
@@ -168,16 +180,17 @@ pub trait TransactionConnection: ClarityConnection {
     /// * the asset changes during `to_do` in an `AssetMap`
     /// * the Stacks events during the transaction
     ///
-    /// and a `bool` value which is `true` if the `abort_call_back` caused the changes to abort.
+    /// and an optional string value which is the result of `abort_call_back`,
+    /// containing a human-readable reason for aborting the transaction.
     ///
     /// If `to_do` returns an `Err` variant, then the changes are aborted.
     fn with_abort_callback<F, A, R, E>(
         &mut self,
         to_do: F,
         abort_call_back: A,
-    ) -> Result<(R, AssetMap, Vec<StacksTransactionEvent>, bool), E>
+    ) -> Result<(R, AssetMap, Vec<StacksTransactionEvent>, Option<String>), E>
     where
-        A: FnOnce(&AssetMap, &mut ClarityDatabase) -> bool,
+        A: FnOnce(&AssetMap, &mut ClarityDatabase) -> Option<String>,
         F: FnOnce(&mut OwnedEnvironment) -> Result<(R, AssetMap, Vec<StacksTransactionEvent>), E>,
         E: From<InterpreterError>;
 
@@ -283,16 +296,16 @@ pub trait TransactionConnection: ClarityConnection {
                     .stx_transfer(from, to, amount, memo)
                     .map_err(Error::from)
             },
-            |_, _| false,
+            |_, _| None,
         )
         .map(|(value, assets, events, _)| (value, assets, events))
     }
 
     /// Execute a contract call in the current block.
-    ///  If an error occurs while processing the transaction, its modifications will be rolled back.
-    /// abort_call_back is called with an AssetMap and a ClarityDatabase reference,
-    ///   if abort_call_back returns true, all modifications from this transaction will be rolled back.
-    ///      otherwise, they will be committed (though they may later be rolled back if the block itself is rolled back).
+    /// If an error occurs while processing the transaction, its modifications will be rolled back.
+    /// `abort_call_back` is called with an `AssetMap` and a `ClarityDatabase` reference,
+    /// If `abort_call_back` returns `Some(reason)`, all modifications from this transaction will be rolled back.
+    /// Otherwise, they will be committed (though they may later be rolled back if the block itself is rolled back).
     #[allow(clippy::too_many_arguments)]
     fn run_contract_call<F>(
         &mut self,
@@ -305,7 +318,7 @@ pub trait TransactionConnection: ClarityConnection {
         max_execution_time: Option<std::time::Duration>,
     ) -> Result<(Value, AssetMap, Vec<StacksTransactionEvent>), Error>
     where
-        F: FnOnce(&AssetMap, &mut ClarityDatabase) -> bool,
+        F: FnOnce(&AssetMap, &mut ClarityDatabase) -> Option<String>,
     {
         let expr_args: Vec<_> = args
             .iter()
@@ -331,20 +344,25 @@ pub trait TransactionConnection: ClarityConnection {
             },
             abort_call_back,
         )
-        .and_then(|(value, assets, events, aborted)| {
-            if aborted {
-                Err(Error::AbortedByCallback(Some(value), assets, events))
+        .and_then(|(value, assets_modified, tx_events, reason)| {
+            if let Some(reason) = reason {
+                Err(Error::AbortedByCallback {
+                    output: Some(value),
+                    assets_modified,
+                    tx_events,
+                    reason,
+                })
             } else {
-                Ok((value, assets, events))
+                Ok((value, assets_modified, tx_events))
             }
         })
     }
 
     /// Initialize a contract in the current block.
     ///  If an error occurs while processing the initialization, it's modifications will be rolled back.
-    /// abort_call_back is called with an AssetMap and a ClarityDatabase reference,
-    ///   if abort_call_back returns true, all modifications from this transaction will be rolled back.
-    ///      otherwise, they will be committed (though they may later be rolled back if the block itself is rolled back).
+    /// `abort_call_back` is called with an `AssetMap` and a `ClarityDatabase` reference,
+    /// If `abort_call_back` returns `Some(reason)`, all modifications from this transaction will be rolled back.
+    /// Otherwise, they will be committed (though they may later be rolled back if the block itself is rolled back).
     #[allow(clippy::too_many_arguments)]
     fn initialize_smart_contract<F>(
         &mut self,
@@ -357,9 +375,9 @@ pub trait TransactionConnection: ClarityConnection {
         max_execution_time: Option<std::time::Duration>,
     ) -> Result<(AssetMap, Vec<StacksTransactionEvent>), Error>
     where
-        F: FnOnce(&AssetMap, &mut ClarityDatabase) -> bool,
+        F: FnOnce(&AssetMap, &mut ClarityDatabase) -> Option<String>,
     {
-        let (_, asset_map, events, aborted) = self.with_abort_callback(
+        let (_, assets_modified, tx_events, reason) = self.with_abort_callback(
             |vm_env| {
                 if let Some(max_execution_time_duration) = max_execution_time {
                     vm_env
@@ -378,10 +396,15 @@ pub trait TransactionConnection: ClarityConnection {
             },
             abort_call_back,
         )?;
-        if aborted {
-            Err(Error::AbortedByCallback(None, asset_map, events))
+        if let Some(reason) = reason {
+            Err(Error::AbortedByCallback {
+                output: None,
+                assets_modified,
+                tx_events,
+                reason,
+            })
         } else {
-            Ok((asset_map, events))
+            Ok((assets_modified, tx_events))
         }
     }
 }

clarity/src/vm/clarity.rs

@@ -33,6 +33,21 @@ SignerAgreementStateChangeReason {
     InactiveMiner("inactive_miner"),
     /// Signer agreement protocol version has been upgraded
     ProtocolUpgrade("protocol_upgrade"),
+    /// An update related to the Miner view
+    MinerViewUpdate("miner_view_update"),
+    /// A specific Miner View update related to the parent tenure
+    MinerParentTenureUpdate("miner_parent_tenure_update"),
+});
+
+define_named_enum!(
+/// Represent different conflict types on signer agreement protocol
+SignerAgreementStateConflict {
+    /// Waiting for burn block propagation to be aligned with the signer set
+    BurnBlockDelay("burn_block_delay"),
+    /// Waiting for stacks block propagation to be aligned with the signer set
+    StacksBlockDelay("stacks_block_delay"),
+    /// No agreement on miner view with the signer set
+    MinerView("miner_view"),
 });
 
 /// Actions for updating metrics
@@ -44,7 +59,7 @@ pub mod actions {
 
     use crate::config::GlobalConfig;
     use crate::monitoring::prometheus::*;
-    use crate::monitoring::SignerAgreementStateChangeReason;
+    use crate::monitoring::{SignerAgreementStateChangeReason, SignerAgreementStateConflict};
     use crate::v0::signer_state::LocalStateMachine;
 
     /// Update stacks tip height gauge
@@ -134,6 +149,21 @@ pub mod actions {
             .inc();
     }
 
+    /// Increment signer agreement state conflict counter
+    pub fn increment_signer_agreement_state_conflict(conflict: SignerAgreementStateConflict) {
+        let label_value = conflict.get_name();
+        SIGNER_AGREEMENT_STATE_CONFLICTS
+            .with_label_values(&[&label_value])
+            .inc();
+    }
+
+    /// Record the time (seconds) taken for a signer to agree with the signer set
+    pub fn record_signer_agreement_capitulation_latency(latency_s: u64) {
+        SIGNER_AGREEMENT_CAPITULATION_LATENCIES_HISTOGRAM
+            .with_label_values(&[])
+            .observe(latency_s as f64);
+    }
+
     /// Start serving monitoring metrics.
     /// This will only serve the metrics if the `monitoring_prom` feature is enabled.
     pub fn start_serving_monitoring_metrics(config: GlobalConfig) -> Result<(), String> {
@@ -157,7 +187,7 @@ pub mod actions {
     use blockstack_lib::chainstate::nakamoto::NakamotoBlock;
     use stacks_common::info;
 
-    use crate::monitoring::SignerAgreementStateChangeReason;
+    use crate::monitoring::{SignerAgreementStateChangeReason, SignerAgreementStateConflict};
     use crate::v0::signer_state::LocalStateMachine;
     use crate::GlobalConfig;
 
@@ -212,6 +242,12 @@ pub mod actions {
     ) {
     }
 
+    /// Increment signer agreement state conflict counter
+    pub fn increment_signer_agreement_state_conflict(_conflict: SignerAgreementStateConflict) {}
+
+    /// Record the time (seconds) taken for a signer to agree with the signer set
+    pub fn record_signer_agreement_capitulation_latency(_latency_s: u64) {}
+
     /// Start serving monitoring metrics.
     /// This will only serve the metrics if the `monitoring_prom` feature is enabled.
     pub fn start_serving_monitoring_metrics(config: GlobalConfig) -> Result<(), String> {

stacks-signer/src/monitoring/mod.rs

@@ -81,10 +81,22 @@ lazy_static! {
 
     pub static ref SIGNER_AGREEMENT_STATE_CHANGE_REASONS: IntCounterVec = register_int_counter_vec!(
         "stacks_signer_agreement_state_change_reasons",
-        "The number of state machine changes in signer agreement protocol. `reason` can be one of: 'burn_block_arrival', 'stacks_block_arrival', 'inactive_miner', 'protocol_upgrade'",
+        "The number of state machine changes in signer agreement protocol. `reason` can be one of: 'burn_block_arrival', 'stacks_block_arrival', 'inactive_miner', 'protocol_upgrade', 'miner_view_update', 'miner_parent_tenure_update'",
         &["reason"]
     ).unwrap();
 
+    pub static ref SIGNER_AGREEMENT_STATE_CONFLICTS: IntCounterVec = register_int_counter_vec!(
+        "stacks_signer_agreement_state_conflicts",
+        "The number of state machine conflicts in signer agreement protocol. `conflict` can be one of: 'burn_block_delay', 'stacks_block_delay', 'miner_view'",
+        &["conflict"]
+    ).unwrap();
+
+    pub static ref SIGNER_AGREEMENT_CAPITULATION_LATENCIES_HISTOGRAM: HistogramVec = register_histogram_vec!(histogram_opts!(
+        "stacks_signer_agreement_capitulation_latencies_histogram",
+        "Measuring the time (in seconds) for the signer to agree (capitulate) with the signer set",
+        vec![0.0, 1.0, 3.0, 5.0, 10.0, 20.0, 30.0, 60.0, 120.0]
+    ), &[]).unwrap();
+
     pub static ref SIGNER_LOCAL_STATE_MACHINE: Mutex<Option<LocalStateMachine>> = Mutex::new(None);
 }