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.

Sign up for GitHub

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

Jump to bottom

fix(net): Try harder to drop connections when they shut down, Credit: Ziggurat Team #6832

Merged
merged 5 commits into from
Jun 7, 2023
Merged

fix(net): Try harder to drop connections when they shut down, Credit: Ziggurat Team #6832

merged 5 commits into from
Jun 7, 2023

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jun 6, 2023

Changelog

The Ziggurat team needs to be credited in this PR's changelog entry. This should happen automatically.

Motivation

Some users report that Zebra v1.0.0-rc.8 opens multiple connections to the same node.

Close #6805

Complex Code or Requirements

This is concurrent code, but this PR mainly modifies the shutdown sequence. The task shutdowns could have timing issues, but the other shutdown code is not concurrent.

Solution

  • Force Client tasks to shut down when it is dropped
  • Try to close the Connection peer sender sink on drop
  • Reliably shut down the peer sender when the Connection is shut down
  • Add PeerSet logging for duplicate peer connections, checking both IP:port and just IP

Testing

I couldn't reproduce the errors on the latest main branch, and I didn't find any bugs that would be easy to test.

I added logging so these kinds of issues are easier to detect in future.

Review

This is a routine security fix.

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?

Follow Up Work

Ask the Ziggurat team to re-test.

@teor2345 teor2345 added P-Medium ⚡ C-security Category: Security issues I-remote-node-overload Zebra can overload other nodes on the network A-network Area: Network protocol updates or fixes A-concurrency Area: Async code, needs extra work to make it work properly. labels Jun 6, 2023
@teor2345 teor2345 requested a review from a team as a code owner June 6, 2023 03:45
@teor2345 teor2345 self-assigned this Jun 6, 2023
@teor2345 teor2345 requested a review from a team as a code owner June 6, 2023 03:45
@teor2345 teor2345 requested review from upbqdn and removed request for a team June 6, 2023 03:45
@github-actions github-actions bot added the C-bug Category: This is a bug label Jun 6, 2023
@codecov
Copy link

codecov bot commented Jun 6, 2023

Codecov Report

Merging #6832 (5a97f45) into main (628ddd6) will decrease coverage by 0.05%.
The diff coverage is 30.85%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6832      +/-   ##
==========================================
- Coverage   77.97%   77.92%   -0.05%     
==========================================
  Files         308      308              
  Lines       41023    41081      +58     
==========================================
+ Hits        31988    32013      +25     
- Misses       9035     9068      +33

@arya2 arya2 self-requested a review June 6, 2023 20:32
arya2
arya2 approved these changes Jun 6, 2023
View reviewed changes
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

Thank you!

zebra-network/src/peer_set/set.rs Show resolved Hide resolved
zebra-network/src/peer_set/set.rs Show resolved Hide resolved
mergify bot added a commit that referenced this pull request Jun 7, 2023
@mergify
Merge of #6832
524383a
@mergify mergify bot mentioned this pull request Jun 7, 2023
Closed
@teor2345 teor2345 mentioned this pull request Jun 7, 2023
Merged
41 tasks
@mergify mergify bot merged commit 428493e into main Jun 7, 2023
@mergify mergify bot deleted the stop-peer-multi-connect branch June 7, 2023 03:41
dconnolly added a commit that referenced this pull request Jun 7, 2023
@dconnolly
Include #6832 in the changelog
5866fc4
mergify bot pushed a commit that referenced this pull request Jun 8, 2023
@dconnolly @teor2345
Release 1.0.0-rc.9 (#6821)
9959a6c 
* Bump semvers

* Update zebra-utils/README.md

* Updated mainnet checkpoints against commit b7029b8

* Add testnet checkpoints from b7029b8

* Bump zebrad rust-version to 1.70

* rust-version 1.68

Co-authored-by: teor <[email protected]>

* Add CHANGELOG for 1.0.0-rc.9

* Bump estimated release height to within june 7th 2023 utc-4

* Add #6801 to CHANGELOG in anticipation

* Update CHANGELOG.md

Co-authored-by: teor <[email protected]>

* Update CHANGELOG.md

Co-authored-by: teor <[email protected]>

* Update CHANGELOG.md

Co-authored-by: teor <[email protected]>

* Update CHANGELOG.md

Co-authored-by: teor <[email protected]>

* Update CHANGELOG.md

Co-authored-by: teor <[email protected]>

* Update breaking changes in 1.0.0-rc.9 changelog

* changelog: move #6801 to Fix

* Update CHANGELOG.md

Co-authored-by: teor <[email protected]>

* Include #6832 in the changelog

* Add missing changes to changelog

* Remove #6801 from known issues in the README

* Use the latest bug template link

---------

Co-authored-by: teor <[email protected]>
@teor2345 teor2345 mentioned this pull request Jun 13, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arya2 arya2 arya2 approved these changes

@upbqdn upbqdn Awaiting requested review from upbqdn upbqdn is a code owner automatically assigned from ZcashFoundation/network-reviewers

Assignees

@teor2345 teor2345

Labels
A-concurrency Area: Async code, needs extra work to make it work properly. A-network Area: Network protocol updates or fixes C-bug Category: This is a bug C-security Category: Security issues I-remote-node-overload Zebra can overload other nodes on the network
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Zebra's peer set should drop multiple outbound connections to the same IP and port, Credit: Ziggurat Team
2 participants
@teor2345 @arya2