Refactor/Simplify validation logic #1847
Conversation
…with unimplemented code
MitchTurner
force-pushed
the
refactor/remove-production-logic-from-validation
branch
from
4e4be31
to
6e1762e
Compare
MitchTurner
changed the title
| #[cfg(debug_assertions)] | ||
| if _new_transaction != transaction { | ||
| error!("Provided transaction does not match the transaction built by the executor"); | ||
| } |
There was a problem hiding this comment.
I'm not sure I really like this. It's kinda chatty in the tests. Is there another approach we could take? We can't have it throw an error because the tests are expecting the error that will be used in prod.
There was a problem hiding this comment.
Instead of comparing transactions here, we can do that inside of the check_block_matches. You can even iterate over the block's transactions there and compare transactions and have a behavior like before the change(return InvalidTransactionOutcome)=)
| /// UTXO Validation flag, when disabled the executor skips signature and UTXO existence checks | ||
| pub utxo_validation: bool, | ||
| pub extra_tx_checks: bool, |
There was a problem hiding this comment.
This should simply be named after the situation we want to use it. I'm not sure what it is yet. For now, I just named it extra_tx_checks which is vague, but utxo_validation was incorrect, so it's an improvement.
| debug!( | ||
| "Block {:#x} fees: {} gas: {}", | ||
| pre_exec_block_id.unwrap_or(finalized_block_id), | ||
| coinbase, | ||
| used_gas | ||
| ); |
There was a problem hiding this comment.
I think we still want to have debug log here=)
There was a problem hiding this comment.
We want to have one log for the executor and one log for validator
| &mut tx_st_transaction, | ||
| )?; | ||
| tx_st_transaction.commit()?; | ||
| tx | ||
| }; | ||
|
|
||
| block.transactions.push(tx); | ||
| block.transactions.push(tx.clone()); |
There was a problem hiding this comment.
It may be too expensive to clone transaction. You could return a reference to it. Like block.transactions.as_ref().expect("We just added transaction") instead.
There was a problem hiding this comment.
Turns out we don't need it because we are inspecting the txs after.
| let mut partial_block = PartialFuelBlock::from(block.clone()); | ||
| let transactions = core::mem::take(&mut partial_block.transactions); |
There was a problem hiding this comment.
It would be nice if you avoided cloning of the block here=) You don't need to modify transactions, so you can work with the reference
| #[cfg(debug_assertions)] | ||
| if _new_transaction != transaction { | ||
| error!("Provided transaction does not match the transaction built by the executor"); | ||
| } |
There was a problem hiding this comment.
Instead of comparing transactions here, we can do that inside of the check_block_matches. You can even iterate over the block's transactions there and compare transactions and have a behavior like before the change(return InvalidTransactionOutcome)=)
| fn check_mint_amount(mint: &Mint, expected_amount: u64) -> ExecutorResult<()> { | ||
| if *mint.mint_amount() != expected_amount { | ||
| return Err(ExecutorError::CoinbaseAmountMismatch) | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| let vm_result: StateTransition<_> = vm | ||
| .transact(ready_tx) | ||
| .map_err(|error| ExecutorError::VmExecution { | ||
| error: error.to_string(), | ||
| transaction_id: tx_id, | ||
| })? | ||
| .into(); | ||
| let reverted = vm_result.should_revert(); | ||
| fn check_gas_price(mint: &Mint, expected_gas_price: Word) -> ExecutorResult<()> { | ||
| if *mint.gas_price() != expected_gas_price { | ||
| return Err(ExecutorError::CoinbaseGasPriceMismatch) | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| let (state, mut tx, receipts): (_, Tx, _) = vm_result.into_inner(); | ||
| #[cfg(debug_assertions)] | ||
| { | ||
| tx.precompute(&self.consensus_params.chain_id())?; | ||
| debug_assert_eq!(tx.id(&self.consensus_params.chain_id()), tx_id); | ||
| fn check_mint_has_expected_index( | ||
| checked_mint: &Checked<Mint>, | ||
| execution_data: &ExecutionData, | ||
| ) -> ExecutorResult<()> { | ||
| if checked_mint.transaction().tx_pointer().tx_index() != execution_data.tx_count { | ||
| return Err(ExecutorError::MintHasUnexpectedIndex) | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
I think adding 3 new methods, 3 lines each, is overkill since the if statement and return error type are already descriptive enough.
Could you return it back, please=)
There was a problem hiding this comment.
I strongly prefer the new code. I think the reduction of noise and keeping the caller function as flat as possible is worth the cost of maintaining another function. These benefits add up quickly and I'd like to go farther tbh but I think it's time to wrap up this PR.
| } | ||
|
|
||
| #[allow(clippy::too_many_arguments)] | ||
| fn execute_chargeable_transaction<Tx, T>( | ||
| fn execute_mint_with_vm<T>( |
There was a problem hiding this comment.
It seems this new function shifted execute_chargeable_transaction below and the change diff looks bad now. Could you swap execute_mint_with_vm with execute_chargeable_transaction please for the history sake
| block_st_transaction, | ||
| &coinbase_id, | ||
| &mut mint, | ||
| &mut inputs, |
There was a problem hiding this comment.
It would be nice if you pass input instead=)
| tx_st_transaction: &mut StorageTransaction<T>, | ||
| ) -> ExecutorResult<Transaction> | ||
| where | ||
| Tx: ExecutableTransaction + PartialEq + Cacheable + Send + Sync + 'static, |
There was a problem hiding this comment.
| Tx: ExecutableTransaction + PartialEq + Cacheable + Send + Sync + 'static, | |
| Tx: ExecutableTransaction + Cacheable + Send + Sync + 'static, |
| let fee_params = self.consensus_params.fee_params(); | ||
|
|
||
| let ready_tx = checked_tx | ||
| .clone() |
There was a problem hiding this comment.
Now(because we don't need to compare checked_tx with tx) you can create a vector of predicate_gas_used from inputs and then you don't need to checked_tx.clone()=)
| @@ -1349,7 +1263,35 @@ where | |||
| tx.outputs(), | |||
| )?; | |||
|
|
|||
| // We always need to update outputs with storage state after execution. | |||
| self.update_tx_outputs(tx_st_transaction, tx_id, &mut tx)?; | |||
There was a problem hiding this comment.
I know that it was before like this, but it would be nice if you moved this function to the attempt_tx_execution_with_vm. Then we can remove mut part from the tx=)
| block_st_transaction, | ||
| inputs.as_mut_slice(), | ||
| std::slice::from_ref(&input), |
There was a problem hiding this comment.
| std::slice::from_ref(&input), | |
| core::slice::from_ref(&input), |
| let mut sub_block_db_commit = block_st_transaction | ||
| .write_transaction() | ||
| .with_policy(ConflictPolicy::Overwrite); | ||
| self.compute_inputs(std::slice::from_mut(&mut input), block_st_transaction)?; |
There was a problem hiding this comment.
| self.compute_inputs(std::slice::from_mut(&mut input), block_st_transaction)?; | |
| self.compute_inputs(core::slice::from_mut(&mut input), block_st_transaction)?; |
|
|
||
| let block_height = *header.height(); | ||
| Self::check_mint_amount(&mint, execution_data.coinbase)?; | ||
| Self::check_gas_price(&mint, gas_price)?; |
There was a problem hiding this comment.
I think we need to move this check before if statement, otherwise in the case of zero CotnractId it is possible to lie about the gas price=)
| tx.outputs(), | ||
| coinbase_id, | ||
| block_st_transaction, | ||
| std::slice::from_ref(input), |
There was a problem hiding this comment.
| std::slice::from_ref(input), | |
| core::slice::from_ref(input), |
| )?; | ||
| self.compute_state_of_not_utxo_outputs( | ||
| outputs.as_mut_slice(), | ||
| std::slice::from_ref(input), |
There was a problem hiding this comment.
| std::slice::from_ref(input), | |
| core::slice::from_ref(input), |
## Version v0.26.0 ### Fixed #### Breaking - [#1868](#1868): Include the `event_inbox_root` in the header hash. Changed types of the `transactions_count` to `u16` and `message_receipt_count` to `u32` instead of `u64`. Updated the application hash root calculation to not pad numbers. - [#1866](#1866): Fixed a runtime panic that occurred when restarting a node. The panic happens when the relayer database is already populated, and the relayer attempts an empty commit during start up. This invalid commit is removed in this PR. - [#1871](#1871): Fixed `block` endpoint to return fetch the blocks from both databases after regenesis. - [#1856](#1856): Replaced instances of `Union` with `Enum` for GraphQL definitions of `ConsensusParametersVersion` and related types. This is needed because `Union` does not support multiple `Version`s inside discriminants or empty variants. - [#1870](#1870): Fixed benchmarks for the `0.25.3`. - [#1870](#1870): Improves the performance of getting the size of the contract from the `InMemoryTransaction`. - [#1851](#1851): Provided migration capabilities (enabled addition of new column families) to RocksDB instance. ### Added - [#1853](#1853): Added a test case to verify the database's behavior when new columns are added to the RocksDB database. - [#1860](#1860): Regenesis now preserves `FuelBlockIdsToHeights` off-chain table. ### Changed - [#1847](#1847): Simplify the validation interface to use `Block`. Remove `Validation` variant of `ExecutionKind`. - [#1832](#1832): Snapshot generation can be cancelled. Progress is also reported. - [#1837](#1837): Refactor the executor and separate validation from the other use cases ## What's Changed * Weekly `cargo update` by @github-actions in #1850 * Refactor/separate validation from other executions by @MitchTurner in #1837 * fix: Use `Enum` for `ConsensusParametersVersion` and related types by @bvrooman in #1856 * feat: snapshot generation graceful shutdown by @segfault-magnet in #1832 * regenesis: migrate FuelBlockIdsToHeights by @Dentosal in #1860 * Weekly `cargo update` by @github-actions in #1869 * Refactor/Simplify validation logic by @MitchTurner in #1847 * Fixed `block` endpoint to return fetch the blocks from both databases after regenesis by @xgreenx in #1871 * Add Eq and Partial Eq to tx response and status by @MujkicA in #1872 * test: restart with relayer data by @bvrooman in #1866 * Fix `BlockHeader` hash by @MitchTurner in #1868 * Added a test for the case of adding new columns into the existing RocksDB database by @xgreenx in #1853 * Fixed benchmarks for the `0.25.3` by @xgreenx in #1870 **Full Changelog**: v0.25.3...v0.26.0
Part of: #1751
Have
validationfunctions just take aBlockrather than components. RemoveValidationvariant ofExecutionKind. Clean up a bunch of other little details by removing generalizations that existed before for things like txsource.A lot of these changes are breaking down huge functions and trying to make it a bit more declarative. This will hopefully mitigate the need for hand-holding comments all the way down and you can just read the name of the functions to know what is going on. For example:
became
about 20x better.
I'm still kinda unsatisfied with the clarity with some spots though. For example, I'm not sure what state is supposed to change in the case of a
revert, I'm just trusting the order things existed before and the tests. I think it could be better to have a match case forif reverted { ... } else { ... }instead of threading therevertedbool through to subsequent functions.Checklist
Before requesting review
After merging, notify other teams
[Add or remove entries as needed]