core/state: introduce stateupdate structure #29530
Conversation
rjl493456442
force-pushed
the
state-update
branch
3 times, most recently
from
8e190d8
to
ea9d92e
Compare
rjl493456442
force-pushed
the
state-update
branch
4 times, most recently
from
005bb86
to
60746bb
Compare
rjl493456442
force-pushed
the
state-update
branch
6 times, most recently
from
354717c
to
bd6b20a
Compare
rjl493456442
force-pushed
the
state-update
branch
2 times, most recently
from
2fe48c1
to
2089fdb
Compare
rjl493456442
force-pushed
the
state-update
branch
from
2089fdb
to
3d70c5b
Compare
|
This pull request has been deployed full sync for a few days, since the genesis. It can sync more than 16m blocks with no error. |
rjl493456442
force-pushed
the
state-update
branch
from
3d70c5b
to
d940577
Compare
rjl493456442
force-pushed
the
state-update
branch
from
d940577
to
7b1ff9b
Compare
core/state/state_object.go
Outdated
| // boundary; however post the byzantium fork, the commit will only be performed | ||
| // at the end of block, this set essentially tracks all the modifications | ||
| // made within the block. | ||
| needCommit Storage |
There was a problem hiding this comment.
Given that the previous 3 items are all XXXStorage, I'd recommend using uncommittedStorage for this one.
There was a problem hiding this comment.
Though, it's a bit weird, because it contains the "origin" values too, which also somewhat clash with originStorage. Does it make sense to have the origin in 2 different variables? Can't we just use this field as a map[hash]struct{} to track which slots need commit, but otherwise rely on originStorage for the actual value content?
There was a problem hiding this comment.
Unfortunately the tracked values in origin, pending and uncommitted have different meanings:
- origin: the value before the entire state transition of block
- pending: the latest value after mutation (e.g. after applying each transaction)
- uncommittedStorage: the value before applying the "uncommitted" mutations.
Before the byzantium, the value being tracked in uncommittedStorage is the original value before the transaction; Post the byzantium, the value being tracked in uncommittedStorage is the original value before the block.
The reason for maintaining this value is, we want to make sure only dirty slots will be committed.
| @@ -193,6 +191,7 @@ func (s *stateObject) GetCommittedState(key common.Hash) common.Hash { | |||
| // have been handles via pendingStorage above. | |||
| // 2) we don't have new values, and can deliver empty response back | |||
| if _, destructed := s.db.stateObjectsDestruct[s.address]; destructed { | |||
| s.originStorage[key] = common.Hash{} // track the empty slot as origin value | |||
There was a problem hiding this comment.
Any particular reason for starting to track this here?
There was a problem hiding this comment.
Yeah, i think it would be better to track the "empty" value as origin, reasons:
Here, https://github.com/rjl493456442/go-ethereum/blob/state-update/core/state/state_object.go#L428 we blindly assume the original value is tracked in the s.originStorage map. It would still be correct if we don't track empty value, but it feels pretty weird.
Besides, it aligns with the behavior in https://github.com/rjl493456442/go-ethereum/blob/state-update/core/state/state_object.go#L233, as the slot read from database could also be empty.
What's more, tracking the empty value here could potentially be useful for differentiating "existence" and "emptiness" in the future? e.g. we could error out if a slot is never accessed (not existent in the originStorage map) but later be modified?
core/state/state_object.go
Outdated
| } | ||
| s.pendingStorage = make(Storage) // reset pending map | ||
| s.needCommit = make(Storage) // empties the commit markers |
There was a problem hiding this comment.
Might want to use some map clearing method here to retain the memory and not reallocate for the next tx (pre byzantium).
There was a problem hiding this comment.
maps.Delete is still in golang/exp package. Probably wait a bit until it's fully shipped in standard maps package?
There was a problem hiding this comment.
This one?
core/state/state_object.go
Outdated
| data: types.SlimAccountRLP(s.data), | ||
| } | ||
| if s.origin == nil { | ||
| op.origin = nil // the account was not present |
There was a problem hiding this comment.
This is a noop, right? Can't we just have a single if branch to set origin if s.origin != nil?
core/state/state_object.go
Outdated
| return blob | ||
| } | ||
| ) | ||
| for key, slot := range s.pendingStorage { |
There was a problem hiding this comment.
Pls rename slot to value. Slot is really the key itself.
| // Overwrite the clean value of storage slots | ||
| s.originStorage[key] = slot | ||
| } | ||
| s.pendingStorage = make(Storage) |
There was a problem hiding this comment.
Please use a map clearing method here
There was a problem hiding this comment.
maps.Delete is still in golang/exp package. Probably wait a bit until it's fully shipped in standard maps package?
There was a problem hiding this comment.
Wouldn't this work?
* core/state: introduce stateUpate structure * core/state: remove outdated function description * core/state: address comments
* core/state: introduce stateUpate structure * core/state: remove outdated function description * core/state: address comments
This pull request rewrites the statedb by introducing a state update struct.
A state update struct will be constructed once the
statedb.Commitis invoked. It contains all the statechanges caused by previous state transition, e.g.
Instead of committing the state change into state snapshot and trie database blindly, the state update
will be returned and then can be consumed by different state database implementation in different
ways. It gives more flexibilities to design database abstraction for different purposes.
What's more, the statedb has been simplified significantly, especially in state change construction.