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(net): Reject nodes using ZClassic ports, and warn if configured with those ports #6567

Merged
merged 5 commits into from
Apr 28, 2023

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Apr 25, 2023

Motivation

We want to reject ZClassic nodes by port, because some block explorers are picking them up and sharing their addresses:
zcash/zcash#6505 (comment)

Zebra currently rejects ZClassic nodes because their protocol versions are too low, but we need to make a connection to do that. ZClassic could also increase their protocol versions in future.

We also want to warn the user if they configure their node with those ports.

Solution

  • Add the ZClassic ports to our existing node port check
  • Do the regtest and coin checks even if we don't have a network
  • Replace our Mainnet/Tesnet wrong port check with the check we do on remote node addresses, including ports used by other coins

Review

This is a low priority peer handling and config issue.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@teor2345 teor2345 added P-Low ❄️ C-security Category: Security issues I-slow Problems with performance or responsiveness A-network Area: Network protocol updates or fixes A-compatibility Area: Compatibility with other nodes or wallets, or standard rules labels Apr 25, 2023
@teor2345 teor2345 self-assigned this Apr 25, 2023
@teor2345 teor2345 requested a review from a team as a code owner April 25, 2023 23:29
@teor2345 teor2345 requested review from oxarbitrage and removed request for a team April 25, 2023 23:29
@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Merging #6567 (31bf87b) into main (1461c91) will increase coverage by 0.15%.
The diff coverage is 58.82%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6567      +/-   ##
==========================================
+ Coverage   77.78%   77.93%   +0.15%     
==========================================
  Files         307      307              
  Lines       40273    40278       +5     
==========================================
+ Hits        31325    31390      +65     
+ Misses       8948     8888      -60     

@teor2345 teor2345 changed the title Reject nodes using the ZClassic default ports fix(net): Reject nodes using ZClassic ports, and warn if configured with those ports Apr 26, 2023
@teor2345 teor2345 added I-usability Zebra is hard to understand or use A-diagnostics Area: Diagnosing issues or monitoring performance labels Apr 26, 2023
@github-actions github-actions bot added the C-bug Category: This is a bug label Apr 26, 2023
@teor2345
Copy link
Contributor Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Apr 27, 2023

update

✅ Branch has been successfully updated

@teor2345 teor2345 requested a review from oxarbitrage April 27, 2023 20:23
mergify bot added a commit that referenced this pull request Apr 27, 2023
@mergify mergify bot merged commit 9c4b3c2 into main Apr 28, 2023
@mergify mergify bot deleted the net-reject-zclassic branch April 28, 2023 00:17
@oxarbitrage oxarbitrage mentioned this pull request May 9, 2023
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compatibility Area: Compatibility with other nodes or wallets, or standard rules A-diagnostics Area: Diagnosing issues or monitoring performance A-network Area: Network protocol updates or fixes C-bug Category: This is a bug C-security Category: Security issues I-slow Problems with performance or responsiveness I-usability Zebra is hard to understand or use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants