feat(katana): pool validation #2344
Conversation
kariy
changed the title
WalkthroughOhayo, sensei! The changes involve significant updates across multiple files in the Changes
Recent review detailsConfiguration used: .coderabbit.yaml Files ignored due to path filters (1)
Files selected for processing (27)
Files skipped from review due to trivial changes (3)
Additional comments not posted (69)
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| #[allow(missing_debug_implementations)] | ||
| struct StatefulValidatorAdapter { | ||
| inner: StatefulValidator<StateProviderDb<'static>>, | ||
| } | ||
|
|
||
| impl StatefulValidatorAdapter { | ||
| fn new( | ||
| state: Box<dyn StateProvider>, | ||
| block_env: &BlockEnv, | ||
| cfg_env: &CfgEnv, | ||
| ) -> StatefulValidatorAdapter { | ||
| let inner = Self::new_inner(state, block_env, cfg_env); | ||
| Self { inner } | ||
| } | ||
|
|
||
| fn new_inner( | ||
| state: Box<dyn StateProvider>, | ||
| block_env: &BlockEnv, | ||
| cfg_env: &CfgEnv, | ||
| ) -> StatefulValidator<StateProviderDb<'static>> { | ||
| let state = CachedState::new(StateProviderDb::new(state)); | ||
| let block_context = block_context_from_envs(block_env, cfg_env); | ||
| StatefulValidator::create(state, block_context) | ||
| } | ||
|
|
||
| /// Used only in the [`Validator::validate`] trait | ||
| fn validate( | ||
| &mut self, | ||
| tx: ExecutableTxWithHash, | ||
| skip_validate: bool, | ||
| skip_fee_check: bool, | ||
| ) -> ValidationResult<ExecutableTxWithHash> { | ||
| match to_executor_tx(tx.clone()) { | ||
| Transaction::AccountTransaction(blockifier_tx) => { | ||
| // Check if the transaction nonce is higher than the current account nonce, | ||
| // if yes, dont't run its validation logic but tag it as dependent | ||
| let account = to_blk_address(tx.sender()); | ||
| let account_nonce = self.inner.get_nonce(account).expect("state err"); | ||
|
|
||
| if tx.nonce() > account_nonce.0 { | ||
| return Ok(ValidationOutcome::Dependent { | ||
| current_nonce: account_nonce.0, | ||
| tx_nonce: tx.nonce(), | ||
| tx, | ||
| }); | ||
| } | ||
|
|
||
| match self.inner.perform_validations(blockifier_tx, skip_validate, skip_fee_check) { | ||
| Ok(()) => Ok(ValidationOutcome::Valid(tx)), | ||
| Err(e) => match map_invalid_tx_err(e) { | ||
| Ok(error) => Ok(ValidationOutcome::Invalid { tx, error }), | ||
| Err(error) => Err(Error { hash: tx.hash, error }), | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| // we skip validation for L1HandlerTransaction | ||
| Transaction::L1HandlerTransaction(_) => Ok(ValidationOutcome::Valid(tx)), | ||
| } | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Consider improving error handling in validate.
The validate method uses expect which could cause a panic. Consider handling the error more gracefully.
Apply this diff to improve error handling:
- let account_nonce = self.inner.get_nonce(account).expect("state err");
+ let account_nonce = self.inner.get_nonce(account).unwrap_or_else(|_| {
+ panic!("Failed to get nonce for account: {:?}", account);
+ });Committable suggestion was skipped due to low confidence.
| impl Validator for TxValidator { | ||
| type Transaction = ExecutableTxWithHash; | ||
|
|
||
| fn validate(&self, tx: Self::Transaction) -> ValidationResult<Self::Transaction> { | ||
| let this = &mut *self.validator.lock(); | ||
|
|
||
| // Check if validation of an invoke transaction should be skipped due to deploy_account not | ||
| // being proccessed yet. This feature is used to improve UX for users sending | ||
| // deploy_account + invoke at once. | ||
| let skip_validate = match tx.transaction { | ||
| // we skip validation for invoke tx with nonce 1 and nonce 0 in the state, this | ||
| ExecutableTx::DeployAccount(_) | ExecutableTx::Declare(_) => false, | ||
|
|
||
| // we skip validation for invoke tx with nonce 1 and nonce 0 in the state, this | ||
| _ => { | ||
| let address = to_blk_address(tx.sender()); | ||
| let account_nonce = this.inner.get_nonce(address).expect("state err"); | ||
| tx.nonce() == Nonce::ONE && account_nonce.0 == Nonce::ZERO | ||
| } | ||
| }; | ||
|
|
||
| StatefulValidatorAdapter::validate( | ||
| this, | ||
| tx, | ||
| self.execution_flags.skip_validate || skip_validate, | ||
| self.execution_flags.skip_fee_transfer, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Consider improving error handling in the validate method.
The validate method uses expect which could cause a panic. Consider handling the error more gracefully.
Apply this diff to improve error handling:
- let account_nonce = this.inner.get_nonce(address).expect("state err");
+ let account_nonce = this.inner.get_nonce(address).unwrap_or_else(|_| {
+ panic!("Failed to get nonce for address: {:?}", address);
+ });Committable suggestion was skipped due to low confidence.
| impl TxValidator { | ||
| pub fn new( | ||
| state: Box<dyn StateProvider>, | ||
| execution_flags: SimulationFlag, | ||
| cfg_env: CfgEnv, | ||
| block_env: &BlockEnv, | ||
| ) -> Self { | ||
| let inner = StatefulValidatorAdapter::new(state, block_env, &cfg_env); | ||
| Self { cfg_env, execution_flags, validator: Arc::new(Mutex::new(inner)) } | ||
| } | ||
|
|
||
| /// Reset the state of the validator with the given params. This method is used to update the | ||
| /// validator's state with a new state and block env after a block is mined. | ||
| pub fn update(&self, state: Box<dyn StateProvider>, block_env: &BlockEnv) { | ||
| let updated = StatefulValidatorAdapter::new(state, block_env, &self.cfg_env); | ||
| *self.validator.lock() = updated; | ||
| } | ||
|
|
||
| // NOTE: | ||
| // If you check the get_nonce method of StatefulValidator in blockifier, under the hood it | ||
| // unwraps the Option to get the state of the TransactionExecutor struct. StatefulValidator | ||
| // guaranteees that the state will always be present so it is safe to uwnrap. However, this | ||
| // safety is not guaranteed by TransactionExecutor itself. | ||
| pub fn get_nonce(&self, address: ContractAddress) -> Nonce { | ||
| let address = to_blk_address(address); | ||
| let nonce = self.validator.lock().inner.get_nonce(address).expect("state err"); | ||
| nonce.0 | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Consider improving error handling in get_nonce.
The get_nonce method uses expect which could cause a panic. Consider handling the error more gracefully.
Apply this diff to improve error handling:
- let nonce = self.validator.lock().inner.get_nonce(address).expect("state err");
+ let nonce = self.validator.lock().inner.get_nonce(address).unwrap_or_else(|_| {
+ panic!("Failed to get nonce for address: {:?}", address);
+ });Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl TxValidator { | |
| pub fn new( | |
| state: Box<dyn StateProvider>, | |
| execution_flags: SimulationFlag, | |
| cfg_env: CfgEnv, | |
| block_env: &BlockEnv, | |
| ) -> Self { | |
| let inner = StatefulValidatorAdapter::new(state, block_env, &cfg_env); | |
| Self { cfg_env, execution_flags, validator: Arc::new(Mutex::new(inner)) } | |
| } | |
| /// Reset the state of the validator with the given params. This method is used to update the | |
| /// validator's state with a new state and block env after a block is mined. | |
| pub fn update(&self, state: Box<dyn StateProvider>, block_env: &BlockEnv) { | |
| let updated = StatefulValidatorAdapter::new(state, block_env, &self.cfg_env); | |
| *self.validator.lock() = updated; | |
| } | |
| // NOTE: | |
| // If you check the get_nonce method of StatefulValidator in blockifier, under the hood it | |
| // unwraps the Option to get the state of the TransactionExecutor struct. StatefulValidator | |
| // guaranteees that the state will always be present so it is safe to uwnrap. However, this | |
| // safety is not guaranteed by TransactionExecutor itself. | |
| pub fn get_nonce(&self, address: ContractAddress) -> Nonce { | |
| let address = to_blk_address(address); | |
| let nonce = self.validator.lock().inner.get_nonce(address).expect("state err"); | |
| nonce.0 | |
| } | |
| impl TxValidator { | |
| pub fn new( | |
| state: Box<dyn StateProvider>, | |
| execution_flags: SimulationFlag, | |
| cfg_env: CfgEnv, | |
| block_env: &BlockEnv, | |
| ) -> Self { | |
| let inner = StatefulValidatorAdapter::new(state, block_env, &cfg_env); | |
| Self { cfg_env, execution_flags, validator: Arc::new(Mutex::new(inner)) } | |
| } | |
| /// Reset the state of the validator with the given params. This method is used to update the | |
| /// validator's state with a new state and block env after a block is mined. | |
| pub fn update(&self, state: Box<dyn StateProvider>, block_env: &BlockEnv) { | |
| let updated = StatefulValidatorAdapter::new(state, block_env, &self.cfg_env); | |
| *self.validator.lock() = updated; | |
| } | |
| // NOTE: | |
| // If you check the get_nonce method of StatefulValidator in blockifier, under the hood it | |
| // unwraps the Option to get the state of the TransactionExecutor struct. StatefulValidator | |
| // guaranteees that the state will always be present so it is safe to uwnrap. However, this | |
| // safety is not guaranteed by TransactionExecutor itself. | |
| pub fn get_nonce(&self, address: ContractAddress) -> Nonce { | |
| let address = to_blk_address(address); | |
| let nonce = self.validator.lock().inner.get_nonce(address).unwrap_or_else(|_| { | |
| panic!("Failed to get nonce for address: {:?}", address); | |
| }); | |
| nonce.0 | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2344 +/- ##
==========================================
+ Coverage 67.26% 67.50% +0.24%
==========================================
Files 357 359 +2
Lines 46634 46836 +202
==========================================
+ Hits 31367 31616 +249
+ Misses 15267 15220 -47 ☔ View full report in Codecov by Sentry. |
ref #2335
add a validator for validating incoming transaction on the rpc level. the validation will happen right before the transaction is added to the pool, and its result will determine whether the tx will get added to the pool or not.
before this all txs regardless of their validity are added to the pool. the validation will happen asynchronously where txs will get validated the same time as it is being executed. this happen later in the execution stage of the pipeline which is detached from the rpc side, at this point the rpc has already finished its request and has no context of the tx validity (hence what it means by asyncly).
this doesn't follow the same behaviour as mainnet, which would invalidate the txs on the rpc level. meaning if your tx, eg has invalid signatures, the sequencer would return the validation error as the response in the same
add_*_transactionrequest you used to send the invalid tx.this change also requires some changes on the blockifier side. added the necessary changes in a new branch blockifier:cairo-2.7-new
expected regression as now we're executing the tx 1.5 times; validation on the rpc side plus (validation + execution) on the block production side.
Summary by CodeRabbit
New Features
TxValidator.Bug Fixes
Documentation
Chores
Cargo.tomlfor better modularity and functionality.