Allow whitelisting outgoing connections

luke-jr commented at 9:35 pm on October 16, 2019: member

Rethinking/rewrite of #10594 in light of whitelist flags: new flags are added for “in” and “out” to specify the connection direction matched. Both can be specified to match either direction.

luke-jr force-pushed on Oct 16, 2019

fanquake added the label P2P on Oct 16, 2019

luke-jr force-pushed on Oct 16, 2019

DrahtBot commented at 11:30 pm on October 16, 2019: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25515 ([draft] PeerManager unit tests by dergoegge)
#25514 (net processing: Move CNode::nServices and CNode::nLocalServices to Peer by dergoegge)
#25500 (refactor: Move inbound eviction logic to its own translation unit by dergoegge)
#25268 (refactor: Introduce EvictionManager by dergoegge)

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.

luke-jr force-pushed on Oct 17, 2019

laanwj commented at 8:19 am on October 17, 2019: member

Concept ACK.

As for implementation, I’m not sure about having In and Out as permissions.

This doesn’t allow having different flags for a IP for incoming and outgoing. Not sure that’s ever necessary, but this seems a bit muddled to me. Might also consider e.g. whiteconnect (for outgoing connections) could have a completely different flag list structure from the normal whitelist (for incoming connections).

Edit: in a way you already do this by having separate connOptions.vWhitelistedRangeOutgoing versus connOptions.vWhitelistedRange, it’s just not exposed all the way.

luke-jr commented at 12:50 pm on October 17, 2019: member

As you noticed, the implementation already doesn’t treat in/out as permissions.

Using -whitelist for both allows for easily specifying the same flags for both in,out. I can’t think of any benefit to having separate options. (I’m not sure why we’d want to support different sets of flags…)

DrahtBot added the label Needs rebase on Oct 30, 2019

luke-jr force-pushed on Nov 4, 2019

DrahtBot removed the label Needs rebase on Nov 4, 2019

luke-jr force-pushed on Nov 15, 2019

DrahtBot added the label Needs rebase on Dec 4, 2019

luke-jr force-pushed on Mar 28, 2020

luke-jr force-pushed on Aug 20, 2020

luke-jr commented at 7:43 pm on August 20, 2020: member

Rebased

luke-jr force-pushed on Aug 20, 2020

DrahtBot removed the label Needs rebase on Aug 20, 2020

DrahtBot added the label Needs rebase on Sep 4, 2020

maflcko commented at 10:58 am on September 5, 2020: member

Previously whitelisting outgoing connections wasn’t possible, so no guards have been added to protect those from disconnection. Would be good to revisit those places and evaluate if guards are needed.

For example commit c60fd71a65e841efe187992f46c583a704cc37f5 by @sdaftuar checks for pfrom->fWhitelisted, but the commit immediately after it (5a6d00c6defc587e22c93e63029fdd538ce8858d) removes the check.

luke-jr commented at 4:33 pm on September 10, 2020: member

@MarcoFalke Even on inbound peers, noban isn’t absolute. There are many places that can be made more noban-friendly, but due to the complexity of at least a few of them, I think they would be better as followup PRs.

luke-jr force-pushed on Sep 10, 2020

DrahtBot removed the label Needs rebase on Sep 10, 2020

luke-jr force-pushed on Sep 10, 2020

luke-jr force-pushed on Sep 11, 2020

DrahtBot added the label Needs rebase on Sep 23, 2020

luke-jr force-pushed on Oct 30, 2020

DrahtBot removed the label Needs rebase on Oct 30, 2020

DrahtBot added the label Needs rebase on Dec 1, 2020

luke-jr force-pushed on Mar 4, 2021

DrahtBot removed the label Needs rebase on Mar 4, 2021

maflcko referenced this in commit 8c049fe9af on Mar 7, 2021

rebroad commented at 10:12 am on March 19, 2021: contributor

More reasons to merge than to not merge, based on my understanding of the use of whitelist (which is essentially peers you trust/own), so untested ACK.

jonatack commented at 9:41 pm on April 9, 2021: contributor

I’ve looking at the net permissions lately, will review.

DrahtBot added the label Needs rebase on Apr 30, 2021

luke-jr commented at 1:23 am on July 7, 2021: member

Rebased

luke-jr force-pushed on Jul 7, 2021

DrahtBot removed the label Needs rebase on Jul 7, 2021

DrahtBot added the label Needs rebase on Feb 4, 2022

net: Move PF_ISIMPLICIT interpretation from AcceptConnection to new InitializePermissionFlags 1daa9d1a91

net: Move extra service flag into InitializePermissionFlags a2a21f995f

Accept "in" and "out" flags to -whitelist to allow whitelisting outgoing connections 36cc299bae

luke-jr force-pushed on May 12, 2022

DrahtBot removed the label Needs rebase on May 12, 2022

maflcko commented at 1:26 pm on May 17, 2022: member

Concept ACK

luke-jr referenced this in commit facd0cd196 on May 21, 2022

luke-jr referenced this in commit 06163b0921 on May 21, 2022

luke-jr referenced this in commit 90bb84efb7 on May 21, 2022

maflcko referenced this in commit fa2b5fe0c1 on May 27, 2022

maflcko referenced this in commit fafddafc2c on Jun 14, 2022

maflcko referenced this in commit ede9089096 on Jun 15, 2022

sidhujag referenced this in commit a46aa0a2b9 on Jun 15, 2022

ghost commented at 2:19 am on June 23, 2022: none

Concept ACK

vasild commented at 11:36 am on June 28, 2022: contributor

This would allow us to use transient (aka one-time / random / disposable) I2P addresses when making outgoing connections to all nodes, except to whitelisted ones for which we would use our persistent I2P address.

DrahtBot commented at 6:13 pm on July 7, 2022: 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 added the label Needs rebase on Jul 7, 2022

janus referenced this in commit 646a84b1a1 on Aug 4, 2022

achow101 commented at 6:03 pm on October 12, 2022: member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

achow101 closed this on Oct 12, 2022

brunoerg commented at 7:03 pm on October 24, 2022: contributor

@luke-jr Are you intending to work on this?

ghost commented at 7:58 pm on October 24, 2022: none

Seems a little weird because I tried to filter bitcoin core pull requests with is:pr is:open sort:updated-asc and first PR in the result has 1 Concept ACK from dev who no longer contributes to this repo in reviews and second was only active for a few days/weeks. Those comments were posted in May 2022 and PR is in draft mode.

Whereas, this PR got last 2 Concept ACK or comments agreeing from active contributors in May 2022. And a Concept ACK from present maintainer.

@luke-jr Are you intending to work on this?

Anyway, PR author has not commented since Jul 7, 2021 so we cannot blame others for this PR to get more review and eventually merged.

Whitelist could be a taboo in bitcoin, however this is already possible manually. This PR makes it easier to manage your outgoing connections. I haven’t reviewed each commit but I conceptually agree with the idea.

luke-jr commented at 11:30 pm on October 24, 2022: member

@brunoerg I have never stopped maintaining it.

brunoerg commented at 11:47 pm on October 24, 2022: contributor

@luke-jr Cool, just to know, because it’s been closed and I am interesting on it.

brunoerg commented at 11:47 pm on October 24, 2022: contributor

Concept ACK

bitcoin deleted a comment on Nov 2, 2022

vijaydasmp referenced this in commit 91535fda98 on Aug 8, 2023

vijaydasmp referenced this in commit f0079dcf4a on Aug 8, 2023

vijaydasmp referenced this in commit 6728914385 on Aug 9, 2023

bitcoin locked this on Nov 2, 2023

Allow whitelisting outgoing connections #17167

Conflicts