Fluffy state offer validation #2170
Conversation
| return false | ||
|
|
||
| if not isValidTrieNode(expectedRootHash, proof[0]): | ||
| return false |
There was a problem hiding this comment.
@kdeme Do you think it's worth adding a warning log to each of the locations where validation can fail? Might reduce readability but could help when debugging issues down the track.
There was a problem hiding this comment.
This would be mostly for the case if during initial debug / interop with other clients, there is invalid data provided over the network?
I think in that case, we can log more verbose for now (warning or error log level) and then once things have mostly settled, move them to debug log level.
To make that an easier process it might be a better idea to use a Result[void, string] instead of the bool and do the logging at a higher level.
There was a problem hiding this comment.
Yes that's right, during integration testing it would be helpful. Ok, I'll refactor to use the Result type then log the error message.
| @@ -13,6 +13,7 @@ import | |||
| eth/common, | |||
| eth/p2p/discoveryv5/[protocol, enr], | |||
| ../../database/content_db, | |||
| ../history/[history_network], | |||
There was a problem hiding this comment.
nitpick:
| ../history/[history_network], | |
| ../history/history_network, |
There was a problem hiding this comment.
No problem, will update.
| ): Future[Result[void, string]] {.async.} = | ||
| doAssert(contentKey.contentType == contentValue.contentType) | ||
|
|
||
| let res = |
There was a problem hiding this comment.
You don't really have to do this in-between res value that you then return. If you end each case of with an expression or a return, that should work.
There was a problem hiding this comment.
Good point. I believe I refactored this one, there was previously some code before the return. I'll clean it up.
| let res = await n.validateContent(decodedKey, decodedValue) | ||
| res.isOkOr: |
There was a problem hiding this comment.
| let res = await n.validateContent(decodedKey, decodedValue) | |
| res.isOkOr: | |
| (await n.validateContent(decodedKey, decodedValue)).isOkOr: |
Does this not work instead?
There was a problem hiding this comment.
Yep that works
| if nextHash.len() != 32: | ||
| return false | ||
|
|
||
| # is there a better way to build a KeccakHash from bytes? |
There was a problem hiding this comment.
This doesn't work unfortunately because nextHash is a seq and not an array.
There was a problem hiding this comment.
Ah, right.
Then I think it is better to use initCopyFrom: https://github.com/status-im/nim-stew/blob/master/stew/arrayops.nim#L57 or toArray: https://github.com/status-im/nim-stew/blob/master/stew/objects.nim#L24
Example usage on initCopyFrom:
nimbus-eth1/fluffy/eth_data/era1.nim
Line 414 in 8767bbd
There was a problem hiding this comment.
Thanks, will do
| keccakHash(value.asSeq()) == expectedHash | ||
|
|
||
| proc isValidNextNode(nodeRlp: Rlp, rlpIdx: int, nextNode: TrieNode): bool = | ||
| let nextHashRlp = nodeRlp.listElem(rlpIdx) |
There was a problem hiding this comment.
I think before doing listElem an isList() check is required (there is a doAssert self.isList() in there, yes, perhaps not the best API).
Or are we certain that these RLP bytes are a list at this point?
There was a problem hiding this comment.
We are sure its a list because it only gets called from validateTrieProof which calls listLen() which internally checks if the data is a list. If its not then it return len of zero.
| "0x1ad7b80af0c28bc1489513346d2706885be90abb07f23ca28e50482adb392d61".hexToSeqByte(), | ||
| "0x1ad7b80af0c28bc1489513346d2706885be90abb07f23ca28e50482adb392d61".hexToSeqByte(), | ||
| "0xd7f8974fb5ac78d9ac099b9ad5018bedc2ce0a72dad1827a1709da30580f0544".hexToSeqByte(), |
There was a problem hiding this comment.
So the state roots are missing data from the yaml test vectors?
There was a problem hiding this comment.
Well the test vectors contain the blockHash inside the encoded offer data which can be used to get the stateRoot in theory. But this test doesn't run the history network and simply passes in the stateRoot so I had to provide it. I can open a PR on the test vectors repo to add them if you think its a good idea?
| @@ -0,0 +1,142 @@ | |||
| # Nimbus | |||
There was a problem hiding this comment.
Perhaps a better name for this file would be test_state_validation_genesis.nim
There was a problem hiding this comment.
Sure, I'll update
| suite "State Proof Verification Tests": | ||
| let genesisFiles = [ | ||
| "berlin2000.json", "calaveras.json", "chainid1.json", "chainid7.json", | ||
| "devnet4.json", "devnet5.json", "holesky.json", "mainshadow1.json", "merge.json", |
There was a problem hiding this comment.
You got all these testnet genesis files from https://github.com/eth-clients repos I assume?
I thought we had access to most of these in the vendors git submodules, but apparently not. That's only the case for nimbus-eth2 currently.
There was a problem hiding this comment.
No actually these are from the Nimbus-Eth1 test directory here: tests/customgenesis. I just copied a few more over as a part of this change.
No description provided.