-
Notifications
You must be signed in to change notification settings - Fork 20.1k
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: remove notion of fake storage #24916
Conversation
internal/ethapi/api.go
Outdated
@@ -885,6 +885,8 @@ func (diff *StateOverride) Apply(state *state.StateDB) error { | |||
} | |||
} | |||
} | |||
// Now commit the changes | |||
state.Commit(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will actually write into the trie database, which is a big nono. It might still be "just in ram", but it will compete with the memory cache of real trie nodes. I'd wager it also creates a diff layer on top of the snapshots if one exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha! Using finalize seems to work too
core/state/state_object.go
Outdated
s.fakeStorage[key] = value | ||
// | ||
//@deprecated use SetState per item instead. This method no longer fully replaces | ||
// all slots. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not following this deprecation warning? It didn't fully replace all the slots before either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just delete this altogether if there's a better alternative?
The change now seems ok, apart from the notes |
Triage discussion: we need to somethow fix the SetStorage too |
// Now finalize the changes. Finalize is normally performed between transactions. | ||
// By using finalize, the overrides are semantically behaving as | ||
// if they were created in a transaction just before the tracing occur. | ||
state.Finalise(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain what that parameter affects and why it should be false?
ee353ea
to
f4e1d9c
Compare
I've made a stab at fixing this PR now, so that it should be possible to override the whole state. I haven't added any test for it yet. |
I will take a look tomorrow. |
Test added! |
core/state/state_object.go
Outdated
// We set the root to empty root, to make prevent GetCommittedState from | ||
// reading the original data, it should thus only have access to what is | ||
// set in the loop below. | ||
s.data.Root = emptyRoot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it's not enough. State can still be hit in snapshot. e.g. https://github.com/ethereum/go-ethereum/blob/master/core/state/state_object.go#L216
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think we're executing snapshot-free when we're tracing
My feeling is: this approach is OK, but in the future we should introduce some extra notions: The former mostly be used for serving requests and read-only actions(of course, state can be mutated but change should never be committed), and latter is used for state mutation. |
Lifting this out @rjl493456442 said:
@holiman said:
The state overrides come from two places, either
This one uses
->
So, if the Or
This one is a bit different, though, because TL;DRFor tracing, we are (probably) trie-only, and don't have to worry about snapshot. For |
@holiman I think we shouldn't implicitly hold the assumption that snapshot is not available. Theoretically, if a state is for reading, both trie and snapshot might be available. |
Yeah, that makes a lot of sense |
8d284a4
to
092b16a
Compare
I think this PR now respects this idea. One might consider the way it's done to be slightly hacky, but I really do not see a better way to achieve the same effect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR removes the notion of fakeStorage from the state objects, and instead, for any state modifications that are needed, it simply makes the changes.
This PR fixes #24456 .
This PR removes the notion of
fakeStorage
from the state objects, and instead, for any state modifications that are needed, it simply makes the changes and commits it.I'm not fully certain if there are any tangible negative side-effects of doing a
Commit
(such as, does it have potential to flush stuff to disk? I think not).One other change is that this change makes it not possible to replace the entire storage in one go, rather we can only update slots explicitly. I don't know how much of a biggie that is.