-
Notifications
You must be signed in to change notification settings - Fork 110
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps explain in a comment the differences between these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Kim, this should be explained with comments or at least a self describe template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will add a comment here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, I did skim rather fast over it but it makes sense to me . Perhaps @jangko who knows this code better wants to have a look still.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to still have this test compile in CI to avoid bit rot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can I get it to compile in the CI without running the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could add it to the tools in the Makefile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: