Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Beyond EVM] performant StateDB implementation - part 2 #5167

Merged
merged 13 commits into from
Jan 5, 2024

Conversation

ramtinms
Copy link
Member

@ramtinms ramtinms commented Dec 19, 2023

see #5129

@ramtinms ramtinms changed the base branch from master to ramtin/5129-new-evm-stateDB-part1 December 19, 2023 21:22
@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2023

Codecov Report

Attention: 38 lines in your changes are missing coverage. Please review.

Comparison is base (ffddae1) 56.50% compared to head (036c370) 56.61%.

Files Patch % Lines
fvm/evm/emulator/state/delta.go 87.11% 26 Missing and 12 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5167      +/-   ##
==========================================
+ Coverage   56.50%   56.61%   +0.10%     
==========================================
  Files         983      984       +1     
  Lines       93757    94052     +295     
==========================================
+ Hits        52979    53247     +268     
- Misses      36828    36844      +16     
- Partials     3950     3961      +11     
Flag Coverage Δ
unittests 56.61% <87.20%> (+0.10%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ramtinms ramtinms marked this pull request as ready for review December 19, 2023 21:29
fvm/evm/emulator/state/delta.go Outdated Show resolved Hide resolved
fvm/evm/emulator/state/delta.go Outdated Show resolved Hide resolved
fvm/evm/emulator/state/delta.go Outdated Show resolved Hide resolved
fvm/evm/emulator/state/delta.go Outdated Show resolved Hide resolved
Base automatically changed from ramtin/5129-new-evm-stateDB-part1 to master January 4, 2024 04:56
@ramtinms ramtinms added this pull request to the merge queue Jan 4, 2024
@ramtinms ramtinms removed this pull request from the merge queue due to a manual request Jan 4, 2024
@ramtinms ramtinms added this pull request to the merge queue Jan 4, 2024
Copy link
Member

@fxamacker fxamacker left a comment

Choose a reason for hiding this comment

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

Nice! I left some comments and suggestions.

Some account related questions:

  • Can account be present in both created and suicided list in the same view?
  • If so, what is the status of that account?
  • Do we need to reset account cache, such as codes, balances, etc after account is flagged for deletion?
  • Do we want to return error in setters after account is flagged for deletion?

fvm/evm/emulator/state/delta.go Outdated Show resolved Hide resolved
fvm/evm/emulator/state/delta.go Outdated Show resolved Hide resolved
fvm/evm/emulator/state/delta.go Outdated Show resolved Hide resolved
fvm/evm/emulator/state/delta.go Outdated Show resolved Hide resolved
fvm/evm/emulator/state/delta.go Outdated Show resolved Hide resolved
fvm/evm/emulator/state/delta.go Show resolved Hide resolved
fvm/evm/emulator/state/delta.go Show resolved Hide resolved
fvm/evm/emulator/state/delta.go Outdated Show resolved Hide resolved
fvm/evm/emulator/state/delta.go Show resolved Hide resolved
fvm/evm/emulator/state/delta.go Outdated Show resolved Hide resolved
@ramtinms ramtinms removed this pull request from the merge queue due to a manual request Jan 4, 2024
@ramtinms
Copy link
Member Author

ramtinms commented Jan 5, 2024

@fxamacker @sideninja I spent some time going over the Geth code and updated the logics for creation and suicide at 036c370

@ramtinms
Copy link
Member Author

ramtinms commented Jan 5, 2024

To answer your questions @fxamacker :

Can account be present in both created and suicided list in the same view? If so, what is the status of that account?

Yes, it could be in the state of suicided then created (then we flag it with recreate), clean up the old data and delete the account at the end of the transaction and create a new one with changes.
The reverse is not allowed right now, if an account is created then suicided.

Do we need to reset the account cache, such as codes, balances, etc after account is flagged for deletion?

when an account is set for deletion, everything except the balance has to be still accessible throughout the transaction, except for when recreation is called then we clean all the account values (in this case we also have to carry over the balance of the old account before flagging for deletion.

Do we want to return error in setters after account is flagged for deletion?

we don't do anything even if an account is flagged for deletion, we accept them and ignore them at the end and delete the account. Yeah I know it feels odd but that's how Geth behaves.

@ramtinms
Copy link
Member Author

ramtinms commented Jan 5, 2024

going to merge this PR but feel free to provide feedback so I can address it in the next PR

@ramtinms ramtinms added this pull request to the merge queue Jan 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 5, 2024
@ramtinms ramtinms added this pull request to the merge queue Jan 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 5, 2024
@ramtinms ramtinms added this pull request to the merge queue Jan 5, 2024
Merged via the queue into master with commit 91c7f08 Jan 5, 2024
51 checks passed
@ramtinms ramtinms deleted the ramtin/5129-new-evm-stateDB-part2 branch January 5, 2024 16:41
This pull request was closed.
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