Skip to content

Conversation

fjahr
Copy link
Contributor

@fjahr fjahr commented Jul 26, 2022

(This is just a very rough draft, I am only looking for conceptual feedback right now, so this is the bare minimum code to understand what it should do. I will add tests and fix docs when the conceptual review hurdle is cleared.)

This is an alternative approach to what was already attempted with #25690 and #24835. Please have a look there for more background information.

The -allowinbound option lets users explicitly mark networks as available for inbound connections, even if that network may be explicitly deactivated for outbound connections or if it is blocked for other reasons that are out of the user's control. The effect is that the -externalip addresses of that network may be announced and the -getnetworkinfo RPC will show these addresses. These several users seem to have run into these issues, see for example #25669.

@fjahr
Copy link
Contributor Author

fjahr commented Jul 26, 2022

@mzumsande has suggested having separate -onlynet options for inbound and outbound. I think that's a valid option as well and if reviewers prefer that approach I can draft that one as well.

@vasild
Copy link
Contributor

vasild commented Jul 28, 2022

I think this can and should be resolved in a simpler way by removing IsReachable() from AddLocal() and IsPeerAddrLocalGood().

The new option -allowinbound would be difficult to explain to users. By its name, I would guess that -allowinbound=0 should do the same as -listen=0.

@Rspigler
Copy link
Contributor

I like the onlynet inbound/outbound. There is consistently confusion about the network settings and how they interact (and bugs found), and I think this would really clear things up. Also, this would translate easily to GUI settings.

@vasild
Copy link
Contributor

vasild commented Jul 29, 2022

What would -onlynet inbound do? Is that not the same as omitting the network from -bind=?

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 29, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25775 (docs: remove non-signaling mentions of BIP125 by glozow)
  • #24170 (p2p, rpc: Manual block-relay-only connections with addnode by mzumsande)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@Rspigler
Copy link
Contributor

Maybe I don't understand the purpose of the PR, but I thought it would make changes such as:
-cjdnsreachable -> -allowinbound=cjdns
-onlynet=cjdns -> '-allowoutbound=cjdns

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

1 similar comment
@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

@fjahr
Copy link
Contributor Author

fjahr commented Mar 12, 2023

Closing this for now as I currently don't have time to continue with this. I hope I can revisit it in the future.

@fjahr fjahr closed this Mar 12, 2023
@bitcoin bitcoin locked and limited conversation to collaborators Mar 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants