p2p: Allow whitelisting manual connections #27114

brunoerg · 2023-02-16T20:31:10Z

Revives #17167. It allows whitelisting manual connections. Fixes #9923

Since there are some PRs/issues around this topic, I'll list some motivations/comments for whitelisting outbound connections from them:

Speed-up tx relay/mempool sync for testing purposes (my personal motivation for this) - In test: fix immediate tx relay in wallet_groups.py #26970, theStack pointed out that we whitelist peers to speed up tx relay for fast mempool synchronization, however, since it applies only for inbound connections and considering the topology node0 <--- node1 <---- node2 <--- ... <-- nodeN, if a tx is submitted from any node other than node0, the mempool synchronization can take quite long.
net, cli: use v2transport for manual/addrfetch connections, add to -netinfo #29058 (comment) - "Before enabling -v2transport by default (which I'd image may happen after test/BIP324: functional tests for v2 P2P encryption #24748) we could consider a way to force manual connections to be only-v1 or even only-v2 (disabling reconnect-with-v1). A possibility could be through a net permission flag, if p2p: Allow whitelisting manual connections #27114 makes it in."
Allow whitelisting outgoing connections #17167 (comment) - "This would allow us to use I2P: add support for transient addresses for outbound connections #25355 when making outgoing connections to all nodes, except to whitelisted ones for which we would use our persistent I2P address."
Force-relay/mempool permissions for a node you intentionally connected to.

DrahtBot · 2023-02-16T20:31:13Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sr-gi, pinheadmz, achow101
Concept NACK	luke-jr
Concept ACK	furszy, RandyMcMillan
Approach ACK	jonatack
Stale ACK	pablomartin4btc, naumenkogs, stratospher

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#28834 (net: attempts to connect to all resolved addresses when connecting to a node by sr-gi)
#28710 (Remove the legacy wallet and BDB dependency by achow101)

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.

brunoerg · 2023-02-16T20:31:25Z

cc: @furszy @theStack

naumenkogs · 2023-02-23T12:55:58Z

Concept ACK

stratospher

concept ACK. whitelisting outgoing connections is a useful feature to have! went through the code and it looks good.

integration tests in p2p_permissions.py would be nice! (It’d be interesting to see the behaviour when different permissions are set in different connection directions.)
something i had trouble understanding was if we give a trusted peer special permissions, would it matter who initiated the connection?
micro nit: next time you rebase/update, maybe reword the 1st commit message to use NetPermissionFlags::Implicit instead of PF_ISIMPLICIT?

src/init.cpp

furszy

Concept ACK

Could also add a test framework option to whitelist peers. So we don't have to add -whitelist flag everywhere.

brunoerg · 2023-03-13T21:13:05Z

Could also add a test framework option to whitelist peers. So we don't have to add -whitelist flag everywhere.

Sounds good! Gonna implement it!

brunoerg · 2023-03-15T00:21:40Z

force-pushed: Added self.whitelist_peers in test_framework to avoid having to add -whitelist flag everywhere. Also, I changed the 1st commit message as suggested by @stratospher.

brunoerg · 2023-03-28T17:29:25Z

Rebased

glozow · 2023-04-20T09:57:33Z

linking #9923
perhaps also relevant: #10594, #10051

jonatack

ACK 19f3f3a with some suggestions

src/net.h

src/net_permissions.cpp

src/test/fuzz/net_permissions.cpp

src/test/netbase_tests.cpp

src/init.cpp

test/functional/test_framework/test_framework.py

jonatack · 2023-05-05T18:14:46Z

test/functional/mempool_reorg.py

-            ],
-            []
-        ]
+        self.whitelist_peers = True


19f3f3a This doesn't seem to be a pure refactoring here and in mempool_packages.py and p2p_segwit.py

brunoerg · 2024-02-28T13:39:03Z

Force-pushed addressing:

thanks, @stratospher

sr-gi

re-ACK 0a53361

pinheadmz

ACK 0a53361

confirmed minimal diff since last ack

Show Signature

pinheadmz's public key is on keybase

stratospher

concept ACK! it's useful to be able to whitelist and give special permissions to manual peers which are essentially peers you trust.

I've gone through all the commits but not leaving a code review/approach ACK since I don't understand why we'd need different permissions for each connection direction. (related comments - #17167 (comment), #27114 (comment), #27114 (comment))

stratospher · 2024-03-06T11:34:38Z

test/functional/mempool_packages.py

@@ -27,10 +27,11 @@
 class MempoolPackagesTest(BitcoinTestFramework):
    def set_test_params(self):
        self.num_nodes = 2
+        # whitelist peers to speed up tx relay / mempool sync
+        self.noban_tx_relay = True


c985eb8: isn't this a behaviour change in the functional test? now, the second node also has noban permission - previously it didn't. same situation happens in test/functional/p2p_segwit.py.

so maybe keep "-whitelist=noban@127.0.0.1" instead of self.noban_tx_relay = True in both these tests.

(haven't looked into how having noban permission for second node affects the functional test though.)

I does not affect the behaviour of the tests since those tests does not test any misbehaviour. It is just to speed up tx relay.

achow101 · 2024-03-12T16:50:42Z

ACK 0a53361

Github-Pull: bitcoin#27114 Rebased-From: 2863d7d

…tPermissionFlags` Github-Pull: bitcoin#27114 Rebased-From: 9133fd6

Github-Pull: bitcoin#27114 Rebased-From: 8e06be3

…l connections Github-Pull: bitcoin#27114 Rebased-From: 66bc6e2

Github-Pull: bitcoin#27114 Rebased-From: e6b8f19

Github-Pull: bitcoin#27114 Rebased-From: 1bae3b2 (partial)

Reverts commit ab4efad (PR bitcoin#26970). This workaround is not needed anymore, as since bitcoin#27114 the test sets the noban permission for both in- and outbound connections via the `noban_tx_relay` setting, and we don't have to rely on these topology hacks anymore. See commit c985eb8.

…oups.py 93fae5a test: remove immediate tx relay workaround in wallet_groups.py (Sebastian Falbesoner) Pull request description: Reverts commit ab4efad (PR #26970). This workaround is not needed anymore, as since #27114 the test sets the noban permission for both in- and outbound connections via the `noban_tx_relay` setting, and we don't have to rely on this topology hack anymore. See commit c985eb8 (kudos to brunoerg!). Can be tested by executing `$ time ./test/functional/wallet_groups.py` both on master and PR and verifying that the execution time is roughly equal. ACKs for top commit: maflcko: lgtm ACK 93fae5a brunoerg: utACK 93fae5a Tree-SHA512: b949fd05b4308815ba02d0ee4d1318f642b930288dd03223f46db7db745177af1c070bc7058743ac27963c5ad90564089867cc12f31fee94812a16919c353bab

sr-gi · 2024-04-10T08:26:31Z

src/net_permissions.cpp

+    // By default, whitelist only applies to incoming connections
+    if (connection_direction == ConnectionDirection::None) {
+        connection_direction = ConnectionDirection::In;
+    } else if (flags == NetPermissionFlags::None) {


Going over this PR again, shouldn't this have been a separate if statement instead of an if/else (or not a condition at all)?

Right now we are accepting expressions of the type @ip:port (which were already accepted before), but not expressions of the type in/out@ip:port.

The former will pass and default to ConnectionDirection::In + NetPermissionFlags::Implicit, while the latter will be rejected. That feels inconsistent

…llet_groups.py 93fae5a test: remove immediate tx relay workaround in wallet_groups.py (Sebastian Falbesoner) Pull request description: Reverts commit ab4efad (PR bitcoin#26970). This workaround is not needed anymore, as since bitcoin#27114 the test sets the noban permission for both in- and outbound connections via the `noban_tx_relay` setting, and we don't have to rely on this topology hack anymore. See commit c985eb8 (kudos to brunoerg!). Can be tested by executing `$ time ./test/functional/wallet_groups.py` both on master and PR and verifying that the execution time is roughly equal. ACKs for top commit: maflcko: lgtm ACK 93fae5a brunoerg: utACK 93fae5a Tree-SHA512: b949fd05b4308815ba02d0ee4d1318f642b930288dd03223f46db7db745177af1c070bc7058743ac27963c5ad90564089867cc12f31fee94812a16919c353bab

Summary: > net: store `-whitelist{force}relay` values in `CConnman` > net: Move NetPermissionFlags::Implicit verification to `AddWhitelistPermissionFlags` > net_processing: Move extra service flag into InitializeNode > Accept "in" and "out" flags to -whitelist to allow whitelisting manual connections > test: add coverage for whitelisting manual connections This is a partial backport of [[bitcoin/bitcoin#27114 | core#27114]] bitcoin/bitcoin@2863d7d bitcoin/bitcoin@9133fd6 bitcoin/bitcoin@8e06be3 bitcoin/bitcoin@66bc6e2 bitcoin/bitcoin@e6b8f19 bitcoin/bitcoin@0a53361 Depends on D17112 Note that CNode::m_permissionFlags was made public by core in [[bitcoin/bitcoin#20881 | core#20881]] which we are missing so it had to be made public in this diff. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D17114

Summary: when `self.noban_tx_relay=True`, the following flag `-whitelist=noban,in,out@127.0.0.1`is added to `extra_args` to speed up tx relay/mempool sync. Before (all functional tests): ``` ALL | ✓ Passed | 1465 s (accumulated) Runtime: 163 s ``` After: ``` ALL | ✓ Passed | 1350 s (accumulated) Runtime: 150 s ``` This makes test duration shorter and hopefully solves some rare timeouts in interface_rest.py This concludes backport of [[bitcoin/bitcoin#27114 | core#27114]] bitcoin/bitcoin@c985eb8 Bypasses [[bitcoin/bitcoin#26970 | core#26970]] and [[bitcoin/bitcoin#29822 | core#29822]] Depends on D17114 Test Plan: `ninja check-functional` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D17115

Summary: > net: store `-whitelist{force}relay` values in `CConnman` > net: Move NetPermissionFlags::Implicit verification to `AddWhitelistPermissionFlags` > net_processing: Move extra service flag into InitializeNode > Accept "in" and "out" flags to -whitelist to allow whitelisting manual connections > test: add coverage for whitelisting manual connections This is a partial backport of [[bitcoin/bitcoin#27114 | core#27114]] bitcoin/bitcoin@2863d7d bitcoin/bitcoin@9133fd6 bitcoin/bitcoin@8e06be3 bitcoin/bitcoin@66bc6e2 bitcoin/bitcoin@e6b8f19 bitcoin/bitcoin@0a53361 Depends on D17112 Note that CNode::m_permissionFlags was made public by core in [[bitcoin/bitcoin#20881 | core#20881]] which we are missing so it had to be made public in this diff. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D17114

Summary: when `self.noban_tx_relay=True`, the following flag `-whitelist=noban,in,out@127.0.0.1`is added to `extra_args` to speed up tx relay/mempool sync. Before (all functional tests): ``` ALL | ✓ Passed | 1465 s (accumulated) Runtime: 163 s ``` After: ``` ALL | ✓ Passed | 1350 s (accumulated) Runtime: 150 s ``` This makes test duration shorter and hopefully solves some rare timeouts in interface_rest.py This concludes backport of [[bitcoin/bitcoin#27114 | core#27114]] bitcoin/bitcoin@c985eb8 Bypasses [[bitcoin/bitcoin#26970 | core#26970]] and [[bitcoin/bitcoin#29822 | core#29822]] Depends on D17114 Test Plan: `ninja check-functional` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D17115

Github-Pull: bitcoin#27114 Rebased-From: 1bae3b2 (partial)

DrahtBot added the P2P label Feb 16, 2023

brunoerg mentioned this pull request Feb 16, 2023

rpc, p2p: add addpermissionflags RPC and allow whitelisting outbound #26441

Closed

DrahtBot added the Needs rebase label Feb 22, 2023

brunoerg force-pushed the 2023-02-outgoing-whitelist branch from cf19251 to 4a2aa6c Compare February 22, 2023 18:46

DrahtBot removed the Needs rebase label Feb 22, 2023

stratospher reviewed Mar 10, 2023

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

furszy reviewed Mar 13, 2023

View reviewed changes

brunoerg force-pushed the 2023-02-outgoing-whitelist branch 2 times, most recently from 67b32c5 to 6ed6fca Compare March 14, 2023 23:24

rajarshimaitra mentioned this pull request Mar 20, 2023

p2p: Allow whitelisting outgoing connections #27114 Bitshala/BitcoinCore-PR-Review-Club#19

Closed

DrahtBot mentioned this pull request Mar 24, 2023

net: #27257 follow-ups #27324

Merged

DrahtBot added the Needs rebase label Mar 28, 2023

brunoerg force-pushed the 2023-02-outgoing-whitelist branch from 6ed6fca to 19f3f3a Compare March 28, 2023 17:29

DrahtBot removed the Needs rebase label Mar 28, 2023

DrahtBot mentioned this pull request Apr 6, 2023

Remove BIP35 mempool p2p message #27426

Closed

jonatack reviewed May 5, 2023

View reviewed changes

sr-gi reviewed Feb 28, 2024

View reviewed changes

DrahtBot requested a review from pinheadmz February 28, 2024 20:11

pinheadmz approved these changes Feb 29, 2024

View reviewed changes

stratospher reviewed Mar 6, 2024

View reviewed changes

DrahtBot requested a review from stratospher March 6, 2024 17:32

rajarshimaitra mentioned this pull request Mar 12, 2024

Together we review series Bitshala/BitcoinCore-PR-Review-Club#49

Closed

achow101 merged commit bef9917 into bitcoin:master Mar 12, 2024

kevkevinpal pushed a commit to kevkevinpal/bitcoin that referenced this pull request Mar 13, 2024

docs: add release notes for bitcoin#27114

c8589c0

kevkevinpal pushed a commit to kevkevinpal/bitcoin that referenced this pull request Mar 13, 2024

docs: add release notes for bitcoin#27114

9860a03

sr-gi pushed a commit to sr-gi/bitcoin that referenced this pull request Mar 25, 2024

docs: add release notes for bitcoin#27114

f8c54c8

Retropex pushed a commit to Retropex/bitcoin that referenced this pull request Mar 28, 2024

net: store -whitelist{force}relay values in CConnman

7076169

Github-Pull: bitcoin#27114 Rebased-From: 2863d7d

Retropex pushed a commit to Retropex/bitcoin that referenced this pull request Mar 28, 2024

net: Move NetPermissionFlags::Implicit verification to `AddWhitelis…

b99db20

…tPermissionFlags` Github-Pull: bitcoin#27114 Rebased-From: 9133fd6

Retropex pushed a commit to Retropex/bitcoin that referenced this pull request Mar 28, 2024

net_processing: Move extra service flag into InitializeNode

66d5aa3

Github-Pull: bitcoin#27114 Rebased-From: 8e06be3

Retropex pushed a commit to Retropex/bitcoin that referenced this pull request Mar 28, 2024

Accept "in" and "out" flags to -whitelist to allow whitelisting manua…

96d715f

…l connections Github-Pull: bitcoin#27114 Rebased-From: 66bc6e2

Retropex pushed a commit to Retropex/bitcoin that referenced this pull request Mar 28, 2024

test: add coverage for whitelisting manual connections

8d290f4

Github-Pull: bitcoin#27114 Rebased-From: e6b8f19

Retropex pushed a commit to Retropex/bitcoin that referenced this pull request Mar 28, 2024

QA: p2p_invalid_messages: Check misbehaving with noban,out whitelisting

f33cd88

Github-Pull: bitcoin#27114 Rebased-From: 1bae3b2 (partial)

theStack mentioned this pull request Apr 6, 2024

test: remove immediate tx relay workaround in wallet_groups.py #29822

Merged

sr-gi reviewed Apr 10, 2024

View reviewed changes

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Feb 22, 2025

QA: p2p_invalid_messages: Check misbehaving with noban,out whitelisting

dbbb11e

Github-Pull: bitcoin#27114 Rebased-From: 1bae3b2 (partial)

xyephy mentioned this pull request Feb 26, 2025

Upgrade to bitcoin v28 whiveio/whive#12

Open

bitcoin locked and limited conversation to collaborators Apr 10, 2025

p2p: Allow whitelisting manual connections #27114

p2p: Allow whitelisting manual connections #27114

Uh oh!

Conversation

brunoerg commented Feb 16, 2023 • edited by achow101 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

brunoerg commented Feb 16, 2023

Uh oh!

naumenkogs commented Feb 23, 2023

Uh oh!

stratospher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

brunoerg commented Mar 13, 2023

Uh oh!

brunoerg commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brunoerg commented Mar 28, 2023

Uh oh!

glozow commented Apr 20, 2023

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack May 5, 2023

Choose a reason for hiding this comment

Uh oh!

brunoerg commented Feb 28, 2024

Uh oh!

sr-gi left a comment

Choose a reason for hiding this comment

Uh oh!

pinheadmz left a comment

Choose a reason for hiding this comment

Uh oh!

stratospher left a comment • 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!

stratospher Mar 6, 2024

Choose a reason for hiding this comment

Uh oh!

brunoerg Mar 12, 2024

Choose a reason for hiding this comment

Uh oh!

achow101 commented Mar 12, 2024

Uh oh!

sr-gi Apr 10, 2024 • 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!

Uh oh!

brunoerg commented Feb 16, 2023 •

edited by achow101

Loading

DrahtBot commented Feb 16, 2023 •

edited

Loading

brunoerg commented Mar 15, 2023 •

edited

Loading

stratospher left a comment •

edited

Loading

sr-gi Apr 10, 2024 •

edited

Loading