Merge pull request #6600 from jferrant/chore/cleanup-wellformed-tenure-fns

Make is_wellformed_tenure_*_block return Error and remove custom conversions

Author: jferrant

contrib/stacks-inspect/src/lib.rs

@@ -1105,12 +1105,7 @@ fn replay_block_nakamoto(
     };
 
     // find commit and sortition burns if this is a tenure-start block
-    let Ok(new_tenure) = block.is_wellformed_tenure_start_block() else {
-        return Err(ChainstateError::InvalidStacksBlock(
-            "Invalid Nakamoto block: invalid tenure change tx(s)".into(),
-        ));
-    };
-
+    let new_tenure = block.is_wellformed_tenure_start_block()?;
     let (commit_burn, sortition_burn) = if new_tenure {
         // find block-commit to get commit-burn
         let block_commit = SortitionDB::get_block_commit(

stackslib/src/chainstate/nakamoto/mod.rs

@@ -1005,13 +1005,13 @@ impl NakamotoBlock {
     /// If it's present, then it's the first transaction (i.e. tx 0).
     /// NOTE: this does _not_ return a tenure-extend transaction payload.
     pub fn get_tenure_change_tx_payload(&self) -> Option<&TenureChangePayload> {
-        if self.is_wellformed_tenure_start_block() != Ok(true) {
+        let Ok(true) = self.is_wellformed_tenure_start_block() else {
             // no tenure-change, or invalid
             return None;
-        }
+        };
 
         // if it exists, it's the first
-        self.txs.get(0).and_then(|tx| {
+        self.txs.first().and_then(|tx| {
             if let TransactionPayload::TenureChange(ref tc) = &tx.payload {
                 Some(tc)
             } else {
@@ -1024,13 +1024,13 @@ impl NakamotoBlock {
     /// If it's present, then it's the first transaction (i.e. tx 0)
     /// NOTE: this does _not_ return a tenure-change transaction payload.
     pub fn get_tenure_extend_tx_payload(&self) -> Option<&TenureChangePayload> {
-        if self.is_wellformed_tenure_extend_block() != Ok(true) {
+        let Ok(true) = self.is_wellformed_tenure_extend_block() else {
             // no tenure-extend, or invalid
             return None;
-        }
+        };
 
         // if it exists, it's the first
-        self.txs.get(0).and_then(|tx| {
+        self.txs.first().and_then(|tx| {
             if let TransactionPayload::TenureChange(ref tc) = &tx.payload {
                 Some(tc)
             } else {
@@ -1048,16 +1048,16 @@ impl NakamotoBlock {
         if let Some(payload) = self.get_tenure_extend_tx_payload() {
             return Some(payload);
         }
-        return None;
+        None
     }
 
     /// Get the coinbase transaction in Nakamoto.
     /// It's the first non-TenureChange transaction (i.e. tx 1)
     pub fn get_coinbase_tx(&self) -> Option<&StacksTransaction> {
-        if self.is_wellformed_tenure_start_block() != Ok(true) {
+        let Ok(true) = self.is_wellformed_tenure_start_block() else {
             // not a tenure-change block, or invalid
             return None;
-        }
+        };
 
         // there is one coinbase.
         // go find it.
@@ -1083,7 +1083,7 @@ impl NakamotoBlock {
     /// Return Some(tenure-change-payload) if it's a tenure change
     /// Return None if not
     pub fn try_get_tenure_change_payload(&self) -> Option<&TenureChangePayload> {
-        if let TransactionPayload::TenureChange(ref tc) = &self.txs.get(0)?.payload {
+        if let TransactionPayload::TenureChange(ref tc) = &self.txs.first()?.payload {
             Some(tc)
         } else {
             None
@@ -1099,14 +1099,14 @@ impl NakamotoBlock {
     ///
     /// Returns Ok(true) if the above are true
     /// Returns Ok(false) if it is not a tenure-extend block
-    /// Returns Err(()) if this block cannot be a valid block
-    pub fn is_wellformed_tenure_extend_block(&self) -> Result<bool, ()> {
+    /// Returns Err(ChainstateError::InvalidStacksBlock) if this block cannot be a valid block
+    pub fn is_wellformed_tenure_extend_block(&self) -> Result<bool, ChainstateError> {
+        let err = ChainstateError::InvalidStacksBlock("Malformed tenure-extend block".into());
         // find coinbases
         let has_coinbase = self
             .txs
             .iter()
-            .find(|tx| matches!(&tx.payload, TransactionPayload::Coinbase(..)))
-            .is_some();
+            .any(|tx| matches!(&tx.payload, TransactionPayload::Coinbase(..)));
 
         if has_coinbase {
             // can't be
@@ -1137,7 +1137,7 @@ impl NakamotoBlock {
                 "Invalid block -- {} tenure txs",
                 tenure_change_positions.len()
             );
-            return Err(());
+            return Err(err);
         }
 
         let Some(tc_payload) = self.try_get_tenure_change_payload() else {
@@ -1146,7 +1146,7 @@ impl NakamotoBlock {
                 "stacks_block_hash" => %self.header.block_hash(),
                 "stacks_block_id" => %self.header.block_id()
             );
-            return Err(());
+            return Err(err);
         };
         if tc_payload.cause != TenureChangeCause::Extended {
             // not a tenure-extend, and can't be valid since all other tenure-change types require
@@ -1156,7 +1156,7 @@ impl NakamotoBlock {
                 "stacks_block_hash" => %self.header.block_hash(),
                 "stacks_block_id" => %self.header.block_id()
             );
-            return Err(());
+            return Err(err);
         }
 
         if tc_payload.previous_tenure_end != self.header.parent_block_id {
@@ -1169,7 +1169,7 @@ impl NakamotoBlock {
                 "stacks_block_hash" => %self.header.block_hash(),
                 "stacks_block_id" => %self.header.block_id()
             );
-            return Err(());
+            return Err(err);
         }
 
         if tc_payload.tenure_consensus_hash != self.header.consensus_hash
@@ -1184,7 +1184,7 @@ impl NakamotoBlock {
                 "stacks_block_hash" => %self.header.block_hash(),
                 "stacks_block_id" => %self.header.block_id()
             );
-            return Err(());
+            return Err(err);
         }
 
         Ok(true)
@@ -1198,9 +1198,10 @@ impl NakamotoBlock {
     ///
     /// Returns Ok(true) if the above are true
     /// Returns Ok(false) if this is not a tenure-start block
-    /// Returns Err(()) if this block cannot be a valid block
-    pub fn is_wellformed_tenure_start_block(&self) -> Result<bool, ()> {
+    /// Returns Err(ChainstateError::InvalidStacksBlock) if this block cannot be a valid block
+    pub fn is_wellformed_tenure_start_block(&self) -> Result<bool, ChainstateError> {
         // sanity check -- this may contain no coinbases or tenure-changes
+        let err = ChainstateError::InvalidStacksBlock("Malformed tenure-start block".into());
         let coinbase_positions = self
             .txs
             .iter()
@@ -1243,7 +1244,7 @@ impl NakamotoBlock {
                 "stacks_block_hash" => %self.header.block_hash(),
                 "stacks_block_id" => %self.header.block_id()
             );
-            return Err(());
+            return Err(err);
         }
 
         let coinbase_position = coinbase_positions.first().copied();
@@ -1256,7 +1257,7 @@ impl NakamotoBlock {
                 "stacks_block_hash" => %self.header.block_hash(),
                 "stacks_block_id" => %self.header.block_id()
             );
-            return Err(());
+            return Err(err);
         }
 
         if let (None, Some(tenure_change_position)) = (coinbase_position, tenure_change_position) {
@@ -1265,13 +1266,12 @@ impl NakamotoBlock {
             if tenure_change_position != 0 {
                 // wrong position
                 warn!(
-                    "Invalid block -- tenure change positions = {:?}, expected [0]",
-                    &tenure_change_positions;
+                    "Invalid block -- tenure change positions = {tenure_change_positions:?}, expected [0]";
                     "consensus_hash" => %self.header.consensus_hash,
                     "stacks_block_hash" => %self.header.block_hash(),
                     "stacks_block_id" => %self.header.block_id()
                 );
-                return Err(());
+                return Err(err);
             }
 
             // must be a non-sortition-triggered tenure change
@@ -1285,7 +1285,7 @@ impl NakamotoBlock {
                     "stacks_block_hash" => %self.header.block_hash(),
                     "stacks_block_id" => %self.header.block_id()
                 );
-                return Err(());
+                return Err(err);
             };
 
             if tc_payload.cause.expects_sortition() {
@@ -1295,7 +1295,7 @@ impl NakamotoBlock {
                     "stacks_block_hash" => %self.header.block_hash(),
                     "stacks_block_id" => %self.header.block_id()
                 );
-                return Err(());
+                return Err(err);
             }
 
             // not a tenure-start block, but syntactically valid w.r.t. tenure changes
@@ -1308,20 +1308,20 @@ impl NakamotoBlock {
         if coinbase_position != Some(coinbase_idx) && tenure_change_position != Some(tc_idx) {
             // invalid -- expect exactly one sortition-induced tenure change and exactly one coinbase expected,
             // and the tenure change must be the first transaction and the coinbase must be the second transaction
-            warn!("Invalid block -- coinbase and/or tenure change txs are in the wrong position -- ({:?}, {:?}) != [{}], [{}]", &coinbase_positions, &tenure_change_positions, coinbase_idx, tc_idx;
+            warn!("Invalid block -- coinbase and/or tenure change txs are in the wrong position -- ({coinbase_positions:?}, {tenure_change_positions:?}) != [{coinbase_idx}], [{tc_idx}]";
                 "consensus_hash" => %self.header.consensus_hash,
                 "stacks_block_hash" => %self.header.block_hash(),
                 "stacks_block_id" => %self.header.block_id()
             );
-            return Err(());
+            return Err(err);
         }
         let Some(tc_payload) = self.try_get_tenure_change_payload() else {
             warn!("Invalid block -- tx at index 0 is not a tenure tx";
                 "consensus_hash" => %self.header.consensus_hash,
                 "stacks_block_hash" => %self.header.block_hash(),
                 "stacks_block_id" => %self.header.block_id()
             );
-            return Err(());
+            return Err(err);
         };
         if !tc_payload.cause.expects_sortition() {
             // the only tenure change allowed in a block with a coinbase is a sortition-triggered
@@ -1331,7 +1331,7 @@ impl NakamotoBlock {
                 "stacks_block_hash" => %self.header.block_hash(),
                 "stacks_block_id" => %self.header.block_id()
             );
-            return Err(());
+            return Err(err);
         }
         if tc_payload.previous_tenure_end != self.header.parent_block_id {
             // discontinuous
@@ -1342,7 +1342,7 @@ impl NakamotoBlock {
                 "stacks_block_hash" => %self.header.block_hash(),
                 "stacks_block_id" => %self.header.block_id()
             );
-            return Err(());
+            return Err(err);
         }
 
         // must be a Nakamoto coinbase
@@ -1351,13 +1351,12 @@ impl NakamotoBlock {
         else {
             // this transaction is not a coinbase (but this should be unreachable)
             warn!(
-                "Invalid block -- tx index {} is not a coinbase",
-                coinbase_idx;
+                "Invalid block -- tx index {coinbase_idx} is not a coinbase";
                 "consensus_hash" => %self.header.consensus_hash,
                 "stacks_block_hash" => %self.header.block_hash(),
                 "stacks_block_id" => %self.header.block_id()
             );
-            return Err(());
+            return Err(err);
         };
         if vrf_proof_opt.is_none() {
             // not a Nakamoto coinbase
@@ -1366,10 +1365,10 @@ impl NakamotoBlock {
                 "stacks_block_hash" => %self.header.block_hash(),
                 "stacks_block_id" => %self.header.block_id()
             );
-            return Err(());
+            return Err(err);
         }
 
-        return Ok(true);
+        Ok(true)
     }
 
     /// Verify that the VRF seed of this block's block-commit is the hash of the parent tenure's
@@ -1649,27 +1648,14 @@ impl NakamotoBlock {
         chain_id: u32,
         epoch_id: StacksEpochId,
     ) -> bool {
-        if self.txs.is_empty() {
-            return false;
-        }
-        if !StacksBlock::validate_transactions_unique(&self.txs) {
-            return false;
-        }
-        if !StacksBlock::validate_transactions_network(&self.txs, mainnet) {
-            return false;
-        }
-        if !StacksBlock::validate_transactions_chain_id(&self.txs, chain_id) {
+        if self.txs.is_empty()
+            || !StacksBlock::validate_transactions_unique(&self.txs)
+            || !StacksBlock::validate_transactions_network(&self.txs, mainnet)
+            || !StacksBlock::validate_transactions_chain_id(&self.txs, chain_id)
+        {
             return false;
         }
-        let valid_tenure_start = self.is_wellformed_tenure_start_block();
-        if valid_tenure_start == Ok(true) {
-            if self.get_coinbase_tx().is_none() {
-                return false;
-            }
-            if self.get_tenure_change_tx_payload().is_none() {
-                return false;
-            }
-        } else if valid_tenure_start.is_err() {
+        if self.is_wellformed_tenure_start_block().is_err() {
             // bad tenure change
             warn!("Not a well-formed tenure-start block";
                 "consensus_hash" => %self.header.consensus_hash,
@@ -1678,24 +1664,16 @@ impl NakamotoBlock {
             );
             return false;
         }
-        let valid_tenure_extend = self.is_wellformed_tenure_extend_block();
-        if valid_tenure_extend == Ok(true) {
-            if self.get_tenure_extend_tx_payload().is_none() {
-                return false;
-            }
-        } else if valid_tenure_extend.is_err() {
+        if self.is_wellformed_tenure_extend_block().is_err() {
             // bad tenure extend
             warn!("Not a well-formed tenure-extend block";
                 "consensus_hash" => %self.header.consensus_hash,
                 "stacks_block_hash" => %self.header.block_hash(),
                 "stacks_block_id" => %self.header.block_id()
             );
             return false;
-        }
-        if !StacksBlock::validate_transactions_static_epoch(&self.txs, epoch_id) {
-            return false;
-        }
-        return true;
+        };
+        StacksBlock::validate_transactions_static_epoch(&self.txs, epoch_id)
     }
 }
 
@@ -1992,11 +1970,7 @@ impl NakamotoChainState {
         };
 
         // find commit and sortition burns if this is a tenure-start block
-        let Ok(new_tenure) = next_ready_block.is_wellformed_tenure_start_block() else {
-            return Err(ChainstateError::InvalidStacksBlock(
-                "Invalid Nakamoto block: invalid tenure change tx(s)".into(),
-            ));
-        };
+        let new_tenure = next_ready_block.is_wellformed_tenure_start_block()?;
 
         let (commit_burn, sortition_burn) = if new_tenure && !next_ready_block.is_shadow_block() {
             // find block-commit to get commit-burn
@@ -2417,15 +2391,13 @@ impl NakamotoChainState {
         }
 
         // if this is the first tenure block, then make sure it's well-formed
-        block.is_wellformed_tenure_start_block().map_err(|_| {
+        block.is_wellformed_tenure_start_block().inspect_err(|_| {
             warn!("Block {block_id} is not a well-formed first tenure block");
-            ChainstateError::InvalidStacksBlock("Not a well-formed first-tenure block".into())
         })?;
 
         // if this is a tenure-extend block, then make sure it's well-formed
-        block.is_wellformed_tenure_extend_block().map_err(|_| {
+        block.is_wellformed_tenure_extend_block().inspect_err(|_| {
             warn!("Block {block_id} is not a well-formed tenure-extend block");
-            ChainstateError::InvalidStacksBlock("Not a well-formed tenure-extend block".into())
         })?;
 
         // it's okay if this fails because we might not have the parent block yet.  It will be
@@ -4537,21 +4509,15 @@ impl NakamotoChainState {
             Self::check_sortition_exists(burn_dbconn, &block.header.consensus_hash)?;
         let block_hash = block.header.block_hash();
 
-        let new_tenure = block.is_wellformed_tenure_start_block().map_err(|_| {
-            ChainstateError::InvalidStacksBlock("Invalid tenure changes in nakamoto block".into())
-        })?;
-
+        let new_tenure = block.is_wellformed_tenure_start_block()?;
         // this block is mined in the ongoing tenure.
         if !new_tenure
             && !Self::check_tenure_continuity(chainstate_tx.as_tx(), &parent_ch, &block.header)?
         {
             // this block is not part of the ongoing tenure; it's invalid
             return Err(ChainstateError::ExpectedTenureChange);
         }
-        let tenure_extend = block.is_wellformed_tenure_extend_block().map_err(|_| {
-            ChainstateError::InvalidStacksBlock("Invalid tenure changes in nakamoto block".into())
-        })?;
-
+        let tenure_extend = block.is_wellformed_tenure_extend_block()?;
         if tenure_extend && new_tenure {
             return Err(ChainstateError::InvalidStacksBlock(
                 "Both started and extended tenure".into(),