Conversation
Co-authored-by: Emilia Hane <elsaemiliaevahane@gmail.com>
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de> Co-authored-by: Emilia Hane <elsaemiliaevahane@gmail.com>
emhane
added
C-debt
| // If the address is not already in the bundle builder's state, add it. | ||
| if !bundle_builder.get_states().contains(&address) { | ||
| // Seek the address in the plain account state. | ||
| if let Some(new_info) = plain_accounts_cursor.seek_exact(address)?.map(|kv| kv.1) { | ||
| // Add the new info to the bundle builder. | ||
| bundle_builder = | ||
| bundle_builder.state_present_account_info(address, new_info.into()); | ||
| } | ||
| } | ||
| // insert old info into reverts. | ||
| reverts.entry(block_number).or_default().entry(address).or_default().0 = Some(old_info); | ||
|
|
||
| // Add the old account info to the bundle builder if it exists. | ||
| if let Some(old_info) = old_info { | ||
| bundle_builder = | ||
| bundle_builder.state_original_account_info(address, old_info.into()); | ||
| } |
There was a problem hiding this comment.
review from https://github.com/paradigmxyz/reth/pull/8264/files#r1646535481 @rkrasiuk
previous behavior: if the entry is occuppied, overwrite the original account state even if old_info is None
current behavior: only overwrite the original account state if old_info is Some.
if old_info is None, original info must still be overwritten
the methods BundleBuilder::state_original_account_info and BundleBuilder::state_present_account_info don't take an Option<AccountInfo as param. so I'm wondering:
- if entry is occupied but the original info is
None, should we pass thenew_infotoBundleBuilder::state_original_account_info? - there is another method
BundleBuilder::set_revert_account_info, that takesOption<Option<AccountInfo>>, is that the one we should be calling here?
There was a problem hiding this comment.
@DaniPopes, you're probably familiar with the revm api and can answer this too?
There was a problem hiding this comment.
I'm not familiar with state/transitions/bundles APIs
|
as i mentioned in reviews for #8264, bundle builder API must be improved first before integration in reth |
5 participants
Merges changes from #8264 and #8861