refactor!: Wrap fallible guesser fee helper functions in Result

Function `Block::total_guesser_reward`, along with other helper
methods related to guesser fees, are undefined for un-validated
blocks, so the result should be wrapped in a `Result` in order to
avoid passing inconsistent values up the call graph. This change
cascades into many function return type modifications and many
`.unwrap`s and `.expect`s downstream.

The inconsistency was exposed by test
`rpc_server::rpc_server_tests::public_announcements_in_block_test`
which is now fixed instead of ignored. Thanks to @skaunov for PR #543
and the effort leading to this exposition.

Also:
 - Change logic for computing block proposal favorability: use
   `prev_block_digest` instead of block height. However, the old
   method (now suffixed with `_legacy`) continues to exist to
   enable unmodified processing of `PeerMessage`
   `BlockProposalNotification`, which comes with a block height and
   not a digest. (We cannot change `PeerMessage`, unfortunately.)
 - Rewrite handler for `PeerMessage` `BlockProposal`. Simplify and
   reduce indentation.

Co-authored-by: Alan Szepieniec <[email protected]>
Co-authored-by: Thorkil Schmidiger <[email protected]>

Author: Sword-Smith

benches/wallet_state.rs

@@ -41,7 +41,7 @@ mod maintain_membership_proofs {
             rt.block_on(async {
                 wallet_state
                     .update_wallet_state_with_new_block(
-                        &genesis.mutator_set_accumulator_after(),
+                        &genesis.mutator_set_accumulator_after().unwrap(),
                         &block1,
                     )
                     .await
@@ -64,7 +64,7 @@ mod maintain_membership_proofs {
                 rt.block_on(async {
                     wallet_state
                         .update_wallet_state_with_new_block(
-                            &block1.mutator_set_accumulator_after(),
+                            &block1.mutator_set_accumulator_after().unwrap(),
                             &block2,
                         )
                         .await

src/api/regtest/regtest_impl.rs

@@ -122,7 +122,7 @@ impl RegTestPrivate {
             SIZE_20MB_IN_BYTES,
             Some(MAX_NUM_TXS_TO_MERGE),
             true, //only_merge_single_proofs
-            tip_block.mutator_set_accumulator_after().hash(),
+            tip_block.mutator_set_accumulator_after().unwrap().hash(),
         );
 
         drop(gs);

src/api/tx_initiation/builder/transaction_details_builder.rs

@@ -257,7 +257,9 @@ impl TransactionDetailsBuilder {
             fee,
             coinbase,
             timestamp,
-            tip_block.mutator_set_accumulator_after(),
+            tip_block
+                .mutator_set_accumulator_after()
+                .expect("Block from state must be valid"),
             state_lock.cli().network,
         ))
     }

src/bench_helpers.rs

@@ -89,7 +89,7 @@ pub async fn next_block_incoming_utxos(
         .sum::<NativeCurrencyAmount>()
         + fee;
 
-    let msa = parent.mutator_set_accumulator_after();
+    let msa = parent.mutator_set_accumulator_after().unwrap();
     let wallet_status = sender.get_wallet_status(parent.hash(), &msa).await;
     let change_amt = wallet_status
         .synced_unspent_available_amount(timestamp)

src/main_loop.rs

@@ -893,8 +893,10 @@ impl MainLoopHandler {
                     info!("Received new favorable block proposal for mining operation.");
                     let mut global_state_mut = self.global_state_lock.lock_guard_mut().await;
                     let verdict = global_state_mut.favor_incoming_block_proposal(
-                        block.header().height,
-                        block.total_guesser_reward(),
+                        block.header().prev_block_digest,
+                        block
+                            .total_guesser_reward()
+                            .expect("block received by main loop should be valid"),
                     );
                     if let Err(reject_reason) = verdict {
                         warn!("main loop got unfavorable block proposal. Reason: {reject_reason}");

src/main_loop/proof_upgrader.rs

@@ -380,7 +380,8 @@ impl UpgradeJob {
                 let tip_mutator_set = global_state
                     .chain
                     .light_state()
-                    .mutator_set_accumulator_after();
+                    .mutator_set_accumulator_after()
+                    .expect("Block from state must be valid");
 
                 let transaction_is_up_to_date =
                     upgraded.kernel.mutator_set_hash == tip_mutator_set.hash();
@@ -740,7 +741,8 @@ pub(super) fn get_upgrade_task_from_mempool(
     let tip_mutator_set = global_state
         .chain
         .light_state()
-        .mutator_set_accumulator_after();
+        .mutator_set_accumulator_after()
+        .expect("Block from state must be valid");
     let gobbling_fraction = global_state.gobbling_fraction();
     let min_gobbling_fee = global_state.min_gobbling_fee();
     let num_proofs_threshold = global_state.max_num_proofs();
@@ -1123,7 +1125,8 @@ mod tests {
                 .await
                 .chain
                 .light_state()
-                .mutator_set_accumulator_after();
+                .mutator_set_accumulator_after()
+                .unwrap();
             assert!(mempool_tx.is_confirmable_relative_to(&mutator_set_accumulator_after));
         }
     }

src/mine_loop.rs

@@ -527,13 +527,17 @@ pub(crate) fn composer_outputs(
 
 /// Compute `TransactionDetails` and a list of `TxOutput`s for a coinbase
 /// transaction.
+///
+/// # Panics
+///
+///  - If `latest_block` is invalid.
 pub(super) fn prepare_coinbase_transaction_stateless(
     latest_block: &Block,
     composer_parameters: ComposerParameters,
     timestamp: Timestamp,
     network: Network,
 ) -> (TxOutputList, TransactionDetails) {
-    let mutator_set_accumulator = latest_block.mutator_set_accumulator_after().clone();
+    let mutator_set_accumulator = latest_block.mutator_set_accumulator_after().unwrap();
     let next_block_height: BlockHeight = latest_block.header().height.next();
     info!("Creating coinbase for block of height {next_block_height}.");
 
@@ -591,6 +595,8 @@ pub(crate) async fn create_block_transaction(
     .await
 }
 
+/// # Panics
+///  - If predecessor is invalid
 pub(crate) async fn create_block_transaction_from(
     predecessor_block: &Block,
     global_state_lock: &GlobalStateLock,
@@ -600,7 +606,9 @@ pub(crate) async fn create_block_transaction_from(
 ) -> Result<(Transaction, Vec<ExpectedUtxo>)> {
     let block_capacity_for_transactions = SIZE_20MB_IN_BYTES;
 
-    let predecessor_block_ms = predecessor_block.mutator_set_accumulator_after();
+    let predecessor_block_ms = predecessor_block
+        .mutator_set_accumulator_after()
+        .expect("predecessor should be valid");
     let mutator_set_hash = predecessor_block_ms.hash();
     debug!("Creating block transaction with mutator set hash: {mutator_set_hash}",);
 
@@ -645,7 +653,7 @@ pub(crate) async fn create_block_transaction_from(
     // Guarantees that some merge happens in below loop, which sets merge-bit.
     if transactions_to_merge.is_empty() {
         let nop = TransactionDetails::nop(
-            predecessor_block.mutator_set_accumulator_after(),
+            predecessor_block_ms,
             timestamp,
             global_state_lock.cli().network,
         );
@@ -1178,7 +1186,10 @@ pub(crate) mod tests {
                 make_mock_transaction_with_mutator_set_hash(
                     vec![],
                     outputs,
-                    previous_block.mutator_set_accumulator_after().hash(),
+                    previous_block
+                        .mutator_set_accumulator_after()
+                        .unwrap()
+                        .hash(),
                 ),
                 dummy_expected_utxo(),
             )
@@ -1759,7 +1770,7 @@ pub(crate) mod tests {
             let transaction = make_mock_transaction_with_mutator_set_hash(
                 vec![],
                 vec![],
-                prev_block.mutator_set_accumulator_after().hash(),
+                prev_block.mutator_set_accumulator_after().unwrap().hash(),
             );
 
             let guesser_key = HashLockKey::from_preimage(Digest::default());
@@ -2394,7 +2405,7 @@ pub(crate) mod tests {
             let transaction = make_mock_transaction_with_mutator_set_hash(
                 vec![],
                 vec![],
-                prev_block.mutator_set_accumulator_after().hash(),
+                prev_block.mutator_set_accumulator_after().unwrap().hash(),
             );
 
             // gen guesser key

src/models/blockchain/block/mock_block_generator.rs

@@ -146,7 +146,7 @@ impl MockBlockGenerator {
             // create the nop-tx and merge into the coinbase transaction to set the
             // merge bit to allow the tx to be included in a block.
             let nop_details = TransactionDetails::nop(
-                predecessor_block.mutator_set_accumulator_after(),
+                predecessor_block.mutator_set_accumulator_after().unwrap(),
                 timestamp,
                 network,
             );

src/models/blockchain/block/mod.rs

@@ -374,12 +374,16 @@ impl Block {
     ///
     /// Includes the guesser-fee UTXOs which are not included by the
     /// `mutator_set_accumulator` field on the block body.
-    pub fn mutator_set_accumulator_after(&self) -> MutatorSetAccumulator {
+    pub fn mutator_set_accumulator_after(
+        &self,
+    ) -> Result<MutatorSetAccumulator, BlockValidationError> {
         let mut msa = self.kernel.body.mutator_set_accumulator.clone();
-        let mutator_set_update = MutatorSetUpdate::new(vec![], self.guesser_fee_addition_records());
+        let mutator_set_update =
+            MutatorSetUpdate::new(vec![], self.guesser_fee_addition_records()?);
         mutator_set_update.apply_to_accumulator(&mut msa)
             .expect("mutator set update derived from guesser fees should be applicable to mutator set accumulator contained in body");
-        msa
+
+        Ok(msa)
     }
 
     #[inline]
@@ -819,11 +823,9 @@ impl Block {
         }
 
         // 2.a)
+        let msa_before = previous_block.mutator_set_accumulator_after()?;
         for removal_record in &self.kernel.body.transaction_kernel.inputs {
-            if !previous_block
-                .mutator_set_accumulator_after()
-                .can_remove(removal_record)
-            {
+            if !msa_before.can_remove(removal_record) {
                 return Err(BlockValidationError::RemovalRecordsValid);
             }
         }
@@ -847,7 +849,7 @@ impl Block {
             self.body().transaction_kernel.inputs.clone(),
             self.body().transaction_kernel.outputs.clone(),
         );
-        let mut msa = previous_block.mutator_set_accumulator_after();
+        let mut msa = msa_before;
         let ms_update_result = mutator_set_update.apply_to_accumulator(&mut msa);
 
         // 2.c)
@@ -1025,26 +1027,32 @@ impl Block {
 
     /// The amount rewarded to the guesser who finds a valid nonce for this
     /// block.
-    pub(crate) fn total_guesser_reward(&self) -> NativeCurrencyAmount {
-        self.body().transaction_kernel.fee
+    pub(crate) fn total_guesser_reward(
+        &self,
+    ) -> Result<NativeCurrencyAmount, BlockValidationError> {
+        if self.body().transaction_kernel.fee.is_negative() {
+            return Err(BlockValidationError::NegativeFee);
+        }
+
+        Ok(self.body().transaction_kernel.fee)
     }
 
     /// Get the block's guesser fee UTXOs.
     ///
     /// The amounts in the UTXOs are taken from the transaction fee.
     ///
     /// The genesis block does not have a guesser reward.
-    pub(crate) fn guesser_fee_utxos(&self) -> Vec<Utxo> {
+    pub(crate) fn guesser_fee_utxos(&self) -> Result<Vec<Utxo>, BlockValidationError> {
         const MINER_REWARD_TIME_LOCK_PERIOD: Timestamp = Timestamp::years(3);
 
         if self.header().height.is_genesis() {
-            return vec![];
+            return Ok(vec![]);
         }
 
         let lock = self.header().guesser_digest;
         let lock_script = HashLockKey::lock_script_from_after_image(lock);
 
-        let total_guesser_reward = self.total_guesser_reward();
+        let total_guesser_reward = self.total_guesser_reward()?;
         let mut value_locked = total_guesser_reward;
         value_locked.div_two();
         let value_unlocked = total_guesser_reward.checked_sub(&value_locked).unwrap();
@@ -1056,15 +1064,18 @@ impl Block {
         let locked_utxo = Utxo::new(lock_script.clone(), coins);
         let unlocked_utxo = Utxo::new_native_currency(lock_script, value_unlocked);
 
-        vec![locked_utxo, unlocked_utxo]
+        Ok(vec![locked_utxo, unlocked_utxo])
     }
 
     /// Compute the addition records that correspond to the UTXOs generated for
     /// the block's guesser
     ///
     /// The genesis block does not have this addition record.
-    pub(crate) fn guesser_fee_addition_records(&self) -> Vec<AdditionRecord> {
-        self.guesser_fee_utxos()
+    pub(crate) fn guesser_fee_addition_records(
+        &self,
+    ) -> Result<Vec<AdditionRecord>, BlockValidationError> {
+        Ok(self
+            .guesser_fee_utxos()?
             .into_iter()
             .map(|utxo| {
                 let item = Tip5::hash(&utxo);
@@ -1078,21 +1089,22 @@ impl Block {
 
                 commit(item, sender_randomness, receiver_digest)
             })
-            .collect_vec()
+            .collect_vec())
     }
 
     /// Return the mutator set update corresponding to this block, which sends
     /// the mutator set accumulator after the predecessor to the mutator set
     /// accumulator after self.
-    pub(crate) fn mutator_set_update(&self) -> MutatorSetUpdate {
+    pub(crate) fn mutator_set_update(&self) -> Result<MutatorSetUpdate, BlockValidationError> {
         let mut mutator_set_update = MutatorSetUpdate::new(
             self.body().transaction_kernel.inputs.clone(),
             self.body().transaction_kernel.outputs.clone(),
         );
 
-        let extra_addition_records = self.guesser_fee_addition_records();
+        let extra_addition_records = self.guesser_fee_addition_records()?;
         mutator_set_update.additions.extend(extra_addition_records);
-        mutator_set_update
+
+        Ok(mutator_set_update)
     }
 }
 
@@ -1245,7 +1257,7 @@ pub(crate) mod tests {
                 block_proof_witness.appendix(),
                 BlockProof::Invalid,
             );
-            let total_guesser_reward = block1.total_guesser_reward();
+            let total_guesser_reward = block1.total_guesser_reward().unwrap();
             let total_miner_reward = total_composer_reward + total_guesser_reward;
             assert_eq!(NativeCurrencyAmount::coins(128), total_miner_reward);
 
@@ -1385,7 +1397,7 @@ pub(crate) mod tests {
         }
     }
 
-    #[proptest(async = "tokio")]
+    #[proptest(async = "tokio", cases = 5)]
     async fn can_prove_block_ancestry(
         #[strategy(collection::vec(arb::<Digest>(), 55))] mut sender_randomness_vec: Vec<Digest>,
         #[strategy(0..54usize)] index: usize,
@@ -1813,9 +1825,10 @@ pub(crate) mod tests {
                 (0.4, guesser_preimage),
             )
             .await;
-            let ars = block1.guesser_fee_addition_records();
+            let ars = block1.guesser_fee_addition_records().unwrap();
             let ars_from_wallet = block1
                 .guesser_fee_utxos()
+                .unwrap()
                 .iter()
                 .map(|utxo| commit(Tip5::hash(utxo), block1.hash(), guesser_preimage.hash()))
                 .collect_vec();
@@ -1824,7 +1837,7 @@ pub(crate) mod tests {
             let MutatorSetUpdate {
                 removals: _,
                 additions,
-            } = block1.mutator_set_update();
+            } = block1.mutator_set_update().unwrap();
             assert!(
                 ars.iter().all(|ar| additions.contains(ar)),
                 "All addition records must be present in block's mutator set update"
@@ -1845,7 +1858,7 @@ pub(crate) mod tests {
             let preimage = rand::rng().random::<Digest>();
             block.set_header_guesser_digest(preimage.hash());
 
-            let guesser_fee_utxos = block.guesser_fee_utxos();
+            let guesser_fee_utxos = block.guesser_fee_utxos().unwrap();
 
             let lock_script_and_witness =
                 HashLockKey::from_preimage(preimage).lock_script_and_witness();
@@ -1868,7 +1881,7 @@ pub(crate) mod tests {
             let network = Network::Main;
             let genesis_block = Block::genesis(network);
             assert!(
-                genesis_block.guesser_fee_utxos().is_empty(),
+                genesis_block.guesser_fee_utxos().unwrap().is_empty(),
                 "Genesis block has no guesser fee UTXOs"
             );
 
@@ -1932,7 +1945,7 @@ pub(crate) mod tests {
             let mut ms = block1.body().mutator_set_accumulator.clone();
 
             let mutator_set_update_guesser_fees =
-                MutatorSetUpdate::new(vec![], block1.guesser_fee_addition_records());
+                MutatorSetUpdate::new(vec![], block1.guesser_fee_addition_records().unwrap());
             let mut mutator_set_update_tx = MutatorSetUpdate::new(
                 block2.body().transaction_kernel.inputs.clone(),
                 block2.body().transaction_kernel.outputs.clone(),