fix: better coinbase handling (see issue #4353) (#4386)

Description
--- 
The current coinbase handling relies on height and amount. If a match is not found for that height and amount, then it creates a new tx, deleting all the existing coinbases for that height. This is a problem, as miners can be working concurrently on a (to-be deleted) coinbase. This PR addresses this issue (see #4353).

Motivation and Context
--- 
Fixes #4353.

How Has This Been Tested?
--- 
Unit and cucumber tests

base_layer/core/src/transactions/coinbase_builder.rs

@@ -21,13 +21,15 @@
 // USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 //
 
-use rand::rngs::OsRng;
 use tari_common_types::types::{BlindingFactor, PrivateKey, PublicKey, Signature};
 use tari_crypto::{
     commitment::HomomorphicCommitmentFactory,
-    keys::{PublicKey as PK, SecretKey},
+    hash::blake2::Blake256,
+    hashing::DomainSeparatedHasher,
+    keys::PublicKey as PK,
 };
 use tari_script::{inputs, script, TariScript};
+use tari_utilities::ByteArray;
 use thiserror::Error;
 
 use crate::{
@@ -52,6 +54,7 @@ use crate::{
             UnblindedOutput,
         },
         transaction_protocol::{RewindData, TransactionMetadata},
+        types::WalletServiceHashingDomain,
     },
 };
 
@@ -73,6 +76,8 @@ pub enum CoinbaseBuildError {
     BuildError(String),
     #[error("Some inconsistent data was given to the builder. This transaction is not valid")]
     InvalidTransaction,
+    #[error("Unable to produce a spender offset key from spend key hash")]
+    InvalidSenderOffsetKey,
 }
 
 pub struct CoinbaseBuilder {
@@ -205,7 +210,13 @@ impl CoinbaseBuilder {
         let sig = Signature::sign(spending_key.clone(), nonce, &challenge)
             .map_err(|_| CoinbaseBuildError::BuildError("Challenge could not be represented as a scalar".into()))?;
 
-        let sender_offset_private_key = PrivateKey::random(&mut OsRng);
+        let hasher =
+            DomainSeparatedHasher::<Blake256, WalletServiceHashingDomain>::new_with_label("sender_offset_private_key");
+        let spending_key_hash = hasher.chain(spending_key.as_bytes()).finalize();
+        let spending_key_hash_bytes = spending_key_hash.as_ref();
+
+        let sender_offset_private_key =
+            PrivateKey::from_bytes(spending_key_hash_bytes).map_err(|_| CoinbaseBuildError::InvalidSenderOffsetKey)?;
         let sender_offset_public_key = PublicKey::from_secret_key(&sender_offset_private_key);
         let covenant = self.covenant;
 

base_layer/core/src/transactions/types.rs

@@ -19,3 +19,7 @@
 // SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
 // WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
 // USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+use tari_crypto::hash_domain;
+
+hash_domain!(WalletServiceHashingDomain, "base_layer.wallet.service");

base_layer/wallet/src/output_manager_service/service.rs

@@ -962,7 +962,7 @@ where
     }
 
     /// Request a Coinbase transaction for a specific block height. All existing pending transactions with
-    /// this blockheight will be cancelled.
+    /// the corresponding output hash will be cancelled.
     /// The key will be derived from the coinbase specific keychain using the blockheight as an index. The coinbase
     /// keychain is based on the wallets master_key and the "coinbase" branch.
     async fn get_coinbase_transaction(
@@ -1010,37 +1010,26 @@ where
             None,
         )?;
 
-        // Clear any existing pending coinbase transactions for this blockheight if they exist
-        match self
-            .resources
-            .db
-            .clear_pending_coinbase_transaction_at_block_height(block_height)
-        {
-            Ok(_) => {
-                debug!(
-                    target: LOG_TARGET,
-                    "An existing pending coinbase was cleared for block height {}", block_height
-                )
-            },
-            Err(e) => match e {
-                OutputManagerStorageError::DieselError(DieselError::NotFound) => {},
-                _ => return Err(OutputManagerError::from(e)),
+        // If there is no existing output available, we store the one we produced.
+        match self.resources.db.fetch_by_commitment(output.commitment.clone()) {
+            Ok(outs) => {
+                if outs.is_empty() {
+                    self.resources
+                        .db
+                        .add_output_to_be_received(tx_id, output, Some(block_height))?;
+
+                    self.confirm_encumberance(tx_id)?;
+                }
             },
-        };
+            Err(OutputManagerStorageError::ValueNotFound) => {
+                self.resources
+                    .db
+                    .add_output_to_be_received(tx_id, output, Some(block_height))?;
 
-        // Clear any matching outputs for this commitment. Even if the older output is valid
-        // we are losing no information as this output has the same commitment.
-        match self.resources.db.remove_output_by_commitment(output.commitment.clone()) {
-            Ok(_) => {},
-            Err(OutputManagerStorageError::ValueNotFound) => {},
+                self.confirm_encumberance(tx_id)?;
+            },
             Err(e) => return Err(e.into()),
-        }
-
-        self.resources
-            .db
-            .add_output_to_be_received(tx_id, output, Some(block_height))?;
-
-        self.confirm_encumberance(tx_id)?;
+        };
 
         Ok(tx)
     }

base_layer/wallet/src/output_manager_service/storage/database/backend.rs

@@ -96,11 +96,6 @@ pub trait OutputManagerBackend: Send + Sync + Clone {
     fn get_last_mined_output(&self) -> Result<Option<DbUnblindedOutput>, OutputManagerStorageError>;
     /// Get the output that was most recently spent, ordered descending by mined height
     fn get_last_spent_output(&self) -> Result<Option<DbUnblindedOutput>, OutputManagerStorageError>;
-    /// Check if there is a pending coinbase transaction at this block height, if there is clear it.
-    fn clear_pending_coinbase_transaction_at_block_height(
-        &self,
-        block_height: u64,
-    ) -> Result<(), OutputManagerStorageError>;
     /// Set if a coinbase output is abandoned or not
     fn set_coinbase_abandoned(&self, tx_id: TxId, abandoned: bool) -> Result<(), OutputManagerStorageError>;
     /// Reinstate a cancelled inbound output

base_layer/wallet/src/output_manager_service/storage/database/mod.rs

@@ -219,14 +219,6 @@ where T: OutputManagerBackend + 'static
         self.db.cancel_pending_transaction(tx_id)
     }
 
-    /// Check if there is a pending coinbase transaction at this block height, if there is clear it.
-    pub fn clear_pending_coinbase_transaction_at_block_height(
-        &self,
-        block_height: u64,
-    ) -> Result<(), OutputManagerStorageError> {
-        self.db.clear_pending_coinbase_transaction_at_block_height(block_height)
-    }
-
     pub fn fetch_all_unspent_outputs(&self) -> Result<Vec<DbUnblindedOutput>, OutputManagerStorageError> {
         let result = match self.db.fetch(&DbKey::UnspentOutputs)? {
             Some(DbValue::UnspentOutputs(outputs)) => outputs,
@@ -236,6 +228,18 @@ where T: OutputManagerBackend + 'static
         Ok(result)
     }
 
+    pub fn fetch_by_commitment(
+        &self,
+        commitment: Commitment,
+    ) -> Result<Vec<DbUnblindedOutput>, OutputManagerStorageError> {
+        let result = match self.db.fetch(&DbKey::AnyOutputByCommitment(commitment))? {
+            Some(DbValue::UnspentOutputs(outputs)) => outputs,
+            Some(other) => return unexpected_result(DbKey::UnspentOutputs, other),
+            None => vec![],
+        };
+        Ok(result)
+    }
+
     pub fn fetch_with_features(
         &self,
         feature: OutputType,

base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs

@@ -1074,30 +1074,6 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase {
         Ok(())
     }
 
-    fn clear_pending_coinbase_transaction_at_block_height(
-        &self,
-        block_height: u64,
-    ) -> Result<(), OutputManagerStorageError> {
-        let start = Instant::now();
-        let conn = self.database_connection.get_pooled_connection()?;
-        let acquire_lock = start.elapsed();
-
-        let output = OutputSql::find_pending_coinbase_at_block_height(block_height, &conn)?;
-
-        output.delete(&conn)?;
-        if start.elapsed().as_millis() > 0 {
-            trace!(
-                target: LOG_TARGET,
-                "sqlite profile - clear_pending_coinbase_transaction_at_block_height: lock {} + db_op {} = {} ms",
-                acquire_lock.as_millis(),
-                (start.elapsed() - acquire_lock).as_millis(),
-                start.elapsed().as_millis()
-            );
-        }
-
-        Ok(())
-    }
-
     fn set_coinbase_abandoned(&self, tx_id: TxId, abandoned: bool) -> Result<(), OutputManagerStorageError> {
         let start = Instant::now();
         let conn = self.database_connection.get_pooled_connection()?;

base_layer/wallet/tests/output_manager_service_tests/service.rs

@@ -1312,6 +1312,7 @@ async fn handle_coinbase_with_bulletproofs_rewinding() {
             .pending_incoming_balance,
         value1
     );
+
     let _tx2 = oms
         .output_manager_handle
         .get_coinbase_transaction(2u64.into(), reward2, fees2, 1)
@@ -1324,7 +1325,7 @@ async fn handle_coinbase_with_bulletproofs_rewinding() {
             .await
             .unwrap()
             .pending_incoming_balance,
-        value2
+        value1 + value2
     );
     let tx3 = oms
         .output_manager_handle
@@ -1338,7 +1339,7 @@ async fn handle_coinbase_with_bulletproofs_rewinding() {
             .await
             .unwrap()
             .pending_incoming_balance,
-        value2 + value3
+        value1 + value2 + value3
     );
 
     let output = tx3.body.outputs()[0].clone();

base_layer/wallet/tests/transaction_service_tests/service.rs

@@ -3059,7 +3059,7 @@ async fn test_restarting_transaction_protocols() {
 }
 
 #[tokio::test]
-async fn test_coinbase_transactions_rejection_same_height() {
+async fn test_coinbase_transactions_rejection_same_hash_but_accept_on_same_height() {
     let factories = CryptoFactories::default();
 
     let (connection, _temp_dir) = make_wallet_database_connection(None);
@@ -3105,7 +3105,35 @@ async fn test_coinbase_transactions_rejection_same_height() {
         fees1 + reward1
     );
 
-    // Create another coinbase Txn at the same block height; the previous one will be cancelled
+    // Create a second coinbase txn at the first block height, with same output hash as the previous one
+    // the previous one should be cancelled
+    let _tx1b = alice_ts_interface
+        .transaction_service_handle
+        .generate_coinbase_transaction(reward1, fees1, block_height_a)
+        .await
+        .unwrap();
+    let transactions = alice_ts_interface
+        .transaction_service_handle
+        .get_completed_transactions()
+        .await
+        .unwrap();
+    assert_eq!(transactions.len(), 1);
+    let _tx_id1b = transactions
+        .values()
+        .find(|tx| tx.amount == fees1 + reward1)
+        .unwrap()
+        .tx_id;
+    assert_eq!(
+        alice_ts_interface
+            .output_manager_service_handle
+            .get_balance()
+            .await
+            .unwrap()
+            .pending_incoming_balance,
+        fees1 + reward1
+    );
+
+    // Create another coinbase Txn at the same block height; the previous one should not be cancelled
     let _tx2 = alice_ts_interface
         .transaction_service_handle
         .generate_coinbase_transaction(reward2, fees2, block_height_a)
@@ -3129,10 +3157,10 @@ async fn test_coinbase_transactions_rejection_same_height() {
             .await
             .unwrap()
             .pending_incoming_balance,
-        fees2 + reward2
+        fees1 + reward1 + fees2 + reward2
     );
 
-    // Create a third coinbase Txn at the second block height; only the last two will be valid
+    // Create a third coinbase Txn at the second block height; all the three should be valid
     let _tx3 = alice_ts_interface
         .transaction_service_handle
         .generate_coinbase_transaction(reward3, fees3, block_height_b)
@@ -3156,7 +3184,7 @@ async fn test_coinbase_transactions_rejection_same_height() {
             .await
             .unwrap()
             .pending_incoming_balance,
-        fees2 + reward2 + fees3 + reward3
+        fees1 + reward1 + fees2 + reward2 + fees3 + reward3
     );
 
     assert!(!transactions.values().any(|tx| tx.amount == fees1 + reward1));
@@ -3241,7 +3269,7 @@ async fn test_coinbase_generation_and_monitoring() {
         fees1 + reward1 + fees2 + reward2
     );
 
-    // Take out a second one at the second height which should overwrite the initial one
+    // Take out a second one at the second height which should not overwrite the initial one
     let _tx2b = alice_ts_interface
         .transaction_service_handle
         .generate_coinbase_transaction(reward2, fees2b, block_height_b)
@@ -3265,7 +3293,7 @@ async fn test_coinbase_generation_and_monitoring() {
             .await
             .unwrap()
             .pending_incoming_balance,
-        fees1 + reward1 + fees2b + reward2
+        fees1 + reward1 + fees2b + reward2 + fees2 + reward2
     );
 
     assert!(transactions.values().any(|tx| tx.amount == fees1 + reward1));
@@ -3914,6 +3942,7 @@ async fn test_coinbase_transaction_reused_for_same_height() {
     for tx in transactions.values() {
         assert_eq!(tx.amount, fees1 + reward1);
     }
+    // balance should be fees1 + reward1, not double
     assert_eq!(
         ts_interface
             .output_manager_service_handle
@@ -3948,7 +3977,7 @@ async fn test_coinbase_transaction_reused_for_same_height() {
             .await
             .unwrap()
             .pending_incoming_balance,
-        fees2 + reward2
+        fees1 + reward1 + fees2 + reward2
     );
 
     // a requested coinbase transaction for a new height should be different
@@ -3975,7 +4004,7 @@ async fn test_coinbase_transaction_reused_for_same_height() {
             .await
             .unwrap()
             .pending_incoming_balance,
-        2 * (fees2 + reward2)
+        fees1 + reward1 + 2 * (fees2 + reward2)
     );
 }
 

integration_tests/features/WalletTransactions.feature

@@ -83,6 +83,19 @@ Feature: Wallet Transactions
     Then I wait for wallet WALLET_C to have at least 1000000 uT
     Then I check if last imported transactions are valid in wallet WALLET_C
 
+  Scenario: Wallet has two connected miners, coinbase's are computed correctly
+    Given I have a seed node NODE
+    And I have 1 base nodes connected to all seed nodes
+    And I have wallet WALLET_A connected to all seed nodes
+    And I have mining node MINER connected to base node NODE and wallet WALLET_A
+    And I have mining node MINER2 connected to base node NODE and wallet WALLET_A
+    When mining node MINER mines 2 blocks
+    When mining node MINER2 mines 2 blocks
+    When mining node MINER mines 3 blocks
+    When mining node MINER2 mines 3 blocks
+    Then all nodes are at height 10
+    Then I wait for wallet WALLET_A to have at least 20000000000 uT
+
   @flaky
   Scenario: Wallet imports spent outputs that become invalidated
     Given I have a seed node NODE