Refactor nonce rollback to remove overly complex code #1428
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1428 +/- ##
=======================================
Coverage 82.7% 82.7%
=======================================
Files 871 871
Lines 370249 370150 -99
=======================================
- Hits 306391 306311 -80
+ Misses 63858 63839 -19 |
| let maybe_nonce = match (execution_status, &loaded_transaction.nonce) { | ||
| (Ok(_), _) => None, // Success, don't do any additional nonce processing | ||
| (Err(_), Some(nonce)) => { | ||
| Some((nonce, true /* rollback */)) | ||
| } |
There was a problem hiding this comment.
Before, maybe_nonce would only be Some(..) if the tx execution status was failed and the transaction used a nonce. Also, rollback is always true in this case, it's never false. This means that prepare_if_nonce_account doesn't need to check rollback and doesn't need to check execution status if maybe_nonce is Some(..)
accounts-db/src/accounts.rs
Outdated
| TransactionStatus::Successful => true, | ||
| TransactionStatus::FailedWithNonce { nonce } => { | ||
| let is_fee_payer = i == 0; | ||
| let is_nonce_account = prepare_if_nonce_account( |
There was a problem hiding this comment.
Since prepare_if_nonce_account always exited early if maybe_nonce was None, we can refactor to only call it for failed nonce transactions where the nonce is available.
| let should_store_account = match tx_status { | ||
| TransactionStatus::Successful => true, | ||
| TransactionStatus::FailedWithNonce { nonce } => { | ||
| let is_fee_payer = i == 0; |
There was a problem hiding this comment.
nit: since you're touching very sensible piece of code, i'd take the chance to further simplify the code by factoring out the address == nonce.address() into caller by defining let is_nonce_account = ... just below let is_fee_payer = ... here.
Then, make prepare_if_nonce_account return (), which in turn make the nested if is_fee_payer { ... } go way. Also, stop passing address.
Finally, after that, prepare_if_nonce_account()'s job clearer: it does its jobs only if is_nonce_account || is_fee_payer.
There was a problem hiding this comment.
also, rename to post_process_failed_nonce?
accounts-db/src/accounts.rs
Outdated
| // | ||
| // Since we know we are dealing with a valid nonce account, | ||
| // unwrap is safe here | ||
| let nonce_versions = StateMut::<NonceVersions>::state(nonce.account()).unwrap(); |
There was a problem hiding this comment.
nit: nonce.account() => account for slight increase of readability?
accounts-db/src/accounts.rs
Outdated
| enum TransactionStatus<'a> { | ||
| Successful, | ||
| FailedWithNonce { nonce: &'a NonceFull }, | ||
| } |
There was a problem hiding this comment.
TransactionStatus is a bit too general.
how about these?
enum StoredAccountProcessingStatus {
Normal,
FailedWithNonce(...)
}
// or
enum AccountCollectionMode {
Normal,
FailedWithNonce(...)
}
// or
enum NonceProcessing {
Skip,
UpdateAfterFailure(...)
}
|
Thanks for the review @ryoqun, I've applied your feedback |
Problem
Nonce rollback code is overly complex and hard to read. This mess is making it hard to implement handling for transactions that skip execution but are still committed as part of SIMD-0082
Summary of Changes
Refactor with more descriptive types / scenarios
Fixes #