Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

Cap max incoming new block hashes at c_maxIncomingNewHashes #5676

Merged
merged 2 commits into from
Jul 18, 2019

Conversation

halfalicious
Copy link
Contributor

@halfalicious halfalicious commented Jul 17, 2019

Fix #5677

When we receive a NewBlockHashes ethereum capability packet which contains more hashes than c_maxIncomingNewHashes, process the first c_maxIncomingNewHashes hashes rather than disabling the peer.

This bring's Aleth's behavior in line with Parity's (and I suspect Geth's as well).

Parity's processing of the NewBlockHashes packet is here: https://github.com/paritytech/parity-ethereum/blob/7f707fa524baaaccac499d69f9e452b380c31ffa/ethcore/sync/src/chain/handler.rs#L219-L288

@halfalicious halfalicious changed the title Cap max incoming hashes at c_maxIncomingNewHashes Cap max incoming new block hashes at c_maxIncomingNewHashes Jul 17, 2019
@halfalicious halfalicious requested review from gumb0 and chfast July 17, 2019 04:52
Copy link
Member

@chfast chfast left a comment

Choose a reason for hiding this comment

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

Simple enough I can understand it. @gumb0 ?

Copy link
Member

@gumb0 gumb0 left a comment

Choose a reason for hiding this comment

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

Looks fine, did you see this happening?

(our own code looks to be sending out no more than 20 hashes)

@halfalicious
Copy link
Contributor Author

@gumb0 Haven't checked to see if we're actually hitting this, came across this issue when comparing our behavior to the Ethereum Wire Protocol spec / other clients' behavior

@halfalicious
Copy link
Contributor Author

gcc CI failure looks infrastructure-related:


==> Uploading
    .url https://codecov.io
    .query build=12468.0&slug=ethereum%2Faleth&commit=46a0426787f2e86b317820272f08af170b807661&branch=return-maxhashes&yaml=codecov.yml&package=py2.0.15&service=circleci&job=12468.0
    Pinging Codecov...
Error: HTTPSConnectionPool(host='codecov.io', port=443): Max retries exceeded with url: /upload/v4?build=12468.0&slug=ethereum%2Faleth&commit=46a0426787f2e86b317820272f08af170b807661&branch=return-maxhashes&yaml=codecov.yml&package=py2.0.15&service=circleci&job=12468.0 (Caused by NewConnectionError('<requests.packages.urllib3.connection.VerifiedHTTPSConnection object at 0x7f0230dc3978>: Failed to establish a new connection: [Errno 110] Connection timed out',))

Tip: See all example repositories: https://github.com/codecov?query=example
Support channels:
  Email:   hello@codecov.io
  IRC:     #codecov
  Gitter:  https://gitter.im/codecov/support
  Twitter: @codecov

@codecov-io
Copy link

codecov-io commented Jul 17, 2019

Codecov Report

Merging #5676 into master will increase coverage by 0.06%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5676      +/-   ##
==========================================
+ Coverage   62.97%   63.03%   +0.06%     
==========================================
  Files         350      350              
  Lines       29990    29953      -37     
  Branches     3361     3352       -9     
==========================================
- Hits        18885    18881       -4     
+ Misses       9885     9856      -29     
+ Partials     1220     1216       -4

@chfast
Copy link
Member

chfast commented Jul 17, 2019

You can at least see in the coverage report that this code is not hit by any unit test. https://codecov.io/gh/ethereum/aleth/pull/5676/diff

@halfalicious
Copy link
Contributor Author

Fixed CHANGELOG merge conflict

@halfalicious halfalicious merged commit 6e04540 into master Jul 18, 2019
@halfalicious halfalicious deleted the return-maxhashes branch July 18, 2019 02:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Aleth disables peer when receiving NewBlockHashes packet containing more hashes than Aleth's internal max
4 participants