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

Increase number of requested block bodies in chain sync #10247

Merged
merged 22 commits into from
Feb 7, 2019
Merged

Increase number of requested block bodies in chain sync #10247

merged 22 commits into from
Feb 7, 2019

Conversation

elferdo
Copy link
Contributor

@elferdo elferdo commented Jan 25, 2019

In order to speed up syncing and to be on par with geth, we want to increase the number of block bodies requested from a peer. It happens, though, that older parity software would not send any reply at all when a packet went over a given size. To play it safe, if syncing with such a peer we want to keep the lower numbers we have today.

In addition to increasing the number of block bodies per request, I have imlemented a struct, ClientVersion, and associated functionality to help parsing peer Id strings and querying for specific capabilities of the peer software.

Preliminary tests syncing with only geth nodes show an increase in synced blocks for a given fixed length of time of around 30% (with variations between runs).

Thanks to @ngotchac for support and reviewing.

* Increase the number of block bodies requested during Sync.

* Check if our peer is an older parity client with the bug
  of not handling large requests properly

* Add a ClientVersion struct and a ClientCapabilites trait
* Replace strings with ClientVersion in PeerInfo

* Group further functionality in ClientCapabilities
@5chdn 5chdn added this to the 2.4 milestone Jan 25, 2019
@5chdn 5chdn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jan 25, 2019
Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Looks good! I found some issue with version parsing though.

util/network/src/client_version.rs Outdated Show resolved Hide resolved
util/network/src/client_version.rs Outdated Show resolved Hide resolved
util/network/src/client_version.rs Outdated Show resolved Hide resolved
util/network/src/client_version.rs Outdated Show resolved Hide resolved
util/network/src/client_version.rs Outdated Show resolved Hide resolved
ethcore/sync/src/lib.rs Outdated Show resolved Hide resolved
ethcore/sync/src/sync_io.rs Outdated Show resolved Hide resolved
rpc/src/v1/types/sync.rs Outdated Show resolved Hide resolved
util/network/src/client_version.rs Show resolved Hide resolved
util/network/src/client_version.rs Show resolved Hide resolved
util/network/src/client_version.rs Outdated Show resolved Hide resolved
util/network/src/client_version.rs Outdated Show resolved Hide resolved
util/network/src/client_version.rs Outdated Show resolved Hide resolved
util/network/src/client_version.rs Outdated Show resolved Hide resolved
util/network/src/client_version.rs Outdated Show resolved Hide resolved
  if it's parity, remove version check.

* Remove dependency on semver in ethcore-sync

* Remove unnecessary String instantiation

* Rename peer_info to peer_version

* Update RPC test helpers

* Simplify From<String>

* Parse static version string only once

* Update RPC tests to new ClientVersion struct

* Document public members

* More robust parsing of ID string
util/network/src/client_version.rs Show resolved Hide resolved
util/network/src/client_version.rs Outdated Show resolved Hide resolved
rpc/src/v1/tests/helpers/sync_provider.rs Outdated Show resolved Hide resolved
rpc/src/v1/tests/helpers/sync_provider.rs Outdated Show resolved Hide resolved
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.

lgtm

@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 31, 2019
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.

LGTM

util/network/src/client_version.rs Outdated Show resolved Hide resolved
@ngotchac ngotchac added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Feb 5, 2019
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.

There are some leftovers after changing PARITY_CLIENT_LARGE_REQUESTS_VERSION to 2.4.0,

ethcore/sync/src/block_sync.rs Outdated Show resolved Hide resolved
util/network/src/client_version.rs Outdated Show resolved Hide resolved
util/network/src/client_version.rs Outdated Show resolved Hide resolved
ordian and others added 4 commits February 5, 2019 14:45
Co-Authored-By: elferdo <elferdo@gmail.com>
Co-Authored-By: elferdo <elferdo@gmail.com>
Co-Authored-By: elferdo <elferdo@gmail.com>
@ngotchac ngotchac added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Feb 5, 2019
@5chdn
Copy link
Contributor

5chdn commented Feb 7, 2019

@elferdo could you resolve the conflicts?

@tomusdrw are your comments addressed?

Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Awesome, looks good to me!

ethcore/sync/src/chain/mod.rs Outdated Show resolved Hide resolved
rpc/src/v1/tests/mod.rs Outdated Show resolved Hide resolved
util/network/src/client_version.rs Outdated Show resolved Hide resolved
@5chdn
Copy link
Contributor

5chdn commented Feb 7, 2019

It's squash-and-merge day! Sorry for the conflicts again.

@5chdn 5chdn merged commit b7e8621 into openethereum:master Feb 7, 2019
ordian added a commit that referenced this pull request Apr 5, 2019
* master:
  fix: parity-clib/examples/cpp/CMakeLists.txt (#10313)
  CI optimizations (#10297)
  Increase number of requested block bodies in chain sync (#10247)
  Deprecate account management (#10213)
  Properly handle check_epoch_end_signal errors (#10015)
  fix(osx and windows builds): bump parity-daemonize (#10291)
  Add missing step for  Using `systemd` service file (#10175)
  Call private contract methods from another private contract (read-only)  (#10086)
  update ring to 0.14 (#10262)
  fix(secret-store): deprecation warning (#10301)
  Update to jsonrpc-derive 10.0.2, fixes aliases bug (#10300)
  Convert to jsonrpc-derive, use jsonrpc-* from crates.io (#10298)
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.

None yet

6 participants