From 19e78990d7fb0c5e0b6edb449c1d990f949b2b3b Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 24 May 2024 12:37:15 +0200 Subject: [PATCH] fixup! Merge branch 'grarco/masp-no-transfer-dep' (#3232) evil 3232 --- crates/namada/src/ledger/protocol/mod.rs | 53 ++- crates/node/src/shell/finalize_block.rs | 440 +++++++++++++---------- 2 files changed, 283 insertions(+), 210 deletions(-) diff --git a/crates/namada/src/ledger/protocol/mod.rs b/crates/namada/src/ledger/protocol/mod.rs index 0d995c4e7d..3b481da6a6 100644 --- a/crates/namada/src/ledger/protocol/mod.rs +++ b/crates/namada/src/ledger/protocol/mod.rs @@ -18,8 +18,8 @@ use namada_state::StorageWrite; use namada_token::event::{TokenEvent, TokenOperation, UserAccount}; use namada_tx::data::protocol::{ProtocolTx, ProtocolTxType}; use namada_tx::data::{ - BatchResults, BatchedTxResult, ExtendedTxResult, TxResult, - VpStatusFlags, VpsResult, WrapperTx, + BatchResults, BatchedTxResult, ExtendedTxResult, TxResult, VpStatusFlags, + VpsResult, WrapperTx, }; use namada_tx::{BatchedTxRef, Tx}; use namada_vote_ext::EthereumTxData; @@ -225,31 +225,29 @@ where vp_wasm_cache, tx_wasm_cache, } => { - if let Some(mut tx_result) = wrapper_tx_result { + if let Some(tx_result) = wrapper_tx_result { // TODO(namada#2597): handle masp fee payment in the first inner // tx if necessary - // Replay protection check on the batch - let tx_hash = tx.raw_header_hash(); - if state.write_log().has_replay_protection_entry(&tx_hash) { - // If the same batch has already been committed in - // this block, skip execution and return - return Err(DispatchError { - error: Error::ReplayAttempt(tx_hash), - tx_result: None, - }); - } - - dispatch_inner_txs( - tx, - tx_result, - tx_index, - tx_gas_meter, - state, - vp_wasm_cache, - tx_wasm_cache, - ) - + // Replay protection check on the batch + let tx_hash = tx.raw_header_hash(); + if state.write_log().has_replay_protection_entry(&tx_hash) { + // If the same batch has already been committed in + // this block, skip execution and return + return Err(DispatchError { + error: Error::ReplayAttempt(tx_hash), + tx_result: None, + }); + } + dispatch_inner_txs( + tx, + tx_result, + tx_index, + tx_gas_meter, + state, + vp_wasm_cache, + tx_wasm_cache, + ) } else { // Governance proposal. We don't allow tx batches in this case, // just take the first one @@ -273,8 +271,7 @@ where .collect(), ), } - .to_extended_result(None)) - ) + .to_extended_result(None)) } } DispatchArgs::Protocol(protocol_tx) => { @@ -308,13 +305,13 @@ where ) .map_err(|e| Error::WrapperRunnerError(e.to_string()))?; - Ok(tx_result) + Ok(tx_result.to_extended_result(None)) } } } fn dispatch_inner_txs<'a, D, H, CA>( - tx: Tx, + tx: &Tx, tx_result: TxResult, tx_index: TxIndex, tx_gas_meter: &'a RefCell, diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 9bf02356fc..0d26854544 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -15,7 +15,7 @@ use namada::ledger::events::EmitEvents; use namada::ledger::gas::GasMetering; use namada::ledger::ibc; use namada::ledger::pos::namada_proof_of_stake; -use namada::ledger::protocol::DispatchArgs; +use namada::ledger::protocol::{DispatchArgs, DispatchError}; use namada::masp::MaspTxRefs; use namada::proof_of_stake; use namada::proof_of_stake::storage::{ @@ -353,8 +353,8 @@ where tx_data: TxData<'_>, mut tx_logs: TxLogs<'_>, ) -> Option { - match dispatch_result { - Ok(tx_result) => match tx_data.tx.header.tx_type { + match extended_dispatch_result { + Ok(extended_tx_result) => match tx_data.tx.header.tx_type { TxType::Wrapper(_) => { // Return withouth emitting any events return Some(WrapperCache { @@ -362,12 +362,12 @@ where tx_index: tx_data.tx_index, gas_meter: tx_data.tx_gas_meter, event: tx_logs.tx_event, - tx_result, + tx_result: extended_tx_result.tx_result, }); } _ => self.handle_inner_tx_results( response, - tx_result, + extended_tx_result, tx_data, &mut tx_logs, ), @@ -1354,11 +1354,7 @@ mod test_finalize_block { .enumerate() .find_map( |(idx, tx_hash)| { - if tx_hash == &hash { - Some(idx) - } else { - None - } + if tx_hash == &hash { Some(idx) } else { None } }, ) .unwrap(); @@ -2952,21 +2948,25 @@ mod test_finalize_block { assert_eq!(root_pre.0, root_post.0); // Check transaction's hash in storage - assert!(shell - .shell - .state - .write_log() - .has_replay_protection_entry(&wrapper_tx.raw_header_hash())); + assert!( + shell + .shell + .state + .write_log() + .has_replay_protection_entry(&wrapper_tx.raw_header_hash()) + ); // Check that the hash is not present in the merkle tree shell.state.commit_block().unwrap(); - assert!(!shell - .shell - .state - .in_mem() - .block - .tree - .has_key(&wrapper_hash_key) - .unwrap()); + assert!( + !shell + .shell + .state + .in_mem() + .block + .tree + .has_key(&wrapper_hash_key) + .unwrap() + ); // test that a commitment to replay protection gets added. let reprot_key = replay_protection::commitment_key(); @@ -3013,22 +3013,26 @@ mod test_finalize_block { assert_eq!(root_pre.0, root_post.0); // Check that the hashes are present in the merkle tree shell.state.commit_block().unwrap(); - assert!(shell - .shell - .state - .in_mem() - .block - .tree - .has_key(&convert_key) - .unwrap()); - assert!(shell - .shell - .state - .in_mem() - .block - .tree - .has_key(&commitment_key) - .unwrap()); + assert!( + shell + .shell + .state + .in_mem() + .block + .tree + .has_key(&convert_key) + .unwrap() + ); + assert!( + shell + .shell + .state + .in_mem() + .block + .tree + .has_key(&commitment_key) + .unwrap() + ); } /// Test that a tx that has already been applied in the same block @@ -3106,26 +3110,34 @@ mod test_finalize_block { assert_eq!(code, ResultCode::WasmRuntimeError); for wrapper in [&wrapper, &new_wrapper] { - assert!(shell - .state - .write_log() - .has_replay_protection_entry(&wrapper.raw_header_hash())); - assert!(!shell - .state - .write_log() - .has_replay_protection_entry(&wrapper.header_hash())); + assert!( + shell + .state + .write_log() + .has_replay_protection_entry(&wrapper.raw_header_hash()) + ); + assert!( + !shell + .state + .write_log() + .has_replay_protection_entry(&wrapper.header_hash()) + ); } // Commit to check the hashes from storage shell.commit(); for wrapper in [&wrapper, &new_wrapper] { - assert!(shell - .state - .has_replay_protection_entry(&wrapper.raw_header_hash()) - .unwrap()); - assert!(!shell - .state - .has_replay_protection_entry(&wrapper.header_hash()) - .unwrap()); + assert!( + shell + .state + .has_replay_protection_entry(&wrapper.raw_header_hash()) + .unwrap() + ); + assert!( + !shell + .state + .has_replay_protection_entry(&wrapper.header_hash()) + .unwrap() + ); } } @@ -3408,23 +3420,29 @@ mod test_finalize_block { &unsigned_wrapper, &wrong_commitment_wrapper, ] { - assert!(!shell - .state - .write_log() - .has_replay_protection_entry(&valid_wrapper.raw_header_hash())); - assert!(shell + assert!( + !shell.state.write_log().has_replay_protection_entry( + &valid_wrapper.raw_header_hash() + ) + ); + assert!( + shell + .state + .write_log() + .has_replay_protection_entry(&valid_wrapper.header_hash()) + ); + } + assert!( + shell.state.write_log().has_replay_protection_entry( + &failing_wrapper.raw_header_hash() + ) + ); + assert!( + !shell .state .write_log() - .has_replay_protection_entry(&valid_wrapper.header_hash())); - } - assert!(shell - .state - .write_log() - .has_replay_protection_entry(&failing_wrapper.raw_header_hash())); - assert!(!shell - .state - .write_log() - .has_replay_protection_entry(&failing_wrapper.header_hash())); + .has_replay_protection_entry(&failing_wrapper.header_hash()) + ); // Commit to check the hashes from storage shell.commit(); @@ -3433,23 +3451,33 @@ mod test_finalize_block { unsigned_wrapper, wrong_commitment_wrapper, ] { - assert!(!shell + assert!( + !shell + .state + .has_replay_protection_entry( + &valid_wrapper.raw_header_hash() + ) + .unwrap() + ); + assert!( + shell + .state + .has_replay_protection_entry(&valid_wrapper.header_hash()) + .unwrap() + ); + } + assert!( + shell .state - .has_replay_protection_entry(&valid_wrapper.raw_header_hash()) - .unwrap()); - assert!(shell + .has_replay_protection_entry(&failing_wrapper.raw_header_hash()) + .unwrap() + ); + assert!( + !shell .state - .has_replay_protection_entry(&valid_wrapper.header_hash()) - .unwrap()); - } - assert!(shell - .state - .has_replay_protection_entry(&failing_wrapper.raw_header_hash()) - .unwrap()); - assert!(!shell - .state - .has_replay_protection_entry(&failing_wrapper.header_hash()) - .unwrap()); + .has_replay_protection_entry(&failing_wrapper.header_hash()) + .unwrap() + ); } #[test] @@ -3509,14 +3537,18 @@ mod test_finalize_block { let code = event[0].read_attribute::().expect("Test failed"); assert_eq!(code, ResultCode::InvalidTx); - assert!(shell - .state - .write_log() - .has_replay_protection_entry(&wrapper_hash)); - assert!(!shell - .state - .write_log() - .has_replay_protection_entry(&wrapper.raw_header_hash())); + assert!( + shell + .state + .write_log() + .has_replay_protection_entry(&wrapper_hash) + ); + assert!( + !shell + .state + .write_log() + .has_replay_protection_entry(&wrapper.raw_header_hash()) + ); } // Test that the fees are paid even if the inner transaction fails and its @@ -3914,9 +3946,11 @@ mod test_finalize_block { .unwrap(), Some(ValidatorState::Consensus) ); - assert!(enqueued_slashes_handle() - .at(&Epoch::default()) - .is_empty(&shell.state)?); + assert!( + enqueued_slashes_handle() + .at(&Epoch::default()) + .is_empty(&shell.state)? + ); assert_eq!( get_num_consensus_validators(&shell.state, Epoch::default()) .unwrap(), @@ -3935,17 +3969,21 @@ mod test_finalize_block { .unwrap(), Some(ValidatorState::Jailed) ); - assert!(enqueued_slashes_handle() - .at(&epoch) - .is_empty(&shell.state)?); + assert!( + enqueued_slashes_handle() + .at(&epoch) + .is_empty(&shell.state)? + ); assert_eq!( get_num_consensus_validators(&shell.state, epoch).unwrap(), 5_u64 ); } - assert!(!enqueued_slashes_handle() - .at(&processing_epoch) - .is_empty(&shell.state)?); + assert!( + !enqueued_slashes_handle() + .at(&processing_epoch) + .is_empty(&shell.state)? + ); // Advance to the processing epoch loop { @@ -3968,9 +4006,11 @@ mod test_finalize_block { // println!("Reached processing epoch"); break; } else { - assert!(enqueued_slashes_handle() - .at(&shell.state.in_mem().block.epoch) - .is_empty(&shell.state)?); + assert!( + enqueued_slashes_handle() + .at(&shell.state.in_mem().block.epoch) + .is_empty(&shell.state)? + ); let stake1 = read_validator_stake( &shell.state, ¶ms, @@ -4454,11 +4494,13 @@ mod test_finalize_block { ) .unwrap(); assert_eq!(last_slash, Some(misbehavior_epoch)); - assert!(namada_proof_of_stake::storage::validator_slashes_handle( - &val1.address - ) - .is_empty(&shell.state) - .unwrap()); + assert!( + namada_proof_of_stake::storage::validator_slashes_handle( + &val1.address + ) + .is_empty(&shell.state) + .unwrap() + ); tracing::debug!("Advancing to epoch 7"); @@ -4523,18 +4565,22 @@ mod test_finalize_block { ) .unwrap(); assert_eq!(last_slash, Some(Epoch(4))); - assert!(namada_proof_of_stake::is_validator_frozen( - &shell.state, - &val1.address, - current_epoch, - ¶ms - ) - .unwrap()); - assert!(namada_proof_of_stake::storage::validator_slashes_handle( - &val1.address - ) - .is_empty(&shell.state) - .unwrap()); + assert!( + namada_proof_of_stake::is_validator_frozen( + &shell.state, + &val1.address, + current_epoch, + ¶ms + ) + .unwrap() + ); + assert!( + namada_proof_of_stake::storage::validator_slashes_handle( + &val1.address + ) + .is_empty(&shell.state) + .unwrap() + ); let pre_stake_10 = namada_proof_of_stake::storage::read_validator_stake( @@ -5412,9 +5458,11 @@ mod test_finalize_block { shell.vp_wasm_cache.clone(), ); let parameters = ParametersVp { ctx }; - assert!(parameters - .validate_tx(&batched_tx, &keys_changed, &verifiers) - .is_ok()); + assert!( + parameters + .validate_tx(&batched_tx, &keys_changed, &verifiers) + .is_ok() + ); // we advance forward to the next epoch let mut req = FinalizeBlock::default(); @@ -5487,11 +5535,13 @@ mod test_finalize_block { let inner_results = inner_tx_result.batch_results.0; for cmt in batch.commitments() { - assert!(inner_results - .get(&cmt.get_hash()) - .unwrap() - .clone() - .is_ok_and(|res| res.is_accepted())); + assert!( + inner_results + .get(&cmt.get_hash()) + .unwrap() + .clone() + .is_ok_and(|res| res.is_accepted()) + ); } // Check storage modifications @@ -5529,18 +5579,24 @@ mod test_finalize_block { let inner_tx_result = event[0].read_attribute::>().unwrap(); let inner_results = inner_tx_result.batch_results.0; - assert!(inner_results - .get(&batch.commitments()[0].get_hash()) - .unwrap() - .clone() - .is_ok_and(|res| res.is_accepted())); - assert!(inner_results - .get(&batch.commitments()[1].get_hash()) - .unwrap() - .clone() - .is_err()); + assert!( + inner_results + .get(&batch.commitments()[0].get_hash()) + .unwrap() + .clone() + .is_ok_and(|res| res.is_accepted()) + ); + assert!( + inner_results + .get(&batch.commitments()[1].get_hash()) + .unwrap() + .clone() + .is_err() + ); // Assert that the last tx didn't run - assert!(!inner_results.contains_key(&batch.commitments()[2].get_hash())); + assert!( + !inner_results.contains_key(&batch.commitments()[2].get_hash()) + ); // Check storage modifications are missing for key in ["random_key_1", "random_key_2", "random_key_3"] { @@ -5571,21 +5627,27 @@ mod test_finalize_block { let inner_tx_result = event[0].read_attribute::>().unwrap(); let inner_results = inner_tx_result.batch_results.0; - assert!(inner_results - .get(&batch.commitments()[0].get_hash()) - .unwrap() - .clone() - .is_ok_and(|res| res.is_accepted())); - assert!(inner_results - .get(&batch.commitments()[1].get_hash()) - .unwrap() - .clone() - .is_err()); - assert!(inner_results - .get(&batch.commitments()[2].get_hash()) - .unwrap() - .clone() - .is_ok_and(|res| res.is_accepted())); + assert!( + inner_results + .get(&batch.commitments()[0].get_hash()) + .unwrap() + .clone() + .is_ok_and(|res| res.is_accepted()) + ); + assert!( + inner_results + .get(&batch.commitments()[1].get_hash()) + .unwrap() + .clone() + .is_err() + ); + assert!( + inner_results + .get(&batch.commitments()[2].get_hash()) + .unwrap() + .clone() + .is_ok_and(|res| res.is_accepted()) + ); // Check storage modifications assert_eq!( @@ -5596,10 +5658,12 @@ mod test_finalize_block { .unwrap(), STORAGE_VALUE ); - assert!(!shell - .state - .has_key(&"random_key_2".parse().unwrap()) - .unwrap()); + assert!( + !shell + .state + .has_key(&"random_key_2".parse().unwrap()) + .unwrap() + ); assert_eq!( shell .state @@ -5631,18 +5695,24 @@ mod test_finalize_block { let inner_tx_result = event[0].read_attribute::>().unwrap(); let inner_results = inner_tx_result.batch_results.0; - assert!(inner_results - .get(&batch.commitments()[0].get_hash()) - .unwrap() - .clone() - .is_ok_and(|res| res.is_accepted())); - assert!(inner_results - .get(&batch.commitments()[1].get_hash()) - .unwrap() - .clone() - .is_err()); + assert!( + inner_results + .get(&batch.commitments()[0].get_hash()) + .unwrap() + .clone() + .is_ok_and(|res| res.is_accepted()) + ); + assert!( + inner_results + .get(&batch.commitments()[1].get_hash()) + .unwrap() + .clone() + .is_err() + ); // Assert that the last tx didn't run - assert!(!inner_results.contains_key(&batch.commitments()[2].get_hash())); + assert!( + !inner_results.contains_key(&batch.commitments()[2].get_hash()) + ); // Check storage modifications are missing for key in ["random_key_1", "random_key_2", "random_key_3"] { @@ -5672,18 +5742,24 @@ mod test_finalize_block { let inner_tx_result = event[0].read_attribute::>().unwrap(); let inner_results = inner_tx_result.batch_results.0; - assert!(inner_results - .get(&batch.commitments()[0].get_hash()) - .unwrap() - .clone() - .is_ok_and(|res| res.is_accepted())); - assert!(inner_results - .get(&batch.commitments()[1].get_hash()) - .unwrap() - .clone() - .is_err()); + assert!( + inner_results + .get(&batch.commitments()[0].get_hash()) + .unwrap() + .clone() + .is_ok_and(|res| res.is_accepted()) + ); + assert!( + inner_results + .get(&batch.commitments()[1].get_hash()) + .unwrap() + .clone() + .is_err() + ); // Assert that the last tx didn't run - assert!(!inner_results.contains_key(&batch.commitments()[2].get_hash())); + assert!( + !inner_results.contains_key(&batch.commitments()[2].get_hash()) + ); // Check storage modifications assert_eq!(