Verify block syncing responses against requests #9670

jimpo · 2018-09-28T11:10:01Z

This uses the asking_hash and asking_blocks fields set on the peer before making a request to ensure their responses are expected and valid. One major issue I noticed is clients with corrupted chains returning a sequence of block headers that do not connect with one another. The additional validation on block headers especially has improved the reliability of ancient block syncing in my testing.

parity-cla-bot · 2018-09-28T11:10:03Z

It looks like @jimpo signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

ascjones

I like this approach. I've tested this and it does indeed avoid the corrupt headers and makes progress downloading the ancient blocks. However progress is slow compared to #9531:

andrew@Ubuntu-1804-bionic-64-minimal:~/sync-repro/fix-ancient-blocks-sync$ egrep 'import\s+\#[0-9]+ ' sync.log 
2018-10-01 13:05:00  IO Worker #3 INFO import       #7747   27/50 peers   6 MiB chain 79 MiB db 0 bytes queue 8 MiB sync  RPC:  0 conn,    0 req/s,    0 µs
2018-10-01 13:05:33  IO Worker #3 INFO import      #19564   26/50 peers   6 MiB chain 78 MiB db 0 bytes queue 9 MiB sync  RPC:  0 conn,    0 req/s,    0 µs
...
2018-10-01 13:10:28  IO Worker #1 INFO import     #113772   28/50 peers   6 MiB chain 82 MiB db 0 bytes queue 17 MiB sync  RPC:  0 conn,    0 req/s,    0 µs

2018-10-01 11:55:09  IO Worker #2 INFO import       #8014   14/25 peers   5 MiB chain 73 MiB db 0 bytes queue 51 MiB sync  RPC:  0 conn,    0 req/s,    0 µs
2018-10-01 11:55:43  IO Worker #3 INFO import      #12438   12/25 peers   5 MiB chain 74 MiB db 0 
18-10-01 11:56:50  IO Worker #1 INFO import      #16920   21/25 peers   6 MiB chain 75 MiB db 0 bytes queue 71 MiB sync  RPC:  0 conn,    0 req/s,    0 µs
2018-10-01 11:57:24  IO Worker #1 DEBUG sync  69 -> Dispatching packet: 22018-10-01 11:57:24  IO Worker #3 INFO import      #16920   21/25 peers   6 MiB chain 75 MiB db 0 bytes queue 71 MiB sync  RPC:  0 conn,    0 req/s,    0 µs
...
2018-10-01 12:00:54  IO Worker #2 INFO import      #16920   20/25 peers   6 MiB chain 75 MiB db 0 bytes queue 71 MiB sync  RPC:  0 conn,    0 req/s,    0 µs2018-10-01 12:00:54  IO Worker #3 TRACE sync  158 -> Transactions (1 entries)
2018-10-01 12:02:04  IO Worker #0 INFO import      #16920   20/25 peers   6 MiB chain 75 MiB db 0 bytes queue 71 MiB sync  RPC:  0 conn,    0 req/s,    0 µs2018-10-01 12:02:04  IO Worker #2 DEBUG sync  224 -> Dispatching packet: 2
2018-10-01 12:02:40  IO Worker #2 INFO import      #16920   20/25 peers   6 MiB chain 75 MiB db 0 bytes queue 71 MiB sync  RPC:  0 conn,    0 req/s,    0 µs
2018-10-01 12:03:14  IO Worker #0 INFO import      #16920   20/25 peers   6 MiB chain 75 MiB db 0 bytes queue 71 MiB sync  RPC:  0 conn,    0 req/s,    0 µs

From a brief look at the logs, it could be because it is not resetting the download when the import queue is full, and it continues to download all the blocks in that round which would get rejected with Unknown new block parent.

Since this PR gets ancient blocks from not working -> working, happy to get this merged ASAP and can always modify my PR to build on this.

ascjones · 2018-10-01T10:47:09Z

ethcore/src/client/test_client.rs

-			self.import_block(unverified).unwrap();
-		}
-	}
+	pub fn add_block<F>(&self, with: EachBlockWith, hook: F)


Compiler warning for missing docs

jimpo · 2018-10-01T22:11:37Z

@ascjones Thanks for the review. This PR is complimentary to #9531 and definitely does not fix all of the issues that that one addresses. I believe it mostly does not conflict in a way that will complicate merging. There are some other changes in this branch that conflict more directly that we can look into after your PR is merged.

ordian · 2018-10-02T10:33:09Z

@jimpo Thanks for your awesome contributions! Could you rebase on master to include recent CI changes?

5chdn · 2018-10-02T10:33:46Z

(sorry for the CI mess, once rebased we can merge this)

…mon-deps * origin/master: Add Foundation Bootnodes (#9666) Docker: run as parity user (#9689) ethcore: mcip3 block reward contract (#9605) Verify block syncing responses against requests (#9670)

* sync: Validate received BlockHeaders packets against stored request. * sync: Validate received BlockBodies and BlockReceipts. * sync: Fix broken tests. * sync: Unit tests for BlockDownloader::import_headers. * sync: Unit tests for import_{bodies,receipts}. * tests: Add missing method doc.

* produce portable binaries (#9725) * HF in POA Core (2018-10-22) (#9724) poanetwork/poa-chain-spec#87 * Use static call and apparent value transfer for block reward contract code (#9603) * Verify block syncing responses against requests (#9670) * sync: Validate received BlockHeaders packets against stored request. * sync: Validate received BlockBodies and BlockReceipts. * sync: Fix broken tests. * sync: Unit tests for BlockDownloader::import_headers. * sync: Unit tests for import_{bodies,receipts}. * tests: Add missing method doc. * Fix ancient blocks sync (#9531) * Log block set in block_sync for easier debugging * logging macros * Match no args in sync logging macros * Add QueueFull error * Only allow importing headers if the first matches requested * WIP * Test for chain head gaps and log * Calc distance even with 2 heads * Revert previous commits, preparing simple fix This reverts commit 5f38aa8. * Reject headers with no gaps when ChainHead * Reset block sync download when queue full * Simplify check for subchain heads * Add comment to explain subchain heads filter * Fix is_subchain_heads check and comment * Prevent premature round completion after restart This is a problem on mainnet where multiple stale peer requests will force many rounds to complete quickly, forcing the retraction. * Reset stale old blocks request after queue full * Revert "Reject headers with no gaps when ChainHead" This reverts commit 0eb8655. * Add BlockSet to BlockDownloader logging Currently it is difficult to debug this because there are two instances, one for OldBlocks and one for NewBlocks. This adds the BlockSet to all log messages for easy log filtering. * Reset OldBlocks download from last enqueued Previously when the ancient block queue was full it would restart the download from the last imported block, so the ones still in the queue would be redownloaded. Keeping the existing downloader instance and just resetting it will start again from the last enqueued block.:wq * Ignore expired Body and Receipt requests * Log when ancient block download being restarted * Only request old blocks from peers with >= difficulty #9226 might be too permissive and causing the behaviour of the retraction soon after the fork block. With this change the peer difficulty has to be greater than or euqal to our syncing difficulty, so should still fix #9225 * Some logging and clear stalled blocks head * Revert "Some logging and clear stalled blocks head" This reverts commit 757641d. * Reset stalled header if useless more than once * Store useless headers in HashSet * Add sync target to logging macro * Don't disable useless peer and fix log macro * Clear useless headers on reset and comments * Use custom error for collecting blocks Previously we resued BlockImportError, however only the Invalid case and this made little sense with the QueueFull error. * Remove blank line * Test for reset sync after consecutive useless headers * Don't reset after consecutive headers when chain head * Delete commented out imports * Return DownloadAction from collect_blocks instead of error * Don't reset after round complete, was causing test hangs * Add comment explaining reset after useless * Replace HashSet with counter for useless headers * Refactor sync reset on bad block/queue full * Add missing target for log message * Fix compiler errors and test after merge * ethcore: revert ethereum tests submodule update * Add hardcoded headers (#9730) * add foundation hardcoded header #6486017 * add ropsten hardcoded headers #4202497 * add kovan hardcoded headers #9023489 * gitlab ci: releasable_branches: change variables condition to schedule (#9729)

jimpo force-pushed the verify-block-response branch from 4dda9f9 to 78a0a1d Compare September 28, 2018 11:14

5chdn added this to the 2.2 milestone Sep 29, 2018

5chdn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Sep 29, 2018

ascjones approved these changes Oct 1, 2018

View reviewed changes

ascjones added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 1, 2018

ascjones mentioned this pull request Oct 1, 2018

Fix ancient blocks sync #9531

Merged

ordian approved these changes Oct 2, 2018

View reviewed changes

jimpo added 6 commits October 2, 2018 12:22

sync: Validate received BlockHeaders packets against stored request.

08f2567

sync: Validate received BlockBodies and BlockReceipts.

65da830

sync: Fix broken tests.

f666914

sync: Unit tests for BlockDownloader::import_headers.

46b1042

sync: Unit tests for import_{bodies,receipts}.

731971c

tests: Add missing method doc.

ca9a3dc

jimpo force-pushed the verify-block-response branch from 20f71b3 to ca9a3dc Compare October 2, 2018 19:23

sorpaas approved these changes Oct 3, 2018

View reviewed changes

sorpaas merged commit 2fc1679 into openethereum:master Oct 3, 2018

jimpo deleted the verify-block-response branch October 3, 2018 18:04

andresilva mentioned this pull request Oct 10, 2018

[beta] More backports for 2.1.2 #9733

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify block syncing responses against requests #9670

Verify block syncing responses against requests #9670

jimpo commented Sep 28, 2018

parity-cla-bot commented Sep 28, 2018

ascjones left a comment

ascjones Oct 1, 2018

jimpo commented Oct 1, 2018

ordian commented Oct 2, 2018

5chdn commented Oct 2, 2018

Verify block syncing responses against requests #9670

Verify block syncing responses against requests #9670

Conversation

jimpo commented Sep 28, 2018

parity-cla-bot commented Sep 28, 2018

ascjones left a comment

Choose a reason for hiding this comment

ascjones Oct 1, 2018

Choose a reason for hiding this comment

jimpo commented Oct 1, 2018

ordian commented Oct 2, 2018

5chdn commented Oct 2, 2018