core/state: fix bug in copy of copy State #16485
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@holiman @karalabe Thanks again for proposing a fix for the reported issue this quickly. I just applied the patch to check if this fixes the tests that started failing in my automated smart contract analysis tool after I had updated go-ethereum. It seems like this did the trick and the tests pass again. |
karalabe
reviewed
Apr 11, 2018
| state.stateObjects[addr] = self.stateObjects[addr].deepCopy(state) | ||
| state.stateObjectsDirty[addr] = struct{}{} | ||
| } | ||
| } |
There was a problem hiding this comment.
Here you are iterating over all the state objects, even non dirty ones and set them to dirty in lower layers. Are you sure this is what you intended? Am I missing something?
There was a problem hiding this comment.
Yeah, that is most likely wrong
|
How the heck did this PR succeed building? |
|
Apparently @holiman you based this PR on your own master branch (which was stale) and not upstream master. Or not sure what funky issue happened, but this branch is 28 commits behind master. Will force push. EDIT: Done. |
karalabe
requested changes
Apr 11, 2018
There was a problem hiding this comment.
Please modify the code so it only copies the actually dirty objects, but not the clean ones:
diff --git a/core/state/statedb.go b/core/state/statedb.go
index 3b8faa287..f18e0a47d 100644
--- a/core/state/statedb.go
+++ b/core/state/statedb.go
@@ -477,13 +477,12 @@ func (self *StateDB) Copy() *StateDB {
// Above, we don't copy the actual journal. This means that if the copy is copied, the
// loop above will be a no-op, since the copy:s journal is empty.
// Thus, here we iterate over stateObjects, to enable copies of copies
- for addr := range self.stateObjects {
+ for addr := range self.stateObjectsDirty {
if _, exist := state.stateObjects[addr]; !exist {
state.stateObjects[addr] = self.stateObjects[addr].deepCopy(state)
state.stateObjectsDirty[addr] = struct{}{}
}
}
-
for hash, logs := range self.logs {
state.logs[hash] = make([]*types.Log, len(logs))
copy(state.logs[hash], logs)
|
Oh, and pls also fix |
|
Done, ptal |
|
@holiman @karalabe I just pulled these changes and encountered a crash that seems to be related. Unfortunately, I can't easily reproduce it. However, here is parts of the stack trace: Any idea what might be going wrong here? This might be completely unrelated, but while looking at the code for |
|
Interesting. so there is an |
|
Regarding the second question, I believe that is correct. It copies over all modified storage entries to the copy, but does not bother copying non-modified data. |
|
Oh, btw @wuestholz , did you use the original PR or the fixed/squashed one? What's the commit hash? |
|
@holiman I see. Thanks for looking into it! In case that helps, I'm using a local in-memory chain (with the chain config for mainnet) to run sequences of transactions to detect issues in smart contracts. I might be able to get the sequence of transactions in case that helps. Yes, I'm using the fixed one (5a79aca). |
|
If you are using pre-byzantium rules, it may be the that you run into the case documented here: https://github.com/ethereum/go-ethereum/blob/master/core/state/statedb.go#L532 . If that's the case, that would be good news. Otherwise, you may have found some other cornercase, which I would be very grateful if you were able to reproduce/diagnose. |
|
@holiman Hm. I'm using the same chain config as mainnet and it seems that the block numbers are around 4007428. I assume that would be pre-byzantium, right? |
|
Yes, byzantium is at |
|
@holiman Yes, that's what seems to be happening. The non-existing address is indeed 0000000000000000000000000000000000000003 for my run. Below you can see the sequence of transactions (as json) that seem to trigger the behavior in case you want to try to reproduce it: |
|
Thanks for confirming! I'll follow up with a nil-check |
|
Great! Thank you very much for fixing this so quickly! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #15225 (comment) . The copied state did not contain the full journal, and when that was copied, the dirty tracking was lost.