Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

sync: retry different peer after empty subchain heads response #9753

Merged
merged 4 commits into from
Oct 25, 2018

Conversation

ascjones
Copy link
Contributor

@ascjones ascjones commented Oct 15, 2018

Further to #9531, I've experienced another intermittent cause of the ancient block sync retracting:

  • Ancient block queue fills up, reset downloads (blocks is empty)
  • Start a new sync round; request subchain heads
  • Receive empty/useless response; we end up here
  • When syncing old blocks this will return Ok, which will end up giving the request to the same peer again
  • Since blocks is empty and we didn't download any blocks this round, we retract a block for the next round (and later exponentially)
  • That dodgy peer returns an empty/useless response again, and this process repeats which quickly causes the sync to retract to 0.

Fix is to return Err(BlockDownloaderImportError::Useless) in this case, which will result in that peer being deactivated for this round.

Edit: this appears to have been causing the behaviour detailed here: #9306 (comment)

Have tested this with full ancient block sync successfully twice.

@ascjones ascjones added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Oct 15, 2018
@5chdn 5chdn added this to the 2.2 milestone Oct 15, 2018
@eduadiez
Copy link
Collaborator

I'm still experiencing this error:

2018-10-18 09:41:41 UTC Starting Parity-Ethereum/v2.0.8-stable-ef8f95e-20181015/x86_64-linux-gnu/rustc1.29.0
2018-10-18 09:41:41 UTC Keys path /root/.local/share/io.parity.ethereum/keys/ethereum
2018-10-18 09:41:41 UTC DB path /root/.local/share/io.parity.ethereum/chains/ethereum/db/906a34e69aec8c0d
2018-10-18 09:41:41 UTC State DB configuration: fast
2018-10-18 09:41:41 UTC Operating mode: active
2018-10-18 09:41:42 UTC Configured for Foundation using Ethash engine
2018-10-18 09:41:42 UTC Removed existing file '/root/.local/share/io.parity.ethereum/jsonrpc.ipc'.
2018-10-18 09:41:47 UTC Failed to auto-update latest ETH price: Fetch(Timeout)
2018-10-18 09:41:48 UTC Public node URL: enode://770ad51c9ae88643fb59ac4e06e67190ee1d92d68c0cecd9b55f42ff653bc7c69acaea70e1392f24ffc5f0ce5facbe7ecb60bf68ac05d11bc80e231fc9aa7dc4@172.33.1.6:30303
2018-10-18 09:41:49 UTC Imported #6537317 0x48e7…9532 (26 txs, 0.86 Mgas, 189 ms, 4.00 KiB) + another 1 block(s) containing 97 tx(s)
2018-10-18 09:41:51 UTC Imported #6537318 0x3af7…11f1 (96 txs, 7.57 Mgas, 290 ms, 23.75 KiB)
2018-10-18 09:42:17 UTC        #57   25/25 peers    644 KiB chain   70 MiB db  0 bytes queue  225 KiB sync  RPC: 14 conn,    1 req/s,   77 µs
2018-10-18 09:42:52 UTC        #57   25/25 peers    644 KiB chain   70 MiB db  0 bytes queue  225 KiB sync  RPC: 14 conn,    1 req/s,   77 µs
2018-10-18 09:42:58 UTC Imported #6537323 0xa079…2166 (128 txs, 7.99 Mgas, 651 ms, 21.32 KiB) + another 2 block(s) containing 259 tx(s)
2018-10-18 09:43:27 UTC        #57   26/50 peers    899 KiB chain   71 MiB db  0 bytes queue  225 KiB sync  RPC: 14 conn,    1 req/s,   77 µs
2018-10-18 09:43:47 UTC Imported #6537326 0x8c6f…552d (23 txs, 0.92 Mgas, 265 ms, 3.77 KiB) + another 2 block(s) containing 161 tx(s)
2018-10-18 09:44:03 UTC Imported #6537328 0x8ef8…62a7 (65 txs, 3.25 Mgas, 75 ms, 11.49 KiB)
2018-10-18 09:44:03 UTC        #57   24/25 peers    995 KiB chain   70 MiB db  0 bytes queue  382 KiB sync  RPC: 14 conn,    2 req/s,   83 µs
2018-10-18 09:44:25 UTC Imported #6537332 0xa892…e57e (84 txs, 8.00 Mgas, 403 ms, 12.63 KiB) + another 2 block(s) containing 116 tx(s)
2018-10-18 09:44:38 UTC        #57   26/50 peers      1 MiB chain   69 MiB db  0 bytes queue  382 KiB sync  RPC: 14 conn,    1 req/s,   88 µs
2018-10-18 09:45:07 UTC Imported #6537333 0x021e…d2db (39 txs, 7.96 Mgas, 199 ms, 6.36 KiB)
2018-10-18 09:45:13 UTC        #57   24/25 peers      1 MiB chain   70 MiB db  0 bytes queue  382 KiB sync  RPC: 14 conn,    3 req/s,   91 µs

@ascjones
Copy link
Contributor Author

@eduadiez the fix hasn't been backported to stable (you are using v2.0.8-stable), it should be available in the latest beta.

Copy link
Contributor

@ngotchac ngotchac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find! Small grumble, LGTM otherwise

@@ -327,6 +327,9 @@ impl BlockDownloader {
if self.limit_reorg && best > last && (last == 0 || last < oldest_reorg) {
trace_sync!(self, "No common block, disabling peer");
return Err(BlockDownloaderImportError::Invalid);
} else if !self.limit_reorg {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this ensures that it's downloading OldBlocks, right? Can't we use a more specific name or fn?

Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be backported to beta?

@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. B1-patch-beta 🕷🕷 and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 25, 2018
@ascjones
Copy link
Contributor Author

@ordian yes.

@ascjones ascjones merged commit 9a2c4a3 into master Oct 25, 2018
@ascjones ascjones deleted the aj-sync-dont-retry-same-peer branch October 25, 2018 14:57
@5chdn 5chdn mentioned this pull request Oct 25, 2018
13 tasks
5chdn pushed a commit that referenced this pull request Oct 26, 2018
* If no subchain heads then try a different peer

* Add log when useless chain head

* Restrict ChainHead useless peer to ancient blocks

* sync: replace `limit_reorg` with `block_set` condition
5chdn added a commit that referenced this pull request Oct 28, 2018
* version: bump parity beta to 2.1.4

* ethcore: bump ropsten forkblock checkpoint (#9775)

* ethcore: handle vm exception when estimating gas (#9615)

* removed "rustup" & added new runner tag (#9731)

* removed "rustup" & added new runner tag

* exchanged tag "rust-windows" with "windows"

* revert windows tag change

* sync: retry different peer after empty subchain heads response (#9753)

* If no subchain heads then try a different peer

* Add log when useless chain head

* Restrict ChainHead useless peer to ancient blocks

* sync: replace `limit_reorg` with `block_set` condition

* update jsonrpc-core to a1b2bb742ce16d1168669ffb13ffe856e8131228

* Allow zero chain id in EIP155 signing process (#9792)

* Allow zero chain id in EIP155 signing process

* Rename test

* Fix test failure

* Insert dev account before unlocking (#9813)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants