fix(mine_loop): Avoid using different blocks in block composition (#267)

Prior to this fix, a race condition could lead to two different blocks
being used in the witness composition for a new block. This happened
since one block was input to the function, and inside the function to
construct the coinbase transaction, a block was read from global state.
With this fix, the block used as input argument is always used.

This closes #266.

src/mine_loop.rs

@@ -292,6 +292,7 @@ fn guess_nonce_iteration(
 }
 
 pub(crate) async fn make_coinbase_transaction(
+    latest_block: &Block,
     global_state_lock: &GlobalStateLock,
     guesser_block_subsidy_fraction: f64,
     timestamp: Timestamp,
@@ -304,19 +305,18 @@ pub(crate) async fn make_coinbase_transaction(
     //     we actually win the mining lottery.
     //  3. also this way we do not have to modify global/wallet state.
 
+    // It's important to use the input `latest_block` here instead of
+    // reading it from state, since that could, because of a race condition
+    // lead to an inconsistent witness higher up in the call graph. This is
+    // done to avoid holding a read-lock throughout this function.
+
     let coinbase_recipient_spending_key = global_state_lock
         .lock_guard()
         .await
         .wallet_state
         .wallet_secret
         .nth_generation_spending_key(0);
     let receiving_address = coinbase_recipient_spending_key.to_address();
-    let latest_block = global_state_lock
-        .lock_guard()
-        .await
-        .chain
-        .light_state()
-        .clone();
     let mutator_set_accumulator = latest_block.mutator_set_accumulator_after().clone();
     let next_block_height: BlockHeight = latest_block.header().height.next();
 
@@ -418,8 +418,13 @@ pub(crate) async fn create_block_transaction(
 ) -> Result<(Transaction, Vec<ExpectedUtxo>)> {
     let block_capacity_for_transactions = SIZE_20MB_IN_BYTES;
 
-    let (coinbase_transaction, composer_utxos) =
-        make_coinbase_transaction(global_state_lock, guesser_fee_fraction, timestamp).await?;
+    let (coinbase_transaction, composer_utxos) = make_coinbase_transaction(
+        predecessor_block,
+        global_state_lock,
+        guesser_fee_fraction,
+        timestamp,
+    )
+    .await?;
 
     debug!(
         "Creating block transaction with mutator set hash: {}",
@@ -840,10 +845,14 @@ pub(crate) mod mine_loop_tests {
         )
         .await;
         let tick = std::time::SystemTime::now();
-        let (transaction, _coinbase_utxo_info) =
-            make_coinbase_transaction(&global_state_lock, 0f64, network.launch_date())
-                .await
-                .unwrap();
+        let (transaction, _coinbase_utxo_info) = make_coinbase_transaction(
+            &genesis_block,
+            &global_state_lock,
+            0f64,
+            network.launch_date(),
+        )
+        .await
+        .unwrap();
 
         let in_seven_months = network.launch_date() + Timestamp::months(7);
         let block = Block::block_template_invalid_proof(
@@ -919,7 +928,7 @@ pub(crate) mod mine_loop_tests {
                 "Mempool must be empty at start of loop"
             );
             let (transaction_empty_mempool, _coinbase_utxo_info) = {
-                make_coinbase_transaction(&alice, guesser_fee_fraction, now)
+                make_coinbase_transaction(&genesis_block, &alice, guesser_fee_fraction, now)
                     .await
                     .unwrap()
             };
@@ -1069,7 +1078,7 @@ pub(crate) mod mine_loop_tests {
         let (worker_task_tx, worker_task_rx) = oneshot::channel::<NewBlockFound>();
 
         let (transaction, coinbase_utxo_info) =
-            make_coinbase_transaction(&global_state_lock, 0f64, launch_date)
+            make_coinbase_transaction(&tip_block_orig, &global_state_lock, 0f64, launch_date)
                 .await
                 .unwrap();
 
@@ -1132,7 +1141,7 @@ pub(crate) mod mine_loop_tests {
         let ten_seconds_ago = now - Timestamp::seconds(10);
 
         let (transaction, coinbase_utxo_info) =
-            make_coinbase_transaction(&global_state_lock, 0f64, ten_seconds_ago)
+            make_coinbase_transaction(&tip_block_orig, &global_state_lock, 0f64, ten_seconds_ago)
                 .await
                 .unwrap();
 

src/models/blockchain/block/mod.rs

@@ -1210,7 +1210,7 @@ mod block_tests {
 
             let guesser_fraction = 0f64;
             let (block_tx, _expected_utxo) =
-                make_coinbase_transaction(&genesis_state, guesser_fraction, now)
+                make_coinbase_transaction(&genesis_block, &genesis_state, guesser_fraction, now)
                     .await
                     .unwrap();
             let mut block1 = Block::make_block_template_with_valid_proof(

src/models/state/archival_state.rs

@@ -1760,10 +1760,20 @@ mod archival_state_tests {
         println!("Generated transaction for Alice and Bob.");
 
         let guesser_fraction = 0f64;
-        let (cbtx, expected_composer_utxos) =
-            make_coinbase_transaction(&genesis, guesser_fraction, in_seven_months)
+        let (cbtx, expected_composer_utxos) = make_coinbase_transaction(
+            &genesis
+                .global_state_lock
+                .lock_guard()
                 .await
-                .unwrap();
+                .chain
+                .light_state()
+                .clone(),
+            &genesis,
+            guesser_fraction,
+            in_seven_months,
+        )
+        .await
+        .unwrap();
 
         let block_tx = cbtx
             .merge_with(
@@ -1984,10 +1994,20 @@ mod archival_state_tests {
         // Make block_2 with tx that contains:
         // - 4 inputs: 2 from Alice and 2 from Bob
         // - 6 outputs: 2 from Alice to Genesis, 3 from Bob to Genesis, and 1 coinbase to Genesis
-        let (cbtx2, expected_composer_utxos2) =
-            make_coinbase_transaction(&genesis, guesser_fraction, in_seven_months)
+        let (cbtx2, expected_composer_utxos2) = make_coinbase_transaction(
+            &genesis
+                .global_state_lock
+                .lock_guard()
                 .await
-                .unwrap();
+                .chain
+                .light_state()
+                .clone(),
+            &genesis,
+            guesser_fraction,
+            in_seven_months,
+        )
+        .await
+        .unwrap();
         let block_tx2 = cbtx2
             .merge_with(
                 tx_from_alice,

src/models/state/mempool.rs

@@ -1194,10 +1194,19 @@ mod tests {
 
         // Create next block which includes Bob's, but not Alice's, transaction.
         let guesser_fraction = 0f64;
-        let (coinbase_transaction, _expected_utxo) =
-            make_coinbase_transaction(&bob, guesser_fraction, in_eight_months)
+        let (coinbase_transaction, _expected_utxo) = make_coinbase_transaction(
+            &bob.global_state_lock
+                .lock_guard()
                 .await
-                .unwrap();
+                .chain
+                .light_state()
+                .clone(),
+            &bob,
+            guesser_fraction,
+            in_eight_months,
+        )
+        .await
+        .unwrap();
         let block_transaction = tx_by_bob
             .merge_with(
                 coinbase_transaction,
@@ -1264,9 +1273,20 @@ mod tests {
 
         tx_by_alice_updated = mempool.get_transactions_for_block(usize::MAX, None, true)[0].clone();
         let block_5_timestamp = previous_block.header().timestamp + Timestamp::hours(1);
-        let (cbtx, _eutxo) = make_coinbase_transaction(&alice, guesser_fraction, block_5_timestamp)
-            .await
-            .unwrap();
+        let (cbtx, _eutxo) = make_coinbase_transaction(
+            &alice
+                .global_state_lock
+                .lock_guard()
+                .await
+                .chain
+                .light_state()
+                .clone(),
+            &alice,
+            guesser_fraction,
+            block_5_timestamp,
+        )
+        .await
+        .unwrap();
         let block_tx_5 = cbtx
             .merge_with(
                 tx_by_alice_updated,

src/models/state/mod.rs

@@ -2267,10 +2267,14 @@ mod global_state_tests {
         let in_eight_months = in_seven_months + Timestamp::months(1);
 
         let guesser_fraction = 0f64;
-        let (coinbase_transaction, coinbase_expected_utxos) =
-            make_coinbase_transaction(&premine_receiver, guesser_fraction, in_seven_months)
-                .await
-                .unwrap();
+        let (coinbase_transaction, coinbase_expected_utxos) = make_coinbase_transaction(
+            &genesis_block,
+            &premine_receiver,
+            guesser_fraction,
+            in_seven_months,
+        )
+        .await
+        .unwrap();
 
         // Send two outputs each to Alice and Bob, from genesis receiver
         let sender_randomness: Digest = rng.gen();
@@ -2548,10 +2552,20 @@ mod global_state_tests {
         // Make block_2 with tx that contains:
         // - 4 inputs: 2 from Alice and 2 from Bob
         // - 6 outputs: 2 from Alice to Genesis, 3 from Bob to Genesis, and 1 coinbase
-        let (coinbase_transaction2, _expected_utxo) =
-            make_coinbase_transaction(&premine_receiver, guesser_fraction, in_seven_months)
+        let (coinbase_transaction2, _expected_utxo) = make_coinbase_transaction(
+            &premine_receiver
+                .global_state_lock
+                .lock_guard()
                 .await
-                .unwrap();
+                .chain
+                .light_state()
+                .clone(),
+            &premine_receiver,
+            guesser_fraction,
+            in_seven_months,
+        )
+        .await
+        .unwrap();
 
         let block_transaction2 = coinbase_transaction2
             .merge_with(
@@ -2604,9 +2618,10 @@ mod global_state_tests {
         let now = genesis_block.kernel.header.timestamp + Timestamp::hours(1);
 
         let guesser_fraction = 0f64;
-        let (cb, _) = make_coinbase_transaction(&global_state_lock, guesser_fraction, now)
-            .await
-            .unwrap();
+        let (cb, _) =
+            make_coinbase_transaction(&genesis_block, &global_state_lock, guesser_fraction, now)
+                .await
+                .unwrap();
         let block_1 = Block::compose(
             &genesis_block,
             cb,
@@ -2651,9 +2666,14 @@ mod global_state_tests {
         ) -> Block {
             let genesis_block = Block::genesis_block(global_state_lock.cli().network);
             let timestamp = genesis_block.header().timestamp + Timestamp::hours(1);
-            let (cb, _) = make_coinbase_transaction(global_state_lock, guesser_fraction, timestamp)
-                .await
-                .unwrap();
+            let (cb, _) = make_coinbase_transaction(
+                &genesis_block,
+                global_state_lock,
+                guesser_fraction,
+                timestamp,
+            )
+            .await
+            .unwrap();
 
             Block::compose(
                 &genesis_block,

src/models/state/wallet/mod.rs

@@ -1220,6 +1220,13 @@ mod wallet_tests {
 
         let guesser_fraction = 0f64;
         let (coinbase_tx, expected_composer_utxos) = make_coinbase_transaction(
+            &alice
+                .global_state_lock
+                .lock_guard()
+                .await
+                .chain
+                .light_state()
+                .clone(),
             &alice,
             guesser_fraction,
             block_2_b.header().timestamp + MINIMUM_BLOCK_TIME,
@@ -1392,10 +1399,19 @@ mod wallet_tests {
         let mut rng = StdRng::seed_from_u64(87255549301u64);
 
         let guesser_fraction = 0f64;
-        let (cbtx, _cb_expected) =
-            make_coinbase_transaction(&bob, guesser_fraction, in_seven_months)
+        let (cbtx, _cb_expected) = make_coinbase_transaction(
+            &bob.global_state_lock
+                .lock_guard()
                 .await
-                .unwrap();
+                .chain
+                .light_state()
+                .clone(),
+            &bob,
+            guesser_fraction,
+            in_seven_months,
+        )
+        .await
+        .unwrap();
         let one_money: NeptuneCoins = NeptuneCoins::new(1);
         let anyone_can_spend_utxo =
             Utxo::new_native_currency(LockScript::anyone_can_spend(), one_money);

src/tests/shared.rs

@@ -876,7 +876,7 @@ pub(crate) async fn valid_block_for_tests(
     guesser_fraction: f64,
 ) -> Block {
     let current_tip = state_lock.lock_guard().await.chain.light_state().clone();
-    let (cb, _) = make_coinbase_transaction(state_lock, guesser_fraction, timestamp)
+    let (cb, _) = make_coinbase_transaction(&current_tip, state_lock, guesser_fraction, timestamp)
         .await
         .unwrap();
     valid_block_from_tx_for_tests(&current_tip, cb, seed).await