-
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
all: track state changes in state db #27349
Conversation
f6da10e
to
c40c8a0
Compare
c1cd59d
to
bad3103
Compare
11b2c40
to
11e25f7
Compare
if ch.prevAccountOriginExist { | ||
s.accountsOrigin[ch.prev.addrHash] = ch.prevAccountOrigin | ||
} |
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.
We should document, somewhere, what this means. For example, if prevAccountOriginExist
is true
, can prevAccountOrigin
be nil
? And what kind of scenario would lead to that?
Simiarly, can prevAccountOriginExist
be false
and prevAccountOrigin
be non-nil
? If so, how?
And if they're both 'strongly' tied to eachother, do we really need prevAccountOriginExist
?
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.
It's possible that prevAccountOriginExist
is true and prevAccountOrigin
is nil.
nil
means the account was not present and it's created in this block. If we want
to reset this block, we need the explicit flag to distinguish:
- accountOrigin is nil
- accountOrigin is not existent
Regarding prevAccountOriginExist
is false while prevAccountOrigin
is non-nil, it's an impossible combo.
if _, ok := s.accounts[addrHash]; ok { | ||
s.accountsOrigin[addrHash] = nil // case (b) | ||
} | ||
continue |
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.
add comment
d273dc4
to
d9926e3
Compare
address: s.address, | ||
addrHash: s.addrHash, | ||
origin: s.origin, | ||
data: s.data, |
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 think this only deep copies the pointer to s.data, I'm not sure if this is what you want
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.
s.data is a struct, it's ok to directly copy it.
Re s.origin, seems we will never change the content of origin via pointer, it's also OK to copy the pointer.
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 change makes the StateDB track the state key value diff of a block transition. We already tracked current account and storage values for the purpose of updating the state snapshot. With this PR, we now also track the original (pre-transition) values of accounts and storage slots.
This reverts commit 5b3e45b.
This reverts commit 5b3e45b.
state.accounts = copyAccounts(s.accounts) | ||
state.storages = copyStorages(s.storages) | ||
state.accountsOrigin = copyAccounts(state.accountsOrigin) | ||
state.storagesOrigin = copyStorages(state.storagesOrigin) |
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.
Flaw: #29563 (comment)
This PR tracks the state changes in statedb.
The change set can be used to build the state history in path-based storage scheme
for state rollback and archive state serving.