[ETCM-283] Add block bodies validation in block fetcher #771

mirkoAlic · 2020-11-03T18:40:58Z

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.BlockFetcherState now is coupled to consensus.validators.std.StdBlockValidator
^ Ideally we shouldn't do such thing!

TODO

~~add test regarding a sub ordered sequence of bodies received~~

src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcher.scala

src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcherState.scala

src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcher.scala

… we want to process

kapke

Tests need a bit more comments, besides that the code looks good to me!

kapke · 2020-11-18T13:56:22Z

src/it/scala/io/iohk/ethereum/sync/util/RegularSyncItSpecUtils.scala

@@ -71,6 +73,7 @@ object RegularSyncItSpecUtils {
        ledger,
        bl,
        blockchainConfig, // FIXME: remove in ETCM-280
+        validators.blockValidator,


minor: should this validator be used here instead of more close to production one?

kapke · 2020-11-18T14:04:21Z

src/test/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcherSpec.scala

+      peersClient.reply(PeersClient.Response(fakePeer, getBlockBodiesResponse))
+
+      peersClient.expectMsgClass(classOf[BlacklistPeer])
+      peersClient.expectMsgPF() { case PeersClient.Request(msg, _, _) if msg == getBlockBodiesRequest => () }


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.

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.

(main issue is current impl of bodiesAreOrderedSubsetOfRequested, given i can't use a simple mock validator to match headers and bodies)

kapke · 2020-11-18T14:07:53Z

src/test/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcherSpec.scala

+      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)


block bodies per request? I know these are same values in tests, but the right config value should be used anyway I think

kapke · 2020-11-18T14:08:26Z

src/test/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcherSpec.scala

+      val getBlockBodiesResponse2 = BlockBodies(subChain2.map(_.body))
+      peersClient.reply(PeersClient.Response(fakePeer, getBlockBodiesResponse2))
+
+      peersClient.expectNoMessage()


Is there a better way? Maybe try to pick blocks or sth like that?

can this expectNoMessageCall be removed now?

The approval, I'd like the tests to be a bit more polished ;)

mirkoAlic · 2020-11-18T17:42:32Z

@kapke i will wait til your pr #787 is merged in order to polish the tests and adjust my code, given there will be some little overlapping between: addBodies <-> receiveBlocks.

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

…s into etcm-283-validate-bodies-in-block-fetcher Conflicts: src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcher.scala

kapke

The tests look much better now!

src/test/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcherSpec.scala

kapke · 2020-11-24T08:46:21Z

src/test/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcherSpec.scala

+      val getBlockBodiesResponse2 = BlockBodies(subChain2.map(_.body))
+      peersClient.reply(PeersClient.Response(fakePeer, getBlockBodiesResponse2))
+
+      peersClient.expectNoMessage()


can this expectNoMessageCall be removed now?

src/test/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcherSpec.scala

[ETCM-283] Add block bodies validation in block fetcher

3e6d236

mirkoAlic commented Nov 3, 2020

View reviewed changes

src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcher.scala Show resolved Hide resolved

mirkoAlic added 2 commits November 4, 2020 10:58

[ETCM-283] Pass block validator as param

6a71cb3

[ETCM-283] Add unit test for the failure scenario

85cbcf5

mirkoAlic marked this pull request as ready for review November 4, 2020 17:45

mirkoAlic requested review from kapke and KonradStaniec November 4, 2020 17:49

kapke reviewed Nov 6, 2020

View reviewed changes

ntallar mentioned this pull request Nov 9, 2020

Etcm 290 invalid branches built by fetcher #787

Merged

[ETCM-283] Check that blocks correspond to the requested headers that…

79be12a

… we want to process

mirkoAlic added the WIP label Nov 13, 2020

[ETCM-283] Add unit tests

21d65f8

mirkoAlic force-pushed the etcm-283-validate-bodies-in-block-fetcher branch from 640d8c5 to 21d65f8 Compare November 17, 2020 15:28

[ETCM-283] Fix log message

149a549

mirkoAlic removed the WIP label Nov 17, 2020

kapke previously approved these changes Nov 18, 2020

View reviewed changes

kapke self-requested a review November 18, 2020 14:12

KonradStaniec approved these changes Nov 18, 2020

View reviewed changes

mirkoAlic force-pushed the etcm-283-validate-bodies-in-block-fetcher branch 3 times, most recently from 5a91450 to 5c1fecd Compare November 19, 2020 20:16

[ETCM-283] Fix and Refactor tests

0a4b94a

mirkoAlic force-pushed the etcm-283-validate-bodies-in-block-fetcher branch from 5c1fecd to 0a4b94a Compare November 19, 2020 20:24

mirkoAlic added 3 commits November 19, 2020 17:32

[ETCM-283] Remove unnecesary lastFullBlockNumber

ef937c4

[ETCM-283] Fix it

071356b

[ETCM-283] Merge branch 'develop' of github.com:input-output-hk/manti…

7a5a881

…s into etcm-283-validate-bodies-in-block-fetcher Conflicts: src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcher.scala

kapke approved these changes Nov 24, 2020

View reviewed changes

mirkoAlic and others added 3 commits November 24, 2020 08:49

[ETCM-283] Tests clean up

2b0edd7

Merge branch 'develop' into etcm-283-validate-bodies-in-block-fetcher

b7ca4b6

[ETCM-283] Add Thread.sleep in order to fix test

5f0e5c2

mirkoAlic merged commit b5cb34b into develop Nov 24, 2020

mirkoAlic deleted the etcm-283-validate-bodies-in-block-fetcher branch November 24, 2020 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ETCM-283] Add block bodies validation in block fetcher #771

[ETCM-283] Add block bodies validation in block fetcher #771

mirkoAlic commented Nov 3, 2020 •

edited

Loading

kapke left a comment

kapke Nov 18, 2020

kapke Nov 18, 2020

mirkoAlic Nov 19, 2020

mirkoAlic Nov 19, 2020

kapke Nov 18, 2020

kapke Nov 18, 2020

kapke Nov 24, 2020

mirkoAlic commented Nov 18, 2020

kapke left a comment

kapke Nov 24, 2020

[ETCM-283] Add block bodies validation in block fetcher #771

[ETCM-283] Add block bodies validation in block fetcher #771

Conversation

mirkoAlic commented Nov 3, 2020 • edited Loading

Description

Proposed solution

Important Changes Introduced

TODO

kapke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirkoAlic commented Nov 18, 2020

kapke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirkoAlic commented Nov 3, 2020 •

edited

Loading