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

Fix syncing with pv63 peers #2204

Merged
merged 1 commit into from
Sep 20, 2016
Merged

Fix syncing with pv63 peers #2204

merged 1 commit into from
Sep 20, 2016

Conversation

arkpar
Copy link
Collaborator

@arkpar arkpar commented Sep 20, 2016

No description provided.

@arkpar arkpar added the A0-pleasereview 🤓 Pull request needs code review. label Sep 20, 2016
@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 20, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 85.278% when pulling 302f8a1 on sync-fix into 48be609 on master.

@@ -1274,7 +1273,7 @@ impl ChainSync {
let pv64 = io.eth_protocol_version(peer) >= 64;
let mut packet = RlpStream::new_list(if pv64 { 7 } else { 5 });
let chain = io.chain().chain_info();
packet.append(&(PROTOCOL_VERSION as u32));
packet.append(&(io.eth_protocol_version(peer) as u32));
Copy link
Contributor

Choose a reason for hiding this comment

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

digging into the code, seems that eth_protocol_version is a fairly expensive call. maybe keep the result of the first call?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is done properly in #2098

@rphmeier rphmeier added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. M4-core ⛓ Core client code / Rust. A8-looksgood 🦄 Pull request is reviewed well. and removed A8-looksgood 🦄 Pull request is reviewed well. A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Sep 20, 2016
@rphmeier rphmeier merged commit 28adfc3 into master Sep 20, 2016
@rphmeier rphmeier deleted the sync-fix branch September 20, 2016 13:51
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.

4 participants