From 59a7fcc24c61a63d2d532c4e7ece9c7bc1cbcf2e Mon Sep 17 00:00:00 2001 From: yito88 Date: Wed, 12 Jul 2023 23:07:34 +0200 Subject: [PATCH 1/3] fix changes in finalize_block --- .../lib/node/ledger/shell/finalize_block.rs | 37 +++++++++++-------- core/src/ledger/storage/wl_storage.rs | 7 +++- 2 files changed, 27 insertions(+), 17 deletions(-) diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index 3af8ab9e3f..4485ebaccb 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -201,7 +201,6 @@ where let tx_hash_key = replay_protection::get_tx_hash_key(&tx_hash); self.wl_storage - .storage .delete(&tx_hash_key) .expect("Error while deleting tx hash from storage"); } @@ -221,16 +220,14 @@ where processed_tx.header_hash().0, )); self.wl_storage - .storage - .write(&wrapper_tx_hash_key, vec![]) + .write_bytes(&wrapper_tx_hash_key, vec![]) .expect("Error while writing tx hash to storage"); let inner_tx_hash_key = replay_protection::get_tx_hash_key( &tx.clone().update_header(TxType::Raw).header_hash(), ); self.wl_storage - .storage - .write(&inner_tx_hash_key, vec![]) + .write_bytes(&inner_tx_hash_key, vec![]) .expect("Error while writing tx hash to storage"); #[cfg(not(feature = "mainnet"))] @@ -256,11 +253,7 @@ where match balance.checked_sub(wrapper_fees) { Some(amount) => { self.wl_storage - .storage - .write( - &balance_key, - amount.try_to_vec().unwrap(), - ) + .write(&balance_key, amount) .unwrap(); } None => { @@ -271,12 +264,9 @@ where if reject { // Burn remaining funds self.wl_storage - .storage .write( &balance_key, - Amount::native_whole(0) - .try_to_vec() - .unwrap(), + Amount::native_whole(0), ) .unwrap(); tx_event["info"] = @@ -481,6 +471,7 @@ where ); stats.increment_errored_txs(); + self.wl_storage.drop_tx(); // If transaction type is Decrypted and failed because of // out of gas, remove its hash from storage to allow // rewrapping it @@ -491,15 +482,16 @@ where let tx_hash_key = replay_protection::get_tx_hash_key(&hash); self.wl_storage - .storage .delete(&tx_hash_key) .expect( "Error while deleting tx hash key from storage", ); + // Apply only to remove its hash, + // since all other changes have already been dropped + self.wl_storage.commit_tx(); } } - self.wl_storage.drop_tx(); tx_event["gas_used"] = self .gas_meter .get_current_transaction_gas() @@ -1834,7 +1826,14 @@ mod test_finalize_block { votes: votes.clone(), ..Default::default() }; + // merkle tree root before finalize_block + let root_pre = shell.shell.wl_storage.storage.block.tree.root(); + let _events = shell.finalize_block(req).unwrap(); + + // the merkle tree root should not change after finalize_block + let root_post = shell.shell.wl_storage.storage.block.tree.root(); + assert_eq!(root_pre.0, root_post.0); let new_state = store_block_state(&shell); // The new state must be unchanged itertools::assert_equal( @@ -2226,6 +2225,8 @@ mod test_finalize_block { }, }; shell.enqueue_tx(wrapper_tx); + // merkle tree root before finalize_block + let root_pre = shell.shell.wl_storage.storage.block.tree.root(); let _event = &shell .finalize_block(FinalizeBlock { @@ -2234,6 +2235,10 @@ mod test_finalize_block { }) .expect("Test failed")[0]; + // the merkle tree root should not change after finalize_block + let root_post = shell.shell.wl_storage.storage.block.tree.root(); + assert_eq!(root_pre.0, root_post.0); + // FIXME: uncomment when proper gas metering is in place // // Check inner tx hash has been removed from storage // assert_eq!(event.event_type.to_string(), String::from("applied")); diff --git a/core/src/ledger/storage/wl_storage.rs b/core/src/ledger/storage/wl_storage.rs index 1cb7e56a27..4fb2490ab9 100644 --- a/core/src/ledger/storage/wl_storage.rs +++ b/core/src/ledger/storage/wl_storage.rs @@ -143,6 +143,12 @@ where /// Commit the current block's write log to the storage and commit the block /// to DB. Starts a new block write log. pub fn commit_block(&mut self) -> storage_api::Result<()> { + if self.storage.last_epoch != self.storage.block.epoch { + self.storage + .update_epoch_in_merkle_tree() + .into_storage_result()?; + } + let mut batch = D::batch(); self.write_log .commit_block(&mut self.storage, &mut batch) @@ -205,7 +211,6 @@ where .new_epoch(height, evidence_max_age_num_blocks); tracing::info!("Began a new epoch {}", self.storage.block.epoch); } - self.storage.update_epoch_in_merkle_tree()?; Ok(new_epoch) } } From f1669e7cb7724544edec36ffd30756cb289aaf16 Mon Sep 17 00:00:00 2001 From: yito88 Date: Wed, 12 Jul 2023 23:12:38 +0200 Subject: [PATCH 2/3] add changelog --- .../unreleased/bug-fixes/1709-fix_changes_before_commit.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/1709-fix_changes_before_commit.md diff --git a/.changelog/unreleased/bug-fixes/1709-fix_changes_before_commit.md b/.changelog/unreleased/bug-fixes/1709-fix_changes_before_commit.md new file mode 100644 index 0000000000..33eb543193 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/1709-fix_changes_before_commit.md @@ -0,0 +1,2 @@ +- Fix inconsistency state before commit + ([\#1709](https://github.com/anoma/namada/issues/1709)) \ No newline at end of file From d87df2a56a8f93d8a25667b7d2c443ad32cd247a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Thu, 13 Jul 2023 09:01:30 +0100 Subject: [PATCH 3/3] test/shell/finalize_block: add some txs to DB commit test --- .../lib/node/ledger/shell/finalize_block.rs | 213 ++++++++++-------- 1 file changed, 117 insertions(+), 96 deletions(-) diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index 4485ebaccb..e32b4db993 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -1018,6 +1018,85 @@ mod test_finalize_block { FinalizeBlock, ProcessedTx, }; + /// Make a wrapper tx and a processed tx from the wrapped tx that can be + /// added to `FinalizeBlock` request. + fn mk_wrapper_tx( + shell: &TestShell, + keypair: &common::SecretKey, + ) -> (Tx, ProcessedTx) { + let mut wrapper_tx = + Tx::new(TxType::Wrapper(Box::new(WrapperTx::new( + Fee { + amount: MIN_FEE_AMOUNT, + token: shell.wl_storage.storage.native_token.clone(), + }, + keypair.ref_to(), + Epoch(0), + Default::default(), + #[cfg(not(feature = "mainnet"))] + None, + )))); + wrapper_tx.header.chain_id = shell.chain_id.clone(); + wrapper_tx.set_code(Code::new("wasm_code".as_bytes().to_owned())); + wrapper_tx.set_data(Data::new( + "Encrypted transaction data".as_bytes().to_owned(), + )); + wrapper_tx.add_section(Section::Signature(Signature::new( + wrapper_tx.sechashes(), + keypair, + ))); + let tx = wrapper_tx.to_bytes(); + ( + wrapper_tx, + ProcessedTx { + tx, + result: TxResult { + code: ErrorCodes::Ok.into(), + info: "".into(), + }, + }, + ) + } + + /// Make a wrapper tx and a processed tx from the wrapped tx that can be + /// added to `FinalizeBlock` request. + fn mk_decrypted_tx( + shell: &mut TestShell, + keypair: &common::SecretKey, + ) -> ProcessedTx { + let tx_code = TestWasms::TxNoOp.read_bytes(); + let mut outer_tx = Tx::new(TxType::Wrapper(Box::new(WrapperTx::new( + Fee { + amount: MIN_FEE_AMOUNT, + token: shell.wl_storage.storage.native_token.clone(), + }, + keypair.ref_to(), + Epoch(0), + Default::default(), + #[cfg(not(feature = "mainnet"))] + None, + )))); + outer_tx.header.chain_id = shell.chain_id.clone(); + outer_tx.set_code(Code::new(tx_code)); + outer_tx.set_data(Data::new( + "Decrypted transaction data".as_bytes().to_owned(), + )); + shell.enqueue_tx(outer_tx.clone()); + outer_tx.update_header(TxType::Decrypted(DecryptedTx::Decrypted { + #[cfg(not(feature = "mainnet"))] + has_valid_pow: false, + })); + outer_tx.decrypt(::G2Affine::prime_subgroup_generator()) + .expect("Test failed"); + ProcessedTx { + tx: outer_tx.to_bytes(), + result: TxResult { + code: ErrorCodes::Ok.into(), + info: "".into(), + }, + } + } + /// Check that if a wrapper tx was rejected by [`process_proposal`], /// check that the correct event is returned. Check that it does /// not appear in the queue of txs to be decrypted @@ -1044,36 +1123,11 @@ mod test_finalize_block { // create some wrapper txs for i in 1u64..5 { - let mut wrapper = - Tx::new(TxType::Wrapper(Box::new(WrapperTx::new( - Fee { - amount: MIN_FEE_AMOUNT, - token: shell.wl_storage.storage.native_token.clone(), - }, - keypair.ref_to(), - Epoch(0), - Default::default(), - #[cfg(not(feature = "mainnet"))] - None, - )))); - wrapper.header.chain_id = shell.chain_id.clone(); - wrapper.set_data(Data::new("wasm_code".as_bytes().to_owned())); - wrapper.set_code(Code::new( - format!("transaction data: {}", i).as_bytes().to_owned(), - )); - wrapper.add_section(Section::Signature(Signature::new( - wrapper.sechashes(), - &keypair, - ))); + let (wrapper, mut processed_tx) = mk_wrapper_tx(&shell, &keypair); if i > 1 { - processed_txs.push(ProcessedTx { - tx: wrapper.to_bytes(), - result: TxResult { - code: u32::try_from(i.rem_euclid(2)) - .expect("Test failed"), - info: "".into(), - }, - }); + processed_tx.result.code = + u32::try_from(i.rem_euclid(2)).unwrap(); + processed_txs.push(processed_tx); } else { shell.enqueue_tx(wrapper.clone()); } @@ -1239,75 +1293,14 @@ mod test_finalize_block { .unwrap(); // create two decrypted txs - let tx_code = TestWasms::TxNoOp.read_bytes(); - for i in 0..2 { - let mut outer_tx = - Tx::new(TxType::Wrapper(Box::new(WrapperTx::new( - Fee { - amount: MIN_FEE_AMOUNT, - token: shell.wl_storage.storage.native_token.clone(), - }, - keypair.ref_to(), - Epoch(0), - Default::default(), - #[cfg(not(feature = "mainnet"))] - None, - )))); - outer_tx.header.chain_id = shell.chain_id.clone(); - outer_tx.set_code(Code::new(tx_code.clone())); - outer_tx.set_data(Data::new( - format!("Decrypted transaction data: {}", i) - .as_bytes() - .to_owned(), - )); - shell.enqueue_tx(outer_tx.clone()); - outer_tx.update_header(TxType::Decrypted(DecryptedTx::Decrypted { - #[cfg(not(feature = "mainnet"))] - has_valid_pow: false, - })); - outer_tx.decrypt(::G2Affine::prime_subgroup_generator()) - .expect("Test failed"); - processed_txs.push(ProcessedTx { - tx: outer_tx.to_bytes(), - result: TxResult { - code: ErrorCodes::Ok.into(), - info: "".into(), - }, - }); + for _ in 0..2 { + processed_txs.push(mk_decrypted_tx(&mut shell, &keypair)); } // create two wrapper txs - for i in 0..2 { - let mut wrapper_tx = - Tx::new(TxType::Wrapper(Box::new(WrapperTx::new( - Fee { - amount: MIN_FEE_AMOUNT, - token: shell.wl_storage.storage.native_token.clone(), - }, - keypair.ref_to(), - Epoch(0), - Default::default(), - #[cfg(not(feature = "mainnet"))] - None, - )))); - wrapper_tx.header.chain_id = shell.chain_id.clone(); - wrapper_tx.set_code(Code::new("wasm_code".as_bytes().to_owned())); - wrapper_tx.set_data(Data::new( - format!("Encrypted transaction data: {}", i) - .as_bytes() - .to_owned(), - )); - wrapper_tx.add_section(Section::Signature(Signature::new( - wrapper_tx.sechashes(), - &keypair, - ))); - valid_txs.push(wrapper_tx.clone()); - processed_txs.push(ProcessedTx { - tx: wrapper_tx.to_bytes(), - result: TxResult { - code: ErrorCodes::Ok.into(), - info: "".into(), - }, - }); + for _ in 0..2 { + let (tx, processed_tx) = mk_wrapper_tx(&shell, &keypair); + valid_txs.push(tx.clone()); + processed_txs.push(processed_tx); } // Put the wrapper txs in front of the decrypted txs processed_txs.rotate_left(2); @@ -1726,6 +1719,21 @@ mod test_finalize_block { shell.wl_storage.storage.next_epoch_min_start_height = BlockHeight(5); shell.wl_storage.storage.next_epoch_min_start_time = DateTimeUtc::now(); + let txs_key = gen_keypair(); + // Add unshielded balance for fee payment + let balance_key = token::balance_key( + &shell.wl_storage.storage.native_token, + &Address::from(&txs_key.ref_to()), + ); + shell + .wl_storage + .storage + .write( + &balance_key, + Amount::native_whole(1000).try_to_vec().unwrap(), + ) + .unwrap(); + // Add a proposal to be executed on next epoch change. let mut add_proposal = |proposal_id, vote| { let validator = shell.mode.get_validator_address().unwrap().clone(); @@ -1821,7 +1829,20 @@ mod test_finalize_block { // Need to supply a proposer address and votes to flow through the // inflation code for _ in 0..20 { + // Add some txs + let mut txs = vec![]; + // create two decrypted txs + for _ in 0..2 { + txs.push(mk_decrypted_tx(&mut shell, &txs_key)); + } + // create two wrapper txs + for _ in 0..2 { + let (_tx, processed_tx) = mk_wrapper_tx(&shell, &txs_key); + txs.push(processed_tx); + } + let req = FinalizeBlock { + txs, proposer_address: proposer_address.clone(), votes: votes.clone(), ..Default::default()