From bbead89c54288b6fb38eaab61963ce4e49eee2ee Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 22 Sep 2023 17:07:22 +0200 Subject: [PATCH] Refactors tx hash removal out of `finalize_block` --- .../lib/node/ledger/shell/finalize_block.rs | 332 ++++++++---------- apps/src/lib/node/ledger/shell/governance.rs | 1 - apps/src/lib/node/ledger/shell/mod.rs | 1 - shared/src/ledger/protocol/mod.rs | 42 ++- shared/src/ledger/queries/shell.rs | 1 - 5 files changed, 171 insertions(+), 206 deletions(-) diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index a9932b853ab..d0eb9a4a8b2 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -282,178 +282,167 @@ where continue; } - let ( - mut tx_event, - decrypted_tx_hash, - mut tx_gas_meter, - has_valid_pow, - wrapper, - ) = match &tx_header.tx_type { - TxType::Wrapper(wrapper) => { - stats.increment_wrapper_txs(); - let tx_event = Event::new_tx_event(&tx, height.0); - #[cfg(not(feature = "mainnet"))] - let has_valid_pow = - self.invalidate_pow_solution_if_valid(wrapper); - - let gas_meter = TxGasMeter::new(wrapper.gas_limit); - - ( - tx_event, - None, - gas_meter, + let (mut tx_event, mut tx_gas_meter, has_valid_pow, wrapper) = + match &tx_header.tx_type { + TxType::Wrapper(wrapper) => { + stats.increment_wrapper_txs(); + let tx_event = Event::new_tx_event(&tx, height.0); #[cfg(not(feature = "mainnet"))] - has_valid_pow, - Some(tx.clone()), - ) - } - TxType::Decrypted(inner) => { - // We remove the corresponding wrapper tx from the queue - let mut tx_in_queue = self - .wl_storage - .storage - .tx_queue - .pop() - .expect("Missing wrapper tx in queue"); - let mut event = Event::new_tx_event(&tx, height.0); - - match inner { - DecryptedTx::Decrypted { has_valid_pow: _ } => { - if let Some(code_sec) = tx - .get_section(tx.code_sechash()) - .and_then(|x| Section::code_sec(x.as_ref())) - { - stats.increment_tx_type( - code_sec.code.hash().to_string(), - ); + let has_valid_pow = + self.invalidate_pow_solution_if_valid(wrapper); + + let gas_meter = TxGasMeter::new(wrapper.gas_limit); + + ( + tx_event, + gas_meter, + #[cfg(not(feature = "mainnet"))] + has_valid_pow, + Some(tx.clone()), + ) + } + TxType::Decrypted(inner) => { + // We remove the corresponding wrapper tx from the queue + let mut tx_in_queue = self + .wl_storage + .storage + .tx_queue + .pop() + .expect("Missing wrapper tx in queue"); + let mut event = Event::new_tx_event(&tx, height.0); + + match inner { + DecryptedTx::Decrypted { has_valid_pow: _ } => { + if let Some(code_sec) = tx + .get_section(tx.code_sechash()) + .and_then(|x| Section::code_sec(x.as_ref())) + { + stats.increment_tx_type( + code_sec.code.hash().to_string(), + ); + } } - } - DecryptedTx::Undecryptable => { - tracing::info!( - "Tx with hash {} was un-decryptable", - tx_in_queue.tx.header_hash() - ); - // Remove inner tx hash from storage - let inner_tx_hash = + DecryptedTx::Undecryptable => { + tracing::info!( + "Tx with hash {} was un-decryptable", + tx_in_queue.tx.header_hash() + ); + // Remove inner tx hash from storage + let inner_tx_hash = replay_protection::get_replay_protection_key( &tx_in_queue .tx .update_header(TxType::Raw) .header_hash(), ); - self.wl_storage.delete(&inner_tx_hash).expect( - "Error while deleting tx hash from storage", - ); - event["info"] = "Transaction is invalid.".into(); - event["log"] = - "Transaction could not be decrypted.".into(); - event["code"] = ErrorCodes::Undecryptable.into(); - continue; + self.wl_storage.delete(&inner_tx_hash).expect( + "Error while deleting tx hash from storage", + ); + event["info"] = + "Transaction is invalid.".into(); + event["log"] = "Transaction could not be \ + decrypted." + .into(); + event["code"] = + ErrorCodes::Undecryptable.into(); + continue; + } } - } - ( - event, - Some( - tx_in_queue - .tx - .update_header(TxType::Raw) - .header_hash(), + ( + event, + TxGasMeter::new_from_sub_limit(tx_in_queue.gas), + #[cfg(not(feature = "mainnet"))] + false, + None, + ) + } + 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 => ( + Event::new_tx_event(&tx, height.0), + TxGasMeter::new_from_sub_limit(0.into()), + #[cfg(not(feature = "mainnet"))] + false, + None, ), - TxGasMeter::new_from_sub_limit(tx_in_queue.gas), - #[cfg(not(feature = "mainnet"))] - false, - None, - ) - } - 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 => ( - Event::new_tx_event(&tx, height.0), - None, - TxGasMeter::new_from_sub_limit(0.into()), - #[cfg(not(feature = "mainnet"))] - false, - None, - ), - ProtocolTxType::EthEventsVext => { - let ext = + 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); + 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); + } } + ( + Event::new_tx_event(&tx, height.0), + TxGasMeter::new_from_sub_limit(0.into()), + #[cfg(not(feature = "mainnet"))] + false, + None, + ) } - ( - Event::new_tx_event(&tx, height.0), - None, - TxGasMeter::new_from_sub_limit(0.into()), - #[cfg(not(feature = "mainnet"))] - false, - None, - ) - } - ProtocolTxType::EthereumEvents => { - let digest = + 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.wl_storage.storage.get_last_block_height(), - ); - for MultiSignedEthEvent { event, signers } in - &digest.events + if let Some(address) = + self.mode.get_validator_address().cloned() { - if signers.contains(this_signer) { - self.mode.dequeue_eth_event(event); + let this_signer = &( + address, + self.wl_storage + .storage + .get_last_block_height(), + ); + for MultiSignedEthEvent { event, signers } in + &digest.events + { + if signers.contains(this_signer) { + self.mode.dequeue_eth_event(event); + } } } + ( + Event::new_tx_event(&tx, height.0), + TxGasMeter::new_from_sub_limit(0.into()), + #[cfg(not(feature = "mainnet"))] + false, + None, + ) } - ( - Event::new_tx_event(&tx, height.0), - None, - TxGasMeter::new_from_sub_limit(0.into()), - #[cfg(not(feature = "mainnet"))] - false, - None, - ) - } - ref protocol_tx_type => { - tracing::error!( - ?protocol_tx_type, - "Internal logic error: FinalizeBlock received an \ - unsupported TxType::Protocol transaction: {:?}", - protocol_tx - ); - continue; - } - }, - }; - - let mut invalid_sig_sentinel = false; + ref protocol_tx_type => { + tracing::error!( + ?protocol_tx_type, + "Internal logic error: FinalizeBlock received \ + an unsupported TxType::Protocol transaction: \ + {:?}", + protocol_tx + ); + continue; + } + }, + }; match protocol::dispatch_tx( tx, @@ -464,7 +453,6 @@ where .expect("transaction index out of bounds"), ), &mut tx_gas_meter, - &mut invalid_sig_sentinel, &mut self.wl_storage, &mut self.vp_wasm_cache, &mut self.tx_wasm_cache, @@ -536,21 +524,6 @@ where result.vps_result.rejected_vps ); - if let Some(hash) = decrypted_tx_hash { - if invalid_sig_sentinel { - // Invalid signature was found, remove the tx - // hash from storage to allow replay - let tx_hash_key = - replay_protection::get_replay_protection_key( - &hash, - ); - self.wl_storage.delete(&tx_hash_key).expect( - "Error while deleting tx hash key from \ - storage", - ); - } - } - stats.increment_rejected_txs(); self.wl_storage.drop_tx(); tx_event["code"] = ErrorCodes::InvalidTx.into(); @@ -565,37 +538,6 @@ where msg ); - if let Some(hash) = decrypted_tx_hash { - if invalid_sig_sentinel { - // Invalid signature was found, remove the tx - // hash from storage to allow replay - let tx_hash_key = - replay_protection::get_replay_protection_key( - &hash, - ); - self.wl_storage.delete(&tx_hash_key).expect( - "Error while deleting tx hash key from storage", - ); - } - } - - // If transaction type is Decrypted and failed because of - // out of gas, remove its hash from storage to allow - // rewrapping it - if let Some(hash) = decrypted_tx_hash { - if let Error::TxApply(protocol::Error::GasError(_)) = - msg - { - let tx_hash_key = - replay_protection::get_replay_protection_key( - &hash, - ); - self.wl_storage.delete(&tx_hash_key).expect( - "Error while deleting tx hash key from storage", - ); - } - } - stats.increment_errored_txs(); self.wl_storage.drop_tx(); diff --git a/apps/src/lib/node/ledger/shell/governance.rs b/apps/src/lib/node/ledger/shell/governance.rs index 2b2c2893e5d..7e9d72eac37 100644 --- a/apps/src/lib/node/ledger/shell/governance.rs +++ b/apps/src/lib/node/ledger/shell/governance.rs @@ -280,7 +280,6 @@ where * need it here. */ TxIndex::default(), &mut TxGasMeter::new_from_sub_limit(u64::MAX.into()), /* No gas limit for governance proposal */ - &mut false, &mut shell.wl_storage, &mut shell.vp_wasm_cache, &mut shell.tx_wasm_cache, diff --git a/apps/src/lib/node/ledger/shell/mod.rs b/apps/src/lib/node/ledger/shell/mod.rs index fb58d8df6ca..46a985b8fcf 100644 --- a/apps/src/lib/node/ledger/shell/mod.rs +++ b/apps/src/lib/node/ledger/shell/mod.rs @@ -1431,7 +1431,6 @@ where match apply_wasm_tx( unshield, &TxIndex::default(), - &mut false, ShellParams::new( &mut TxGasMeter::new(fee_unshielding_gas_limit), temp_wl_storage, diff --git a/shared/src/ledger/protocol/mod.rs b/shared/src/ledger/protocol/mod.rs index 2dcb38a0005..496b0c7f80d 100644 --- a/shared/src/ledger/protocol/mod.rs +++ b/shared/src/ledger/protocol/mod.rs @@ -143,7 +143,6 @@ pub fn dispatch_tx<'a, D, H, CA>( tx_bytes: &'a [u8], tx_index: TxIndex, tx_gas_meter: &'a mut TxGasMeter, - invalid_sig_sentinel: &'a mut bool, wl_storage: &'a mut WlStorage, vp_wasm_cache: &'a mut VpCache, tx_wasm_cache: &'a mut TxCache, @@ -163,7 +162,6 @@ where }) => apply_wasm_tx( tx, &tx_index, - invalid_sig_sentinel, ShellParams { tx_gas_meter, wl_storage, @@ -350,7 +348,6 @@ where match apply_wasm_tx( fee_unshielding_tx, &TxIndex::default(), - &mut false, ShellParams { tx_gas_meter: &mut tx_gas_meter, wl_storage: *wl_storage, @@ -614,7 +611,6 @@ where pub fn apply_wasm_tx<'a, D, H, CA, WLS>( tx: Tx, tx_index: &TxIndex, - invalid_sig_sentinel: &mut bool, shell_params: ShellParams<'a, CA, WLS>, #[cfg(not(feature = "mainnet"))] has_valid_pow: bool, ) -> Result @@ -652,18 +648,48 @@ where tx_wasm_cache, )?; - let vps_result = check_vps(CheckVps { + let mut invalid_sig_sentinel = false; + let vps_result = match check_vps(CheckVps { tx: &tx, tx_index, storage, tx_gas_meter, - invalid_sig_sentinel, + invalid_sig_sentinel: &mut invalid_sig_sentinel, write_log, verifiers_from_tx: &verifiers, vp_wasm_cache, #[cfg(not(feature = "mainnet"))] has_valid_pow, - })?; + }) { + Ok(result) => { + if invalid_sig_sentinel && !result.rejected_vps.is_empty() { + // If invalid signature was found and transaction was rejected, + // remove the tx hash from storage to allow + // replay + let tx_hash_key = replay_protection::get_replay_protection_key( + &tx.clone().update_header(TxType::Raw).header_hash(), + ); + write_log + .protocol_delete(&tx_hash_key) + .expect("Error while deleting tx hash key from storage"); + } + result + } + Err(e) => { + // If transaction failed because of + // out of gas or an invalid signature was found, remove its hash + // from storage to allow rewrapping it + if invalid_sig_sentinel || matches!(e, Error::GasError(_)) { + let tx_hash_key = replay_protection::get_replay_protection_key( + &tx.clone().update_header(TxType::Raw).header_hash(), + ); + write_log + .protocol_delete(&tx_hash_key) + .expect("Error while deleting tx hash key from storage"); + } + return Err(e); + } + }; let gas_used = tx_gas_meter.get_tx_consumed_gas(); let initialized_accounts = write_log.get_initialized_accounts(); @@ -1137,7 +1163,7 @@ where Err(err) => match err { // Execution of VPs can (and must) be short-circuited // only in case of a gas overflow to prevent the - // transaction from conuming resources that have not + // transaction from consuming resources that have not // been acquired in the corresponding wrapper tx. For // all the other errors we keep evaluating the vps. This // allow to display a consistent VpsResult accross all diff --git a/shared/src/ledger/queries/shell.rs b/shared/src/ledger/queries/shell.rs index 2efc42416d9..2db40f0a072 100644 --- a/shared/src/ledger/queries/shell.rs +++ b/shared/src/ledger/queries/shell.rs @@ -181,7 +181,6 @@ where let mut data = protocol::apply_wasm_tx( tx, &TxIndex(0), - &mut false, ShellParams::new( &mut tx_gas_meter, &mut temp_wl_storage,