Merge pull request #6585 from jferrant/chore/limit-staging-blocks-db-access

Make access to Nakamoto staging blocks restricted to one single pub fn

Author: jferrant

stacks-node/src/nakamoto_node/miner.rs

@@ -1029,18 +1029,13 @@ impl BlockMinerThread {
         let burn_view_ch =
             NakamotoChainState::get_block_burn_view(sort_db, block, &parent_block_info)?;
         let mut sortition_handle = sort_db.index_handle_at_ch(&burn_view_ch)?;
-        let chainstate_config = chain_state.config();
-        let (headers_conn, staging_tx) = chain_state.headers_conn_and_staging_tx_begin()?;
         let accepted = NakamotoChainState::accept_block(
-            &chainstate_config,
+            chain_state,
             block,
             &mut sortition_handle,
-            &staging_tx,
-            headers_conn,
             reward_set,
             NakamotoBlockObtainMethod::Mined,
         )?;
-        staging_tx.commit()?;
 
         if !accepted {
             // this can happen if the p2p network and relayer manage to receive this block prior to

stackslib/src/chainstate/nakamoto/mod.rs

@@ -2391,76 +2391,6 @@ impl NakamotoChainState {
         Ok(())
     }
 
-    /// Insert a Nakamoto block into the staging blocks DB.
-    /// We only store a block in the following cases:
-    ///
-    /// * No block with this block's sighash exists in the DB
-    /// * A block with this block's sighash exists, AND
-    ///     * this block represents more signing power
-    ///
-    /// If neither of the above is true, then this is a no-op.
-    pub(crate) fn store_block_if_better(
-        staging_db_tx: &NakamotoStagingBlocksTx,
-        block: &NakamotoBlock,
-        burn_attachable: bool,
-        signing_weight: u32,
-        obtain_method: NakamotoBlockObtainMethod,
-    ) -> Result<bool, ChainstateError> {
-        let block_id = block.block_id();
-        let block_hash = block.header.block_hash();
-
-        // case 1 -- no block with this sighash exists.
-        if staging_db_tx.try_store_block_with_new_signer_sighash(
-            block,
-            burn_attachable,
-            signing_weight,
-            obtain_method,
-        )? {
-            debug!("Stored block with new sighash";
-                   "block_id" => %block_id,
-                   "block_hash" => %block_hash);
-            return Ok(true);
-        }
-
-        // case 2 -- the block exists. Consider replacing it, but only if its
-        // signing weight is higher.
-        let (existing_block_id, _processed, orphaned, existing_signing_weight) = staging_db_tx.conn().get_block_processed_and_signed_weight(&block.header.consensus_hash, &block_hash)?
-            .ok_or_else(|| {
-                // this should be unreachable -- there's no record of this block
-                error!("Could not store block {} ({}) with block hash {} -- no record of its processed status or signing weight!", &block_id, &block.header.consensus_hash, &block_hash);
-                ChainstateError::NoSuchBlockError
-            })?;
-
-        if orphaned {
-            // nothing to do
-            debug!("Will not store alternative copy of block {} ({}) with block hash {}, since a block with the same block hash was orphaned", &block_id, &block.header.consensus_hash, &block_hash);
-            return Ok(false);
-        }
-
-        let ret = if existing_signing_weight < signing_weight {
-            staging_db_tx.replace_block(block, signing_weight, obtain_method)?;
-            debug!("Replaced block";
-                   "existing_block_id" => %existing_block_id,
-                   "block_id" => %block_id,
-                   "block_hash" => %block_hash,
-                   "existing_signing_weight" => existing_signing_weight,
-                   "signing_weight" => signing_weight);
-            true
-        } else {
-            if existing_signing_weight > signing_weight {
-                debug!("Will not store alternative copy of block {} ({}) with block hash {}, since it has less signing power", &block_id, &block.header.consensus_hash, &block_hash);
-            } else {
-                debug!(
-                    "Will not store duplicate copy of block {} ({}) with block hash {}",
-                    &block_id, &block.header.consensus_hash, &block_hash
-                );
-            }
-            false
-        };
-
-        return Ok(ret);
-    }
-
     /// Accept a Nakamoto block into the staging blocks DB.
     /// Fails if:
     /// * the public key cannot be recovered from the miner's signature
@@ -2470,17 +2400,17 @@ impl NakamotoChainState {
     /// * we already have the block
     /// Returns true if we stored the block; false if not.
     pub fn accept_block(
-        config: &ChainstateConfig,
+        chainstate: &mut StacksChainState,
         block: &NakamotoBlock,
         db_handle: &mut SortitionHandleConn,
-        staging_db_tx: &NakamotoStagingBlocksTx,
-        headers_conn: &Connection,
         reward_set: &RewardSet,
         obtain_method: NakamotoBlockObtainMethod,
     ) -> Result<bool, ChainstateError> {
         let block_id = block.block_id();
         test_debug!("Consider Nakamoto block {block_id}");
+        let config = chainstate.config();
         // do nothing if we already have this block
+        let (headers_conn, staging_db_tx) = chainstate.headers_conn_and_staging_tx_begin()?;
         if Self::get_block_header(headers_conn, &block_id)?.is_some() {
             debug!("Already have block {block_id}");
             return Ok(false);
@@ -2555,13 +2485,13 @@ impl NakamotoChainState {
         // same sortition history as `db_handle` (and thus it must be burn_attachable)
         let burn_attachable = true;
 
-        let ret = Self::store_block_if_better(
-            staging_db_tx,
+        let ret = staging_db_tx.store_block_if_better(
             block,
             burn_attachable,
             signing_weight,
             obtain_method,
         )?;
+        staging_db_tx.commit()?;
         if ret {
             test_debug!("Stored Nakamoto block {block_id}");
         } else {

stackslib/src/chainstate/nakamoto/shadow.rs

@@ -865,7 +865,7 @@ impl NakamotoStagingBlocksTx<'_> {
         // shadow blocks cannot be replaced
         let signing_weight = u32::MAX;
 
-        self.store_block(
+        self.store_block_if_better(
             shadow_block,
             burn_attachable,
             signing_weight,

stackslib/src/chainstate/nakamoto/staging_blocks.rs

@@ -590,8 +590,81 @@ impl NakamotoStagingBlocksTx<'_> {
         Ok(())
     }
 
+    /// Insert a Nakamoto block into the staging blocks DB.
+    /// We only store a block in the following cases:
+    ///
+    /// * No block with this block's sighash exists in the DB
+    /// * A block with this block's sighash exists, AND
+    ///     * this block represents more signing power
+    ///
+    /// If neither of the above is true, then this is a no-op.
+    /// NOTE: This is intentionally the only public access into
+    /// storing additional blocks inside the staging blocks DB
+    pub fn store_block_if_better(
+        &self,
+        block: &NakamotoBlock,
+        burn_attachable: bool,
+        signing_weight: u32,
+        obtain_method: NakamotoBlockObtainMethod,
+    ) -> Result<bool, ChainstateError> {
+        let block_id = block.block_id();
+        let block_hash = block.header.block_hash();
+        let consensus_hash = block.header.consensus_hash.clone();
+
+        // case 1 -- no block with this sighash exists.
+        if self.try_store_block_with_new_signer_sighash(
+            block,
+            burn_attachable,
+            signing_weight,
+            obtain_method,
+        )? {
+            debug!("Stored block with new sighash";
+                   "block_id" => %block_id,
+                   "block_hash" => %block_hash);
+            return Ok(true);
+        }
+
+        // case 2 -- the block exists. Consider replacing it, but only if its
+        // signing weight is higher.
+        let (existing_block_id, _processed, orphaned, existing_signing_weight) = self.conn().get_block_processed_and_signed_weight(&consensus_hash, &block_hash)?
+            .ok_or_else(|| {
+                // this should be unreachable -- there's no record of this block
+                error!("Could not store block {block_id} ({consensus_hash}) with block hash {block_hash} -- no record of its processed status or signing weight!");
+                ChainstateError::NoSuchBlockError
+            })?;
+
+        if orphaned {
+            // nothing to do
+            debug!("Will not store alternative copy of block {block_id} ({consensus_hash}) with block hash {block_hash}, since a block with the same block hash was orphaned");
+            return Ok(false);
+        }
+
+        let ret = if existing_signing_weight < signing_weight {
+            self.replace_block(block, signing_weight, obtain_method)?;
+            debug!("Replaced block";
+                   "existing_block_id" => %existing_block_id,
+                   "block_id" => %block_id,
+                   "block_hash" => %block_hash,
+                   "existing_signing_weight" => existing_signing_weight,
+                   "signing_weight" => signing_weight);
+            true
+        } else {
+            if existing_signing_weight > signing_weight {
+                debug!("Will not store alternative copy of block {block_id} ({consensus_hash}) with block hash {block_hash}, since it has less signing power");
+            } else {
+                debug!(
+                    "Will not store duplicate copy of block {block_id} ({consensus_hash}) with block hash {block_hash}"
+                );
+            }
+            false
+        };
+
+        Ok(ret)
+    }
+
     /// Store a block into the staging DB.
-    pub(crate) fn store_block(
+    /// NOTE: This should not be made public
+    fn store_block(
         &self,
         block: &NakamotoBlock,
         burn_attachable: bool,
@@ -665,7 +738,7 @@ impl NakamotoStagingBlocksTx<'_> {
 
     /// Do we have a block with the given signer sighash?
     /// NOTE: the block hash and sighash are the same for Nakamoto blocks
-    pub(crate) fn has_nakamoto_block_with_block_hash(
+    fn has_nakamoto_block_with_block_hash(
         &self,
         consensus_hash: &ConsensusHash,
         block_hash: &BlockHeaderHash,
@@ -681,7 +754,8 @@ impl NakamotoStagingBlocksTx<'_> {
     /// NOTE: the block hash and sighash are the same for Nakamoto blocks, so this is equivalent to
     /// storing a new block.
     /// Return true if stored; false if not.
-    pub(crate) fn try_store_block_with_new_signer_sighash(
+    /// NOTE: This should not be made public
+    fn try_store_block_with_new_signer_sighash(
         &self,
         block: &NakamotoBlock,
         burn_attachable: bool,
@@ -698,7 +772,8 @@ impl NakamotoStagingBlocksTx<'_> {
 
     /// Replace an already-stored block with a newer copy with more signing
     /// power.  Arguments will not be validated; the caller must do this.
-    pub(crate) fn replace_block(
+    /// NOTE: This should not be made public
+    fn replace_block(
         &self,
         block: &NakamotoBlock,
         signing_weight: u32,