From 48709ce5608632d898d0893e69ea189e07c005fd Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 20 May 2024 17:54:50 +0200 Subject: [PATCH 01/15] Fixes fee payment --- crates/namada/src/ledger/protocol/mod.rs | 177 +++---- crates/node/src/shell/finalize_block.rs | 576 ++++++++++++----------- crates/node/src/shell/governance.rs | 1 + 3 files changed, 392 insertions(+), 362 deletions(-) diff --git a/crates/namada/src/ledger/protocol/mod.rs b/crates/namada/src/ledger/protocol/mod.rs index 74e17fbae3..3417eaa924 100644 --- a/crates/namada/src/ledger/protocol/mod.rs +++ b/crates/namada/src/ledger/protocol/mod.rs @@ -184,6 +184,7 @@ impl From for DispatchError { #[allow(clippy::too_many_arguments)] pub fn dispatch_tx<'a, D, H, CA>( tx: Tx, + //FIXME: some params are only needed for some tx types, should also pass the txtype with the associated data here? Maybe yes tx_bytes: &'a [u8], tx_index: TxIndex, tx_gas_meter: &'a RefCell, @@ -191,6 +192,7 @@ pub fn dispatch_tx<'a, D, H, CA>( vp_wasm_cache: &'a mut VpCache, tx_wasm_cache: &'a mut TxCache, block_proposer: Option<&Address>, + wrapper_tx_result: Option>, ) -> std::result::Result, DispatchError> where D: 'static + DB + for<'iter> DBIter<'iter> + Sync, @@ -200,27 +202,98 @@ where match tx.header().tx_type { // Raw trasaction type is allowed only for governance proposals TxType::Raw => { - // No bundles of governance transactions, just take the first one - let cmt = tx.first_commitments().ok_or(Error::MissingInnerTxs)?; - let batched_tx_result = apply_wasm_tx( - BatchedTxRef { tx: &tx, cmt }, - &tx_index, - ShellParams { - tx_gas_meter, - state, - vp_wasm_cache, - tx_wasm_cache, - }, - )?; + if let Some(mut tx_result) = wrapper_tx_result { + // 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, + }); + } - Ok(TxResult { - gas_used: tx_gas_meter.borrow().get_tx_consumed_gas(), - batch_results: BatchResults( - [(cmt.get_hash(), Ok(batched_tx_result))] - .into_iter() - .collect(), - ), - }) + // TODO(namada#2597): handle masp fee payment in the first inner tx + // if necessary + for cmt in tx.commitments() { + match apply_wasm_tx( + tx.batch_ref_tx(cmt), + &tx_index, + ShellParams { + tx_gas_meter, + state, + vp_wasm_cache, + tx_wasm_cache, + }, + ) { + Err(Error::GasError(ref msg)) => { + // Gas error aborts the execution of the entire batch + tx_result.gas_used = + tx_gas_meter.borrow().get_tx_consumed_gas(); + tx_result.batch_results.0.insert( + cmt.get_hash(), + Err(Error::GasError(msg.to_owned())), + ); + state.write_log_mut().drop_tx(); + return Err(DispatchError { + error: Error::GasError(msg.to_owned()), + tx_result: Some(tx_result), + }); + } + res => { + let is_accepted = matches!(&res, Ok(result) if result.is_accepted()); + + tx_result + .batch_results + .0 + .insert(cmt.get_hash(), res); + tx_result.gas_used = + tx_gas_meter.borrow().get_tx_consumed_gas(); + if is_accepted { + state.write_log_mut().commit_tx_to_batch(); + } else { + state.write_log_mut().drop_tx(); + + if tx.header.atomic { + // Stop the execution of an atomic batch at the + // first failed transaction + return Err(DispatchError { + error: Error::FailingAtomicBatch( + cmt.get_hash(), + ), + tx_result: Some(tx_result), + }); + } + } + } + }; + } + + Ok(tx_result) + } else { + // No bundles of governance transactions, just take the first one + let cmt = + tx.first_commitments().ok_or(Error::MissingInnerTxs)?; + let batched_tx_result = apply_wasm_tx( + BatchedTxRef { tx: &tx, cmt }, + &tx_index, + ShellParams { + tx_gas_meter, + state, + vp_wasm_cache, + tx_wasm_cache, + }, + )?; + Ok(TxResult { + gas_used: tx_gas_meter.borrow().get_tx_consumed_gas(), + batch_results: BatchResults( + [(cmt.get_hash(), Ok(batched_tx_result))] + .into_iter() + .collect(), + ), + }) + } } TxType::Protocol(protocol_tx) => { // No bundles of protocol transactions, only take the first one @@ -252,70 +325,6 @@ where ) .map_err(|e| Error::WrapperRunnerError(e.to_string()))?; - // 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, - }); - } - - // TODO(namada#2597): handle masp fee payment in the first inner tx - // if necessary - for cmt in tx.commitments() { - match apply_wasm_tx( - tx.batch_ref_tx(cmt), - &tx_index, - ShellParams { - tx_gas_meter, - state, - vp_wasm_cache, - tx_wasm_cache, - }, - ) { - Err(Error::GasError(ref msg)) => { - // Gas error aborts the execution of the entire batch - tx_result.gas_used = - tx_gas_meter.borrow().get_tx_consumed_gas(); - tx_result.batch_results.0.insert( - cmt.get_hash(), - Err(Error::GasError(msg.to_owned())), - ); - state.write_log_mut().drop_tx(); - return Err(DispatchError { - error: Error::GasError(msg.to_owned()), - tx_result: Some(tx_result), - }); - } - res => { - let is_accepted = - matches!(&res, Ok(result) if result.is_accepted()); - - tx_result.batch_results.0.insert(cmt.get_hash(), res); - tx_result.gas_used = - tx_gas_meter.borrow().get_tx_consumed_gas(); - if is_accepted { - state.write_log_mut().commit_tx_to_batch(); - } else { - state.write_log_mut().drop_tx(); - - if tx.header.atomic { - // Stop the execution of an atomic batch at the - // first failed transaction - return Err(DispatchError { - error: Error::FailingAtomicBatch( - cmt.get_hash(), - ), - tx_result: Some(tx_result), - }); - } - } - } - }; - } Ok(tx_result) } } diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 257d16216e..80660e7a9b 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -142,10 +142,16 @@ where proposer from tendermint raw hash", ) }; + //FIXME: need uni test on fee pyament when inner txs touch balance of fee payer (also run this test on the old version with the bug to check that the bug was indeed there) // Tracks the accepted transactions self.state.in_mem_mut().block.results = BlockResults::default(); let mut changed_keys = BTreeSet::new(); + //FIXME: maybe better to cache the transactions themselves to avoid another deserialization + let mut successful_wrappers = vec![]; + + //FIXME: move this to a separate function? Maybe yes + // Execute wrapper and protocol transactions for (tx_index, processed_tx) in req.txs.iter().enumerate() { let tx = if let Ok(tx) = Tx::try_from(processed_tx.tx.as_ref()) { tx @@ -303,21 +309,13 @@ where } }, }; - let replay_protection_hashes = - if matches!(tx_header.tx_type, TxType::Wrapper(_)) { - Some(ReplayProtectionHashes { - raw_header_hash: tx.raw_header_hash(), - header_hash: tx.header_hash(), - }) - } else { - None - }; let tx_gas_meter = RefCell::new(tx_gas_meter); - let mut tx_event = new_tx_event(&tx, height.0); + let tx_event = new_tx_event(&tx, height.0); let is_atomic_batch = tx.header.atomic; let commitments_len = tx.commitments().len() as u64; let tx_hash = tx.header_hash(); + //FIXME: this thing is the same for raw txs, merge them? Maybe not let dispatch_result = protocol::dispatch_tx( tx, processed_tx.tx.as_ref(), @@ -327,6 +325,7 @@ where &mut self.vp_wasm_cache, &mut self.tx_wasm_cache, block_proposer, + None, ); let tx_gas_meter = tx_gas_meter.into_inner(); let consumed_gas = tx_gas_meter.get_tx_consumed_gas(); @@ -334,6 +333,82 @@ where // save the gas cost self.update_tx_gas(tx_hash, consumed_gas.into()); + //FIXME: this is becoming very cluttered because it needs to handle wrappers, inners and protocols, should split in different functions? + if let Some(wrapper_cache) = self.evaluate_tx_result( + &mut response, + dispatch_result, + TxData { + is_atomic_batch, + tx_header: &tx_header, + commitments_len, + tx_index, + replay_protection_hashes: None, + tx_gas_meter, + height, + }, + TxLogs { + tx_event, + stats: &mut stats, + changed_keys: &mut changed_keys, + }, + ) { + successful_wrappers.push(wrapper_cache); + } + } + + // Execute inner transactions + //FIXME: export this to another function + for WrapperCache { + tx_index, + gas_meter: tx_gas_meter, + event: tx_event, + tx_result: wrapper_tx_result, + } in successful_wrappers + { + //FIXME: should also enqueue the tx directly to avoid deserializing again? POssibly yes + //FIXME: manage unwrap + let processed_tx = req.txs.get(tx_index).unwrap(); + let mut tx = if let Ok(tx) = Tx::try_from(processed_tx.tx.as_ref()) + { + tx + } else { + tracing::error!( + "FinalizeBlock received a tx that could not be \ + deserialized to a Tx type. This is likely a protocol \ + transaction." + ); + continue; + }; + + let tx_header = tx.header(); + let tx_hash = tx.header_hash(); + let is_atomic_batch = tx.header.atomic; + let commitments_len = tx.commitments().len() as u64; + let replay_protection_hashes = Some(ReplayProtectionHashes { + raw_header_hash: tx.raw_header_hash(), + header_hash: tx.header_hash(), + }); + + // change tx type to raw for execution + tx.update_header(TxType::Raw); + let tx_gas_meter = RefCell::new(tx_gas_meter); + let dispatch_result = protocol::dispatch_tx( + tx, + processed_tx.tx.as_ref(), + TxIndex::must_from_usize(tx_index), + &tx_gas_meter, + &mut self.state, + &mut self.vp_wasm_cache, + &mut self.tx_wasm_cache, + None, + Some(wrapper_tx_result), + ); + let tx_gas_meter = tx_gas_meter.into_inner(); + let consumed_gas = tx_gas_meter.get_tx_consumed_gas(); + + // update the gas cost of the corresponding wrapper + self.update_tx_gas(tx_hash, consumed_gas.into()); + self.evaluate_tx_result( &mut response, dispatch_result, @@ -343,16 +418,15 @@ where commitments_len, tx_index, replay_protection_hashes, - consumed_gas, + tx_gas_meter, height, }, TxLogs { - tx_event: &mut tx_event, + tx_event, stats: &mut stats, changed_keys: &mut changed_keys, }, ); - response.events.emit(tx_event); } stats.set_tx_cache_size( @@ -524,8 +598,8 @@ where } } - // Evaluate the result of a batch. Commit or drop the storage changes, - // update stats and event, manage replay protection. + // Evaluate the result of a transaction. Commit or drop the storage changes, + // update stats and event, manage replay protection. For successful wrapper transactions return the relevant data and delay the evaluation after the batch execution fn evaluate_tx_result( &mut self, response: &mut shim::response::FinalizeBlock, @@ -535,14 +609,27 @@ where >, tx_data: TxData<'_>, mut tx_logs: TxLogs<'_>, - ) { + ) -> Option { match dispatch_result { - Ok(tx_result) => self.handle_inner_tx_results( - response, - tx_result, - tx_data, - &mut tx_logs, - ), + Ok(tx_result) => match tx_data.tx_header.tx_type { + //FIXME: manage unwrap + TxType::Wrapper(_) => + // Return withouth emitting the event + { + return Some(WrapperCache { + tx_index: tx_data.tx_index, + gas_meter: tx_data.tx_gas_meter, + event: tx_logs.tx_event, + tx_result, + }) + } + _ => self.handle_inner_tx_results( + response, + tx_result, + tx_data, + &mut tx_logs, + ), + }, Err(DispatchError { error: protocol::Error::WrapperRunnerError(msg), tx_result: _, @@ -557,7 +644,7 @@ where ); tx_logs .tx_event - .extend(GasUsed(tx_data.consumed_gas)) + .extend(GasUsed(tx_data.tx_gas_meter.get_tx_consumed_gas())) .extend(Info(msg.to_string())) .extend(Code(ResultCode::InvalidTx)); // Make sure to clean the write logs for the next transaction @@ -582,7 +669,7 @@ where tx_logs .tx_event - .extend(GasUsed(tx_data.consumed_gas)) + .extend(GasUsed(tx_data.tx_gas_meter.get_tx_consumed_gas())) .extend(Info(msg.to_string())) .extend(Code(ResultCode::WasmRuntimeError)); @@ -595,6 +682,9 @@ where ); } } + + response.events.emit(tx_logs.tx_event); + None } // Evaluate the results of all the transactions of the batch. Commit or drop @@ -747,18 +837,28 @@ where } } +// Caches the execution of a wrapper transaction to be used when later executing the inner batch +struct WrapperCache { + tx_index: usize, + gas_meter: TxGasMeter, + event: Event, + tx_result: namada::tx::data::TxResult, +} + struct TxData<'tx> { is_atomic_batch: bool, + // FIXME: need the actual tx but it's hard becasue that is passed to dispatch_tx so I'd need to clone it + //FIXME: actually I need the tx only for wrappers, for everything else I only need the header tx_header: &'tx namada::tx::Header, commitments_len: u64, tx_index: usize, replay_protection_hashes: Option, - consumed_gas: Gas, + tx_gas_meter: TxGasMeter, height: BlockHeight, } struct TxLogs<'finalize> { - tx_event: &'finalize mut Event, + tx_event: Event, stats: &'finalize mut InternalStats, changed_keys: &'finalize mut BTreeSet, } @@ -2798,25 +2898,21 @@ 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(); @@ -2863,26 +2959,22 @@ 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 @@ -2960,34 +3052,26 @@ 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()); } } @@ -3270,29 +3354,23 @@ 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 - .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 + assert!(!shell .state .write_log() - .has_replay_protection_entry(&failing_wrapper.header_hash()) - ); + .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(&failing_wrapper.header_hash())); // Commit to check the hashes from storage shell.commit(); @@ -3301,33 +3379,23 @@ mod test_finalize_block { unsigned_wrapper, wrong_commitment_wrapper, ] { - 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 + assert!(!shell .state - .has_replay_protection_entry(&failing_wrapper.raw_header_hash()) - .unwrap() - ); - assert!( - !shell + .has_replay_protection_entry(&valid_wrapper.raw_header_hash()) + .unwrap()); + assert!(shell .state - .has_replay_protection_entry(&failing_wrapper.header_hash()) - .unwrap() - ); + .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()); } #[test] @@ -3387,18 +3455,14 @@ 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 @@ -3796,11 +3860,9 @@ 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(), @@ -3819,21 +3881,17 @@ 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 { @@ -3856,11 +3914,9 @@ 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, @@ -4344,13 +4400,11 @@ 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"); @@ -4415,22 +4469,18 @@ 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( @@ -5308,11 +5358,9 @@ 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(); @@ -5385,13 +5433,11 @@ 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 @@ -5429,24 +5475,18 @@ 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"] { @@ -5477,27 +5517,21 @@ 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!( @@ -5508,12 +5542,10 @@ 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 @@ -5545,24 +5577,18 @@ 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"] { @@ -5592,24 +5618,18 @@ 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!( diff --git a/crates/node/src/shell/governance.rs b/crates/node/src/shell/governance.rs index dbde3d0204..b8d82d6df4 100644 --- a/crates/node/src/shell/governance.rs +++ b/crates/node/src/shell/governance.rs @@ -413,6 +413,7 @@ where &mut shell.vp_wasm_cache, &mut shell.tx_wasm_cache, None, + None, ); shell .state From 57e5f46a6273fc516dd670171736832bd3038719 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 20 May 2024 18:42:34 +0200 Subject: [PATCH 02/15] Refactors tx execution of `finalize_block` into separate functions --- crates/node/src/shell/finalize_block.rs | 582 +++++++++++++----------- 1 file changed, 305 insertions(+), 277 deletions(-) diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 80660e7a9b..c7e477afab 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -147,287 +147,27 @@ where // Tracks the accepted transactions self.state.in_mem_mut().block.results = BlockResults::default(); let mut changed_keys = BTreeSet::new(); - //FIXME: maybe better to cache the transactions themselves to avoid another deserialization - let mut successful_wrappers = vec![]; - //FIXME: move this to a separate function? Maybe yes // Execute wrapper and protocol transactions - for (tx_index, processed_tx) in req.txs.iter().enumerate() { - let tx = if let Ok(tx) = Tx::try_from(processed_tx.tx.as_ref()) { - tx - } else { - tracing::error!( - "FinalizeBlock received a tx that could not be \ - deserialized to a Tx type. This is likely a protocol \ - transaction." - ); - continue; - }; - - let result_code = ResultCode::from_u32(processed_tx.result.code) - .expect("Result code conversion should not fail"); - - // If [`process_proposal`] rejected a Tx due to invalid signature, - // emit an event here and move on to next tx. - if result_code == ResultCode::InvalidSig { - let base_event = match tx.header().tx_type { - TxType::Wrapper(_) | TxType::Protocol(_) => { - new_tx_event(&tx, height.0) - } - _ => { - tracing::error!( - "Internal logic error: FinalizeBlock received a \ - tx with an invalid signature error code that \ - could not be deserialized to a WrapperTx / \ - ProtocolTx type" - ); - continue; - } - }; - response.events.emit( - base_event - .with(Code(result_code)) - .with(Info(format!( - "Tx rejected: {}", - &processed_tx.result.info - ))) - .with(GasUsed(0.into())), - ); - continue; - } - - if tx.validate_tx().is_err() { - tracing::error!( - "Internal logic error: FinalizeBlock received tx that \ - could not be deserialized to a valid TxType" - ); - continue; - }; - let tx_header = tx.header(); - // If [`process_proposal`] rejected a Tx, emit an event here and - // move on to next tx - if result_code != ResultCode::Ok { - response.events.emit( - new_tx_event(&tx, height.0) - .with(Code(result_code)) - .with(Info(format!( - "Tx rejected: {}", - &processed_tx.result.info - ))) - .with(GasUsed(0.into())), - ); - continue; - } - - let (tx_gas_meter, block_proposer) = match &tx_header.tx_type { - TxType::Wrapper(wrapper) => { - stats.increment_wrapper_txs(); - let gas_limit = match Gas::try_from(wrapper.gas_limit) { - Ok(value) => value, - Err(_) => { - response.events.emit( - new_tx_event(&tx, height.0) - .with(Code(ResultCode::InvalidTx)) - .with(Info( - "The wrapper gas limit overflowed gas \ - representation" - .to_owned(), - )) - .with(GasUsed(0.into())), - ); - continue; - } - }; - let gas_meter = TxGasMeter::new(gas_limit); - for cmt in tx.commitments() { - if let Some(code_sec) = tx - .get_section(cmt.code_sechash()) - .and_then(|x| Section::code_sec(x.as_ref())) - { - stats.increment_tx_type( - code_sec.code.hash().to_string(), - ); - } - } - (gas_meter, Some(&native_block_proposer_address)) - } - TxType::Raw => { - tracing::error!( - "Internal logic error: FinalizeBlock received a \ - TxType::Raw transaction" - ); - continue; - } - TxType::Protocol(protocol_tx) => match protocol_tx.tx { - ProtocolTxType::BridgePoolVext - | ProtocolTxType::BridgePool - | ProtocolTxType::ValSetUpdateVext - | ProtocolTxType::ValidatorSetUpdate => { - (TxGasMeter::new_from_sub_limit(0.into()), None) - } - ProtocolTxType::EthEventsVext => { - let ext = - ethereum_tx_data_variants::EthEventsVext::try_from( - &tx, - ) - .unwrap(); - if self - .mode - .get_validator_address() - .map(|validator| { - validator == &ext.data.validator_addr - }) - .unwrap_or(false) - { - for event in ext.data.ethereum_events.iter() { - self.mode.dequeue_eth_event(event); - } - } - (TxGasMeter::new_from_sub_limit(0.into()), None) - } - ProtocolTxType::EthereumEvents => { - let digest = - ethereum_tx_data_variants::EthereumEvents::try_from( - &tx - ).unwrap(); - if let Some(address) = - self.mode.get_validator_address().cloned() - { - let this_signer = &( - address, - self.state.in_mem().get_last_block_height(), - ); - for MultiSignedEthEvent { event, signers } in - &digest.events - { - if signers.contains(this_signer) { - self.mode.dequeue_eth_event(event); - } - } - } - (TxGasMeter::new_from_sub_limit(0.into()), None) - } - }, - }; - let tx_gas_meter = RefCell::new(tx_gas_meter); - let tx_event = new_tx_event(&tx, height.0); - let is_atomic_batch = tx.header.atomic; - let commitments_len = tx.commitments().len() as u64; - let tx_hash = tx.header_hash(); - - //FIXME: this thing is the same for raw txs, merge them? Maybe not - let dispatch_result = protocol::dispatch_tx( - tx, - processed_tx.tx.as_ref(), - TxIndex::must_from_usize(tx_index), - &tx_gas_meter, - &mut self.state, - &mut self.vp_wasm_cache, - &mut self.tx_wasm_cache, - block_proposer, - None, - ); - let tx_gas_meter = tx_gas_meter.into_inner(); - let consumed_gas = tx_gas_meter.get_tx_consumed_gas(); - - // save the gas cost - self.update_tx_gas(tx_hash, consumed_gas.into()); - - //FIXME: this is becoming very cluttered because it needs to handle wrappers, inners and protocols, should split in different functions? - if let Some(wrapper_cache) = self.evaluate_tx_result( - &mut response, - dispatch_result, - TxData { - is_atomic_batch, - tx_header: &tx_header, - commitments_len, - tx_index, - replay_protection_hashes: None, - tx_gas_meter, - height, - }, - TxLogs { - tx_event, - stats: &mut stats, - changed_keys: &mut changed_keys, - }, - ) { - successful_wrappers.push(wrapper_cache); - } - } + let successful_wrappers = self.retrieve_and_execute_transactions( + &req.txs, + &mut response, + &mut changed_keys, + &mut stats, + height, + &native_block_proposer_address, + ); // Execute inner transactions - //FIXME: export this to another function - for WrapperCache { - tx_index, - gas_meter: tx_gas_meter, - event: tx_event, - tx_result: wrapper_tx_result, - } in successful_wrappers - { - //FIXME: should also enqueue the tx directly to avoid deserializing again? POssibly yes - //FIXME: manage unwrap - let processed_tx = req.txs.get(tx_index).unwrap(); - let mut tx = if let Ok(tx) = Tx::try_from(processed_tx.tx.as_ref()) - { - tx - } else { - tracing::error!( - "FinalizeBlock received a tx that could not be \ - deserialized to a Tx type. This is likely a protocol \ - transaction." - ); - continue; - }; - - let tx_header = tx.header(); - let tx_hash = tx.header_hash(); - let is_atomic_batch = tx.header.atomic; - let commitments_len = tx.commitments().len() as u64; - let replay_protection_hashes = Some(ReplayProtectionHashes { - raw_header_hash: tx.raw_header_hash(), - header_hash: tx.header_hash(), - }); - - // change tx type to raw for execution - tx.update_header(TxType::Raw); - let tx_gas_meter = RefCell::new(tx_gas_meter); - let dispatch_result = protocol::dispatch_tx( - tx, - processed_tx.tx.as_ref(), - TxIndex::must_from_usize(tx_index), - &tx_gas_meter, - &mut self.state, - &mut self.vp_wasm_cache, - &mut self.tx_wasm_cache, - None, - Some(wrapper_tx_result), - ); - let tx_gas_meter = tx_gas_meter.into_inner(); - let consumed_gas = tx_gas_meter.get_tx_consumed_gas(); - - // update the gas cost of the corresponding wrapper - self.update_tx_gas(tx_hash, consumed_gas.into()); - - self.evaluate_tx_result( - &mut response, - dispatch_result, - TxData { - is_atomic_batch, - tx_header: &tx_header, - commitments_len, - tx_index, - replay_protection_hashes, - tx_gas_meter, - height, - }, - TxLogs { - tx_event, - stats: &mut stats, - changed_keys: &mut changed_keys, - }, - ); - } + self.execute_tx_batches( + successful_wrappers, + //FIXME: same args as the previous function, use a struct + &req.txs, + &mut response, + &mut changed_keys, + &mut stats, + height, + ); stats.set_tx_cache_size( self.tx_wasm_cache.get_size(), @@ -835,6 +575,294 @@ where } } } + + // Get the transactions from the consensus engine, preprocess and execute them. Return the cache of successful wrapper transactions later used when executing the inner txs. + fn retrieve_and_execute_transactions( + &mut self, + processed_txs: &[shim::request::ProcessedTx], + response: &mut shim::response::FinalizeBlock, + //FIXME: review how we pass these, custom struct? + changed_keys: &mut BTreeSet, + stats: &mut InternalStats, + height: BlockHeight, + native_block_proposer_address: &Address, + //FIXME: maybe better to cache the transactions themselves to avoid another deserialization + ) -> Vec { + let mut successful_wrappers = vec![]; + + for (tx_index, processed_tx) in processed_txs.iter().enumerate() { + let tx = if let Ok(tx) = Tx::try_from(processed_tx.tx.as_ref()) { + tx + } else { + tracing::error!( + "FinalizeBlock received a tx that could not be \ + deserialized to a Tx type. This is likely a protocol \ + transaction." + ); + continue; + }; + + let result_code = ResultCode::from_u32(processed_tx.result.code) + .expect("Result code conversion should not fail"); + + // If [`process_proposal`] rejected a Tx due to invalid signature, + // emit an event here and move on to next tx. + if result_code == ResultCode::InvalidSig { + let base_event = match tx.header().tx_type { + TxType::Wrapper(_) | TxType::Protocol(_) => { + new_tx_event(&tx, height.0) + } + _ => { + tracing::error!( + "Internal logic error: FinalizeBlock received a \ + tx with an invalid signature error code that \ + could not be deserialized to a WrapperTx / \ + ProtocolTx type" + ); + continue; + } + }; + response.events.emit( + base_event + .with(Code(result_code)) + .with(Info(format!( + "Tx rejected: {}", + &processed_tx.result.info + ))) + .with(GasUsed(0.into())), + ); + continue; + } + + if tx.validate_tx().is_err() { + tracing::error!( + "Internal logic error: FinalizeBlock received tx that \ + could not be deserialized to a valid TxType" + ); + continue; + }; + let tx_header = tx.header(); + // If [`process_proposal`] rejected a Tx, emit an event here and + // move on to next tx + if result_code != ResultCode::Ok { + response.events.emit( + new_tx_event(&tx, height.0) + .with(Code(result_code)) + .with(Info(format!( + "Tx rejected: {}", + &processed_tx.result.info + ))) + .with(GasUsed(0.into())), + ); + continue; + } + + let (tx_gas_meter, block_proposer) = + match &tx_header.tx_type { + TxType::Wrapper(wrapper) => { + stats.increment_wrapper_txs(); + let gas_meter = TxGasMeter::new(wrapper.gas_limit); + for cmt in tx.commitments() { + if let Some(code_sec) = tx + .get_section(cmt.code_sechash()) + .and_then(|x| Section::code_sec(x.as_ref())) + { + stats.increment_tx_type( + code_sec.code.hash().to_string(), + ); + } + } + (gas_meter, Some(native_block_proposer_address)) + } + TxType::Raw => { + tracing::error!( + "Internal logic error: FinalizeBlock received a \ + TxType::Raw transaction" + ); + continue; + } + TxType::Protocol(protocol_tx) => match protocol_tx.tx { + ProtocolTxType::BridgePoolVext + | ProtocolTxType::BridgePool + | ProtocolTxType::ValSetUpdateVext + | ProtocolTxType::ValidatorSetUpdate => { + (TxGasMeter::new_from_sub_limit(0.into()), None) + } + ProtocolTxType::EthEventsVext => { + let ext = + ethereum_tx_data_variants::EthEventsVext::try_from(&tx) + .unwrap(); + if self + .mode + .get_validator_address() + .map(|validator| { + validator == &ext.data.validator_addr + }) + .unwrap_or(false) + { + for event in ext.data.ethereum_events.iter() { + self.mode.dequeue_eth_event(event); + } + } + (TxGasMeter::new_from_sub_limit(0.into()), None) + } + ProtocolTxType::EthereumEvents => { + let digest = + ethereum_tx_data_variants::EthereumEvents::try_from( + &tx, + ) + .unwrap(); + if let Some(address) = + self.mode.get_validator_address().cloned() + { + let this_signer = &( + address, + self.state.in_mem().get_last_block_height(), + ); + for MultiSignedEthEvent { event, signers } in + &digest.events + { + if signers.contains(this_signer) { + self.mode.dequeue_eth_event(event); + } + } + } + (TxGasMeter::new_from_sub_limit(0.into()), None) + } + }, + }; + let tx_gas_meter = RefCell::new(tx_gas_meter); + let tx_event = new_tx_event(&tx, height.0); + let is_atomic_batch = tx.header.atomic; + let commitments_len = tx.commitments().len() as u64; + let tx_hash = tx.header_hash(); + + let dispatch_result = protocol::dispatch_tx( + tx, + processed_tx.tx.as_ref(), + TxIndex::must_from_usize(tx_index), + &tx_gas_meter, + &mut self.state, + &mut self.vp_wasm_cache, + &mut self.tx_wasm_cache, + block_proposer, + None, + ); + let tx_gas_meter = tx_gas_meter.into_inner(); + let consumed_gas = tx_gas_meter.get_tx_consumed_gas(); + + // save the gas cost + self.update_tx_gas(tx_hash, consumed_gas.into()); + + //FIXME: this is becoming very cluttered because it needs to handle wrappers, inners and protocols, should split in different functions? + if let Some(wrapper_cache) = self.evaluate_tx_result( + response, + dispatch_result, + TxData { + is_atomic_batch, + tx_header: &tx_header, + commitments_len, + tx_index, + replay_protection_hashes: None, + tx_gas_meter, + height, + }, + TxLogs { + tx_event, + stats, + changed_keys, + }, + ) { + successful_wrappers.push(wrapper_cache); + } + } + + successful_wrappers + } + + // Execute the transaction batches for successful wrapper transactions + fn execute_tx_batches( + &mut self, + //FIXME: iter trait? + successful_wrappers: Vec, + processed_txs: &[shim::request::ProcessedTx], + response: &mut shim::response::FinalizeBlock, + //FIXME: review how we pass these, custom struct? + changed_keys: &mut BTreeSet, + stats: &mut InternalStats, + height: BlockHeight, + ) { + for WrapperCache { + tx_index, + gas_meter: tx_gas_meter, + event: tx_event, + tx_result: wrapper_tx_result, + } in successful_wrappers + { + //FIXME: should also enqueue the tx directly to avoid deserializing again? POssibly yes + //FIXME: manage unwrap + let processed_tx = processed_txs.get(tx_index).unwrap(); + let mut tx = if let Ok(tx) = Tx::try_from(processed_tx.tx.as_ref()) + { + tx + } else { + tracing::error!( + "FinalizeBlock received a tx that could not be \ + deserialized to a Tx type. This is likely a protocol \ + transaction." + ); + continue; + }; + + let tx_header = tx.header(); + let tx_hash = tx.header_hash(); + let is_atomic_batch = tx.header.atomic; + let commitments_len = tx.commitments().len() as u64; + let replay_protection_hashes = Some(ReplayProtectionHashes { + raw_header_hash: tx.raw_header_hash(), + header_hash: tx.header_hash(), + }); + + // change tx type to raw for execution + tx.update_header(TxType::Raw); + let tx_gas_meter = RefCell::new(tx_gas_meter); + let dispatch_result = protocol::dispatch_tx( + tx, + processed_tx.tx.as_ref(), + TxIndex::must_from_usize(tx_index), + &tx_gas_meter, + &mut self.state, + &mut self.vp_wasm_cache, + &mut self.tx_wasm_cache, + None, + Some(wrapper_tx_result), + ); + let tx_gas_meter = tx_gas_meter.into_inner(); + let consumed_gas = tx_gas_meter.get_tx_consumed_gas(); + + // update the gas cost of the corresponding wrapper + self.update_tx_gas(tx_hash, consumed_gas.into()); + + self.evaluate_tx_result( + response, + dispatch_result, + TxData { + is_atomic_batch, + tx_header: &tx_header, + commitments_len, + tx_index, + replay_protection_hashes, + tx_gas_meter, + height, + }, + TxLogs { + tx_event, + stats, + changed_keys, + }, + ); + } + } } // Caches the execution of a wrapper transaction to be used when later executing the inner batch From d1eafb2817f3d5c7ab0bc30ef77f8aeec8fd64af Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 20 May 2024 19:10:13 +0200 Subject: [PATCH 03/15] Reorganizes arguments for tx execution --- crates/node/src/shell/finalize_block.rs | 60 +++++++++++++++---------- 1 file changed, 36 insertions(+), 24 deletions(-) diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index c7e477afab..25a3ab6d38 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -150,23 +150,26 @@ where // Execute wrapper and protocol transactions let successful_wrappers = self.retrieve_and_execute_transactions( - &req.txs, - &mut response, - &mut changed_keys, - &mut stats, - height, &native_block_proposer_address, + ExecutionArgs { + processed_txs: &req.txs, + response: &mut response, + changed_keys: &mut changed_keys, + stats: &mut stats, + height, + }, ); // Execute inner transactions self.execute_tx_batches( successful_wrappers, - //FIXME: same args as the previous function, use a struct - &req.txs, - &mut response, - &mut changed_keys, - &mut stats, - height, + ExecutionArgs { + processed_txs: &req.txs, + response: &mut response, + changed_keys: &mut changed_keys, + stats: &mut stats, + height, + }, ); stats.set_tx_cache_size( @@ -579,13 +582,14 @@ where // Get the transactions from the consensus engine, preprocess and execute them. Return the cache of successful wrapper transactions later used when executing the inner txs. fn retrieve_and_execute_transactions( &mut self, - processed_txs: &[shim::request::ProcessedTx], - response: &mut shim::response::FinalizeBlock, - //FIXME: review how we pass these, custom struct? - changed_keys: &mut BTreeSet, - stats: &mut InternalStats, - height: BlockHeight, native_block_proposer_address: &Address, + ExecutionArgs { + processed_txs, + response, + changed_keys, + stats, + height, + }: ExecutionArgs, //FIXME: maybe better to cache the transactions themselves to avoid another deserialization ) -> Vec { let mut successful_wrappers = vec![]; @@ -783,14 +787,14 @@ where // Execute the transaction batches for successful wrapper transactions fn execute_tx_batches( &mut self, - //FIXME: iter trait? successful_wrappers: Vec, - processed_txs: &[shim::request::ProcessedTx], - response: &mut shim::response::FinalizeBlock, - //FIXME: review how we pass these, custom struct? - changed_keys: &mut BTreeSet, - stats: &mut InternalStats, - height: BlockHeight, + ExecutionArgs { + processed_txs, + response, + changed_keys, + stats, + height, + }: ExecutionArgs, ) { for WrapperCache { tx_index, @@ -865,6 +869,14 @@ where } } +struct ExecutionArgs<'finalize> { + processed_txs: &'finalize [shim::request::ProcessedTx], + response: &'finalize mut shim::response::FinalizeBlock, + changed_keys: &'finalize mut BTreeSet, + stats: &'finalize mut InternalStats, + height: BlockHeight, +} + // Caches the execution of a wrapper transaction to be used when later executing the inner batch struct WrapperCache { tx_index: usize, From 5711946231b83b03a53b3e2cb3acfbe02bea4090 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 20 May 2024 19:39:01 +0200 Subject: [PATCH 04/15] Cache wrapper transaction to avoid second deserialization --- crates/namada/src/ledger/protocol/mod.rs | 4 +- crates/node/src/shell/finalize_block.rs | 56 ++++++++---------------- crates/node/src/shell/governance.rs | 2 +- 3 files changed, 21 insertions(+), 41 deletions(-) diff --git a/crates/namada/src/ledger/protocol/mod.rs b/crates/namada/src/ledger/protocol/mod.rs index 3417eaa924..56ad5a49a2 100644 --- a/crates/namada/src/ledger/protocol/mod.rs +++ b/crates/namada/src/ledger/protocol/mod.rs @@ -183,7 +183,7 @@ impl From for DispatchError { /// environment, in which case validity predicates will be bypassed. #[allow(clippy::too_many_arguments)] pub fn dispatch_tx<'a, D, H, CA>( - tx: Tx, + tx: &Tx, //FIXME: some params are only needed for some tx types, should also pass the txtype with the associated data here? Maybe yes tx_bytes: &'a [u8], tx_index: TxIndex, @@ -311,7 +311,7 @@ where }) } TxType::Wrapper(ref wrapper) => { - let mut tx_result = apply_wrapper_tx( + let tx_result = apply_wrapper_tx( tx.clone(), wrapper, tx_bytes, diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 25a3ab6d38..310abb7f15 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -151,8 +151,8 @@ where // Execute wrapper and protocol transactions let successful_wrappers = self.retrieve_and_execute_transactions( &native_block_proposer_address, + &req.txs, ExecutionArgs { - processed_txs: &req.txs, response: &mut response, changed_keys: &mut changed_keys, stats: &mut stats, @@ -164,7 +164,6 @@ where self.execute_tx_batches( successful_wrappers, ExecutionArgs { - processed_txs: &req.txs, response: &mut response, changed_keys: &mut changed_keys, stats: &mut stats, @@ -354,17 +353,17 @@ where mut tx_logs: TxLogs<'_>, ) -> Option { match dispatch_result { - Ok(tx_result) => match tx_data.tx_header.tx_type { - //FIXME: manage unwrap + Ok(tx_result) => match tx_data.tx.header.tx_type { TxType::Wrapper(_) => - // Return withouth emitting the event + // Return withouth emitting any events { return Some(WrapperCache { + tx: tx_data.tx.to_owned(), tx_index: tx_data.tx_index, gas_meter: tx_data.tx_gas_meter, event: tx_logs.tx_event, tx_result, - }) + }); } _ => self.handle_inner_tx_results( response, @@ -446,7 +445,7 @@ where is_any_tx_invalid, } = temp_log.check_inner_results( &tx_result, - tx_data.tx_header, + &tx_data.tx.header, tx_data.tx_index, tx_data.height, ); @@ -507,7 +506,7 @@ where is_any_tx_invalid: _, } = temp_log.check_inner_results( &tx_result, - tx_data.tx_header, + &tx_data.tx.header, tx_data.tx_index, tx_data.height, ); @@ -554,7 +553,7 @@ where // If user transaction didn't fail because of out of gas nor // invalid section commitment, commit its hash to prevent // replays - if matches!(tx_data.tx_header.tx_type, TxType::Wrapper(_)) { + if matches!(tx_data.tx.header.tx_type, TxType::Wrapper(_)) { if !matches!( err, Error::TxApply(protocol::Error::GasError(_)) @@ -583,14 +582,13 @@ where fn retrieve_and_execute_transactions( &mut self, native_block_proposer_address: &Address, + processed_txs: &[shim::request::ProcessedTx], ExecutionArgs { - processed_txs, response, changed_keys, stats, height, }: ExecutionArgs, - //FIXME: maybe better to cache the transactions themselves to avoid another deserialization ) -> Vec { let mut successful_wrappers = vec![]; @@ -742,7 +740,7 @@ where let tx_hash = tx.header_hash(); let dispatch_result = protocol::dispatch_tx( - tx, + &tx, processed_tx.tx.as_ref(), TxIndex::must_from_usize(tx_index), &tx_gas_meter, @@ -758,13 +756,12 @@ where // save the gas cost self.update_tx_gas(tx_hash, consumed_gas.into()); - //FIXME: this is becoming very cluttered because it needs to handle wrappers, inners and protocols, should split in different functions? if let Some(wrapper_cache) = self.evaluate_tx_result( response, dispatch_result, TxData { is_atomic_batch, - tx_header: &tx_header, + tx: &tx, commitments_len, tx_index, replay_protection_hashes: None, @@ -789,7 +786,6 @@ where &mut self, successful_wrappers: Vec, ExecutionArgs { - processed_txs, response, changed_keys, stats, @@ -797,28 +793,13 @@ where }: ExecutionArgs, ) { for WrapperCache { + mut tx, tx_index, gas_meter: tx_gas_meter, event: tx_event, tx_result: wrapper_tx_result, } in successful_wrappers { - //FIXME: should also enqueue the tx directly to avoid deserializing again? POssibly yes - //FIXME: manage unwrap - let processed_tx = processed_txs.get(tx_index).unwrap(); - let mut tx = if let Ok(tx) = Tx::try_from(processed_tx.tx.as_ref()) - { - tx - } else { - tracing::error!( - "FinalizeBlock received a tx that could not be \ - deserialized to a Tx type. This is likely a protocol \ - transaction." - ); - continue; - }; - - let tx_header = tx.header(); let tx_hash = tx.header_hash(); let is_atomic_batch = tx.header.atomic; let commitments_len = tx.commitments().len() as u64; @@ -831,8 +812,9 @@ where tx.update_header(TxType::Raw); let tx_gas_meter = RefCell::new(tx_gas_meter); let dispatch_result = protocol::dispatch_tx( - tx, - processed_tx.tx.as_ref(), + &tx, + // not needed here for gas computation + &[], TxIndex::must_from_usize(tx_index), &tx_gas_meter, &mut self.state, @@ -852,7 +834,7 @@ where dispatch_result, TxData { is_atomic_batch, - tx_header: &tx_header, + tx: &tx, commitments_len, tx_index, replay_protection_hashes, @@ -870,7 +852,6 @@ where } struct ExecutionArgs<'finalize> { - processed_txs: &'finalize [shim::request::ProcessedTx], response: &'finalize mut shim::response::FinalizeBlock, changed_keys: &'finalize mut BTreeSet, stats: &'finalize mut InternalStats, @@ -879,6 +860,7 @@ struct ExecutionArgs<'finalize> { // Caches the execution of a wrapper transaction to be used when later executing the inner batch struct WrapperCache { + tx: Tx, tx_index: usize, gas_meter: TxGasMeter, event: Event, @@ -887,9 +869,7 @@ struct WrapperCache { struct TxData<'tx> { is_atomic_batch: bool, - // FIXME: need the actual tx but it's hard becasue that is passed to dispatch_tx so I'd need to clone it - //FIXME: actually I need the tx only for wrappers, for everything else I only need the header - tx_header: &'tx namada::tx::Header, + tx: &'tx Tx, commitments_len: u64, tx_index: usize, replay_protection_hashes: Option, diff --git a/crates/node/src/shell/governance.rs b/crates/node/src/shell/governance.rs index b8d82d6df4..f4a73c29ee 100644 --- a/crates/node/src/shell/governance.rs +++ b/crates/node/src/shell/governance.rs @@ -403,7 +403,7 @@ where let cmt = tx.first_commitments().unwrap().to_owned(); let dispatch_result = protocol::dispatch_tx( - tx, + &tx, &[], /* this is used to compute the fee * based on the code size. We dont * need it here. */ From a329373bfb64dd20430fea82745953dcb3970ae5 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 21 May 2024 12:42:27 +0200 Subject: [PATCH 05/15] Refactors the arguments of `dispatch_tx` for every tx type --- crates/namada/src/ledger/protocol/mod.rs | 63 +++++++++++----- crates/node/src/shell/finalize_block.rs | 94 +++++++++++++----------- crates/node/src/shell/governance.rs | 17 ++--- crates/tx/src/data/protocol.rs | 1 + 4 files changed, 104 insertions(+), 71 deletions(-) diff --git a/crates/namada/src/ledger/protocol/mod.rs b/crates/namada/src/ledger/protocol/mod.rs index 56ad5a49a2..f3ad6f627b 100644 --- a/crates/namada/src/ledger/protocol/mod.rs +++ b/crates/namada/src/ledger/protocol/mod.rs @@ -16,7 +16,7 @@ use namada_gas::TxGasMeter; use namada_sdk::tx::TX_TRANSFER_WASM; use namada_state::StorageWrite; use namada_token::event::{TokenEvent, TokenOperation, UserAccount}; -use namada_tx::data::protocol::ProtocolTxType; +use namada_tx::data::protocol::{ProtocolTx, ProtocolTxType}; use namada_tx::data::{ BatchResults, BatchedTxResult, TxResult, TxType, VpStatusFlags, VpsResult, WrapperTx, @@ -178,30 +178,49 @@ impl From for DispatchError { } } +/// Arguments for transactions' execution +pub enum DispatchArgs<'a, CA: 'static + WasmCacheAccess + Sync> { + //FIXME: do we need thide header and the wrapper one or should we get them from the tx itself? + Protocol(&'a ProtocolTx), + Raw { + tx_index: TxIndex, + wrapper_tx_result: Option>, + vp_wasm_cache: &'a mut VpCache, + tx_wasm_cache: &'a mut TxCache, + }, + Wrapper { + wrapper: &'a WrapperTx, + tx_bytes: &'a [u8], + block_proposer: &'a Address, + //FIXME: why does a wrapper need the caches? We don't, remove! + vp_wasm_cache: &'a mut VpCache, + tx_wasm_cache: &'a mut TxCache, + }, +} + /// Dispatch a given transaction to be applied based on its type. Some storage /// updates may be derived and applied natively rather than via the wasm /// environment, in which case validity predicates will be bypassed. -#[allow(clippy::too_many_arguments)] pub fn dispatch_tx<'a, D, H, CA>( tx: &Tx, - //FIXME: some params are only needed for some tx types, should also pass the txtype with the associated data here? Maybe yes - tx_bytes: &'a [u8], - tx_index: TxIndex, + //FIXME: rename? + dispatch_args: DispatchArgs, + //FIXME: try to move this into the args tx_gas_meter: &'a RefCell, state: &'a mut WlState, - vp_wasm_cache: &'a mut VpCache, - tx_wasm_cache: &'a mut TxCache, - block_proposer: Option<&Address>, - wrapper_tx_result: Option>, ) -> std::result::Result, DispatchError> where D: 'static + DB + for<'iter> DBIter<'iter> + Sync, H: 'static + StorageHasher + Sync, CA: 'static + WasmCacheAccess + Sync, { - match tx.header().tx_type { - // Raw trasaction type is allowed only for governance proposals - TxType::Raw => { + match dispatch_args { + DispatchArgs::Raw { + tx_index, + wrapper_tx_result, + vp_wasm_cache, + tx_wasm_cache, + } => { if let Some(mut tx_result) = wrapper_tx_result { // Replay protection check on the batch let tx_hash = tx.raw_header_hash(); @@ -272,11 +291,11 @@ where Ok(tx_result) } else { - // No bundles of governance transactions, just take the first one + // Governance proposal. We don't allow tx batches in this case, just take the first one let cmt = tx.first_commitments().ok_or(Error::MissingInnerTxs)?; let batched_tx_result = apply_wasm_tx( - BatchedTxRef { tx: &tx, cmt }, + tx.batch_ref_tx(cmt), &tx_index, ShellParams { tx_gas_meter, @@ -295,7 +314,7 @@ where }) } } - TxType::Protocol(protocol_tx) => { + DispatchArgs::Protocol(protocol_tx) => { // No bundles of protocol transactions, only take the first one let cmt = tx.first_commitments().ok_or(Error::MissingInnerTxs)?; let batched_tx_result = @@ -310,9 +329,16 @@ where ..Default::default() }) } - TxType::Wrapper(ref wrapper) => { + DispatchArgs::Wrapper { + wrapper, + tx_bytes, + block_proposer, + vp_wasm_cache, + tx_wasm_cache, + } => { let tx_result = apply_wrapper_tx( - tx.clone(), + //FIXME: actually, do I need to pass ownership? + tx.to_owned(), wrapper, tx_bytes, ShellParams { @@ -321,7 +347,7 @@ where vp_wasm_cache, tx_wasm_cache, }, - block_proposer, + Some(block_proposer), ) .map_err(|e| Error::WrapperRunnerError(e.to_string()))?; @@ -397,6 +423,7 @@ where fn charge_fee( wrapper: &WrapperTx, wrapper_tx_hash: Hash, + //FIXME: don't need the shell param here anymore, just the state! shell_params: &mut ShellParams<'_, S, D, H, CA>, block_proposer: Option<&Address>, ) -> Result<()> diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 310abb7f15..e029fa8e80 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::DispatchError; +use namada::ledger::protocol::{DispatchArgs, DispatchError}; use namada::proof_of_stake; use namada::proof_of_stake::storage::{ find_validator_by_raw_hash, write_last_block_proposer_address, @@ -659,37 +659,46 @@ where continue; } - let (tx_gas_meter, block_proposer) = - match &tx_header.tx_type { - TxType::Wrapper(wrapper) => { - stats.increment_wrapper_txs(); - let gas_meter = TxGasMeter::new(wrapper.gas_limit); - for cmt in tx.commitments() { - if let Some(code_sec) = tx - .get_section(cmt.code_sechash()) - .and_then(|x| Section::code_sec(x.as_ref())) - { - stats.increment_tx_type( - code_sec.code.hash().to_string(), - ); - } + //FIXME: rename? + let (dispatch_args, tx_gas_meter) = match &tx_header.tx_type { + TxType::Wrapper(wrapper) => { + stats.increment_wrapper_txs(); + let tx_gas_meter = TxGasMeter::new(wrapper.gas_limit); + for cmt in tx.commitments() { + if let Some(code_sec) = tx + .get_section(cmt.code_sechash()) + .and_then(|x| Section::code_sec(x.as_ref())) + { + stats.increment_tx_type( + code_sec.code.hash().to_string(), + ); } - (gas_meter, Some(native_block_proposer_address)) } - TxType::Raw => { - tracing::error!( - "Internal logic error: FinalizeBlock received a \ + ( + DispatchArgs::Wrapper { + wrapper, + tx_bytes: processed_tx.tx.as_ref(), + block_proposer: native_block_proposer_address, + vp_wasm_cache: &mut self.vp_wasm_cache, + tx_wasm_cache: &mut self.tx_wasm_cache, + }, + tx_gas_meter, + ) + } + TxType::Raw => { + tracing::error!( + "Internal logic error: FinalizeBlock received a \ TxType::Raw transaction" - ); - continue; - } - TxType::Protocol(protocol_tx) => match protocol_tx.tx { + ); + continue; + } + TxType::Protocol(protocol_tx) => { + match protocol_tx.tx { ProtocolTxType::BridgePoolVext | ProtocolTxType::BridgePool | ProtocolTxType::ValSetUpdateVext - | ProtocolTxType::ValidatorSetUpdate => { - (TxGasMeter::new_from_sub_limit(0.into()), None) - } + | ProtocolTxType::ValidatorSetUpdate => (), + ProtocolTxType::EthEventsVext => { let ext = ethereum_tx_data_variants::EthEventsVext::try_from(&tx) @@ -706,7 +715,6 @@ where self.mode.dequeue_eth_event(event); } } - (TxGasMeter::new_from_sub_limit(0.into()), None) } ProtocolTxType::EthereumEvents => { let digest = @@ -729,26 +737,25 @@ where } } } - (TxGasMeter::new_from_sub_limit(0.into()), None) } - }, - }; - let tx_gas_meter = RefCell::new(tx_gas_meter); + } + ( + DispatchArgs::Protocol(protocol_tx), + TxGasMeter::new_from_sub_limit(0.into()), + ) + } + }; let tx_event = new_tx_event(&tx, height.0); let is_atomic_batch = tx.header.atomic; let commitments_len = tx.commitments().len() as u64; let tx_hash = tx.header_hash(); + let tx_gas_meter = RefCell::new(tx_gas_meter); let dispatch_result = protocol::dispatch_tx( &tx, - processed_tx.tx.as_ref(), - TxIndex::must_from_usize(tx_index), + dispatch_args, &tx_gas_meter, &mut self.state, - &mut self.vp_wasm_cache, - &mut self.tx_wasm_cache, - block_proposer, - None, ); let tx_gas_meter = tx_gas_meter.into_inner(); let consumed_gas = tx_gas_meter.get_tx_consumed_gas(); @@ -813,15 +820,14 @@ where let tx_gas_meter = RefCell::new(tx_gas_meter); let dispatch_result = protocol::dispatch_tx( &tx, - // not needed here for gas computation - &[], - TxIndex::must_from_usize(tx_index), + DispatchArgs::Raw { + tx_index: TxIndex::must_from_usize(tx_index), + wrapper_tx_result: Some(wrapper_tx_result), + vp_wasm_cache: &mut self.vp_wasm_cache, + tx_wasm_cache: &mut self.tx_wasm_cache, + }, &tx_gas_meter, &mut self.state, - &mut self.vp_wasm_cache, - &mut self.tx_wasm_cache, - None, - Some(wrapper_tx_result), ); let tx_gas_meter = tx_gas_meter.into_inner(); let consumed_gas = tx_gas_meter.get_tx_consumed_gas(); diff --git a/crates/node/src/shell/governance.rs b/crates/node/src/shell/governance.rs index f4a73c29ee..18a68e0218 100644 --- a/crates/node/src/shell/governance.rs +++ b/crates/node/src/shell/governance.rs @@ -404,16 +404,15 @@ where let dispatch_result = protocol::dispatch_tx( &tx, - &[], /* this is used to compute the fee - * based on the code size. We dont - * need it here. */ - TxIndex::default(), - &RefCell::new(TxGasMeter::new_from_sub_limit(u64::MAX.into())), /* No gas limit for governance proposal */ + protocol::DispatchArgs::Raw { + tx_index: TxIndex::default(), + wrapper_tx_result: None, + vp_wasm_cache: &mut shell.vp_wasm_cache, + tx_wasm_cache: &mut shell.tx_wasm_cache, + }, + // No gas limit for governance proposal + &RefCell::new(TxGasMeter::new_from_sub_limit(u64::MAX.into())), &mut shell.state, - &mut shell.vp_wasm_cache, - &mut shell.tx_wasm_cache, - None, - None, ); shell .state diff --git a/crates/tx/src/data/protocol.rs b/crates/tx/src/data/protocol.rs index 33995d8a81..6f0125aaf3 100644 --- a/crates/tx/src/data/protocol.rs +++ b/crates/tx/src/data/protocol.rs @@ -56,6 +56,7 @@ impl ProtocolTx { } #[derive( + Copy, Clone, Debug, BorshSerialize, From 2bd2650bfcb12121e8abbf3a268c61597c0ff3ef Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 21 May 2024 13:11:24 +0200 Subject: [PATCH 06/15] Removes unused wasm caches for wrapper execution --- crates/namada/src/ledger/mod.rs | 32 +++++++--------- crates/namada/src/ledger/protocol/mod.rs | 49 ++++++++---------------- crates/node/src/shell/finalize_block.rs | 7 ++-- 3 files changed, 34 insertions(+), 54 deletions(-) diff --git a/crates/namada/src/ledger/mod.rs b/crates/namada/src/ledger/mod.rs index 14c4a8b622..d962c9848e 100644 --- a/crates/namada/src/ledger/mod.rs +++ b/crates/namada/src/ledger/mod.rs @@ -64,12 +64,8 @@ mod dry_run_tx { tx.clone(), &wrapper, &request.data, - ShellParams::new( - &tx_gas_meter, - &mut temp_state, - &mut ctx.vp_wasm_cache, - &mut ctx.tx_wasm_cache, - ), + &tx_gas_meter, + &mut temp_state, None, ) .into_storage_result()?; @@ -256,8 +252,8 @@ mod test { } #[tokio::test] - async fn test_shell_queries_router_with_client() - -> namada_state::StorageResult<()> { + async fn test_shell_queries_router_with_client( + ) -> namada_state::StorageResult<()> { // Initialize the `TestClient` let mut client = TestClient::new(RPC); // store the wasm code @@ -291,17 +287,15 @@ mod test { .dry_run_tx(&client, Some(tx_bytes), None, false) .await .unwrap(); - assert!( - result - .data - .batch_results - .0 - .get(&cmt.get_hash()) - .unwrap() - .as_ref() - .unwrap() - .is_accepted() - ); + assert!(result + .data + .batch_results + .0 + .get(&cmt.get_hash()) + .unwrap() + .as_ref() + .unwrap() + .is_accepted()); // Request storage value for a balance key ... let token_addr = address::testing::established_address_1(); diff --git a/crates/namada/src/ledger/protocol/mod.rs b/crates/namada/src/ledger/protocol/mod.rs index f3ad6f627b..264d3f632d 100644 --- a/crates/namada/src/ledger/protocol/mod.rs +++ b/crates/namada/src/ledger/protocol/mod.rs @@ -192,9 +192,6 @@ pub enum DispatchArgs<'a, CA: 'static + WasmCacheAccess + Sync> { wrapper: &'a WrapperTx, tx_bytes: &'a [u8], block_proposer: &'a Address, - //FIXME: why does a wrapper need the caches? We don't, remove! - vp_wasm_cache: &'a mut VpCache, - tx_wasm_cache: &'a mut TxCache, }, } @@ -333,20 +330,14 @@ where wrapper, tx_bytes, block_proposer, - vp_wasm_cache, - tx_wasm_cache, } => { let tx_result = apply_wrapper_tx( //FIXME: actually, do I need to pass ownership? tx.to_owned(), wrapper, tx_bytes, - ShellParams { - tx_gas_meter, - state, - vp_wasm_cache, - tx_wasm_cache, - }, + tx_gas_meter, + state, Some(block_proposer), ) .map_err(|e| Error::WrapperRunnerError(e.to_string()))?; @@ -378,40 +369,39 @@ where /// - gas accounting // TODO(namada#2597): this must signal to the caller if we need masp fee payment // in the first inner tx of the batch -pub(crate) fn apply_wrapper_tx( +pub(crate) fn apply_wrapper_tx( tx: Tx, + //FIXME: pass dispatch args? wrapper: &WrapperTx, tx_bytes: &[u8], - mut shell_params: ShellParams<'_, S, D, H, CA>, + tx_gas_meter: &RefCell, + state: &mut S, block_proposer: Option<&Address>, ) -> Result> where S: State + Sync, D: 'static + DB + for<'iter> DBIter<'iter> + Sync, H: 'static + StorageHasher + Sync, - CA: 'static + WasmCacheAccess + Sync, { let wrapper_tx_hash = tx.header_hash(); // Write wrapper tx hash to storage - shell_params - .state + state .write_log_mut() .write_tx_hash(wrapper_tx_hash) .expect("Error while writing tx hash to storage"); // Charge fee before performing any fallible operations - charge_fee(wrapper, wrapper_tx_hash, &mut shell_params, block_proposer)?; + charge_fee(wrapper, wrapper_tx_hash, state, block_proposer)?; // Account for gas - shell_params - .tx_gas_meter + tx_gas_meter .borrow_mut() .add_wrapper_gas(tx_bytes) .map_err(|err| Error::GasError(err.to_string()))?; Ok(TxResult { - gas_used: shell_params.tx_gas_meter.borrow().get_tx_consumed_gas(), + gas_used: tx_gas_meter.borrow().get_tx_consumed_gas(), batch_results: BatchResults::default(), }) } @@ -420,35 +410,30 @@ where /// - Fee amount overflows /// - Not enough funds are available to pay the entire amount of the fee /// - The accumulated fee amount to be credited to the block proposer overflows -fn charge_fee( +fn charge_fee( wrapper: &WrapperTx, wrapper_tx_hash: Hash, - //FIXME: don't need the shell param here anymore, just the state! - shell_params: &mut ShellParams<'_, S, D, H, CA>, + state: &mut S, block_proposer: Option<&Address>, ) -> Result<()> where S: State + Sync, D: 'static + DB + for<'iter> DBIter<'iter> + Sync, H: 'static + StorageHasher + Sync, - CA: 'static + WasmCacheAccess + Sync, { // Charge or check fees before propagating any possible error let payment_result = match block_proposer { - Some(block_proposer) => transfer_fee( - shell_params.state, - block_proposer, - wrapper, - wrapper_tx_hash, - ), + Some(block_proposer) => { + transfer_fee(state, block_proposer, wrapper, wrapper_tx_hash) + } None => { - check_fees(shell_params.state, wrapper)?; + check_fees(state, wrapper)?; Ok(()) } }; // Commit tx write log even in case of subsequent errors - shell_params.state.write_log_mut().commit_tx(); + state.write_log_mut().commit_tx(); payment_result } diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index e029fa8e80..5b45bc6593 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -660,7 +660,10 @@ where } //FIXME: rename? - let (dispatch_args, tx_gas_meter) = match &tx_header.tx_type { + let (dispatch_args, tx_gas_meter): ( + DispatchArgs<'_, WasmCacheRwAccess>, + TxGasMeter, + ) = match &tx_header.tx_type { TxType::Wrapper(wrapper) => { stats.increment_wrapper_txs(); let tx_gas_meter = TxGasMeter::new(wrapper.gas_limit); @@ -679,8 +682,6 @@ where wrapper, tx_bytes: processed_tx.tx.as_ref(), block_proposer: native_block_proposer_address, - vp_wasm_cache: &mut self.vp_wasm_cache, - tx_wasm_cache: &mut self.tx_wasm_cache, }, tx_gas_meter, ) From efe95ccecd8d6c6a07e5abaad08b72524635d20a Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 21 May 2024 13:17:01 +0200 Subject: [PATCH 07/15] Pass tx by reference in `apply_wrapper_tx` --- crates/namada/src/ledger/mod.rs | 26 ++++++++-------- crates/namada/src/ledger/protocol/mod.rs | 38 ++++++++++++++---------- crates/node/src/shell/finalize_block.rs | 16 ++++++---- 3 files changed, 48 insertions(+), 32 deletions(-) diff --git a/crates/namada/src/ledger/mod.rs b/crates/namada/src/ledger/mod.rs index d962c9848e..a7d4dbd705 100644 --- a/crates/namada/src/ledger/mod.rs +++ b/crates/namada/src/ledger/mod.rs @@ -61,7 +61,7 @@ mod dry_run_tx { Gas::try_from(wrapper.gas_limit).into_storage_result()?; let tx_gas_meter = RefCell::new(TxGasMeter::new(gas_limit)); let tx_result = protocol::apply_wrapper_tx( - tx.clone(), + &tx, &wrapper, &request.data, &tx_gas_meter, @@ -252,8 +252,8 @@ mod test { } #[tokio::test] - async fn test_shell_queries_router_with_client( - ) -> namada_state::StorageResult<()> { + async fn test_shell_queries_router_with_client() + -> namada_state::StorageResult<()> { // Initialize the `TestClient` let mut client = TestClient::new(RPC); // store the wasm code @@ -287,15 +287,17 @@ mod test { .dry_run_tx(&client, Some(tx_bytes), None, false) .await .unwrap(); - assert!(result - .data - .batch_results - .0 - .get(&cmt.get_hash()) - .unwrap() - .as_ref() - .unwrap() - .is_accepted()); + assert!( + result + .data + .batch_results + .0 + .get(&cmt.get_hash()) + .unwrap() + .as_ref() + .unwrap() + .is_accepted() + ); // Request storage value for a balance key ... let token_addr = address::testing::established_address_1(); diff --git a/crates/namada/src/ledger/protocol/mod.rs b/crates/namada/src/ledger/protocol/mod.rs index 264d3f632d..8045814ec6 100644 --- a/crates/namada/src/ledger/protocol/mod.rs +++ b/crates/namada/src/ledger/protocol/mod.rs @@ -18,7 +18,7 @@ use namada_state::StorageWrite; use namada_token::event::{TokenEvent, TokenOperation, UserAccount}; use namada_tx::data::protocol::{ProtocolTx, ProtocolTxType}; use namada_tx::data::{ - BatchResults, BatchedTxResult, TxResult, TxType, VpStatusFlags, VpsResult, + BatchResults, BatchedTxResult, TxResult, VpStatusFlags, VpsResult, WrapperTx, }; use namada_tx::{BatchedTxRef, Tx}; @@ -180,17 +180,27 @@ impl From for DispatchError { /// Arguments for transactions' execution pub enum DispatchArgs<'a, CA: 'static + WasmCacheAccess + Sync> { - //FIXME: do we need thide header and the wrapper one or should we get them from the tx itself? + /// Protocoli tx data Protocol(&'a ProtocolTx), + /// Raw tx data Raw { + /// The tx index tx_index: TxIndex, + /// The result of the corresponding wrapper tx (missing if governance + /// transaction) wrapper_tx_result: Option>, + /// Vp cache vp_wasm_cache: &'a mut VpCache, + /// Tx cache tx_wasm_cache: &'a mut TxCache, }, + /// Wrapper tx data Wrapper { + /// The wrapper header wrapper: &'a WrapperTx, + /// The transaction bytes for gas accounting tx_bytes: &'a [u8], + /// The block proposer block_proposer: &'a Address, }, } @@ -200,9 +210,7 @@ pub enum DispatchArgs<'a, CA: 'static + WasmCacheAccess + Sync> { /// environment, in which case validity predicates will be bypassed. pub fn dispatch_tx<'a, D, H, CA>( tx: &Tx, - //FIXME: rename? dispatch_args: DispatchArgs, - //FIXME: try to move this into the args tx_gas_meter: &'a RefCell, state: &'a mut WlState, ) -> std::result::Result, DispatchError> @@ -230,8 +238,8 @@ where }); } - // TODO(namada#2597): handle masp fee payment in the first inner tx - // if necessary + // TODO(namada#2597): handle masp fee payment in the first inner + // tx if necessary for cmt in tx.commitments() { match apply_wasm_tx( tx.batch_ref_tx(cmt), @@ -244,7 +252,8 @@ where }, ) { Err(Error::GasError(ref msg)) => { - // Gas error aborts the execution of the entire batch + // Gas error aborts the execution of the entire + // batch tx_result.gas_used = tx_gas_meter.borrow().get_tx_consumed_gas(); tx_result.batch_results.0.insert( @@ -272,8 +281,8 @@ where state.write_log_mut().drop_tx(); if tx.header.atomic { - // Stop the execution of an atomic batch at the - // first failed transaction + // Stop the execution of an atomic batch at + // the first failed transaction return Err(DispatchError { error: Error::FailingAtomicBatch( cmt.get_hash(), @@ -288,7 +297,8 @@ where Ok(tx_result) } else { - // Governance proposal. We don't allow tx batches in this case, just take the first one + // Governance proposal. We don't allow tx batches in this case, + // just take the first one let cmt = tx.first_commitments().ok_or(Error::MissingInnerTxs)?; let batched_tx_result = apply_wasm_tx( @@ -332,8 +342,7 @@ where block_proposer, } => { let tx_result = apply_wrapper_tx( - //FIXME: actually, do I need to pass ownership? - tx.to_owned(), + tx, wrapper, tx_bytes, tx_gas_meter, @@ -370,8 +379,7 @@ where // TODO(namada#2597): this must signal to the caller if we need masp fee payment // in the first inner tx of the batch pub(crate) fn apply_wrapper_tx( - tx: Tx, - //FIXME: pass dispatch args? + tx: &Tx, wrapper: &WrapperTx, tx_bytes: &[u8], tx_gas_meter: &RefCell, @@ -1291,7 +1299,7 @@ mod tests { // "execute" a dummy tx, by manually performing its state changes let (dummy_tx, changed_keys, verifiers) = { - let mut tx = Tx::from_type(TxType::Raw); + let mut tx = Tx::from_type(namada_tx::data::TxType::Raw); tx.set_code(namada_tx::Code::new(vec![], None)); tx.set_data(namada_tx::Data::new(vec![])); diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 5b45bc6593..00b1561471 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -142,7 +142,9 @@ where proposer from tendermint raw hash", ) }; - //FIXME: need uni test on fee pyament when inner txs touch balance of fee payer (also run this test on the old version with the bug to check that the bug was indeed there) + // FIXME: need uni test on fee pyament when inner txs touch balance of + // fee payer (also run this test on the old version with the bug to + // check that the bug was indeed there) // Tracks the accepted transactions self.state.in_mem_mut().block.results = BlockResults::default(); @@ -341,7 +343,9 @@ where } // Evaluate the result of a transaction. Commit or drop the storage changes, - // update stats and event, manage replay protection. For successful wrapper transactions return the relevant data and delay the evaluation after the batch execution + // update stats and event, manage replay protection. For successful wrapper + // transactions return the relevant data and delay the evaluation after the + // batch execution fn evaluate_tx_result( &mut self, response: &mut shim::response::FinalizeBlock, @@ -578,7 +582,9 @@ where } } - // Get the transactions from the consensus engine, preprocess and execute them. Return the cache of successful wrapper transactions later used when executing the inner txs. + // Get the transactions from the consensus engine, preprocess and execute + // them. Return the cache of successful wrapper transactions later used when + // executing the inner txs. fn retrieve_and_execute_transactions( &mut self, native_block_proposer_address: &Address, @@ -659,7 +665,6 @@ where continue; } - //FIXME: rename? let (dispatch_args, tx_gas_meter): ( DispatchArgs<'_, WasmCacheRwAccess>, TxGasMeter, @@ -865,7 +870,8 @@ struct ExecutionArgs<'finalize> { height: BlockHeight, } -// Caches the execution of a wrapper transaction to be used when later executing the inner batch +// Caches the execution of a wrapper transaction to be used when later executing +// the inner batch struct WrapperCache { tx: Tx, tx_index: usize, From b27f45e90aaa00b729ea0c5eff43fa87b172628e Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 21 May 2024 18:22:12 +0200 Subject: [PATCH 08/15] Fixes unit tests --- crates/node/src/shell/finalize_block.rs | 62 ++++++++++++++++++------- 1 file changed, 45 insertions(+), 17 deletions(-) diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 00b1561471..102f6d553d 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -358,9 +358,18 @@ where ) -> Option { match dispatch_result { Ok(tx_result) => match tx_data.tx.header.tx_type { - TxType::Wrapper(_) => - // Return withouth emitting any events - { + TxType::Wrapper(_) => { + // FIXME: review this, really need to do all this stuff? + // Wait if the inenr transaction is empty (no cmts?) + self.state.write_log_mut().commit_tx(); + self.state + .in_mem_mut() + .block + .results + .accept(tx_data.tx_index); + tx_logs.tx_event.extend(Code(ResultCode::Ok)); + + // Return withouth emitting any events return Some(WrapperCache { tx: tx_data.tx.to_owned(), tx_index: tx_data.tx_index, @@ -555,9 +564,12 @@ where fn handle_batch_error_reprot(&mut self, err: &Error, tx_data: TxData<'_>) { // If user transaction didn't fail because of out of gas nor - // invalid section commitment, commit its hash to prevent + // replay attempt, commit its hash to prevent // replays - if matches!(tx_data.tx.header.tx_type, TxType::Wrapper(_)) { + // FIXME: really need the match on the tx type? Wrapper cannot get here + // anyway and I don't know if this is a problem with protocol + // transactions + if matches!(tx_data.tx.header.tx_type, TxType::Raw) { if !matches!( err, Error::TxApply(protocol::Error::GasError(_)) @@ -1009,11 +1021,12 @@ impl<'finalize> TempTxLogs { } } Err(e) => { - // this branch can only be reached by inner txs + // FIXME: what about wrong signatures??? tracing::trace!("Inner tx {} failed: {}", cmt_hash, e); // If inner transaction didn't fail because of invalid // section commitment, commit its hash to prevent replays - if matches!(tx_header.tx_type, TxType::Wrapper(_)) + // FIXME: need the match on the tx type? + if matches!(tx_header.tx_type, TxType::Raw) && !matches!(e, protocol::Error::MissingSection(_)) { flags.commit_batch_hash = true; @@ -1304,9 +1317,8 @@ mod test_finalize_block { ) } - /// 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 + /// Check that if a wrapper tx was rejected by [`process_proposal`], the + /// correct event is returned. #[test] fn test_process_proposal_rejected_wrapper_tx() { let (mut shell, _, _, _) = setup(); @@ -1323,24 +1335,40 @@ mod test_finalize_block { ) .unwrap(); + // Need ordered tx hashes because the events can be emitted out of order + let mut ordered_hashes = vec![]; // create some wrapper txs for i in 0u64..4 { - let (_, mut processed_tx) = mk_wrapper_tx(&shell, &keypair); + let (tx, mut processed_tx) = mk_wrapper_tx(&shell, &keypair); processed_tx.result.code = u32::try_from(i.rem_euclid(2)).unwrap(); processed_txs.push(processed_tx); + ordered_hashes.push(tx.header_hash()); } // check that the correct events were created - for (index, event) in shell + for event in shell .finalize_block(FinalizeBlock { txs: processed_txs.clone(), ..Default::default() }) .expect("Test failed") .iter() - .enumerate() { assert_eq!(*event.kind(), APPLIED_TX); + let hash = event.read_attribute::().expect("Test failed"); + let index = ordered_hashes + .iter() + .enumerate() + .find_map( + |(idx, tx_hash)| { + if tx_hash == &hash { + Some(idx) + } else { + None + } + }, + ) + .unwrap(); let code = event .read_attribute::() .expect("Test failed") @@ -3215,10 +3243,10 @@ mod test_finalize_block { // transaction } - /// Test that if a transaction fails because of out-of-gas, - /// invalid signature or wrong section commitment, its hash - /// is not committed to storage. Also checks that a tx failing for other - /// reason has its hash written to storage. + /// Test that if a transaction fails because of out-of-gas, invalid + /// signature or wrong section commitment, its hash is not committed to + /// storage. Also checks that a tx failing for other reasons has its + /// hash written to storage. #[test] fn test_tx_hash_handling() { let (mut shell, _broadcaster, _, _) = setup(); From b0d770828d3aee2cbaff61413e52ec5078bdebba Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 21 May 2024 18:33:55 +0200 Subject: [PATCH 09/15] Removes useless check on tx type for reprot management --- crates/node/src/shell/finalize_block.rs | 55 ++++++++++--------------- 1 file changed, 22 insertions(+), 33 deletions(-) diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 102f6d553d..313b4bd3b5 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -563,34 +563,27 @@ where } fn handle_batch_error_reprot(&mut self, err: &Error, tx_data: TxData<'_>) { - // If user transaction didn't fail because of out of gas nor - // replay attempt, commit its hash to prevent - // replays - // FIXME: really need the match on the tx type? Wrapper cannot get here - // anyway and I don't know if this is a problem with protocol - // transactions - if matches!(tx_data.tx.header.tx_type, TxType::Raw) { - if !matches!( - err, - Error::TxApply(protocol::Error::GasError(_)) - | Error::TxApply(protocol::Error::ReplayAttempt(_)) - ) { - self.commit_batch_hash(tx_data.replay_protection_hashes); - } else if let Error::TxApply(protocol::Error::ReplayAttempt(_)) = - err - { - // Remove the wrapper hash but keep the inner tx - // hash. A replay of the wrapper is impossible since - // the inner tx hash is committed to storage and - // we validate the wrapper against that hash too - let header_hash = tx_data - .replay_protection_hashes - .expect("This cannot fail") - .header_hash; - self.state - .redundant_tx_hash(&header_hash) - .expect("Error while marking tx hash as redundant"); - } + // If user transaction didn't fail because of out of gas nor replay + // attempt, commit its hash to prevent replays. If it failed because of + // a replay attempt just remove the redundant wrapper hash + if !matches!( + err, + Error::TxApply(protocol::Error::GasError(_)) + | Error::TxApply(protocol::Error::ReplayAttempt(_)) + ) { + self.commit_batch_hash(tx_data.replay_protection_hashes); + } else if let Error::TxApply(protocol::Error::ReplayAttempt(_)) = err { + // Remove the wrapper hash but keep the inner tx + // hash. A replay of the wrapper is impossible since + // the inner tx hash is committed to storage and + // we validate the wrapper against that hash too + let header_hash = tx_data + .replay_protection_hashes + .expect("This cannot fail") + .header_hash; + self.state + .redundant_tx_hash(&header_hash) + .expect("Error while marking tx hash as redundant"); } } @@ -1021,14 +1014,10 @@ impl<'finalize> TempTxLogs { } } Err(e) => { - // FIXME: what about wrong signatures??? tracing::trace!("Inner tx {} failed: {}", cmt_hash, e); // If inner transaction didn't fail because of invalid // section commitment, commit its hash to prevent replays - // FIXME: need the match on the tx type? - if matches!(tx_header.tx_type, TxType::Raw) - && !matches!(e, protocol::Error::MissingSection(_)) - { + if !matches!(e, protocol::Error::MissingSection(_)) { flags.commit_batch_hash = true; } From bd6df8d8a1964ff1efa354e061cf08fe1101f6d8 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 21 May 2024 18:47:35 +0200 Subject: [PATCH 10/15] Removes useless operations on wrapper transactions --- crates/node/src/shell/finalize_block.rs | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 313b4bd3b5..216475056e 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -359,16 +359,6 @@ where match dispatch_result { Ok(tx_result) => match tx_data.tx.header.tx_type { TxType::Wrapper(_) => { - // FIXME: review this, really need to do all this stuff? - // Wait if the inenr transaction is empty (no cmts?) - self.state.write_log_mut().commit_tx(); - self.state - .in_mem_mut() - .block - .results - .accept(tx_data.tx_index); - tx_logs.tx_event.extend(Code(ResultCode::Ok)); - // Return withouth emitting any events return Some(WrapperCache { tx: tx_data.tx.to_owned(), @@ -458,7 +448,6 @@ where is_any_tx_invalid, } = temp_log.check_inner_results( &tx_result, - &tx_data.tx.header, tx_data.tx_index, tx_data.height, ); @@ -519,7 +508,6 @@ where is_any_tx_invalid: _, } = temp_log.check_inner_results( &tx_result, - &tx_data.tx.header, tx_data.tx_index, tx_data.height, ); @@ -956,7 +944,6 @@ impl<'finalize> TempTxLogs { fn check_inner_results( &mut self, tx_result: &namada::tx::data::TxResult, - tx_header: &namada::tx::Header, tx_index: usize, height: BlockHeight, ) -> ValidityFlags { From d5cab98289447f86b11316b38cd619548486ec64 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 22 May 2024 12:02:58 +0200 Subject: [PATCH 11/15] Adds integration test for enforced fees --- crates/node/src/shell/finalize_block.rs | 3 - crates/tests/src/integration/ledger_tests.rs | 227 ++++++++++++++++++- 2 files changed, 221 insertions(+), 9 deletions(-) diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 216475056e..4e02325d6b 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -142,9 +142,6 @@ where proposer from tendermint raw hash", ) }; - // FIXME: need uni test on fee pyament when inner txs touch balance of - // fee payer (also run this test on the old version with the bug to - // check that the bug was indeed there) // Tracks the accepted transactions self.state.in_mem_mut().block.results = BlockResults::default(); diff --git a/crates/tests/src/integration/ledger_tests.rs b/crates/tests/src/integration/ledger_tests.rs index 40816bdf67..ef649e2328 100644 --- a/crates/tests/src/integration/ledger_tests.rs +++ b/crates/tests/src/integration/ledger_tests.rs @@ -5,13 +5,15 @@ use assert_matches::assert_matches; use borsh_ext::BorshSerializeExt; use color_eyre::eyre::Result; use data_encoding::HEXLOWER; +use namada::account::AccountPublicKeysMap; use namada::core::collections::HashMap; -use namada::token; -use namada_apps_lib::wallet::defaults; +use namada::token::{self, Amount, DenominatedAmount}; +use namada_apps_lib::wallet::defaults::{self, albert_keypair}; use namada_core::dec::Dec; use namada_core::storage::Epoch; use namada_core::token::NATIVE_MAX_DECIMAL_PLACES; use namada_node::shell::testing::client::run; +use namada_node::shell::testing::node::NodeResults; use namada_node::shell::testing::utils::{Bin, CapturedOutput}; use namada_sdk::tx::{TX_TRANSFER_WASM, VP_USER_WASM}; use namada_test_utils::TestWasms; @@ -1326,10 +1328,8 @@ fn pgf_steward_change_commission() -> Result<()> { assert!( captured.contains(&format!("- 0.7 to {}", defaults::bertha_address())) ); - assert!( - captured - .contains(&format!("- 0.05 to {}", defaults::christel_address())) - ); + assert!(captured + .contains(&format!("- 0.05 to {}", defaults::christel_address()))); assert!(captured.contains("Pgf fundings: no fundings are currently set.")); Ok(()) @@ -1578,3 +1578,218 @@ fn change_validator_metadata() -> Result<()> { Ok(()) } + +// Test that fee payment is enforced and aligned with process proposal. The test +// generates a tx that subtract funds from the fee payer of a following tx. Test +// that wrappers (and fee payments) are evaluated before the inner transactions. +#[test] +fn enforce_fee_payment() -> Result<()> { + // This address doesn't matter for tests. But an argument is required. + let validator_one_rpc = "http://127.0.0.1:26567"; + // 1. start the ledger node + let (node, _services) = setup::setup()?; + + let tempdir = tempfile::tempdir().unwrap(); + let mut txs_bytes = vec![]; + + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + ALBERT_KEY, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 2000000")); + + run( + &node, + Bin::Client, + vec![ + "transfer", + "--source", + ALBERT_KEY, + "--target", + BERTHA, + "--token", + NAM, + "--amount", + // We want this transaction to consume all the remaining available + // balance. If we executed the inner txs right after the + // corresponding wrapper's fee paymwent this would succeed (but + // this is not the case) + "1900000", + "--output-folder-path", + tempdir.path().to_str().unwrap(), + "--dump-tx", + "--ledger-address", + validator_one_rpc, + ], + )?; + node.assert_success(); + let file_path = tempdir + .path() + .read_dir() + .unwrap() + .next() + .unwrap() + .unwrap() + .path(); + txs_bytes.push(std::fs::read(&file_path).unwrap()); + std::fs::remove_file(&file_path).unwrap(); + + run( + &node, + Bin::Client, + vec![ + "transfer", + "--source", + ALBERT_KEY, + "--target", + CHRISTEL, + "--token", + NAM, + "--amount", + "50", + "--gas-payer", + ALBERT_KEY, + "--output-folder-path", + tempdir.path().to_str().unwrap(), + "--dump-tx", + "--ledger-address", + validator_one_rpc, + ], + )?; + node.assert_success(); + let file_path = tempdir + .path() + .read_dir() + .unwrap() + .next() + .unwrap() + .unwrap() + .path(); + txs_bytes.push(std::fs::read(&file_path).unwrap()); + std::fs::remove_file(&file_path).unwrap(); + + let sk = albert_keypair(); + let pk = sk.to_public(); + + let native_token = node + .shell + .lock() + .unwrap() + .state + .in_mem() + .native_token + .clone(); + + let mut txs = vec![]; + for bytes in txs_bytes { + let mut tx = namada::tx::Tx::deserialize(&bytes).unwrap(); + tx.add_wrapper( + namada::tx::data::wrapper_tx::Fee { + amount_per_gas_unit: DenominatedAmount::native( + Amount::native_whole(1), + ), + token: native_token.clone(), + }, + pk.clone(), + 100_000.into(), + ); + tx.sign_raw(vec![sk.clone()], AccountPublicKeysMap::default(), None); + tx.sign_wrapper(sk.clone()); + + txs.push(tx.to_bytes()); + } + + node.clear_results(); + node.submit_txs(txs); + { + let results = node.results.lock().unwrap(); + // If empty than failed in process proposal + assert!(!results.is_empty()); + + for result in results.iter() { + assert!(matches!(result, NodeResults::Ok)); + } + } + // Finalize the next block to execute the txs + node.clear_results(); + node.finalize_and_commit(); + { + let results = node.results.lock().unwrap(); + for result in results.iter() { + assert!(matches!(result, NodeResults::Ok)); + } + } + + // Assert balances + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + ALBERT_KEY, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + // This is the result of the two fee payemnts and the successful transfer to + // Christel + assert!(captured.contains("nam: 1799950")); + + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + BERTHA, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + // Bertha must not receive anything because the transaction fails. This is + // because we evaluate fee payments before the inner transactions, so by the + // time we execute the transfer, Albert doesn't have enough funds anynmore + assert!(captured.contains("nam: 2000000")); + + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + CHRISTEL, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 2000050")); + Ok(()) +} From b6f200adba9232a906ed2ff151ac44e0994aa5cf Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 22 May 2024 15:09:04 +0200 Subject: [PATCH 12/15] Misc adjustments to v36 --- crates/namada/src/ledger/protocol/mod.rs | 2 +- crates/node/src/shell/finalize_block.rs | 23 +++++++++++++++++--- crates/tests/src/integration/ledger_tests.rs | 8 ++++--- 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/crates/namada/src/ledger/protocol/mod.rs b/crates/namada/src/ledger/protocol/mod.rs index 8045814ec6..2a374a65c0 100644 --- a/crates/namada/src/ledger/protocol/mod.rs +++ b/crates/namada/src/ledger/protocol/mod.rs @@ -210,7 +210,7 @@ pub enum DispatchArgs<'a, CA: 'static + WasmCacheAccess + Sync> { /// environment, in which case validity predicates will be bypassed. pub fn dispatch_tx<'a, D, H, CA>( tx: &Tx, - dispatch_args: DispatchArgs, + dispatch_args: DispatchArgs<'a, CA>, tx_gas_meter: &'a RefCell, state: &'a mut WlState, ) -> std::result::Result, DispatchError> diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 4e02325d6b..215ed23420 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -584,7 +584,7 @@ where changed_keys, stats, height, - }: ExecutionArgs, + }: ExecutionArgs<'_>, ) -> Vec { let mut successful_wrappers = vec![]; @@ -661,7 +661,24 @@ where ) = match &tx_header.tx_type { TxType::Wrapper(wrapper) => { stats.increment_wrapper_txs(); - let tx_gas_meter = TxGasMeter::new(wrapper.gas_limit); + + let gas_limit = match Gas::try_from(wrapper.gas_limit) { + Ok(value) => value, + Err(_) => { + response.events.emit( + new_tx_event(&tx, height.0) + .with(Code(ResultCode::InvalidTx)) + .with(Info( + "The wrapper gas limit overflowed gas \ + representation" + .to_owned(), + )) + .with(GasUsed(0.into())), + ); + continue; + } + }; + let tx_gas_meter = TxGasMeter::new(gas_limit); for cmt in tx.commitments() { if let Some(code_sec) = tx .get_section(cmt.code_sechash()) @@ -793,7 +810,7 @@ where changed_keys, stats, height, - }: ExecutionArgs, + }: ExecutionArgs<'_>, ) { for WrapperCache { mut tx, diff --git a/crates/tests/src/integration/ledger_tests.rs b/crates/tests/src/integration/ledger_tests.rs index ef649e2328..561e93df49 100644 --- a/crates/tests/src/integration/ledger_tests.rs +++ b/crates/tests/src/integration/ledger_tests.rs @@ -1328,8 +1328,10 @@ fn pgf_steward_change_commission() -> Result<()> { assert!( captured.contains(&format!("- 0.7 to {}", defaults::bertha_address())) ); - assert!(captured - .contains(&format!("- 0.05 to {}", defaults::christel_address()))); + assert!( + captured + .contains(&format!("- 0.05 to {}", defaults::christel_address())) + ); assert!(captured.contains("Pgf fundings: no fundings are currently set.")); Ok(()) @@ -1696,7 +1698,7 @@ fn enforce_fee_payment() -> Result<()> { for bytes in txs_bytes { let mut tx = namada::tx::Tx::deserialize(&bytes).unwrap(); tx.add_wrapper( - namada::tx::data::wrapper_tx::Fee { + namada::tx::data::wrapper::Fee { amount_per_gas_unit: DenominatedAmount::native( Amount::native_whole(1), ), From 3c526c4e3fcd27cb280f65f33e033189d82d935c Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 23 May 2024 17:58:59 +0200 Subject: [PATCH 13/15] Fixes typo --- crates/namada/src/ledger/protocol/mod.rs | 2 +- crates/tests/src/integration/ledger_tests.rs | 2 +- crates/tx/src/data/protocol.rs | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/namada/src/ledger/protocol/mod.rs b/crates/namada/src/ledger/protocol/mod.rs index 2a374a65c0..5d67b45d3d 100644 --- a/crates/namada/src/ledger/protocol/mod.rs +++ b/crates/namada/src/ledger/protocol/mod.rs @@ -180,7 +180,7 @@ impl From for DispatchError { /// Arguments for transactions' execution pub enum DispatchArgs<'a, CA: 'static + WasmCacheAccess + Sync> { - /// Protocoli tx data + /// Protocol tx data Protocol(&'a ProtocolTx), /// Raw tx data Raw { diff --git a/crates/tests/src/integration/ledger_tests.rs b/crates/tests/src/integration/ledger_tests.rs index 561e93df49..04138c2eff 100644 --- a/crates/tests/src/integration/ledger_tests.rs +++ b/crates/tests/src/integration/ledger_tests.rs @@ -1626,7 +1626,7 @@ fn enforce_fee_payment() -> Result<()> { "--amount", // We want this transaction to consume all the remaining available // balance. If we executed the inner txs right after the - // corresponding wrapper's fee paymwent this would succeed (but + // corresponding wrapper's fee payment this would succeed (but // this is not the case) "1900000", "--output-folder-path", diff --git a/crates/tx/src/data/protocol.rs b/crates/tx/src/data/protocol.rs index 6f0125aaf3..fd994cb47c 100644 --- a/crates/tx/src/data/protocol.rs +++ b/crates/tx/src/data/protocol.rs @@ -67,7 +67,6 @@ impl ProtocolTx { Deserialize, PartialEq, )] -#[allow(clippy::large_enum_variant)] /// Types of protocol messages to be sent pub enum ProtocolTxType { /// Ethereum events contained in vote extensions that From e82d57d54c4d3c99699e73d17892fda981d47c12 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 22 May 2024 15:55:57 +0200 Subject: [PATCH 14/15] Changelog #3075 --- .changelog/unreleased/bug-fixes/3075-force-fee-payment.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/3075-force-fee-payment.md diff --git a/.changelog/unreleased/bug-fixes/3075-force-fee-payment.md b/.changelog/unreleased/bug-fixes/3075-force-fee-payment.md new file mode 100644 index 0000000000..7b15a59a63 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/3075-force-fee-payment.md @@ -0,0 +1,2 @@ +- Fixed the fee collection logic in `finalize_block` to match that of + `process_proposal`. ([\#3075](https://github.com/anoma/namada/issues/3075)) \ No newline at end of file From d063ac19f4b469eff5b9ed4aba7c1d30b6318993 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 24 May 2024 10:09:33 +0200 Subject: [PATCH 15/15] Fmt --- crates/node/src/shell/finalize_block.rs | 430 ++++++++++++++---------- 1 file changed, 253 insertions(+), 177 deletions(-) diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 215ed23420..ee65f3b057 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -1351,11 +1351,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(); @@ -2949,21 +2945,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(); @@ -3010,22 +3010,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 @@ -3103,26 +3107,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() + ); } } @@ -3405,23 +3417,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(); @@ -3430,23 +3448,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] @@ -3506,14 +3534,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 @@ -3911,9 +3943,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(), @@ -3932,17 +3966,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 { @@ -3965,9 +4003,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, @@ -4451,11 +4491,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"); @@ -4520,18 +4562,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( @@ -5409,9 +5455,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(); @@ -5484,11 +5532,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 @@ -5526,18 +5576,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"] { @@ -5568,21 +5624,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!( @@ -5593,10 +5655,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 @@ -5628,18 +5692,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"] { @@ -5669,18 +5739,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!(