Skip to content

Peers behind a firewall #5241

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
Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Peers behind a firewall #5241

wants to merge 6 commits into from

Conversation

@the-headless-ghost
Copy link
Member

@the-headless-ghost the-headless-ghost commented Nov 8, 2025 •
edited
Loading

Description

Closes #4381

Checklist

Quality

  • Commit sequence makes sense and have useful messages, see ref.
  • New tests are added and existing tests are updated.
  • Self-reviewed the PR.

Maintenance

  • Linked an issue or added the PR to the current sprint of ouroboros-network project.
  • Added labels.
  • Updated changelog files.
  • The documentation has been properly updated, see ref.

@the-headless-ghost the-headless-ghost self-assigned this Nov 8, 2025
@the-headless-ghost the-headless-ghost added connection-manager Issues / PRs related to connection-manager configuration labels Nov 8, 2025
@github-project-automation github-project-automation bot moved this to In Progress in Ouroboros Network Nov 8, 2025
@the-headless-ghost the-headless-ghost changed the title Peers behind the firewall Peers behind a firewall Nov 10, 2025
crocodile-dentist
crocodile-dentist reviewed Nov 10, 2025
View reviewed changes
ouroboros-network/lib/Ouroboros/Network/PeerSelection/Governor/EstablishedPeers.hs Outdated
-- ^ True if peer is not behind a firewall
<$> readLocalRootPeers

peerconn <- establishPeerConnection isBigLedgerPeer diffusionMode peeraddr connectionMode
Copy link
Contributor

@crocodile-dentist crocodile-dentist Nov 10, 2025

Choose a reason for hiding this comment

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

rename connectionMode to provenance.

ouroboros-network/lib/Ouroboros/Network/PeerSelection/Governor/Types.hs
-> DiffusionMode
-> peeraddr -> m peerconn,
-> peeraddr
-> ConnectionMode
Copy link
Contributor

@crocodile-dentist crocodile-dentist Nov 10, 2025

Choose a reason for hiding this comment

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

Use Provenance here instead.

ouroboros-network/framework/lib/Ouroboros/Network/ConnectionManager/Core.hs Outdated
Comment on lines 1491 to 1493
when (inboundRequired connectionMode) $
throwIO (withCallStack $ InboundConnectionNotFound peerAddr)

Copy link
Contributor

@crocodile-dentist crocodile-dentist Nov 10, 2025

Choose a reason for hiding this comment

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

The damage is done by the time you do this check. A few lines above in the Nothing case we have updated the stateVar by inserting this outbound connection into the CM's connection table.

ouroboros-network/lib/Ouroboros/Network/Diffusion/Topology.hs
Comment on lines +75 to +80
localRootsToRelayAccessPoint
:: LocalRoots
-> [(RelayAccessPoint, PeerAdvertise, Bool)]
localRootsToRelayAccessPoint LocalRoots {rootConfig, behindFirewall} =
(\(accessPoint, advertise) -> (accessPoint, advertise, behindFirewall))
<$> rootConfigToRelayAccessPoint rootConfig
Copy link
Contributor

@crocodile-dentist crocodile-dentist Nov 10, 2025

Choose a reason for hiding this comment

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

I don't think it's needed.

ouroboros-network/lib/Ouroboros/Network/PeerSelection/State/LocalRootPeers.hs
extraLocalRootFlags :: !extraFlags
peerAdvertise :: !PeerAdvertise,
diffusionMode :: !DiffusionMode,
localRootBehindFirewall :: !Bool,
Copy link
Contributor

@crocodile-dentist crocodile-dentist Nov 10, 2025

Choose a reason for hiding this comment

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

Change this type to Provenance?

cardano-diffusion/lib/Cardano/Network/Diffusion/Topology.hs
IsTrustable -> not
. null
. rootAccessPoints
. rootConfig
Copy link
Contributor

@crocodile-dentist crocodile-dentist Nov 10, 2025

Choose a reason for hiding this comment

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

I don't think it's needed - see my other comments.

ouroboros-network/framework/lib/Ouroboros/Network/ConnectionManager/Types.hs

type AcquireOutboundConnection peerAddr handle handleError m
= DiffusionMode -> peerAddr -> m (Connected peerAddr handle handleError)
= DiffusionMode -> peerAddr -> ConnectionMode -> m (Connected peerAddr handle handleError)
Copy link
Contributor

@crocodile-dentist crocodile-dentist Nov 10, 2025

Choose a reason for hiding this comment

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

s/ConnectionMode/Provenance/

ouroboros-network/framework/lib/Ouroboros/Network/ConnectionManager/Types.hs Outdated
Comment on lines 744 to 745
--
| InboundConnectionNotFound !peerAddr !CallStack
Copy link
Contributor

@crocodile-dentist crocodile-dentist Nov 10, 2025

Choose a reason for hiding this comment

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

This can be a typical case where an inbound is not found for behind firewall peers. Maybe instead create the opposite trace where we inform that we are establishing a connection back to a peer of 'inbound' provenance?

@the-headless-ghost
Copy link
Member Author

the-headless-ghost commented Nov 11, 2025

@crocodile-dentist I’m not sure the Provenance type is the right choice, as the comments indicate that its interpretation refers to connections initiated either locally or by a remote peer, but not both. However, what we need is a way to specify connections that are either initiated only by a remote peer or initiated in any way—locally or remotely.

@crocodile-dentist
Copy link
Contributor

crocodile-dentist commented Nov 12, 2025

What I had in mind was a little overloading of the meaning of provenance. So jobPromoteColdPeer would be passed this type Provenance and we could name the binding as demandedProvenance or somesuch, which would be Inbound for those peers we marked as being 'behind firewall' in the topology file. In other cases, it would be Outbound. The CM code would have our special handling for those outbound connections for which we demand there is an inbound for, and otherwise it is a plain request for acquiring an outbound connection, so the Outbound value makes sense as well, and the handling is as before. We can change the haddock around the Provenance type to better explain this intent. This has the benefit of not polluting our namespace with another kind of a boolean. If this type you're trying to introduce was more meaningful, then I'd agree with what you're saying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@crocodile-dentist crocodile-dentist crocodile-dentist left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@the-headless-ghost the-headless-ghost

Labels

configuration connection-manager Issues / PRs related to connection-manager

Projects

Ouroboros Network
Status: In Progress

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Optimised connectivity to peers behind firewall

3 participants

@the-headless-ghost @crocodile-dentist