diff --git a/base_layer/core/src/transactions/coinbase_builder.rs b/base_layer/core/src/transactions/coinbase_builder.rs index 0b55b78e6d..8e8b6e9afe 100644 --- a/base_layer/core/src/transactions/coinbase_builder.rs +++ b/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::::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; diff --git a/base_layer/core/src/transactions/types.rs b/base_layer/core/src/transactions/types.rs index d051788bca..1ae2ff9d67 100644 --- a/base_layer/core/src/transactions/types.rs +++ b/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"); diff --git a/base_layer/wallet/src/output_manager_service/service.rs b/base_layer/wallet/src/output_manager_service/service.rs index 97bdfd64a3..ff231bf84f 100644 --- a/base_layer/wallet/src/output_manager_service/service.rs +++ b/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) } diff --git a/base_layer/wallet/src/output_manager_service/storage/database/backend.rs b/base_layer/wallet/src/output_manager_service/storage/database/backend.rs index 4527e1b561..e0576b57f8 100644 --- a/base_layer/wallet/src/output_manager_service/storage/database/backend.rs +++ b/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, OutputManagerStorageError>; /// Get the output that was most recently spent, ordered descending by mined height fn get_last_spent_output(&self) -> Result, 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 diff --git a/base_layer/wallet/src/output_manager_service/storage/database/mod.rs b/base_layer/wallet/src/output_manager_service/storage/database/mod.rs index 91c15e1bbb..d205ec0c8c 100644 --- a/base_layer/wallet/src/output_manager_service/storage/database/mod.rs +++ b/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, 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, 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, diff --git a/base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs b/base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs index 73af04cd9c..cf8f6df4cf 100644 --- a/base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs +++ b/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()?; diff --git a/base_layer/wallet/tests/output_manager_service_tests/service.rs b/base_layer/wallet/tests/output_manager_service_tests/service.rs index 0e50e831a6..20b34de2b6 100644 --- a/base_layer/wallet/tests/output_manager_service_tests/service.rs +++ b/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(); diff --git a/base_layer/wallet/tests/transaction_service_tests/service.rs b/base_layer/wallet/tests/transaction_service_tests/service.rs index 74518321c0..02a7a55505 100644 --- a/base_layer/wallet/tests/transaction_service_tests/service.rs +++ b/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) ); } diff --git a/integration_tests/features/WalletTransactions.feature b/integration_tests/features/WalletTransactions.feature index b2feea999e..ec66c0fcdc 100644 --- a/integration_tests/features/WalletTransactions.feature +++ b/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