More witness fixes #2009
More witness fixes #2009
Conversation
…ofs will be returned when a storage slot is updated to zero. Refactor and simplify implementation of getProofs endpoint.
| continue | ||
| if v.isZero and k in wd.storageKeys: | ||
| wd.storageKeys.excl k | ||
| continue |
There was a problem hiding this comment.
This change is required so that state updates for storage slots from a non zero value to a zero value would be recorded. We want to know about these updates in the new exp_getProofsByBlockNumber endpoint. This change actually doesn't effect the block witness generation because zero value storage slots are deleted in the AccountCache.persist call and therefore the block witness won't contain any zero value storage slots.
| else: | ||
| check stateDB.getStorageRoot(address) == storageHash | ||
|
|
||
| check stateDB.rootHash == expectedStateRoot |
There was a problem hiding this comment.
Improved the validation here to apply the state changes in the test and check that the test state root matches the expected value.
|
|
||
| dbTx.rollback() | ||
|
|
||
|
|
||
| proc getBlockProofs*( |
There was a problem hiding this comment.
Refactored this to simply use the MultikeysRef to build the list of proofs from the state. Avoid using the witness for simplicity.
|
|
||
| for slotKey, _ in account.storage: | ||
| slots.add(slotKey) | ||
| if not keyData.storageKeys.isNil and accDB.accountExists(address): |
There was a problem hiding this comment.
Storage slots won't be returned if the account doesn't exist.
…lock using a local state.
|
I'm using the rpc_getproofs_track_state_changes test to apply the state changes from the getProofs endpoints for each block. While running the test locally against Nimbus I've been able to successfully verify that the state roots are correct for the first 700 000 blocks. This one is ready for final feedback and review. |
|
|
||
| proc rpcGetProofsTrackStateChangesMain*() = | ||
|
|
||
| suite "rpc getProofs track state changes tests": |
There was a problem hiding this comment.
I understand this is a semi-manual test where you also need to have Nimbus up and running and populated with the state until that tested block?
Might want to put a little comment somewhere here about that and/or how to run it.
There was a problem hiding this comment.
Sure, will do.
| if (balance == 0 and nonce == 0 and codeHash == ZERO_HASH256 and storageHash == ZERO_HASH256): | ||
| stateDB.setCode(address, @[]) | ||
| stateDB.clearStorage(address) | ||
| stateDB.deleteAccount(address) | ||
| elif (balance == 0 and nonce == 0 and codeHash == EMPTY_SHA3 and storageHash == EMPTY_ROOT_HASH): |
There was a problem hiding this comment.
Perhaps explain in a comment the differences between these?
There was a problem hiding this comment.
I agree with Kim, this should be explained with comments or at least a self describe template.
There was a problem hiding this comment.
Ok, will add a comment here.
|
No objections. The critical parts in accounts_cache is not modified. It is relatively safe to modify the witness creation part. The tests looks good. |
Thanks. In that case, I'll merge it after adding the suggested comments in the test. |
| # This test is intended to be run manually against a running instance of Nimbus-Eth1 | ||
| # so it should not be added to the test runner or to the CI test suite. | ||
| # |
There was a problem hiding this comment.
It would be good to still have this test compile in CI to avoid bit rot.
There was a problem hiding this comment.
How can I get it to compile in the CI without running the test?
There was a problem hiding this comment.
You could add it to the tools in the Makefile
There was a problem hiding this comment.
Oh, I just noticed there is already a test_tools_build.nim file which builds all these tools in one go: https://github.com/status-im/nimbus-eth1/blob/master/tests/test_tools_build.nim
This test/tool could be added there. (This is done all in one go to make it faster fyi).
There was a problem hiding this comment.
Thanks, good point. Sure, will do.
…g in writeShortRlp assertion.
After improving the tests for the exp_getProofsByBlockNumber endpoint to validate the list of state updates against a local test state, I discovered the following issues which are fixed in this PR: