core, trie: fix state log deep copy #17489
Conversation
rjl493456442
force-pushed
the
fix-state-copy
branch
from
b687fcc to
a453e43
Compare
core/state/statedb.go
Outdated
| } | ||
| for hash, preimage := range self.preimages { | ||
| state.preimages[hash] = preimage | ||
| state.preimages[hash] = common.CopyBytes(preimage) |
There was a problem hiding this comment.
I'm unsure about the changes here. Whilst you are in theory correct that this copy is a bit deeper than the old one, it's important to know that logs and preimages won't really be ever modified in any part of the code (the containing slices yes, hence the copy, but the contents AFAIK not). Thus copying them is just an expensive memory duplication that doesn't really add any value. Please correct me if I'm wrong though.
There was a problem hiding this comment.
I am not sure about the preimage, but for the logs, yes it will be modified. https://github.com/ethereum/go-ethereum/blob/master/miner/worker.go#L540
In my eyes, since the function give out a description of independent copy, i think we should deep copy all the field to prevent misuse in the future
trie/secure_trie.go
Outdated
| trie Trie | ||
| hashKeyBuf [common.HashLength]byte | ||
| secKeyCache map[string][]byte | ||
| secKeyCacheOwner *SecureTrie // Pointer to self, replace the key cache on mismatch |
There was a problem hiding this comment.
This change is dangerous. Most of our code assumes that a trie and a secure trie are immutable data structures. Many parts of the code relies on this and copies them freely, knowing that the result is an independent copy. The reason I added this convoluted hack is to retain that immutability guarantee. You are breaking it by requiring explicit copies from now on. I don't think we can guarantee that this won't break existing code and invariants.
|
Please split this PR into the log deep copy and the preimage deep copy. The logs can be merged imho, the preimage I have to think about a bit more. |
rjl493456442
force-pushed
the
fix-state-copy
branch
from
a453e43 to
4bd9550
Compare
rjl493456442
changed the title
This PR mainly fixes two things: state deep copy and secure trie deep copy.
Regarding the secure trie, the secure cache content (trie key's preimages) will be discarded for the copied trie in the current codebase, it will definitely introduce some trouble when we want to dump state.