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

core/state: auto-finalise state objects during copy #28655

Closed

Conversation

karalabe
Copy link
Member

@karalabe karalabe commented Dec 7, 2023

Fixes #20119.

When we copy a statedb, we auto-flatten the journal into the dirty objects map. The Copy method did not call finalize on the individual objects, rather deep copied the dirtyness too. That's nice and all, but the statedb.Finalize has an optimization to only finalize "touched" objects (i.e. in the journal), so that would omit finalizing just-flattened stuff from commit.

This is a bit of a pickle:

  • Most importantly, this does not affect us. We never copy a non-finalized statedb.
  • Originally copy was meant to be able to copy kind of all the dirtyness too, but some finality is leaking into it already if we flatten the journal (and not flattening the journal leaks the previous execution stack, which is again weird). It's unclear at this point whether we should just force-finalize before copy, should perhaps error if non-finalized is copied; should try to make it work by copying dirtyness and have statedb.Finalize somehow detect it, etc.

@karalabe
Copy link
Member Author

karalabe commented Dec 7, 2023

I think we need to make a conscious decision here: either allow copy of only finalized states (then either implicitly finalize or error out); or allow copying dirty states too, but them IMO copying the entire journal might be needed to avoid forever-cornercases.

In Geth's production code, we would still always Copy finalized stuff, so should be mostly moot for us.

@holiman
Copy link
Contributor

holiman commented Dec 7, 2023

either allow copy of only finalized states (then either implicitly finalize or error out); or allow copying dirty states too

I kind of would like to allow only copying already (explicitly) finalized state. But we currently don't have any error-return on Copy, which we would need to do that properly. And that might be somewhat breaking

@rjl493456442
Copy link
Member

but them IMO copying the entire journal might be needed to avoid forever-cornercases.

I would prefer to go with this direction, I have a pull request to support journal copy.

@karalabe
Copy link
Member Author

karalabe commented Jun 3, 2024

This issue was fixed with a different pr.

@karalabe karalabe closed this Jun 3, 2024
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.

Crash in gasSStoreEIP2200 due to panic in SubRefund
3 participants