Skip to content
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

Closed
holgerd77 opened this issue Feb 11, 2021 · 10 comments · Fixed by #1101
Closed

Client -> Rinkeby: Consensus Error on Block 14182 (invalid receiptTrie) #1090

holgerd77 opened this issue Feb 11, 2021 · 10 comments · Fixed by #1101

Comments

@holgerd77
Copy link
Member

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:

image

image

Block on Rinkeby: 14182

Some first observations and thoughts:

Ok. We'll see. 😄 😋

@jochem-brouwer
Copy link
Member

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.

@holgerd77
Copy link
Member Author

@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 Frontier).

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 😜 )

@holgerd77
Copy link
Member Author

holgerd77 commented Feb 11, 2021

One high-level thing I noticed here: even if the receiptTrie check is failing the block state is actually committed (e.g. seeing from the debug output), see code in VM.runBlock() for context. I am not sure if this behavior hasn't already compromised our state DBs on debugging, normally this SHOULD be the case by the state DB being in an after-blog-state after first run? 🤔

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 stateRoot - so for receiptTrie, bloom and gasUsed.

For stateRoot would it eventually work if we do a double-commit (now with our super-fast checkpointing 😋 ) and do two checkpoint calls before - one commit (which should commit towards checkpoint 2) after - then do the checks and in case of a failure do a revert to checkpoint 1. Is our MPT library prepared for that level of nesting?

@holgerd77
Copy link
Member Author

(please read on site, several corrections)

@jochem-brouwer
Copy link
Member

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.

@holgerd77
Copy link
Member Author

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).

@jochem-brouwer
Copy link
Member

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).

@jochem-brouwer
Copy link
Member

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.

@holgerd77
Copy link
Member Author

The numbers look the same but they are not

That's also a funny statement. 😄 Nice spot!

@jochem-brouwer
Copy link
Member

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 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants