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 Peer Sharing issue #4642 #4644

Merged
merged 4 commits into from
Oct 31, 2023
Merged

Fix Peer Sharing issue #4642 #4644

merged 4 commits into from
Oct 31, 2023

Conversation

bolt12
Copy link
Contributor

@bolt12 bolt12 commented Aug 17, 2023 •
edited
Loading

Fixes #4642 by simplifying the PeerSharing options, i.e. removes PeerSharingPrivate making the handshake negotiation symmetric.

I also updated the wiki page to keep track of these changes on the design docs: https://github.com/input-output-hk/ouroboros-network/wiki/Peer-Sharing-Implementation-Plan#post-implementation-notes

@bolt12 bolt12 requested a review from coot as a code owner August 17, 2023 08:50
@bolt12 bolt12 added networking peer-sharing Issues / PRs related to peer sharing labels Aug 17, 2023
@bolt12 bolt12 force-pushed the bolt12/fix-peerSharing-handshake branch from f9e9ab7 to 1c81439 Compare August 17, 2023 10:15
@bolt12 bolt12 force-pushed the bolt12/fix-peerSharing-handshake branch 2 times, most recently from 8ed5dbc to 12d0e25 Compare September 4, 2023 13:13
@bolt12 bolt12 force-pushed the bolt12/fix-peerSharing-handshake branch 2 times, most recently from c7e9e85 to 7003b2a Compare September 18, 2023 14:19
@bolt12 bolt12 mentioned this pull request Sep 18, 2023
Merged
@bolt12 bolt12 force-pushed the bolt12/fix-peerSharing-handshake branch from 7003b2a to da8df7f Compare September 18, 2023 15:00
@bolt12 bolt12 mentioned this pull request Sep 18, 2023
Closed
@coot
Copy link
Contributor

coot commented Sep 20, 2023 •
edited
Loading

Is this PR ready for review?

@bolt12
Copy link
Contributor Author

bolt12 commented Sep 20, 2023

Yes!

@bolt12 bolt12 force-pushed the bolt12/fix-peerSharing-handshake branch from 6d89f63 to 853fa9b Compare September 25, 2023 13:19
@bolt12 bolt12 linked an issue Sep 25, 2023 that may be closed by this pull request
The encoding of IPv6 addresses for Peersharing is wrong. #4679
Closed
@bolt12 bolt12 force-pushed the bolt12/fix-peerSharing-handshake branch 2 times, most recently from 1878384 to 66cbbb9 Compare September 26, 2023 11:51
coot
coot requested changes Sep 28, 2023
View reviewed changes
ouroboros-network-api/src/Ouroboros/Network/NodeToNode/Version.hs Outdated Show resolved Hide resolved
ouroboros-network-api/src/Ouroboros/Network/NodeToNode/Version.hs Outdated Show resolved Hide resolved
ouroboros-network-api/src/Ouroboros/Network/NodeToNode/Version.hs Outdated Show resolved Hide resolved
ouroboros-network-api/src/Ouroboros/Network/PeerSelection/PeerSharing.hs Outdated Show resolved Hide resolved
ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs Outdated Show resolved Hide resolved
ouroboros-network-protocols/testlib/Ouroboros/Network/Protocol/Handshake/Test.hs Outdated Show resolved Hide resolved
ouroboros-network-protocols/testlib/Ouroboros/Network/Protocol/Handshake/Test.hs Outdated Show resolved Hide resolved
ouroboros-network-framework/src/Ouroboros/Network/Protocol/Handshake/Unversioned.hs Outdated Show resolved Hide resolved
ouroboros-network-api/src/Ouroboros/Network/PeerSelection/PeerSharing.hs Outdated Show resolved Hide resolved
ouroboros-network-api/src/Ouroboros/Network/PeerSelection/PeerSharing.hs Outdated Show resolved Hide resolved
@bolt12 bolt12 force-pushed the bolt12/fix-peerSharing-handshake branch from b2b914d to 84317e9 Compare October 2, 2023 10:38
@bolt12 bolt12 mentioned this pull request Oct 2, 2023
Merged
@bolt12 bolt12 requested a review from coot October 2, 2023 14:50
@bolt12 bolt12 force-pushed the bolt12/fix-peerSharing-handshake branch from 84317e9 to 4127255 Compare October 3, 2023 13:36
coot
coot requested changes Oct 4, 2023
View reviewed changes
ouroboros-network-api/src/Ouroboros/Network/NodeToNode/Version.hs Outdated Show resolved Hide resolved
ouroboros-network-api/src/Ouroboros/Network/NodeToNode/Version.hs Outdated Show resolved Hide resolved
ouroboros-network/CHANGELOG.md Outdated Show resolved Hide resolved
ouroboros-network/CHANGELOG.md Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor/KnownPeers.hs Outdated Show resolved Hide resolved
@bolt12 bolt12 force-pushed the bolt12/fix-peerSharing-handshake branch from 4127255 to eb3d770 Compare October 6, 2023 09:38
@bolt12 bolt12 requested a review from coot October 6, 2023 09:41
@bolt12 bolt12 force-pushed the bolt12/fix-peerSharing-handshake branch 3 times, most recently from 4666ef6 to 7e73302 Compare October 11, 2023 10:30
@bolt12 bolt12 requested a review from coot October 12, 2023 15:13
@bolt12 bolt12 force-pushed the bolt12/fix-peerSharing-handshake branch from 7e73302 to 58f261f Compare October 12, 2023 15:30
@bolt12 bolt12 enabled auto-merge October 13, 2023 14:55
@bolt12 bolt12 force-pushed the bolt12/fix-peerSharing-handshake branch from 58f261f to 3b2eb01 Compare October 23, 2023 09:18
@bolt12 bolt12 mentioned this pull request Oct 23, 2023
Closed
@bolt12 bolt12 force-pushed the bolt12/fix-peerSharing-handshake branch from 3b2eb01 to bdd7d06 Compare October 27, 2023 11:07
@bolt12 bolt12 force-pushed the bolt12/fix-peerSharing-handshake branch 3 times, most recently from e3b9ea9 to c2c4db8 Compare October 27, 2023 15:34
bolt12 added 3 commits October 30, 2023 11:17
@bolt12
Fix Peer Sharing issue #4642
314574f 
- Remove `PeerSharingPrivate` option and renamed others to
  `PeerSharingDisabled` and `PeerSharingEnabled`
- Add test to check that the bug is fixed
- Add new `NodeToNodeV_13`
- Renamed spec files and added V13 CDDL handshake and
  `NodeToNodeVersionData` specs
@bolt12
Fix IPv6 enc/decoding
ce94809 
Fixes #4679
@bolt12
Address review comments
50bab68
@bolt12 bolt12 force-pushed the bolt12/fix-peerSharing-handshake branch from c2c4db8 to 50bab68 Compare October 30, 2023 11:17
coot
coot approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

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

LGTM, just a few minor remarks.

ouroboros-network-api/src/Ouroboros/Network/PeerSelection/PeerSharing/Codec.hs Outdated Show resolved Hide resolved
ouroboros-network-protocols/CHANGELOG.md Outdated Show resolved Hide resolved
ouroboros-network/CHANGELOG.md Outdated Show resolved Hide resolved
ouroboros-network/sim-tests-lib/Test/Ouroboros/Network/Diffusion/Node.hs Outdated Show resolved Hide resolved
@bolt12 bolt12 added this pull request to the merge queue Oct 31, 2023
@coot coot removed this pull request from the merge queue due to a manual request Oct 31, 2023
@bolt12 bolt12 enabled auto-merge October 31, 2023 15:31
@bolt12 bolt12 added this pull request to the merge queue Oct 31, 2023
Merged via the queue into master with commit 6296e40 Oct 31, 2023
8 of 9 checks passed
@bolt12 bolt12 deleted the bolt12/fix-peerSharing-handshake branch October 31, 2023 18:02
github-merge-queue bot pushed a commit to IntersectMBO/ouroboros-consensus that referenced this pull request Nov 13, 2023
@jasagredo
Updates ouroboros-network to fix PeerSharing (#383)
6d0427f 
# Description

This PR accommodates changes from
IntersectMBO/ouroboros-network#4644.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karknu karknu karknu left review comments

@coot coot coot approved these changes

@njd42 njd42 Awaiting requested review from njd42

@peterwthompson peterwthompson Awaiting requested review from peterwthompson

Assignees

@bolt12 bolt12

Labels
peer-sharing Issues / PRs related to peer sharing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The encoding of IPv6 addresses for Peersharing is wrong. PeerSharing negotiated even when it is disabled.
3 participants
@bolt12 @coot @karknu