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

brunoerg · 2022-11-01T18:26:06Z

Built this PR on top of #17167 (that's been closed due to inactivity but had some Concept ACK). So, it allows whitelisting outbound peers.

This PR adds a new RPC addpermissionflags to be able to set up permission flags -whitelist thru RPC, so we don't need to restart our node if we want to add new flags.

E.g.

$ ./src/bitcoin-cli addpermissionflags ["noban", "mempool", "in", "out"] "127.0.0.1"

DrahtBot · 2022-11-01T22:39:34Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	stickies-v, hernanmarino
Approach ACK	pablomartin4btc

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:

#27114 (p2p: Allow whitelisting outgoing connections by brunoerg)
#26988 (rpc: Add test-only RPC getaddrmaninfo for new/tried table address count by stratospher)
#26837 (I2P network optimizations by vasild)
#25515 (net, test: Virtualise CConnman and add initial PeerManager unit tests by dergoegge)
#24748 (test/BIP324: functional tests for v2 P2P encryption by stratospher)
#24545 (BIP324: Enable v2 P2P encrypted transport by dhruv)
#17783 (util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)
#17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
#17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
#17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)

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

I think it would be better to specify as [String "IP" or Number nodeid, [list of permissions], [list of permissions to remove]] or similar.

src/rpc/net.cpp

src/net.cpp

stickies-v

Concept ACK-ish, in that whitelisting shouldn't require a node restart.

Where I'm not 100% sure (as commented), is that this conceptually feels much more like a persistent setting instead of the ephemeral nature (i.e. effect gone after node restart) that these RPC commands have. Could be a source of confusion?

src/rpc/net.cpp

stickies-v · 2022-11-07T16:22:11Z

src/rpc/net.cpp

@@ -272,6 +272,49 @@ static RPCHelpMan getpeerinfo()
    };
 }

+static RPCHelpMan addwhitelist()


A question rather than a suggestion: should the docs highlight that these permissions do not persist beyond a node reboot? Intuitively, I'd have expected this kind of effect to be persistent, so maybe highlighting that is useful if I'm not the only one?

It's intuitive for me these permissions aren't persistent since there is no way to remove them (thinking about a follow-up adding an option for removal in this RPC), but sounds goods for me to add it in the docs to clarify.

src/rpc/net.cpp

stickies-v · 2022-11-07T16:35:18Z

src/net_permissions.cpp

@@ -104,11 +117,11 @@ bool NetWhitebindPermissions::TryParse(const std::string& str, NetWhitebindPermi
    return true;
 }

-bool NetWhitelistPermissions::TryParse(const std::string& str, NetWhitelistPermissions& output, bilingual_str& error)
+bool NetWhitelistPermissions::TryParse(const std::string& str, NetWhitelistPermissions& output, ConnectionDirection& output_connection_direction, bilingual_str& error)


Not a big fan of adding extra out-parameters, I think util::Result would be more appropriate here (but also a bit of an overhaul so wouldn't be opposed to doing it in follow-up)

brunoerg · 2022-11-08T14:19:05Z

I changed it to draft to work on some stuff some reviewers suggested here. Gonna advertise when it ready for review.

…nitializePermissionFlags

…ing connections

brunoerg · 2022-11-14T13:58:27Z

Force-pushed changing the approach and addressing reviews!

Instead of using the same format from -whitelist= (e.g. permissions@ip address or network), now it has 2 params - an array of flags and an ip address or network.
E.g.
```
./src/bitcoin-cli addpermissionflags ["noban", "mempool", "in", "out"] "127.0.0.1"
```
Addressed rpc, p2p: add addpermissionflags RPC and allow whitelisting outbound #26441 (comment) by changing this RPC to addpermissionflags
Addressed other minor changes

codo1 · 2023-02-10T07:58:38Z

src/rpc/net.cpp

+static RPCHelpMan addpermissionflags()
+{
+    return RPCHelpMan{"addpermissionflags",
+                "\nAdd permission flags to the peers using the given IP address (e.g. 1.2.3.4) or\n"


It took a while before I realized the added permission flags are only applied to new peers (connecting after the rpc command). Not sure if that should have been obvious to me. Maybe change "peers" to "future peers" or something similar?

furszy · 2023-02-13T13:55:38Z

src/net.h

-    // Whitelisted ranges. Any node connecting from these is automatically
-    // whitelisted (as well as those connecting to whitelisted binds).
-    std::vector<NetWhitelistPermissions> vWhitelistedRange;
-    // Whitelisted ranges for outgoing connections.
-    std::vector<NetWhitelistPermissions> vWhitelistedRangeOutgoing;
-


I don't think that making the members public is good, it breaks the class encapsulation and allows multiple threads to access/modify the fields without any contention (read/write operations should be guarded).

pablomartin4btc

Approach ACK, run automated tests (unit, functional & fuzz), pending of manual testing.

Regarding whitelisting outbound connections found some comments from @gmaxwell & @laanwj here.

Please add "Review club" label (*).

LarryRuane

I'll review more soon, but here are a couple of instances where changes are not in the correct commits. Each commit should compile (and all tests should pass, but I didn't check that).

src/net.cpp

hernanmarino

tested ACK. This might help with a couple of functional tests.

brunoerg · 2023-02-16T20:32:13Z

Converted it to draft to address suggestions and re-build it on top of #27114.

Riahiamirreza · 2023-02-18T09:59:47Z

src/net_permissions.cpp

@@ -21,9 +21,10 @@ const std::vector<std::string> NET_PERMISSIONS_DOC{
 namespace {

 // Parse the following format: "perm1,perm2@xxxxxx"
-bool TryParsePermissionFlags(const std::string& str, NetPermissionFlags& output, size_t& readen, bilingual_str& error)
+bool TryParsePermissionFlags(const std::string& str, NetPermissionFlags& output, ConnectionDirection* output_connection_direction, size_t& readen, bilingual_str& error)


nit: while the other parameter is output_connection_direction, isn't it good idea to change output to output_permission_flags or output_permissions?

Riahiamirreza · 2023-02-18T10:04:42Z

src/net_permissions.cpp

@@ -85,7 +98,7 @@ bool NetWhitebindPermissions::TryParse(const std::string& str, NetWhitebindPermi
 {
    NetPermissionFlags flags;
    size_t offset;
-    if (!TryParsePermissionFlags(str, flags, offset, error)) return false;
+    if (!TryParsePermissionFlags(str, flags, nullptr, offset, error)) return false;


nit: Why not using ConnectionDirection::None rather than nullptr to make it more clear?

DrahtBot · 2023-02-22T18:46:12Z

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

DrahtBot · 2023-05-23T01:16:21Z

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.

DrahtBot · 2023-08-21T00:49:10Z

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.

DrahtBot · 2023-11-19T01:17:42Z

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.

brunoerg · 2024-01-12T18:09:10Z

Closing it to focus on other approach.

brunoerg changed the title ~~rpc, p2p: allow whitelisting outgoing connections (17167) + add addwhitelist RPC~~ rpc, p2p: add addwhitelist RPC Nov 1, 2022

brunoerg force-pushed the 2022-10-whitelist-rpc branch 2 times, most recently from 0220d6c to d14fbaa Compare November 1, 2022 19:23

brunoerg force-pushed the 2022-10-whitelist-rpc branch from d14fbaa to ea08c79 Compare November 2, 2022 12:18

This was referenced Nov 3, 2022

net, test: Virtualise CConnman and add initial PeerManager unit tests #25515

Closed

BIP324: Enable v2 P2P encrypted transport #24545

Closed

brunoerg force-pushed the 2022-10-whitelist-rpc branch from 0ed1e4d to cf50266 Compare November 5, 2022 15:16

brunoerg marked this pull request as ready for review November 5, 2022 16:16

luke-jr suggested changes Nov 5, 2022

View reviewed changes

maflcko reviewed Nov 7, 2022

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

maflcko reviewed Nov 7, 2022

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

stickies-v reviewed Nov 7, 2022

View reviewed changes

brunoerg marked this pull request as draft November 8, 2022 14:17

brunoerg force-pushed the 2022-10-whitelist-rpc branch from cf50266 to 174d746 Compare November 11, 2022 19:10

luke-jr and others added 4 commits November 11, 2022 16:27

net: Move PF_ISIMPLICIT interpretation from AcceptConnection to new I…

f359fff

…nitializePermissionFlags

net: Move extra service flag into InitializePermissionFlags

229b76e

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

7dc3ac9

…ing connections

net: make vWhitelistedRange and vWhitelistedRangeOutgoing public

d2977e8

brunoerg force-pushed the 2022-10-whitelist-rpc branch from 174d746 to df5d400 Compare November 11, 2022 19:36

brunoerg changed the title ~~rpc, p2p: add addwhitelist RPC~~ rpc, p2p: add addpermissionflags RPC Nov 11, 2022

brunoerg added 3 commits November 12, 2022 10:56

p2p, rpc: add addpermissionflags

6e82ca5

test: add coverage for addpermissionflags RPC

7b442d6

doc: add release notes for addpermissionflags

153e86e

brunoerg force-pushed the 2022-10-whitelist-rpc branch from df5d400 to 153e86e Compare November 12, 2022 14:19

brunoerg marked this pull request as ready for review November 14, 2022 13:58

DrahtBot mentioned this pull request Nov 28, 2022

net, refactor: Kill proxy globals #26590

Closed

DrahtBot mentioned this pull request Dec 17, 2022

test/BIP324: functional tests for v2 P2P encryption #24748

Merged

DrahtBot mentioned this pull request Jan 6, 2023

I2P network optimizations #26837

Merged

glozow added the RPC/REST/ZMQ label Jan 27, 2023

DrahtBot mentioned this pull request Jan 30, 2023

cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency #26988

Open

codo1 reviewed Feb 10, 2023

View reviewed changes

brunoerg mentioned this pull request Feb 12, 2023

test: fix immediate tx relay in wallet_groups.py #26970

Merged

brunoerg changed the title ~~rpc, p2p: add addpermissionflags RPC~~ rpc, p2p: add addpermissionflags RPC and allow whitelisting outbound Feb 13, 2023

furszy reviewed Feb 13, 2023

View reviewed changes

pablomartin4btc reviewed Feb 13, 2023

View reviewed changes

LarryRuane suggested changes Feb 15, 2023

View reviewed changes

src/net.cpp Show resolved Hide resolved

src/net.cpp Show resolved Hide resolved

hernanmarino reviewed Feb 15, 2023

View reviewed changes

brunoerg mentioned this pull request Feb 16, 2023

p2p: Allow whitelisting manual connections #27114

Merged

brunoerg marked this pull request as draft February 16, 2023 20:31

Riahiamirreza approved these changes Feb 18, 2023

View reviewed changes

DrahtBot added the Needs rebase label Feb 22, 2023

rajarshimaitra mentioned this pull request Mar 20, 2023

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

Closed

brunoerg closed this Jan 12, 2024

bitcoin locked and limited conversation to collaborators Jan 11, 2025

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

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

Uh oh!

Conversation

brunoerg commented Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stickies-v 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!

Uh oh!

stickies-v Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

brunoerg Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stickies-v Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

brunoerg commented Nov 8, 2022

Uh oh!

brunoerg commented Nov 14, 2022

Uh oh!

codo1 Feb 10, 2023 • 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!

furszy Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

LarryRuane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hernanmarino left a comment

Choose a reason for hiding this comment

Uh oh!

brunoerg commented Feb 16, 2023

Uh oh!

Riahiamirreza Feb 18, 2023

Choose a reason for hiding this comment

Uh oh!

Riahiamirreza Feb 18, 2023

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Feb 22, 2023

Uh oh!

DrahtBot commented May 23, 2023

Uh oh!

DrahtBot commented Aug 21, 2023

Uh oh!

DrahtBot commented Nov 19, 2023

Uh oh!

brunoerg commented Jan 12, 2024

Uh oh!

Uh oh!

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

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

brunoerg commented Nov 1, 2022 •

edited

Loading

DrahtBot commented Nov 1, 2022 •

edited

Loading

stickies-v left a comment •

edited

Loading

codo1 Feb 10, 2023 •

edited

Loading