-
Notifications
You must be signed in to change notification settings - Fork 775
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
Client -> Rinkeby: Consensus Error on Block 14182 (invalid receiptTrie) #1090
Comments
Hi @holgerd77, first of all these debug messages look great! The problem in #1076 is that this particular transaction seems to trigger multiple, possibly unrelated bugs. The problem is there that a wrong value is written to a storage slot. Therefore the state root is wrong after execution. It might be that the bug here is also caused by that bug, it could be a bug in an opcode which has not changed during Frontier -> SpuriousDragon. |
@jochem-brouwer thanks for the quick response - yeah- always difficult to spot. I then still wonder though why such generic bugs have not been triggered on mainnet with this load of transactions? 🤔 This might be an indicator that wrong behavior is either a) PoA change or b) HF related (since on mainnet we are still running everything on But of course might also be some incidence and a very early transaction has triggered the same bug (probability nevertheless points in the other direction I guess). (argh, I have got a signifcant problem with my "b" on the keyboard, really annoying 😜 ) |
One high-level thing I noticed here: even if the I would very much think this is not what we (anyone) want and these checks should be rather done BEFORE the commit and in case of a failure should revert - actually for all checks except for the For |
(please read on site, several corrections) |
I don't think it compromises the state. Keep in mind that we set the state root to the root of the previous block before using it. Since we never delete any trie nodes, we can successfully find the right data. If this would not be the case, then I would not be able to succesfully execute the mainnet block in #1081. If the block would fail (as it did in client) and it would compromise the database, then I would not be able to execute the block after I've fixed it (I should then re-run the client). As a side node; this also means that we are currently always running in archive node, since the trie is never pruned. |
TBH I never really got this "one is resetting the state root and everything works magically" concept - this is one of my large blind spots. Why are no state root entries overwritten in between - e.g. by these tx commits? 🤔 I am repeatedly not getting this when seeing this pattern (set stateRoot -> everything correct). |
The state root is essentially the hash of the entire database. Consider what would happen if, at any point, we would delete trie nodes if they are updated. For instance, if we have a branch node, and one of the keys has a certain hash A, then. when the trie nodes further down the trie are updated, then hash A changes. We can thus delete the hash A from the database. Consider that we do this approach after every block. This is problematic if there's a reorg; we would then have deleted trie node items which we would need if we have to reset the database to an older state root. Also consider what would happen if we do that by tx commits; if in the end the block turns out to be bad, we have overwritten the old trie and have to do a full re-sync. We also cannot do this directly when we commit inner messages in a transaction, since a transaction at lower depth (say the root transaction) might revert / run out of gas, so then we have to revert our trie also (which we cannot do if we miss nodes). I don't know how Geth does this, I think they persist state for like a rather large amount of blocks, and there is an extra sweeper process which sometimes deletes state roots if they are old (when we are in full mode, not archive). |
Hi, I just noticed that gas usage actually does not match. The numbers look the same but they are not; there is a difference of 600 gas. Taking a further look here. |
That's also a funny statement. 😄 Nice spot! |
Kind of funny how such a gigantic transaction triggers the bug, if there would just be a small transaction with any of the tangerine whistle gas-change operations then we would have found this bug much faster 😂 |
At least we made it up to block 14182 after the fixes from #1089 - that's something. To relativize a bit, there were only 10 or so transactions along the way. 😂 😂 😂
This one is significantly more complex, here is the output:
Block on Rinkeby: 14182
Some first observations and thoughts:
SpuriousDragon
and Client -> Mainnet: Consensus Error on block 226522 "invalid receipt trie" #1076 is fixing a pre-Homestead bug (that said, @jochem-brouwer you mentioned that you need to fix other bugs. Have these already been fixed and are these independent from the Homestead HF and might play a role here?)1339314
), being an indicator that there is likely nothing wrong along opcode execution I guess? 🤔Ok. We'll see. 😄 😋
The text was updated successfully, but these errors were encountered: