Skip to content

state: Clean up state finalization#609

Merged
chfast merged 1 commit into
masterfrom
coinbase-touching-cleanup
Apr 13, 2023
Merged

state: Clean up state finalization#609
chfast merged 1 commit into
masterfrom
coinbase-touching-cleanup

Conversation

@rodiazet
Copy link
Copy Markdown
Member

@rodiazet rodiazet commented Apr 4, 2023

  • Separate selfdestruct handling from empty account clearing.
  • Apply block reward to coinbase consistently.
  • Use it to properly clean state in t8n and state-test-runner.

@rodiazet rodiazet requested a review from chfast April 4, 2023 15:44
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 4, 2023

Codecov Report

Merging #609 (9f5c406) into master (1fb6fa6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #609   +/-   ##
=======================================
  Coverage   97.29%   97.29%           
=======================================
  Files          78       78           
  Lines        7640     7648    +8     
=======================================
+ Hits         7433     7441    +8     
  Misses        207      207           
Flag Coverage Δ
blockchaintests 65.16% <ø> (ø)
statetests 63.18% <100.00%> (+0.08%) ⬆️
unittests 94.84% <100.00%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
test/state/state.hpp 100.00% <ø> (ø)
test/state/state.cpp 97.65% <100.00%> (+0.11%) ⬆️
test/statetest/statetest_runner.cpp 100.00% <100.00%> (ø)
test/unittests/state_transition.cpp 100.00% <100.00%> (ø)

Comment thread test/state/state.cpp Outdated
}
} // namespace

void clear_empty_or_destructed_accounts(State& state, evmc_revision rev)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

image

Maybe not very specific, but can we name it finalize()?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good idea :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This YP part is actually wrong: ethereum/yellowpaper#885.

@rodiazet rodiazet force-pushed the coinbase-touching-cleanup branch 2 times, most recently from 346d056 to a0b39d8 Compare April 5, 2023 10:36
@rodiazet rodiazet requested a review from chfast April 5, 2023 10:37
Comment thread test/state/state.cpp Outdated
@rodiazet rodiazet force-pushed the coinbase-touching-cleanup branch from a0b39d8 to 99695ed Compare April 6, 2023 09:55
Comment thread test/state/state.cpp Outdated
@chfast chfast force-pushed the coinbase-touching-cleanup branch 2 times, most recently from 79f997b to 60e30e5 Compare April 13, 2023 15:56
@chfast chfast changed the title Coinbase touching cleanup state: Clean up state finalization Apr 13, 2023
- Separate selfdestruct handling from empty account clearing.
- Apply block reward to coinbase consistently.

Co-authored-by: Paweł Bylica <[email protected]>
@chfast chfast force-pushed the coinbase-touching-cleanup branch from 60e30e5 to 9f5c406 Compare April 13, 2023 16:10

const auto res = state::transition(state, test.block, tx, rev, vm);

// Finalize block with reward 0.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In state tests it's not 0 on pre-merge revisions.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For state tests this is always 0. This effectively only touches/creates the coinbase account.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

True, apparently it's non-zero only in blockchain tests.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI, this is configurable in t8n via --state.reward.

Comment thread test/state/state.cpp

if (holds_alternative<std::error_code>(validation_result))
{
// Pre EIP-158 coinbase has to be touched also for invalid tx.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this not needed now?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This emulates applying block reward of 0. Previously, I wasn't exactly sure why this is needed. Now the block reward is "properly" applied in the end.

@chfast chfast merged commit 31fd534 into master Apr 13, 2023
@chfast chfast deleted the coinbase-touching-cleanup branch April 13, 2023 19:15
Copy link
Copy Markdown

@AungThuSoe24 AungThuSoe24 left a comment

Choose a reason for hiding this comment

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

Coinbase wallet

hanzo-dev pushed a commit to luxcpp/cevm that referenced this pull request Apr 26, 2026
- Separate selfdestruct handling from empty account clearing.
- Apply block reward to coinbase consistently.

Co-authored-by: Paweł Bylica <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants