Optionally bypass state root verification in reference test worldstate #5960
Conversation
|
|
The is beyond my current understanding of Bonsai, but please can you explain why doing this doesn't undermine the integrity of the tests? IOW, how do we know this won't result in missing some genuine mismatch errors? |
Bonsai defers calculating a state root until we 'persist' the worldstate. When we persist, it is always in the context of a blockheader. As an additional check to ensure consistency, we validate that the stateroot in the blockheader is the same as the one calculated when persisting the worldstate. There is a bit of a chicken and egg issue with the reference tests however, because the tests are designed with stateroot as an output rather than an input. See Dimitry's comment - when writing tests something has to provide the stateroot as an output as some point. As far as not undermining the integrity of the tests, in the original pr 5686 we:
So at least when we run the reference test suites via besu CI we will be validating the state root, and processes that use t8n to run reference tests directly (like retesteth or hive) we will skip state root verification. With currentStateRoot, we have the ability to validate stateRoot for any of our own tests like the ones for t8n tool. All of our current tests except for the one I added in this PR have a non zero stateroot, but we will have to be mindful in the future when developing reference-test style tests to include currentStateRoot. |
|
Thanks for the clear explanation!
This concerns me a bit. How do you think we can mitigate this? I find this area of besu mysterious because it relates to external tools and not well documented so feel like this could easily slip under the radar in the future. |
| @@ -75,6 +75,14 @@ public ReferenceTestWorldState copy() { | |||
| layerCopy, cachedMerkleTrieLoader, trieLogManager, preImageProxy); | |||
| } | |||
|
|
|||
| @Override | |||
| protected void verifyWorldStateRoot(final Hash calculatedStateRoot, final BlockHeader header) { | |||
| // if the supplied header has a non-zero state root, verify. Else bypass | |||
There was a problem hiding this comment.
nit: would be nice to say why we bypass here
There was a problem hiding this comment.
added a verbose javadoc 👍
I suspect that the balance of example will be enough when we develop more of our own reference-style tests. More often than not, tests cases borrow a lot from existing examples, and all of our examples (save this one) have a non-zero stateroot. Willing to be convinced otherwise though. |
garyschulte
force-pushed
the
feature/disable-bonsai-state-root-verfication
branch
3 times, most recently
from
ce98c60
to
d8dcf1d
Compare
…undefined/zero Signed-off-by: garyschulte <[email protected]>
Signed-off-by: garyschulte <[email protected]>
…t check. Signed-off-by: garyschulte <[email protected]>
garyschulte
force-pushed
the
feature/disable-bonsai-state-root-verfication
branch
from
d8dcf1d
to
648368e
Compare
garyschulte
deleted the
feature/disable-bonsai-state-root-verfication
branch
hyperledger#5960) * bypass state root verification in reference test worldstate if it is undefined/zero Signed-off-by: garyschulte <[email protected]> Signed-off-by: Justin Florentine <[email protected]>
hyperledger#5960) * bypass state root verification in reference test worldstate if it is undefined/zero Signed-off-by: garyschulte <[email protected]>
PR description
if the passed block header has a stateroot of Hash.ZERO, bypass bonsai state root verification in bonsai reference test worldstate only.
Fixed Issue(s)
fixes #5934