Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core-p2p): malformed condition for filtering peers #1939

Merged
merged 5 commits into from
Jan 10, 2019

Conversation

vasild
Copy link
Contributor

@vasild vasild commented Jan 3, 2019

Currently we would leave in the list all peers that:
"are not me or have invalid port or have invalid version"
the intention must have been:
"are not me and have valid port and have valid version".

This change was first done in 0c23196 and was later reverted in
109a4d3 with a scarce explanation: "it randomly causes issues". It is
not clear what those issues were.

However the current code is strikingly and obviously wrong - it would
add peers that have invalid port, for example.

So, fix the wrong code and followup by fixing any issues that arise, but
not by resurrecting the wrong code.

Proposed changes

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (improve a current implementation without adding a new feature or fixing a bug)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Build (changes that affect the build system)
  • Docs (documentation only changes)
  • Test (adding missing tests or fixing existing tests)
  • Other... Please describe:

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Currently we would leave in the list all peers that:
"are not me or have invalid port or have invalid version"
the intention must have been:
"are not me and have valid port and have valid version".

This change was first done in 0c23196 and was later reverted in
109a4d3 with a scarce explanation: "it randomly causes issues". It is
not clear what those issues were.

However the current code is strikingly and obviously wrong - it would
add peers that have invalid port, for example.

So, fix the wrong code and followup by fixing any issues that arise, but
not by resurrecting the wrong code.
@vasild vasild requested a review from faustbrian January 3, 2019 12:53
@codecov-io
Copy link

codecov-io commented Jan 3, 2019

Codecov Report

Merging #1939 into develop will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1939   +/-   ##
========================================
  Coverage    39.24%   39.24%           
========================================
  Files          363      363           
  Lines         7840     7840           
  Branches      1172     1172           
========================================
  Hits          3077     3077           
  Misses        4748     4748           
  Partials        15       15
Impacted Files Coverage Δ
packages/core-p2p/src/monitor.ts 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39b6aa8...ed00e0c. Read the comment docs.

@faustbrian
Copy link
Contributor

This caused issues on testnet and seeds where they would endlessly loop because there were no peers after this change, that is why it was reverted.

@vasild
Copy link
Contributor Author

vasild commented Jan 3, 2019

So, on testnet all peers in peers.json have invalid port? How do I reproduce that problem?

@faustbrian faustbrian merged commit a8aa729 into develop Jan 10, 2019
@faustbrian faustbrian deleted the fix-peers-filtering-redo branch January 10, 2019 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants