addpermissionflags RPC and allow whitelisting outbound
#26441
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.
0$ ./src/bitcoin-cli addpermissionflags ["noban", "mempool", "in", "out"] "127.0.0.1"
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
Reviewers, this pull request conflicts with the following ones:
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.
[String "IP" or Number nodeid, [list of permissions], [list of permissions to remove]] or similar.
271@@ -272,6 +272,49 @@ static RPCHelpMan getpeerinfo()
272 };
273 }
274
275+static RPCHelpMan addwhitelist()
276+{
277+ return RPCHelpMan{"addwhitelist",
addpermissionflags, to better convey that this is setting flags and not a single black/white bool?
601+ if (NetPermissions::HasFlag(flags, NetPermissionFlags::BloomFilter)) {
602+ service_flags = static_cast<ServiceFlags>(service_flags | NODE_BLOOM);
603+ }
604+}
605+
606+std::string ConnectionTypeAsString(ConnectionType conn_type)
CNode
296+ CConnman& connman = EnsureConnman(node);
297+
298+ const auto permissions{request.params[0].get_array()};
299+
300+ for (size_t i = 0; i < permissions.size(); i++) {
301+ NetWhitelistPermissions subnet;
nit: m_subnet is a member of NetWhitelistPermissions, isn’t permissions a more accurate name?
0 NetWhitelistPermissions permissions;
299+
300+ for (size_t i = 0; i < permissions.size(); i++) {
301+ NetWhitelistPermissions subnet;
302+ ConnectionDirection connection_direction;
303+ bilingual_str error;
304+ if (!NetWhitelistPermissions::TryParse(permissions[i].get_str(), subnet, connection_direction, error)) throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid permissions");
nit: line length
0 if (!NetWhitelistPermissions::TryParse(permissions[i].get_str(), subnet, connection_direction, error)) {
1 throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid permissions");
2 }
295+ NodeContext& node = EnsureAnyNodeContext(request.context);
296+ CConnman& connman = EnsureConnman(node);
297+
298+ const auto permissions{request.params[0].get_array()};
299+
300+ for (size_t i = 0; i < permissions.size(); i++) {
0 for (const auto& permission : permissions) {
271@@ -272,6 +272,49 @@ static RPCHelpMan getpeerinfo()
272 };
273 }
274
275+static RPCHelpMan addwhitelist()
271@@ -272,6 +272,49 @@ static RPCHelpMan getpeerinfo()
272 };
273 }
274
275+static RPCHelpMan addwhitelist()
276+{
277+ return RPCHelpMan{"addwhitelist",
278+ "\nAdd permission flags to the peers using the given IP address (e.g. 1.2.3.4) or\n"
279+ "CIDR-notated network (e.g. 1.2.3.0/24). Uses the same permissions as -whitebind.\n"
280+ "Additional flags in and out control whether permissions apply to incoming connections and/or outgoing (default: incoming only).\n",
Perhaps also useful to include this in the example?
0 "Additional flags \"in\" and \"out\" control whether permissions apply to incoming and/or outgoing connections (default: incoming only).\n",
116@@ -104,11 +117,11 @@ bool NetWhitebindPermissions::TryParse(const std::string& str, NetWhitebindPermi
117 return true;
118 }
119
120-bool NetWhitelistPermissions::TryParse(const std::string& str, NetWhitelistPermissions& output, bilingual_str& error)
121+bool NetWhitelistPermissions::TryParse(const std::string& str, NetWhitelistPermissions& output, ConnectionDirection& output_connection_direction, bilingual_str& error)
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)
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?
Force-pushed changing the approach and addressing reviews!
-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.
0./src/bitcoin-cli addpermissionflags ["noban", "mempool", "in", "out"] "127.0.0.1"
addpermissionflags271@@ -272,6 +272,61 @@ static RPCHelpMan getpeerinfo()
272 };
273 }
274
275+static RPCHelpMan addpermissionflags()
276+{
277+ return RPCHelpMan{"addpermissionflags",
278+ "\nAdd permission flags to the peers using the given IP address (e.g. 1.2.3.4) or\n"
998@@ -993,12 +999,6 @@ class CConnman
999 // P2P timeout in seconds
1000 std::chrono::seconds m_peer_connect_timeout;
1001
1002- // Whitelisted ranges. Any node connecting from these is automatically
1003- // whitelisted (as well as those connecting to whitelisted binds).
1004- std::vector<NetWhitelistPermissions> vWhitelistedRange;
1005- // Whitelisted ranges for outgoing connections.
1006- std::vector<NetWhitelistPermissions> vWhitelistedRangeOutgoing;
1007-
965- if (gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) NetPermissions::AddFlag(permission_flags, NetPermissionFlags::ForceRelay);
966- if (gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY)) NetPermissions::AddFlag(permission_flags, NetPermissionFlags::Relay);
967- NetPermissions::AddFlag(permission_flags, NetPermissionFlags::Mempool);
968- NetPermissions::AddFlag(permission_flags, NetPermissionFlags::NoBan);
969- }
970+ AddWhitelistPermissionFlags(permission_flags, addr, vWhitelistedRange);
AddWhitelistPermissionFlags() should not have the vWhitelistedRange until the third commit, “Accept “in” and “out” flags to -whitelist…”.
559@@ -555,7 +560,8 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
560 pszDest ? pszDest : "",
561 conn_type,
562 /*inbound_onion=*/false,
563- CNodeOptions{ .i2p_sam_session = std::move(i2p_transient_session) });
564+ CNodeOptions{ .i2p_sam_session = std::move(i2p_transient_session),
565+ .permission_flags = permission_flags });
20@@ -21,9 +21,10 @@ const std::vector<std::string> NET_PERMISSIONS_DOC{
21 namespace {
22
23 // Parse the following format: "perm1,perm2@xxxxxx"
24-bool TryParsePermissionFlags(const std::string& str, NetPermissionFlags& output, size_t& readen, bilingual_str& error)
25+bool TryParsePermissionFlags(const std::string& str, NetPermissionFlags& output, ConnectionDirection* output_connection_direction, size_t& readen, bilingual_str& error)
output_connection_direction, isn’t it good idea to change output to output_permission_flags or output_permissions?
97@@ -85,7 +98,7 @@ bool NetWhitebindPermissions::TryParse(const std::string& str, NetWhitebindPermi
98 {
99 NetPermissionFlags flags;
100 size_t offset;
101- if (!TryParsePermissionFlags(str, flags, offset, error)) return false;
102+ if (!TryParsePermissionFlags(str, flags, nullptr, offset, error)) return false;
ConnectionDirection::None rather than nullptr to make it more clear?
🐙 This pull request conflicts with the target branch and needs rebase.
There hasn’t been much activity lately and the patch still needs rebase. What is the status here?
There hasn’t been much activity lately and the patch still needs rebase. What is the status here?
There hasn’t been much activity lately and the patch still needs rebase. What is the status here?