Peers behind a firewall #5241

the-headless-ghost · 2025-11-08T01:37:41Z

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.

ouroboros-network/lib/Ouroboros/Network/PeerSelection/Governor/EstablishedPeers.hs

crocodile-dentist · 2025-11-10T11:26:45Z

ouroboros-network/lib/Ouroboros/Network/PeerSelection/Governor/EstablishedPeers.hs

+        -- ^ True if peer is not behind a firewall
+        <$> readLocalRootPeers
+
+      peerconn <- establishPeerConnection isBigLedgerPeer diffusionMode peeraddr connectionMode


rename connectionMode to provenance.

crocodile-dentist · 2025-11-10T11:27:08Z

ouroboros-network/lib/Ouroboros/Network/PeerSelection/Governor/Types.hs

                             -> DiffusionMode
-                             -> peeraddr -> m peerconn,
+                             -> peeraddr
+                             -> ConnectionMode


Use Provenance here instead.

crocodile-dentist · 2025-11-10T11:30:28Z

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

+            when (inboundRequired connectionMode)  $
+              throwIO (withCallStack $ InboundConnectionNotFound peerAddr)
+


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.

crocodile-dentist · 2025-11-10T11:35:04Z

ouroboros-network/lib/Ouroboros/Network/Diffusion/Topology.hs

+localRootsToRelayAccessPoint
+  :: LocalRoots
+  -> [(RelayAccessPoint, PeerAdvertise, Bool)]
+localRootsToRelayAccessPoint LocalRoots {rootConfig, behindFirewall} =
+      (\(accessPoint, advertise) -> (accessPoint, advertise, behindFirewall))
+  <$> rootConfigToRelayAccessPoint rootConfig


I don't think it's needed.

crocodile-dentist · 2025-11-10T11:35:49Z

ouroboros-network/lib/Ouroboros/Network/PeerSelection/State/LocalRootPeers.hs

-    extraLocalRootFlags :: !extraFlags
+    peerAdvertise           :: !PeerAdvertise,
+    diffusionMode           :: !DiffusionMode,
+    localRootBehindFirewall :: !Bool,


Change this type to Provenance?

This will also avoid boolean blindness 😄, but it still should be a Boolean value in json.

crocodile-dentist · 2025-11-10T11:36:48Z

cardano-diffusion/lib/Cardano/Network/Diffusion/Topology.hs

              IsTrustable    -> not
                              . null
                              . rootAccessPoints
+                              . rootConfig


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

crocodile-dentist · 2025-11-10T11:38:10Z

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)


s/ConnectionMode/Provenance/

crocodile-dentist · 2025-11-10T11:41:17Z

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

+    --
+    | InboundConnectionNotFound !peerAddr !CallStack


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 · 2025-11-11T02:52:25Z

@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 · 2025-11-12T08:56:30Z

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.

coot

I agree with @crocodile-dentist that we could use Provenance instead of ConnectionMode. But it would be good to ask @karknu if he agrees with the terminology, since it might be a bit surprising for an SPO.

A few other comments & suggestions follow.

coot · 2025-11-27T10:38:03Z

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

+inboundRequired :: ConnectionMode -> Bool
+inboundRequired RequireInbound = True
+inboundRequired _other         = False


This function is unnecessary indirection, we can directly pattern match on the constructors.

coot · 2025-11-27T10:39:01Z

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

+                return ( Just (Right (TrInboundConnectionNotFound connectionMode peerAddr))
+                   , mutableConnState
+                   , Left (withCallStack
+                            (InboundConnectionNotFound connectionMode peerAddr))
+                   )


code style:

Suggested change

return ( Just (Right (TrInboundConnectionNotFound connectionMode peerAddr))

, mutableConnState

, Left (withCallStack

(InboundConnectionNotFound connectionMode peerAddr))

)

return ( Just (Right (TrInboundConnectionNotFound connectionMode peerAddr))

, mutableConnState

, Left (withCallStack

(InboundConnectionNotFound connectionMode peerAddr))

)

coot · 2025-11-27T10:43:33Z

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

+              if inboundRequired connectionMode
+              then do
+                return ( Just (Right (TrInboundConnectionNotFound connectionMode peerAddr))
+                   , mutableConnState


There's not point in returning mutableConnState (this branch of if should not executed newMutableConnState as it must not be used, which requries a bit more refactoring.

coot · 2025-11-27T10:52:00Z

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

+                   , Left (withCallStack
+                            (InboundConnectionNotFound connectionMode peerAddr))


I was trying to find out if throwing an exception in this case is right or should we return an explicit value. After a deliberation, I think it's fine to do so (and thus throw an exception). The outbound governor is not supposed to call establishPeerConnection unless there already is an inbound connection for such a peer. So if we find InboundConnectionNotFound in the logs, it's actually a signal for us to debug it.

There could be a race condition here, but maybe throwing an exception is still a convenient way of dealing with it anyway.

coot · 2025-11-27T11:14:07Z

ouroboros-network/lib/Ouroboros/Network/PeerSelection/State/LocalRootPeers.hs

-    extraLocalRootFlags :: !extraFlags
+    peerAdvertise           :: !PeerAdvertise,
+    diffusionMode           :: !DiffusionMode,
+    localRootBehindFirewall :: !Bool,


This will also avoid boolean blindness 😄, but it still should be a Boolean value in json.

coot · 2025-11-27T11:41:42Z

ouroboros-network/lib/Ouroboros/Network/PeerSelection/State/KnownPeers.hs

+       -- Note: Some peers may be behind a firewall. Local root peers marked as
+       -- behind a firewall are not excluded from this list.
+       --


Thanks for the note.

coot · 2025-11-27T11:50:53Z

cardano-diffusion/tests/lib/Test/Cardano/Network/Diffusion/Testnet.hs

+                 isUnreachablePeer (LocalRootConfig {localRootBehindFirewall}) =
+                   localRootBehindFirewall
+                 unreachablePeers =
+                   Map.keysSet
+                     $ Map.filter isUnreachablePeer
+                     $ LocalRootPeers.toMap local


Suggested change

isUnreachablePeer (LocalRootConfig {localRootBehindFirewall}) =

localRootBehindFirewall

unreachablePeers =

Map.keysSet

$ Map.filter isUnreachablePeer

$ LocalRootPeers.toMap local

unreachablePeers =

Map.keysSet

$ Map.filter localRootBehindFirewall

$ LocalRootPeers.toMap local

coot · 2025-11-27T11:53:18Z

cardano-diffusion/tests/lib/Test/Cardano/Network/Diffusion/Testnet.hs

+          ((,,) <$> govLocalRootPeersSig
+                   <*> govUnreachablePeersSig
+                   <*> govInboundConnectionsSig)


code style

Suggested change

((,,) <$> govLocalRootPeersSig

<*> govUnreachablePeersSig

<*> govInboundConnectionsSig)

((,,) <$> govLocalRootPeersSig

<*> govUnreachablePeersSig

<*> govInboundConnectionsSig)

coot · 2025-11-27T11:56:58Z

cardano-diffusion/tests/lib/Test/Cardano/Network/Diffusion/Testnet.hs

+          govInboundConnectionsSig :: Signal (Set NtNAddr)
+          govInboundConnectionsSig =


Suggested change

govInboundConnectionsSig :: Signal (Set NtNAddr)

govInboundConnectionsSig =

-- all connections which triggered `TrConnectionNotFound Inbound`

govNotFoundInboundConnSig :: Signal (Set NtNAddr)

govNotFoundInboundConnSig =

coot · 2025-11-27T11:58:14Z

ouroboros-network/orphan-instances/Ouroboros/Network/OrphanInstances.hs

+                  <$> (RootConfig
+                        <$> o .:  "accessPoints"
+                        <*> o .:? "advertise" .!= DoNotAdvertisePeer)


Don't we have a FromJSON instance for RootConfig that can be used here?

karknu · 2025-11-28T08:11:12Z

I agree with @crocodile-dentist that we could use Provenance instead of ConnectionMode. But it would be good to ask @karknu if he agrees with the terminology, since it might be a bit surprising for an SPO.

A few other comments & suggestions follow.

I agree with @crocodile-dentist .

Add behind firewall flag to local root config

5d2e86d

the-headless-ghost self-assigned this Nov 8, 2025

the-headless-ghost added connection-manager Issues / PRs related to connection-manager configuration labels Nov 8, 2025

github-project-automation bot added this to Ouroboros Network Nov 8, 2025

github-project-automation bot moved this to In Progress in Ouroboros Network Nov 8, 2025

the-headless-ghost force-pushed the edgr/behind-firewall branch from 6fb89b7 to fd67e72 Compare November 9, 2025 04:08

Add ConnectionMode to AcquireOutboundConnection

c2e3eec

the-headless-ghost force-pushed the edgr/behind-firewall branch from fd67e72 to c2e3eec Compare November 9, 2025 05:46

the-headless-ghost changed the title ~~Peers behind the firewall~~ Peers behind a firewall Nov 10, 2025

crocodile-dentist reviewed Nov 10, 2025

View reviewed changes

the-headless-ghost added 2 commits November 17, 2025 17:15

CM and jobPromoteColdPeer fixes

a12338a

Add diffusion property test

405b005

the-headless-ghost force-pushed the edgr/behind-firewall branch from 439845e to 405b005 Compare November 17, 2025 10:47

the-headless-ghost marked this pull request as ready for review November 17, 2025 10:58

the-headless-ghost requested a review from a team as a code owner November 17, 2025 10:58

coot requested changes Nov 27, 2025

View reviewed changes

		when (inboundRequired connectionMode) $
		throwIO (withCallStack $ InboundConnectionNotFound peerAddr)

		, Left (withCallStack
		(InboundConnectionNotFound connectionMode peerAddr))

		govInboundConnectionsSig :: Signal (Set NtNAddr)
		govInboundConnectionsSig =

-          govInboundConnectionsSig :: Signal (Set NtNAddr)
-          govInboundConnectionsSig =
+          -- all connections which triggered `TrConnectionNotFound Inbound`
+          govNotFoundInboundConnSig :: Signal (Set NtNAddr)
+          govNotFoundInboundConnSig =

Peers behind a firewall #5241

Are you sure you want to change the base?

Peers behind a firewall #5241

Uh oh!

Conversation

the-headless-ghost commented Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Quality

Maintenance

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coot Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

the-headless-ghost commented Nov 11, 2025

Uh oh!

crocodile-dentist commented Nov 12, 2025

Uh oh!

coot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coot Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karknu commented Nov 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

the-headless-ghost commented Nov 8, 2025 •

edited

Loading

coot Nov 27, 2025 •

edited

Loading

coot Nov 27, 2025 •

edited

Loading