Skip to content

fix(poststate): ignore old changed storage values after selfdestruct#4199

Closed
rkrasiuk wants to merge 2 commits intomainfrom
rkrasiuk/changed-storage-after-selfdestruct
Closed

fix(poststate): ignore old changed storage values after selfdestruct#4199
rkrasiuk wants to merge 2 commits intomainfrom
rkrasiuk/changed-storage-after-selfdestruct

Conversation

@rkrasiuk
Copy link
Contributor

@rkrasiuk rkrasiuk commented Aug 14, 2023

Description

If the storage entry was changed after a primary selfdestruct within the same block, during history write its value would be written as 0 because its plain state value would be ignored by storage.storage.contains_key(..) checks below:

if !storage.storage.contains_key(&key) {
storage.storage.insert(entry.key.into(), entry.value);
}
while let Some(entry) = storages_cursor.next_dup_val()? {
let key = U256::from_be_bytes(entry.key.to_fixed_bytes());
if !storage.storage.contains_key(&key) {
storage.storage.insert(entry.key.into(), entry.value);
}
}

Solution

Do not write old storage values for the block storage change, if this account was previously wiped within the same block.

@rkrasiuk rkrasiuk added C-bug An unexpected or incorrect behavior A-execution Related to the Execution and EVM labels Aug 14, 2023
@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Merging #4199 (5125e85) into main (b2be35c) will decrease coverage by 0.03%.
Report is 4 commits behind head on main.
The diff coverage is 100.00%.

Impacted file tree graph

Files Changed Coverage Δ
crates/storage/provider/src/post_state/mod.rs 95.89% <100.00%> (+0.14%) ⬆️
crates/storage/provider/src/post_state/storage.rs 98.59% <100.00%> (+0.08%) ⬆️

... and 16 files with indirect coverage changes

Flag Coverage Δ
integration-tests 16.87% <0.00%> (-0.03%) ⬇️
unit-tests 64.25% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
reth binary 26.60% <ø> (ø)
blockchain tree 82.56% <ø> (ø)
pipeline 90.07% <ø> (ø)
storage (db) 74.98% <100.00%> (+0.13%) ⬆️
trie 94.71% <ø> (ø)
txpool 51.43% <ø> (ø)
networking 77.75% <ø> (-0.16%) ⬇️
rpc 58.68% <ø> (+0.01%) ⬆️
consensus 63.53% <ø> (-0.20%) ⬇️
revm 32.19% <ø> (ø)
payload builder 6.82% <ø> (ø)
primitives 87.84% <ø> (-0.03%) ⬇️

Copy link
Collaborator

@rakita rakita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

let mut post_state = PostState::new();
post_state.destroy_account(1, address1, Account::default());
post_state.create_account(1, address1, Account::default());
post_state.change_storage(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change storage here to some other slot 0x100 for example and check if changeset is set to zero value for that slot

@rkrasiuk
Copy link
Contributor Author

closing yet again in favor of #3512

@rkrasiuk rkrasiuk closed this Aug 15, 2023
@rkrasiuk rkrasiuk deleted the rkrasiuk/changed-storage-after-selfdestruct branch August 15, 2023 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-execution Related to the Execution and EVM C-bug An unexpected or incorrect behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants