fix(mine_loop): Make hash rate independent of tx size

Prior to this commit, the hash rate was lower for blocks with many
inputs/outputs in their transaction. This was because the block body
MAST hash was not cached, so it was recalculated for each PoW iteration.

This strongly incentivized miners to mine empty or small blocks, to the
detriment of the ability to have transactions confirmed and merged.

.github/workflows/main.yml

@@ -44,4 +44,4 @@ jobs:
         uses: actions-rs/cargo@v1
         with:
           command: test
-          args: -- --skip benches --skip mine_20_blocks_in_40_seconds
+          args: -- --skip benches --skip mine_20_blocks_in_40_seconds --skip hash_rate_independent_of_tx_size

src/mine_loop.rs

@@ -726,6 +726,7 @@ pub(crate) mod mine_loop_tests {
     use block_body::BlockBody;
     use block_header::block_header_tests::random_block_header;
     use difficulty_control::Difficulty;
+    use itertools::Itertools;
     use num_bigint::BigUint;
     use num_traits::Pow;
     use num_traits::Zero;
@@ -749,6 +750,7 @@ pub(crate) mod mine_loop_tests {
     use crate::tests::shared::random_transaction_kernel;
     use crate::triton_vm;
     use crate::triton_vm::stark::Stark;
+    use crate::util_types::test_shared::mutator_set::pseudorandom_addition_record;
     use crate::util_types::test_shared::mutator_set::random_mmra;
     use crate::util_types::test_shared::mutator_set::random_mutator_set_accumulator;
     use crate::WalletSecret;
@@ -771,6 +773,7 @@ pub(crate) mod mine_loop_tests {
     async fn estimate_own_hash_rate(
         target_block_interval: Option<Timestamp>,
         sleepy_guessing: bool,
+        num_outputs: usize,
     ) -> f64 {
         let mut rng = thread_rng();
         let network = Network::RegTest;
@@ -790,10 +793,13 @@ pub(crate) mod mine_loop_tests {
             .clone();
 
         let (transaction, _coinbase_utxo_info) = {
+            let outputs = (0..num_outputs)
+                .map(|_| pseudorandom_addition_record(rng.gen()))
+                .collect_vec();
             (
                 make_mock_transaction_with_mutator_set_hash(
                     vec![],
-                    vec![],
+                    outputs,
                     previous_block.mutator_set_accumulator_after().hash(),
                 ),
                 dummy_expected_utxo(),
@@ -1243,7 +1249,9 @@ pub(crate) mod mine_loop_tests {
 
         // set initial difficulty in accordance with own hash rate
         let sleepy_guessing = false;
-        let hash_rate = estimate_own_hash_rate(Some(target_block_interval), sleepy_guessing).await;
+        let num_outputs = 0;
+        let hash_rate =
+            estimate_own_hash_rate(Some(target_block_interval), sleepy_guessing, num_outputs).await;
         println!("estimating hash rate at {} per millisecond", hash_rate);
         let prepare_time = estimate_block_preparation_time_invalid_proof().await;
         println!("estimating block preparation time at {prepare_time} ms");
@@ -1379,6 +1387,26 @@ pub(crate) mod mine_loop_tests {
         Ok(())
     }
 
+    #[traced_test]
+    #[tokio::test]
+    async fn hash_rate_independent_of_tx_size() {
+        // It's crucial that the hash rate is independent of the size of the
+        // block, since miners are otherwise heavily incentivized to mine small
+        // or empty blocks.
+        let sleepy_guessing = false;
+        let hash_rate_empty_tx = estimate_own_hash_rate(None, sleepy_guessing, 0).await;
+        println!("hash_rate_empty_tx: {hash_rate_empty_tx}");
+
+        let hash_rate_big_tx = estimate_own_hash_rate(None, sleepy_guessing, 1000).await;
+        println!("hash_rate_big_tx: {hash_rate_big_tx}");
+
+        assert!(
+            hash_rate_empty_tx * 1.1 > hash_rate_big_tx
+                && hash_rate_empty_tx * 0.9 < hash_rate_big_tx,
+            "Hash rate for big and small block must be within 10 %"
+        );
+    }
+
     #[test]
     fn block_hash_relates_to_predecessor_difficulty() {
         let difficulty = 100u32;

src/models/blockchain/block/block_body.rs

@@ -1,8 +1,10 @@
-use arbitrary::Arbitrary;
+use std::sync::OnceLock;
+
 use get_size::GetSize;
 use serde::Deserialize;
 use serde::Serialize;
 use strum::EnumCount;
+use tasm_lib::triton_vm::prelude::Digest;
 use tasm_lib::twenty_first::math::b_field_element::BFieldElement;
 use tasm_lib::twenty_first::util_types::mmr::mmr_accumulator::MmrAccumulator;
 use twenty_first::math::bfield_codec::BFieldCodec;
@@ -27,7 +29,35 @@ impl HasDiscriminant for BlockBodyField {
     }
 }
 
-#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, BFieldCodec, GetSize, Arbitrary)]
+/// Public fields of `BlockBody` are read-only, enforced by #[readonly::make].
+/// Modifications are possible only through `BlockBody` methods.
+///
+// ## About the private `mast_hash` field:
+//
+// The `mast_hash` field represents the `BlockBody` MAST hash.  It is an
+// optimization so that the hash can be lazily computed at most once (per
+// modification). Without it, the PoW hash rate depends on the number of inputs
+// and outputs in a transaction. This caching of a hash value is similar to that
+// of `Block`.
+//
+// The field must be reset whenever the block body is modified.  As such, we
+// should not permit direct modification of internal fields.
+//
+// Therefore `[readonly::make]` is used to make public `BlockBody` fields read-
+// only (not mutable) outside of this module.  All methods that modify BlockBody
+// must reset this field.
+//
+// We manually implement `PartialEq` and `Eq` so that digest field will not be
+// compared.  Otherwise, we could have identical blocks except one has
+// initialized digest field and the other has not.
+//
+// The field should not be serialized, so it has the `#[serde(skip)]` attribute.
+// Upon deserialization, the field will have Digest::default() which is desired
+// so that the digest will be recomputed if/when hash() is called.
+//
+// We likewise skip the field for `BFieldCodec`, and `GetSize` because there
+// exist no impls for `OnceLock<_>` so derive fails.
+#[derive(Clone, Debug, Serialize, Deserialize, BFieldCodec, GetSize)]
 pub struct BlockBody {
     /// Every block contains exactly one transaction, which represents the merger of all
     /// broadcasted transactions that the miner decided to confirm.
@@ -49,8 +79,22 @@ pub struct BlockBody {
     /// lives on the line between the tip and genesis. This MMRA does not contain the
     /// current block.
     pub(crate) block_mmr_accumulator: MmrAccumulator,
+
+    // This caching ensures that the hash rate is independent of the size of
+    // the block's transaction.
+    #[serde(skip)]
+    #[bfield_codec(ignore)]
+    #[get_size(ignore)]
+    mast_hash: OnceLock<Digest>,
 }
 
+impl PartialEq for BlockBody {
+    fn eq(&self, other: &Self) -> bool {
+        self.mast_hash() == other.mast_hash()
+    }
+}
+impl Eq for BlockBody {}
+
 impl BlockBody {
     pub(crate) fn new(
         transaction_kernel: TransactionKernel,
@@ -63,6 +107,7 @@ impl BlockBody {
             mutator_set_accumulator,
             lock_free_mmr_accumulator,
             block_mmr_accumulator,
+            mast_hash: OnceLock::default(), // calc'd in mast_hash()
         }
     }
 }
@@ -78,6 +123,10 @@ impl MastHash for BlockBody {
             self.block_mmr_accumulator.encode(),
         ]
     }
+
+    fn mast_hash(&self) -> Digest {
+        *self.mast_hash.get_or_init(|| self.merkle_tree().root())
+    }
 }
 
 #[cfg(test)]
@@ -111,6 +160,7 @@ mod test {
                             mutator_set_accumulator: mutator_set_accumulator.clone(),
                             lock_free_mmr_accumulator,
                             block_mmr_accumulator,
+                            mast_hash: OnceLock::default(),
                         }
                     },
                 )