[ETCM-283] Add block bodies validation in block fetcher#771
[ETCM-283] Add block bodies validation in block fetcher#771
Conversation
… we want to process
mirkoAlic
force-pushed
the
etcm-283-validate-bodies-in-block-fetcher
branch
from
640d8c5 to
21d65f8
Compare
kapke
left a comment
kapke
left a comment
There was a problem hiding this comment.
Tests need a bit more comments, besides that the code looks good to me!
| ledger, | ||
| bl, | ||
| blockchainConfig, // FIXME: remove in ETCM-280 | ||
| validators.blockValidator, |
There was a problem hiding this comment.
minor: should this validator be used here instead of more close to production one?
| peersClient.reply(PeersClient.Response(fakePeer, getBlockBodiesResponse)) | ||
|
|
||
| peersClient.expectMsgClass(classOf[BlacklistPeer]) | ||
| peersClient.expectMsgPF() { case PeersClient.Request(msg, _, _) if msg == getBlockBodiesRequest => () } |
There was a problem hiding this comment.
what about test like:
- fetch one batch of blocks
- for the second batch send different bodies (so even proper validator fails)
- pick blocks and verify that only these from first batch are responded
Basically same issue with these tests - lack of explicit assertions or at least comments, why it's enough to check for certain message/lack of it.
There was a problem hiding this comment.
The main issue i see with the test you propose is that i would need the real block validator, and that make things more complex. So i will opt for improve current test assertions if you are ok with that.
There was a problem hiding this comment.
(main issue is current impl of bodiesAreOrderedSubsetOfRequested, given i can't use a simple mock validator to match headers and bodies)
| peersClient.expectMsgPF() { case PeersClient.Request(msg, _, _) if msg == getBlockBodiesRequest1 => () } | ||
|
|
||
| // It will receive all the requested bodies, but splitted in 2 parts. | ||
| val (subChain1, subChain2) = chain.splitAt(syncConfig.blockHeadersPerRequest / 2) |
There was a problem hiding this comment.
block bodies per request? I know these are same values in tests, but the right config value should be used anyway I think
| val getBlockBodiesResponse2 = BlockBodies(subChain2.map(_.body)) | ||
| peersClient.reply(PeersClient.Response(fakePeer, getBlockBodiesResponse2)) | ||
|
|
||
| peersClient.expectNoMessage() |
There was a problem hiding this comment.
Is there a better way? Maybe try to pick blocks or sth like that?
There was a problem hiding this comment.
can this expectNoMessageCall be removed now?
kapke
dismissed
their stale review
The approval, I'd like the tests to be a bit more polished ;)
Merge branch 'develop' of github.com:input-output-hk/mantis into etcm-283-validate-bodies-in-block-fetcher Conflicts: src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcher.scala src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcherState.scala src/test/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcherSpec.scala src/test/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcherStateSpec.scala
mirkoAlic
force-pushed
the
etcm-283-validate-bodies-in-block-fetcher
branch
3 times, most recently
from
5a91450 to
5c1fecd
Compare
mirkoAlic
force-pushed
the
etcm-283-validate-bodies-in-block-fetcher
branch
from
5c1fecd to
0a4b94a
Compare
…s into etcm-283-validate-bodies-in-block-fetcher Conflicts: src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcher.scala
kapke
left a comment
kapke
left a comment
There was a problem hiding this comment.
The tests look much better now!
| val getBlockBodiesResponse2 = BlockBodies(subChain2.map(_.body)) | ||
| peersClient.reply(PeersClient.Response(fakePeer, getBlockBodiesResponse2)) | ||
|
|
||
| peersClient.expectNoMessage() |
There was a problem hiding this comment.
can this expectNoMessageCall be removed now?
3 participants
Description
Our current BlockFetcher doesn't do any validation on the BlockHeaders and BlockBodies messages received and just match them.
Proposed solution
Validate the matching between new bodies received and expected headers, if there isn't a match fetcher will complain about the peer that send those bodies.
Important Changes Introduced
blockchain.sync.regular.BlockFetcherStatenow is coupled toconsensus.validators.std.StdBlockValidator^ Ideally we shouldn't do such thing!
TODO
add test regarding a sub ordered sequence of bodies received