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

Implement Light Peer Sharing #4277

Merged
merged 5 commits into from
Jun 2, 2023
Merged

Implement Light Peer Sharing #4277

merged 5 commits into from
Jun 2, 2023

Conversation

bolt12
Copy link
Contributor

@bolt12 bolt12 commented Jan 12, 2023

This PR introduces Light Peer Sharing on top of the already existing Peer Sharing feature. They're actually somewhat independent, but one should be careful with this feature since it will be prone to Eclipse attacks without the Eclipse Evasion implementation.

The main objective here is to use the information that the Inbound Governor has about incoming connections and feed that information directly to the Peer Selection Governor (aka Outbound Governor). Right now the Outbound Governor only has peers information (Known / Established / Active Peers Set etc..) from local configuration and/or ledger peers. With Light Peer Sharing, when a node connects to us we will make an effort to add it to our Known Peer Set, being yet another source of peers for fulfilling this target and possibly share across the network

Checklist

  • Branch
    • Commit sequence broadly makes sense
    • Commits have useful messages
    • The documentation has been properly updated
    • New tests are added if needed and existing tests are updated
    • If this branch changes Consensus and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
    • If this branch changes Network and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
    • If serialization changes, user-facing consequences (e.g. replay from genesis) are confirmed to be intentional.
  • Pull Request
    • Self-reviewed the diff
    • Useful pull request description at least containing the following information:
      • What does this PR change?
      • Why these changes were needed?
      • How does this affect downstream repositories and/or end-users?
      • Which ticket does this PR close (if any)? If it does, is it linked?
    • Reviewer requested

@bolt12 bolt12 requested a review from coot as a code owner January 12, 2023 18:03
@bolt12
Copy link
Contributor Author

bolt12 commented Jan 13, 2023

Closes #3596

@bolt12 bolt12 linked an issue Jan 13, 2023 that may be closed by this pull request
Light Peer Sharing #3596
Closed
@bolt12 bolt12 force-pushed the bolt12/light-peer-sharing branch 2 times, most recently from 1e7fba5 to f1ce6d3 Compare January 24, 2023 11:39
njd42
njd42 reviewed Feb 22, 2023
View reviewed changes
Copy link
Contributor

@njd42 njd42 left a comment

Choose a reason for hiding this comment

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

Looks good - just the question, for me, about the consequences of self-connection. Is this something we just want to avoid in tests (as kernel would disallow) or is it something that we need to avoid in operation (given that for NAT and multi-homed hosts it could happen)

ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor/KnownPeers.hs Outdated Show resolved Hide resolved
ouroboros-network/test/Test/Ouroboros/Network/Testnet.hs Show resolved Hide resolved
@bolt12 bolt12 force-pushed the bolt12/light-peer-sharing branch 2 times, most recently from 2837d08 to ad3fa0e Compare March 2, 2023 15:23
@bolt12 bolt12 force-pushed the bolt12/light-peer-sharing branch from a9116fb to 54dc5f5 Compare March 3, 2023 12:40
@bolt12 bolt12 force-pushed the bolt12/peer-sharing branch 2 times, most recently from 03e09cb to 9d833ba Compare March 7, 2023 12:49
@bolt12 bolt12 requested review from coot and njd42 March 30, 2023 09:37
coot
coot requested changes Apr 6, 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.

A few minor remarks.

ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs Outdated Show resolved Hide resolved
ouroboros-network-framework/src/Ouroboros/Network/Protocol/Handshake/Unversioned.hs Outdated Show resolved Hide resolved
ouroboros-network/test/Test/Ouroboros/Network/PeerSelection.hs Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor/Monitor.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor/Types.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor/Monitor.hs Outdated Show resolved Hide resolved
@bolt12 bolt12 force-pushed the bolt12/light-peer-sharing branch from a516d53 to 1b64f8e Compare April 6, 2023 10:54
@bolt12 bolt12 requested a review from coot April 6, 2023 10:54
@bolt12 bolt12 force-pushed the bolt12/light-peer-sharing branch from 8d6cec5 to f17f251 Compare May 9, 2023 16:24
coot
coot reviewed May 29, 2023
View reviewed changes
ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs Outdated Show resolved Hide resolved
ouroboros-network-framework/src/Ouroboros/Network/InboundGovernor.hs Outdated Show resolved Hide resolved
ouroboros-network-framework/src/Ouroboros/Network/InboundGovernor/ControlChannel.hs Outdated Show resolved Hide resolved
ouroboros-network-framework/src/Ouroboros/Network/InboundGovernor/ControlChannel.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor/Monitor.hs Outdated Show resolved Hide resolved
ouroboros-network/test/Test/Ouroboros/Network/PeerSelection/MockEnvironment.hs Outdated Show resolved Hide resolved
ouroboros-network/test/Test/Ouroboros/Network/Testnet.hs
Comment on lines +2669 to +2701
-- of a node's listening socket. This is something that in the real world
-- wouldn't happen since the kernel would disallow it.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that's true, have you tried?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, what I meant here is that our net sim would misbehave in the case of self connects, but you fixed it recently, is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bump

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's fixed, it was a bug in connection-manager not sim-net though.

@bolt12
Refactor ControlChannel
090a5c1 
So that we can use it for sharing information between InboundGovernor
and Outbound Governor.
@bolt12 bolt12 force-pushed the bolt12/light-peer-sharing branch from 705cfad to 1dfdc4d Compare May 29, 2023 15:08
@bolt12 bolt12 requested a review from coot May 29, 2023 15:09
@bolt12 bolt12 requested a review from coot June 1, 2023 16:09
coot
coot approved these changes Jun 2, 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

@coot coot added this pull request to the merge queue Jun 2, 2023
@coot coot removed this pull request from the merge queue due to a manual request Jun 2, 2023
@bolt12
Added Light Peer Sharing
36ebebd 
Added a Control Channel between the Inbound Governor and the
OutboundGovernor. Reading from this channel is abstracted as a
PeerSelectionAction. When the InboundGovernor receives an Inbound
NewConnection it will write the address and PeerSharing Willingness
information to the Channel.
@bolt12
Added test to check no self connects
ad98b2b
@bolt12
Small fix
fea3a5a
@bolt12
Stylish
1d2b488
@bolt12 bolt12 force-pushed the bolt12/light-peer-sharing branch from 1dfdc4d to 1d2b488 Compare June 2, 2023 13:26
@bolt12 bolt12 enabled auto-merge June 2, 2023 13:33
@bolt12 bolt12 added this pull request to the merge queue Jun 2, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 2, 2023
@coot coot added this pull request to the merge queue Jun 2, 2023
@coot
Copy link
Contributor

coot commented Jun 2, 2023

Added to merge queue.

Merged via the queue into master with commit 402e30d Jun 2, 2023
@coot coot deleted the bolt12/light-peer-sharing branch June 2, 2023 15:11
karknu pushed a commit to karknu/ouroboros-network that referenced this pull request Oct 18, 2023
@coot @karknu
Merge pull request IntersectMBO#4277 from input-output-hk/bolt12/ligh…
1a9ccee 
…t-peer-sharing

Implement Light Peer Sharing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coot coot coot approved these changes

@nfrisby nfrisby Awaiting requested review from nfrisby

@dnadales dnadales Awaiting requested review from dnadales

@peterwthompson peterwthompson Awaiting requested review from peterwthompson

@njd42 njd42 Awaiting requested review from njd42

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Light Peer Sharing
3 participants
@bolt12 @coot @njd42