p2p: Allow whitelisting manual connections by brunoerg · Pull Request #27114 · bitcoin/bitcoin

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?

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 19f3f3a316660890f03d6ddcb4a4230eb2ff5e91 with some suggestions

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

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

luke-jr · 2024-01-26T16:37:33Z

Concept NACK to restricting it to only manual connections. This will cause random breakage when automatic peering happens to connect to a manual peer first.

mzumsande · 2024-01-26T16:47:56Z

Concept NACK to restricting it to only manual connections. This will cause random breakage when automatic peering happens to connect to a manual peer first.

did you see #28895? We don't make automatic connections to manually specified peers anymore.

luke-jr · 2024-01-26T18:45:33Z

Hmm, it's still weird, but I guess it's not terrible then.

sr-gi

Approach ACK 1d0216e

sr-gi · 2024-02-06T22:49:22Z

It may be worth documenting this in the method docs to prevent potential future footshots

Sounds good

brunoerg · 2024-02-09T12:58:53Z

Force-pushed addressing:

Thanks @sr-gi for reviewing!

sr-gi · 2024-02-09T18:51:40Z

tACK c10f528

naumenkogs · 2024-02-20T09:18:32Z

ACK c10f528350152ca9248e8c167b67993fc7321ca3

pinheadmz

ACK c10f528350152ca9248e8c167b67993fc7321ca3

I left comments on a few nits but none are breaking for me. Happy to re-ACK if you update.

Built and ran all functional and unit tests locally on macOS/arm

Reviewed specific changes since my last ACK:

git range-diff abfc8c901d..d69747ab65 5883a8911a..c10f528350

Move relay flags to netpermissions from net
Only apply new logic to manual connections
Apply flag to new tests added since last review

Show Signature

pinheadmz's public key is on keybase

brunoerg · 2024-02-26T12:00:50Z

Force-pushed addressing #27114 (comment). Just removed an unecessary variable.

cc: @naumenkogs @sr-gi @pinheadmz

pinheadmz

ACK 231e1797023c3502e1605b263b24f29eb659ce55

Show Signature

pinheadmz's public key is on keybase

…tPermissionFlags`

…l connections

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.

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

    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

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

+    // 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

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

Comment thread src/init.cpp Outdated

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

DrahtBot mentioned this pull request Jan 18, 2024

Wallet: Add maxfeerate wallet startup option #29278

Open

sr-gi reviewed Feb 7, 2024

View reviewed changes

sr-gi reviewed Feb 8, 2024

View reviewed changes

Comment thread src/init.cpp Outdated

DrahtBot mentioned this pull request Feb 9, 2024

Broadcast own transactions only via short-lived Tor or I2P connections #29415

Merged

4 tasks

pinheadmz approved these changes Feb 20, 2024

View reviewed changes

Comment thread src/net.cpp Outdated

Comment thread src/net.h Outdated

Comment thread doc/release-notes-27114.md Outdated

pinheadmz approved these changes Feb 26, 2024

View reviewed changes

stratospher reviewed Feb 28, 2024

View reviewed changes

Comment thread src/init.cpp Outdated

Comment thread src/net_permissions.h Outdated

Comment thread src/net_permissions.cpp Outdated

brunoerg and others added 7 commits February 28, 2024 10:04

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

2863d7d

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

9133fd6

…tPermissionFlags`

net_processing: Move extra service flag into InitializeNode

8e06be3

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

66bc6e2

…l connections

test: add option to speed up tx relay/mempool sync

c985eb8

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.

test: add coverage for whitelisting manual connections

e6b8f19

docs: add release notes for bitcoin#27114

0a53361

sr-gi reviewed Feb 28, 2024

View reviewed changes

pinheadmz approved these changes Feb 29, 2024

View reviewed changes

stratospher reviewed Mar 6, 2024

View reviewed changes

rajarshimaitra mentioned this pull request Mar 12, 2024

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

Closed

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

xyephy mentioned this pull request Feb 26, 2025

Upgrade to bitcoin v28 whiveio/whive#12

Open

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!

luke-jr commented Jan 26, 2024

Uh oh!

mzumsande commented Jan 26, 2024

Uh oh!

luke-jr commented Jan 26, 2024

Uh oh!

sr-gi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sr-gi Feb 6, 2024

Choose a reason for hiding this comment

Uh oh!

brunoerg Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brunoerg commented Feb 9, 2024

Uh oh!

sr-gi commented Feb 9, 2024

Uh oh!

naumenkogs commented Feb 20, 2024

Uh oh!

pinheadmz 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!

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

pinheadmz left a comment •

edited

Loading

brunoerg commented Feb 26, 2024 •

edited

Loading

stratospher left a comment •

edited

Loading

sr-gi Apr 10, 2024 •

edited

Loading