From a4fabcb8c6199aae26ec932f370d641973771584 Mon Sep 17 00:00:00 2001 From: SW van Heerden Date: Wed, 1 Nov 2023 14:27:57 +0200 Subject: [PATCH] chore: code review (#5881) Description --- final hazop code review for SMT --- .../core/src/chain_storage/lmdb_db/lmdb_db.rs | 62 ++++++++++++------- .../core/src/chain_storage/lmdb_db/mod.rs | 8 +-- .../transaction_components/test.rs | 9 +++ .../transaction_input.rs | 27 ++++++++ base_layer/core/src/validation/helpers.rs | 2 +- .../tasks/txo_validation_task.rs | 57 +++++++++++------ .../tests/features/Propagation.feature | 2 +- integration_tests/tests/features/Sync.feature | 4 +- 8 files changed, 120 insertions(+), 51 deletions(-) diff --git a/base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs b/base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs index 56104517dd..01ee63293f 100644 --- a/base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs +++ b/base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs @@ -222,7 +222,7 @@ pub struct LMDBDatabase { /// Maps -> (block_hash, output_hash) /// and -> output_hash contract_index: DatabaseRef, - /// Maps output_mmr_pos -> + /// Maps output hash-> deleted_txo_hash_to_header_index: DatabaseRef, /// Maps block_hash -> Block orphans_db: DatabaseRef, @@ -630,15 +630,16 @@ impl LMDBDatabase { // does the key need to include the mmr pos // make index safe key let hash = input.canonical_hash(); + let output_hash = input.output_hash(); + let key = InputKey::new(header_hash, &hash)?; lmdb_insert( txn, &self.deleted_txo_hash_to_header_index, - &hash.to_vec(), - &(height, header_hash), + &output_hash.to_vec(), + &(key.clone().convert_to_comp_key().to_vec()), "deleted_txo_hash_to_header_index", )?; - let key = InputKey::new(header_hash, &hash)?; lmdb_insert( txn, &self.inputs_db, @@ -646,8 +647,8 @@ impl LMDBDatabase { &TransactionInputRowDataRef { input: &input.to_compact(), header_hash, - mined_timestamp: header_timestamp, - height, + spent_timestamp: header_timestamp, + spent_height: height, hash: &hash, }, "inputs_db", @@ -911,7 +912,7 @@ impl LMDBDatabase { lmdb_delete( txn, &self.deleted_txo_hash_to_header_index, - &row.hash.to_vec(), + &output_hash.to_vec(), "deleted_txo_hash_to_header_index", )?; if output_rows.iter().any(|r| r.hash == output_hash) { @@ -945,7 +946,7 @@ impl LMDBDatabase { utxo_mined_info.output.minimum_value_promise, ); let smt_key = NodeKey::try_from(input.commitment()?.as_bytes())?; - let smt_node = ValueHash::try_from(input.smt_hash(row.height).as_slice())?; + let smt_node = ValueHash::try_from(input.smt_hash(row.spent_height).as_slice())?; output_smt.insert(smt_key, smt_node)?; trace!(target: LOG_TARGET, "Input moved to UTXO set: {}", input); @@ -1081,6 +1082,12 @@ impl LMDBDatabase { header: &BlockHeader, body: AggregateBody, ) -> Result<(), ChainStorageError> { + if self.fetch_block_accumulated_data(txn, header.height + 1)?.is_some() { + return Err(ChainStorageError::InvalidOperation(format!( + "Attempted to insert block at height {} while next block already exists", + header.height + ))); + } let block_hash = header.hash(); debug!( target: LOG_TARGET, @@ -1494,27 +1501,27 @@ impl LMDBDatabase { fn fetch_input_in_txn( &self, txn: &ConstTransaction<'_>, - input_hash: &[u8], + output_hash: &[u8], ) -> Result, ChainStorageError> { - if let Some(key) = lmdb_get::<_, Vec>(txn, &self.deleted_txo_hash_to_header_index, input_hash)? { + if let Some(key) = lmdb_get::<_, Vec>(txn, &self.deleted_txo_hash_to_header_index, output_hash)? { debug!( target: LOG_TARGET, "Fetch input: {} Found ({})", - to_hex(input_hash), + to_hex(output_hash), key.to_hex() ); - match lmdb_get::<_, TransactionInputRowData>(txn, &self.utxos_db, &key)? { + match lmdb_get::<_, TransactionInputRowData>(txn, &self.inputs_db, &key)? { Some(TransactionInputRowData { input: i, - height, + spent_height: height, header_hash, - mined_timestamp, + spent_timestamp, .. }) => Ok(Some(InputMinedInfo { input: i, spent_height: height, header_hash, - spent_timestamp: mined_timestamp, + spent_timestamp, })), _ => Ok(None), @@ -1523,7 +1530,7 @@ impl LMDBDatabase { debug!( target: LOG_TARGET, "Fetch input: {} NOT found in index", - to_hex(input_hash) + to_hex(output_hash) ); Ok(None) } @@ -1835,7 +1842,7 @@ impl BlockchainBackend for LMDBDatabase { ) -> Result, ChainStorageError> { let txn = self.read_transaction()?; - let mut utxos: Vec<(TransactionOutput, bool)> = + let mut outputs: Vec<(TransactionOutput, bool)> = lmdb_fetch_matching_after::(&txn, &self.utxos_db, header_hash.deref())? .into_iter() .map(|row| (row.output, false)) @@ -1848,20 +1855,27 @@ impl BlockchainBackend for LMDBDatabase { field: "hash", value: header.to_hex(), })?; - for utxo in &mut utxos { - let hash = utxo.0.hash(); - if let Some((height, _)) = - lmdb_get::<_, (u64, Vec)>(&txn, &self.deleted_txo_hash_to_header_index, hash.as_slice())? + for output in &mut outputs { + let hash = output.0.hash(); + if let Some(key) = + lmdb_get::<_, Vec>(&txn, &self.deleted_txo_hash_to_header_index, hash.as_slice())? { - if height <= header_height { + let input = lmdb_get::<_, TransactionInputRowData>(&txn, &self.inputs_db, &key)?.ok_or( + ChainStorageError::ValueNotFound { + entity: "input", + field: "hash", + value: header.to_hex(), + }, + )?; + if input.spent_height <= header_height { // we know its spend at the header height specified as optional in the fn - utxo.1 = true; + output.1 = true; } } } } - Ok(utxos) + Ok(outputs) } fn fetch_output(&self, output_hash: &HashOutput) -> Result, ChainStorageError> { diff --git a/base_layer/core/src/chain_storage/lmdb_db/mod.rs b/base_layer/core/src/chain_storage/lmdb_db/mod.rs index b2136f2c0b..c710b402e5 100644 --- a/base_layer/core/src/chain_storage/lmdb_db/mod.rs +++ b/base_layer/core/src/chain_storage/lmdb_db/mod.rs @@ -51,8 +51,8 @@ pub(crate) struct TransactionInputRowDataRef<'a> { pub input: &'a TransactionInput, #[allow(clippy::ptr_arg)] pub header_hash: &'a HashOutput, - pub mined_timestamp: u64, - pub height: u64, + pub spent_timestamp: u64, + pub spent_height: u64, #[allow(clippy::ptr_arg)] pub hash: &'a HashOutput, } @@ -61,8 +61,8 @@ pub(crate) struct TransactionInputRowDataRef<'a> { pub(crate) struct TransactionInputRowData { pub input: TransactionInput, pub header_hash: HashOutput, - pub mined_timestamp: u64, - pub height: u64, + pub spent_timestamp: u64, + pub spent_height: u64, pub hash: HashOutput, } diff --git a/base_layer/core/src/transactions/transaction_components/test.rs b/base_layer/core/src/transactions/transaction_components/test.rs index 37c093ceb1..dfb5a993af 100644 --- a/base_layer/core/src/transactions/transaction_components/test.rs +++ b/base_layer/core/src/transactions/transaction_components/test.rs @@ -69,6 +69,15 @@ async fn input_and_output_and_wallet_output_hash_match() { assert_eq!(output.hash(), i.hash(&key_manager).await.unwrap()); } +#[test] +fn test_smt_hashes() { + let input = TransactionInput::default(); + let output = TransactionOutput::default(); + let input_hash = input.smt_hash(10); + let output_hash = output.smt_hash(10); + assert_eq!(input_hash, output_hash); +} + #[tokio::test] async fn key_manager_input() { let key_manager = create_test_core_key_manager_with_memory_db(); diff --git a/base_layer/core/src/transactions/transaction_components/transaction_input.rs b/base_layer/core/src/transactions/transaction_components/transaction_input.rs index ea14498bca..3161445c9a 100644 --- a/base_layer/core/src/transactions/transaction_components/transaction_input.rs +++ b/base_layer/core/src/transactions/transaction_components/transaction_input.rs @@ -561,6 +561,14 @@ impl Ord for TransactionInput { } } +impl Default for TransactionInput { + fn default() -> Self { + let output = SpentOutput::create_from_output(TransactionOutput::default()); + + TransactionInput::new_current_version(output, ExecutionStack::default(), Default::default()) + } +} + #[derive(Clone, Debug, Serialize, Deserialize, BorshSerialize, BorshDeserialize)] #[allow(clippy::large_enum_variant)] pub enum SpentOutput { @@ -587,4 +595,23 @@ impl SpentOutput { SpentOutput::OutputData { .. } => 1, } } + + pub fn create_from_output(output: TransactionOutput) -> SpentOutput { + let rp_hash = match output.proof { + Some(proof) => proof.hash(), + None => FixedHash::zero(), + }; + SpentOutput::OutputData { + version: output.version, + features: output.features, + commitment: output.commitment, + script: output.script, + sender_offset_public_key: output.sender_offset_public_key, + covenant: output.covenant, + encrypted_data: output.encrypted_data, + metadata_signature: output.metadata_signature, + rangeproof_hash: rp_hash, + minimum_value_promise: output.minimum_value_promise, + } + } } diff --git a/base_layer/core/src/validation/helpers.rs b/base_layer/core/src/validation/helpers.rs index b5f9251fa5..9594595eca 100644 --- a/base_layer/core/src/validation/helpers.rs +++ b/base_layer/core/src/validation/helpers.rs @@ -280,7 +280,7 @@ pub fn check_mmr_roots(header: &BlockHeader, mmr_roots: &MmrRoots) -> Result<(), mmr_roots.output_smt_size ); return Err(ValidationError::BlockError(BlockValidationError::MismatchedMmrSize { - mmr_tree: "Utxo".to_string(), + mmr_tree: "UTXO".to_string(), expected: mmr_roots.output_smt_size, actual: header.output_smt_size, })); diff --git a/base_layer/wallet/src/output_manager_service/tasks/txo_validation_task.rs b/base_layer/wallet/src/output_manager_service/tasks/txo_validation_task.rs index 0d9d231c66..419249ef8d 100644 --- a/base_layer/wallet/src/output_manager_service/tasks/txo_validation_task.rs +++ b/base_layer/wallet/src/output_manager_service/tasks/txo_validation_task.rs @@ -61,6 +61,13 @@ pub struct TxoValidationTask { config: OutputManagerServiceConfig, } +struct MinedOutputInfo { + output: DbWalletOutput, + mined_at_height: u64, + mined_block_hash: FixedHash, + mined_timestamp: u64, +} + impl TxoValidationTask where TBackend: OutputManagerBackend + 'static, @@ -151,19 +158,25 @@ where unmined.len(), self.operation_id ); - for (output, mined_height, mined_in_block, mined_timestamp) in &mined { + for mined_info in &mined { info!( target: LOG_TARGET, "Updating output comm:{}: hash {} as mined at height {} with current tip at {} (Operation ID: {})", - output.commitment.to_hex(), - output.hash.to_hex(), - mined_height, + mined_info.output.commitment.to_hex(), + mined_info.output.hash.to_hex(), + mined_info.mined_at_height, tip_height, self.operation_id ); - self.update_output_as_mined(output, mined_in_block, *mined_height, tip_height, *mined_timestamp) - .await?; + self.update_output_as_mined( + &mined_info.output, + &mined_info.mined_block_hash, + mined_info.mined_at_height, + tip_height, + mined_info.mined_timestamp, + ) + .await?; } for output in unmined { self.db @@ -289,18 +302,24 @@ where unmined.len(), self.operation_id ); - for (output, mined_height, mined_in_block, mined_timestamp) in &mined { + for mined_info in &mined { info!( target: LOG_TARGET, "Updating output comm:{}: hash {} as mined at height {} with current tip at {} (Operation ID: {})", - output.commitment.to_hex(), - output.hash.to_hex(), - mined_height, + mined_info.output.commitment.to_hex(), + mined_info.output.hash.to_hex(), + mined_info.mined_at_height, tip_height, self.operation_id ); - self.update_output_as_mined(output, mined_in_block, *mined_height, tip_height, *mined_timestamp) - .await?; + self.update_output_as_mined( + &mined_info.output, + &mined_info.mined_block_hash, + mined_info.mined_at_height, + tip_height, + mined_info.mined_timestamp, + ) + .await?; } } @@ -459,7 +478,7 @@ where &self, batch: &[DbWalletOutput], base_node_client: &mut BaseNodeWalletRpcClient, - ) -> Result<(Vec<(DbWalletOutput, u64, BlockHash, u64)>, Vec, u64), OutputManagerError> { + ) -> Result<(Vec, Vec, u64), OutputManagerError> { let batch_hashes = batch.iter().map(|o| o.hash.to_vec()).collect(); trace!( target: LOG_TARGET, @@ -494,12 +513,12 @@ where for output in batch { if let Some(returned_output) = returned_outputs.get(&output.hash) { match returned_output.mined_in_block.clone().try_into() { - Ok(block_hash) => mined.push(( - output.clone(), - returned_output.mined_at_height, - block_hash, - returned_output.mined_timestamp, - )), + Ok(block_hash) => mined.push(MinedOutputInfo { + output: output.clone(), + mined_at_height: returned_output.mined_at_height, + mined_block_hash: block_hash, + mined_timestamp: returned_output.mined_timestamp, + }), Err(_) => { warn!( target: LOG_TARGET, diff --git a/integration_tests/tests/features/Propagation.feature b/integration_tests/tests/features/Propagation.feature index 8ffddc9aa6..a10112a7bd 100644 --- a/integration_tests/tests/features/Propagation.feature +++ b/integration_tests/tests/features/Propagation.feature @@ -88,7 +88,7 @@ Feature: Block Propagation Then node MINER is at height 7 Then all nodes are at height 7 - @critical @pruned + @critical @pruned @broken @broken_prune Scenario: Pruned node should prune outputs Given I have 1 seed nodes When I have a base node SENDER connected to all seed nodes diff --git a/integration_tests/tests/features/Sync.feature b/integration_tests/tests/features/Sync.feature index 0ff855b85f..7714374add 100644 --- a/integration_tests/tests/features/Sync.feature +++ b/integration_tests/tests/features/Sync.feature @@ -46,7 +46,7 @@ Feature: Block Sync When I have a base node NODE2 connected to all seed nodes Then all nodes are at height 25 - @critical @pruned + @critical @pruned @broken @broken_prune Scenario: Pruned mode simple sync Given I have 1 seed nodes When I have a SHA3 miner NODE1 connected to all seed nodes @@ -57,7 +57,7 @@ Feature: Block Sync When I have a pruned node PNODE1 connected to node NODE1 with pruning horizon set to 5 Then all nodes are at height 20 - @critical @pruned + @critical @pruned @broken @broken_prune Scenario: Pruned node should handle burned output Given I have a seed node NODE When I have a base node NODE1 connected to all seed nodes