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
Open
the-headless-ghost wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Peers behind a firewall #5241

the-headless-ghost wants to merge 1 commit 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 force-pushed the edgr/behind-firewall branch 2 times, most recently from fd67e72 to c2e3eec Compare November 9, 2025 05:46
@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
@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.

@the-headless-ghost the-headless-ghost marked this pull request as ready for review November 17, 2025 10:58
@the-headless-ghost the-headless-ghost requested a review from a team as a code owner November 17, 2025 10:58
coot
coot requested changes Nov 27, 2025
View reviewed changes
Copy link
Collaborator

@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.

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.

@karknu
Copy link
Contributor

karknu commented Nov 28, 2025

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 .

crocodile-dentist
crocodile-dentist reviewed Dec 8, 2025
View reviewed changes
coot
coot approved these changes Dec 10, 2025
View reviewed changes
Copy link
Collaborator

@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 minor comment.

@the-headless-ghost the-headless-ghost force-pushed the edgr/behind-firewall branch 4 times, most recently from ba07064 to 9380a9e Compare December 17, 2025 12:15
@the-headless-ghost the-headless-ghost mentioned this pull request Dec 18, 2025
Open
7 tasks
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

@crocodile-dentist crocodile-dentist crocodile-dentist approved these changes

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

5 participants

@the-headless-ghost @crocodile-dentist @karknu @coot