p2p: Allow whitelisting manual connections #27114

pull brunoerg wants to merge 7 commits into bitcoin:master from brunoerg:2023-02-outgoing-whitelist changing 41 files +199 −108
  1. brunoerg commented at 8:31 pm on February 16, 2023: contributor

    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 #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.
    • #29058 (comment) - “Before enabling -v2transport by default (which I’d image may happen after #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 #27114 makes it in.”
    • #17167 (comment) - “This would allow us to use #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.
  2. DrahtBot commented at 8:31 pm on February 16, 2023: contributor

    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.

  3. DrahtBot added the label P2P on Feb 16, 2023
  4. brunoerg commented at 8:31 pm on February 16, 2023: contributor
    cc: @furszy @theStack
  5. DrahtBot added the label Needs rebase on Feb 22, 2023
  6. brunoerg force-pushed on Feb 22, 2023
  7. DrahtBot removed the label Needs rebase on Feb 22, 2023
  8. naumenkogs commented at 12:55 pm on February 23, 2023: member
    Concept ACK
  9. in src/init.cpp:525 in 5410b13a9c outdated 
    519@@ -520,9 +520,11 @@ void SetupServerArgs(ArgsManager& argsman)
520         "Use [host]:port notation for IPv6. Allowed permissions: " + Join(NET_PERMISSIONS_DOC, ", ") + ". "
521         "Specify multiple permissions separated by commas (default: download,noban,mempool,relay). Can be specified multiple times.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
522 
523-    argsman.AddArg("-whitelist=<[permissions@]IP address or network>", "Add permission flags to the peers connecting from the given IP address (e.g. 1.2.3.4) or "
524+    argsman.AddArg("-whitelist=<[permissions@]IP address or network>", strprintf("Add permission flags to the peers using the given IP address (e.g. 1.2.3.4) or "
525         "CIDR-notated network (e.g. 1.2.3.0/24). Uses the same permissions as "
526-        "-whitebind. Can be specified multiple times." , ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
527+        "-whitebind. "
528+        "Additional flags in and out control whether permissions apply to incoming connections and/or outgoing (default: %s). "
    stratospher commented at 9:12 pm on March 10, 2023:
    why do we mention outgoing? (isn’t it incoming only)
    brunoerg commented at 1:24 pm on March 14, 2023:
    Because this PR changes it to allow outgoing as well
    jonatack commented at 6:07 pm on May 5, 2023:

    https://github.com/bitcoin/bitcoin/commit/f52fc86d0840221a5d106eec201a4dcffd7d5774 without quotes here it doesn’t seem clear that these are values to be passed

    0        "Additional flags \"in\" and \"out\" control whether permissions apply to incoming connections and/or outgoing (default: %s). "

    0$ ./src/bitcoind -h | grep -A7 "\-whitelist="
1  -whitelist=<[permissions@]IP address or network>
2       Add permission flags to the peers using the given IP address (e.g.
3       1.2.3.4) or CIDR-notated network (e.g. 1.2.3.0/24). Uses the same
4       permissions as -whitebind. Additional flags "in" and "out"
5       control whether permissions apply to incoming connections and/or
6       outgoing (default: incoming only). Can be specified multiple
7       times.
    brunoerg commented at 3:39 pm on May 8, 2023:
    Just added it to make it clearer
  10. stratospher commented at 9:17 pm on March 10, 2023: contributor

    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?
  11. furszy commented at 8:19 pm on March 13, 2023: member

    Concept ACK

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

  12. brunoerg commented at 9:13 pm on March 13, 2023: contributor

    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!

  13. brunoerg force-pushed on Mar 14, 2023
  14. brunoerg force-pushed on Mar 14, 2023
  15. brunoerg commented at 0:21 am on March 15, 2023: contributor
    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.
  16. DrahtBot added the label Needs rebase on Mar 28, 2023
  17. brunoerg force-pushed on Mar 28, 2023
  18. brunoerg commented at 5:29 pm on March 28, 2023: contributor
    Rebased
  19. DrahtBot removed the label Needs rebase on Mar 28, 2023
  20. glozow commented at 9:57 am on April 20, 2023: member
    linking #9923 perhaps also relevant: #10594, #10051
  21. in src/net.h:989 in 19f3f3a316 outdated 
    984@@ -983,7 +985,8 @@ class CConnman
985 
986     bool AttemptToEvictConnection();
987     CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex);
988-    void AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr) const;
989+    void AddWhitelistPermissionFlags(NetPermissionFlags& output_flags, const CNetAddr &addr, const std::vector<NetWhitelistPermissions>& ranges) const;
990+    static void InitializePermissionFlags(NetPermissionFlags& flags, ServiceFlags& service_flags);
    jonatack commented at 4:09 pm on May 5, 2023:

    39f6466 could be wrong, but there doesn’t seem to be any need for this to be CConnMan class member instead of a static helper function in the implementation file.

     0diff --git a/src/net.cpp b/src/net.cpp
 1index dc2fa7d2e17..7fd964b8cc3 100644
 2--- a/src/net.cpp
 3+++ b/src/net.cpp
 4@@ -360,6 +360,20 @@ bool IsLocal(const CService& addr)
 5     return mapLocalHost.count(addr) > 0;
 6 }
 7 
 8+static void InitializePermissionFlags(NetPermissionFlags& flags, ServiceFlags& service_flags) {
 9+    if (NetPermissions::HasFlag(flags, NetPermissionFlags::Implicit)) {
10+        NetPermissions::ClearFlag(flags, NetPermissionFlags::Implicit);
11+        if (gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) NetPermissions::AddFlag(flags, NetPermissionFlags::ForceRelay);
12+        if (gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY)) NetPermissions::AddFlag(flags, NetPermissionFlags::Relay);
13+        NetPermissions::AddFlag(flags, NetPermissionFlags::Mempool);
14+        NetPermissions::AddFlag(flags, NetPermissionFlags::NoBan);
15+    }
16+
17+    if (NetPermissions::HasFlag(flags, NetPermissionFlags::BloomFilter)) {
18+        service_flags = static_cast<ServiceFlags>(service_flags | NODE_BLOOM);
19+    }
20+}
21+
22 CNode* CConnman::FindNode(const CNetAddr& ip)
23 {
24     LOCK(m_nodes_mutex);
25@@ -608,20 +622,6 @@ void CConnman::AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNet
26     }
27 }
28 
29-void CConnman::InitializePermissionFlags(NetPermissionFlags& flags, ServiceFlags& service_flags) {
30-    if (NetPermissions::HasFlag(flags, NetPermissionFlags::Implicit)) {
31-        NetPermissions::ClearFlag(flags, NetPermissionFlags::Implicit);
32-        if (gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) NetPermissions::AddFlag(flags, NetPermissionFlags::ForceRelay);
33-        if (gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY)) NetPermissions::AddFlag(flags, NetPermissionFlags::Relay);
34-        NetPermissions::AddFlag(flags, NetPermissionFlags::Mempool);
35-        NetPermissions::AddFlag(flags, NetPermissionFlags::NoBan);
36-    }
37-
38-    if (NetPermissions::HasFlag(flags, NetPermissionFlags::BloomFilter)) {
39-        service_flags = static_cast<ServiceFlags>(service_flags | NODE_BLOOM);
40-    }
41-}
42-
43 CService CNode::GetAddrLocal() const
44 {
45     AssertLockNotHeld(m_addr_local_mutex);
46diff --git a/src/net.h b/src/net.h
47index 5fa8bf3fcf7..4387c5c8756 100644
48--- a/src/net.h
49+++ b/src/net.h
50@@ -987,7 +987,6 @@ private:
51     bool AttemptToEvictConnection();
52     CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex);
53     void AddWhitelistPermissionFlags(NetPermissionFlags& output_flags, const CNetAddr &addr, const std::vector<NetWhitelistPermissions>& ranges) const;
54-    static void InitializePermissionFlags(NetPermissionFlags& flags, ServiceFlags& service_flags);
55 
56     void DeleteNode(CNode* pnode);
    brunoerg commented at 3:26 pm on May 8, 2023:
    Done
  22. in src/net_permissions.cpp:101 in 19f3f3a316 outdated 
     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;
    jonatack commented at 5:10 pm on May 5, 2023:

    f52fc86 named arg, if you retouch and stay with the nullptr approach

    0    if (!TryParsePermissionFlags(str, flags, /*output_connection_direction=*/nullptr, offset, error)) return false;
    brunoerg commented at 3:26 pm on May 8, 2023:
    Done
  23. in src/net_permissions.cpp:24 in 19f3f3a316 outdated 
    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)
    jonatack commented at 5:11 pm on May 5, 2023:

    https://github.com/bitcoin/bitcoin/commit/f52fc86d0840221a5d106eec201a4dcffd7d5774 can make the function static while touching this line

    0static bool TryParsePermissionFlags(const std::string& str, NetPermissionFlags& output, ConnectionDirection* output_connection_direction, size_t& readen, bilingual_str& error)
    brunoerg commented at 3:26 pm on May 8, 2023:
    Done
  24. in src/net_permissions.cpp:59 in 19f3f3a316 outdated 
    52@@ -52,6 +53,14 @@ bool TryParsePermissionFlags(const std::string& str, NetPermissionFlags& output,
53             else if (permission == "all") NetPermissions::AddFlag(flags, NetPermissionFlags::All);
54             else if (permission == "relay") NetPermissions::AddFlag(flags, NetPermissionFlags::Relay);
55             else if (permission == "addr") NetPermissions::AddFlag(flags, NetPermissionFlags::Addr);
56+            else if (permission == "in") connection_direction |= ConnectionDirection::In;
57+            else if (permission == "out") {
58+                if (!output_connection_direction) {
59+                    error = _("whitebind is only used for incoming connections");
    jonatack commented at 5:23 pm on May 5, 2023:

    https://github.com/bitcoin/bitcoin/commit/f52fc86d0840221a5d106eec201a4dcffd7d5774

    • The following error message would be clearer, I think
    0                    error = _("whitebind may only be used for incoming connections (\"out\" was passed)");

    • My first thought was if it would make sense to pass output_connection_direction as a std::optional<ConnectionDirection>.

    • But also, relying on nullptr and overloading this param for the callee to know who the caller is (here and line 77 after) seems needlessly non-robust just to avoid a boolean param. Could either make it very clear

    0-                if (!output_connection_direction) {
1+                if (output_connection_direction == nullptr) {
2+                    // Only NetWhitebindPermissions() should pass a nullptr.

    or use an explicit is_whitebind boolean param for this – a bool is extremely cheap – and pass output_connection_direction as a ConnectionDirection, which would make the code simpler and more self-documenting. (Edit: I realize this commit was pulled in from another author, so feel free to ignore this last point.)

    brunoerg commented at 3:30 pm on May 8, 2023:

    I just changed the error message and adopted the diff (seems so much better to check whether this is a nullptr):

    0-                if (!output_connection_direction) {
1+                if (output_connection_direction == nullptr) {
2+                    // Only NetWhitebindPermissions() should pass a nullptr.
  25. in src/test/fuzz/net_permissions.cpp:37 in 19f3f3a316 outdated 
    31@@ -32,7 +32,8 @@ FUZZ_TARGET(net_permissions)
32 
33     NetWhitelistPermissions net_whitelist_permissions;
34     bilingual_str error_net_whitelist_permissions;
35-    if (NetWhitelistPermissions::TryParse(s, net_whitelist_permissions, error_net_whitelist_permissions)) {
36+    ConnectionDirection connection_direction;
37+    if (NetWhitelistPermissions::TryParse(s, net_whitelist_permissions, connection_direction, error_net_whitelist_permissions)) {
    jonatack commented at 5:41 pm on May 5, 2023:

    https://github.com/bitcoin/bitcoin/commit/f52fc86d0840221a5d106eec201a4dcffd7d5774 missing header, sort

    0 #include <net_permissions.h>
1+#include <netbase.h>
2 #include <test/fuzz/FuzzedDataProvider.h>
3@@ -31,8 +32,8 @@ FUZZ_TARGET(net_permissions)
4 
5     NetWhitelistPermissions net_whitelist_permissions;
6-    bilingual_str error_net_whitelist_permissions;
7     ConnectionDirection connection_direction;
8+    bilingual_str error_net_whitelist_permissions;
9     if (NetWhitelistPermissions::TryParse(s, net_whitelist_permissions, connection_direction, error_net_whitelist_permissions)) {
  26. in src/test/netbase_tests.cpp:447 in 19f3f3a316 outdated 
    450@@ -450,19 +451,25 @@ BOOST_AUTO_TEST_CASE(netpermissions_test)
451     BOOST_CHECK(error.original.find("Invalid P2P permission") != std::string::npos);
452 
453     // Check netmask error
454-    BOOST_CHECK(!NetWhitelistPermissions::TryParse("bloom,forcerelay,noban@1.2.3.4:32", whitelistPermissions, error));
455+    BOOST_CHECK(!NetWhitelistPermissions::TryParse("bloom,forcerelay,noban@1.2.3.4:32", whitelistPermissions, connection_direction, error));
    jonatack commented at 5:59 pm on May 5, 2023:

    https://github.com/bitcoin/bitcoin/commit/f52fc86d0840221a5d106eec201a4dcffd7d5774 Can also test the error case.

    0@@ -446,6 +446,9 @@ BOOST_AUTO_TEST_CASE(netpermissions_test)
1     BOOST_CHECK(NetWhitebindPermissions::TryParse(",,@1.2.3.4:32", whitebindPermissions, error));
2     BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, NetPermissionFlags::None);
3 
4+    BOOST_CHECK(!NetWhitebindPermissions::TryParse("out,forcerelay@1.2.3.4:32", whitebindPermissions, error));
5+    BOOST_CHECK(error.original.find("whitebind may only be used for incoming connections (\"out\" was passed)") != std::string::npos);
6+
7     // Detect invalid flag
8     BOOST_CHECK(!NetWhitebindPermissions::TryParse("bloom,forcerelay,oopsie@1.2.3.4:32", whitebindPermissions, error));
9     BOOST_CHECK(error.original.find("Invalid P2P permission") != std::string::npos);
  27. in test/functional/test_framework/test_framework.py:500 in 19f3f3a316 outdated 
    495@@ -495,6 +496,9 @@ def get_bin_from_version(version, bin_name, bin_default):
496             extra_confs = [[]] * num_nodes
497         if extra_args is None:
498             extra_args = [[]] * num_nodes
499+        if self.whitelist_peers:
    jonatack commented at 6:13 pm on May 5, 2023:
    19f3f3a A comment here for future readers documenting when/why to use this would be good.
    brunoerg commented at 3:35 pm on May 8, 2023:

    Added right before:

    0+ # Whitelist peers to speed up tx relay / mempool sync
  28. in test/functional/mempool_reorg.py:18 in 19f3f3a316 outdated 
    19-            [
20-                '-whitelist=noban@127.0.0.1',  # immediate tx relay
21-            ],
22-            []
23-        ]
24+        self.whitelist_peers = True
    jonatack commented at 6:14 pm on May 5, 2023:
    https://github.com/bitcoin/bitcoin/commit/19f3f3a316660890f03d6ddcb4a4230eb2ff5e91 This doesn’t seem to be a pure refactoring here and in mempool_packages.py and p2p_segwit.py
  29. jonatack commented at 6:24 pm on May 5, 2023: member
    ACK 19f3f3a316660890f03d6ddcb4a4230eb2ff5e91 with some suggestions
  30. brunoerg force-pushed on May 8, 2023
  31. brunoerg commented at 3:43 pm on May 8, 2023: contributor

    Force-pushed addressing @jonatack’s review.

    • Moved InitializePermissionFlags out of CConnman
    • Made TryParsePermissionFlags static
    • Added more test coverage #27114 (review)
    • Improved documentation
  32. in src/net.cpp:406 in f99912850e outdated 
    392@@ -393,6 +393,20 @@ CNode* CConnman::FindNode(const std::string& addrName)
393     return nullptr;
394 }
395 
396+static void InitializePermissionFlags(NetPermissionFlags& flags, ServiceFlags& service_flags) {
    jonatack commented at 1:26 pm on May 11, 2023:
    0a77e6ab37a9b23bd293 I would suggest placing this new helper with the other standalone ones, i.e. after IsLocal() or higher, not between the CConnman::FindNode() class members (unless there’s a reason that I didn’t see).
    brunoerg commented at 4:55 pm on May 15, 2023:
    Make sense, going to address it.
  33. in src/init.cpp:527 in f99912850e outdated 
    520@@ -521,9 +521,11 @@ void SetupServerArgs(ArgsManager& argsman)
521         "Use [host]:port notation for IPv6. Allowed permissions: " + Join(NET_PERMISSIONS_DOC, ", ") + ". "
522         "Specify multiple permissions separated by commas (default: download,noban,mempool,relay). Can be specified multiple times.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
523 
524-    argsman.AddArg("-whitelist=<[permissions@]IP address or network>", "Add permission flags to the peers connecting from the given IP address (e.g. 1.2.3.4) or "
525+    argsman.AddArg("-whitelist=<[permissions@]IP address or network>", strprintf("Add permission flags to the peers using the given IP address (e.g. 1.2.3.4) or "
526         "CIDR-notated network (e.g. 1.2.3.0/24). Uses the same permissions as "
527-        "-whitebind. Can be specified multiple times." , ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
528+        "-whitebind. "
529+        "Additional flags \"in\" and \"in\" control whether permissions apply to incoming connections and/or outgoing (default: %s). "
    jonatack commented at 1:34 pm on May 11, 2023:

    ea46500ece2

    0        "Additional flags \"in\" and \"out\" control whether permissions apply to incoming connections and/or outgoing (default: %s). "
  34. in test/functional/test_framework/test_framework.py:499 in f99912850e outdated 
    495@@ -495,6 +496,10 @@ def get_bin_from_version(version, bin_name, bin_default):
496             extra_confs = [[]] * num_nodes
497         if extra_args is None:
498             extra_args = [[]] * num_nodes
499+        # Whitelist peers to speed up tx relay / mempool sync
    jonatack commented at 1:41 pm on May 11, 2023:

    f99912850e2a50b1a

    0        # Whitelist peers to speed up tx relay / mempool sync. Don't use if testing tx relay or timing.
  35. in src/init.cpp:437 in f99912850e outdated 
    433@@ -434,7 +434,7 @@ void SetupServerArgs(ArgsManager& argsman)
434     argsman.AddArg("-blocknotify=<cmd>", "Execute command when the best block changes (%s in cmd is replaced by block hash)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
435 #endif
436     argsman.AddArg("-blockreconstructionextratxn=<n>", strprintf("Extra transactions to keep in memory for compact block reconstructions (default: %u)", DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
437-    argsman.AddArg("-blocksonly", strprintf("Whether to reject transactions from network peers. Automatic broadcast and rebroadcast of any transactions from inbound peers is disabled, unless the peer has the 'forcerelay' permission. RPC transactions are not affected. (default: %u)", DEFAULT_BLOCKSONLY), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
438+    argsman.AddArg("-blocksonly", strprintf("Whether to reject transactions from network peers. Automatic broadcast and rebroadcast of any transactions from peers is disabled, unless the peer has the 'forcerelay' permission. RPC transactions are not affected. (default: %u)", DEFAULT_BLOCKSONLY), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
    jonatack commented at 1:48 pm on May 11, 2023:
    Why this change?
    brunoerg commented at 5:08 pm on May 15, 2023:
    I think it was accidental, sorry. I will revert it.
  36. jonatack commented at 1:58 pm on May 11, 2023: member
    Approach ACK, modulo comments and #27114 (review) – it might be better for that commit to be a pure refactoring. Thanks for addressing the previous feedback.
  37. brunoerg force-pushed on May 15, 2023
  38. brunoerg commented at 5:10 pm on May 15, 2023: contributor

    Force pushed addressing:

  39. DrahtBot added the label CI failed on May 15, 2023
  40. brunoerg force-pushed on May 15, 2023
  41. DrahtBot removed the label CI failed on May 15, 2023
  42. furszy commented at 0:32 am on June 22, 2023: member
    The InitializePermissionFlags function is added twice, at two different locations, at the first and second commits, then one of them gets erased at the third one. Would be good to cleanup the history so it’s only added once.
  43. brunoerg force-pushed on Jun 22, 2023
  44. brunoerg commented at 5:11 pm on June 22, 2023: contributor

    The InitializePermissionFlags function is added twice, at two different locations, at the first and second commits, then one of them gets erased at the third one. Would be good to cleanup the history so it’s only added once.

    Fixed! Thanks, @furszy

  45. naumenkogs commented at 9:22 am on July 26, 2023: member
    ACK 1e09c265a9598df3cc39336e3bcccb993dacf3d8
  46. in test/functional/test_framework/test_framework.py:500 in 1e09c265a9 outdated 
    495@@ -495,6 +496,10 @@ def get_bin_from_version(version, bin_name, bin_default):
496             extra_confs = [[]] * num_nodes
497         if extra_args is None:
498             extra_args = [[]] * num_nodes
499+        # Whitelist peers to speed up tx relay / mempool sync. Don't use it if testing tx relay or timing.
500+        if self.whitelist_peers:
    maflcko commented at 9:30 am on July 26, 2023:
    If the options is used to speedup tx relay / mempool sync, it should be named appropriately. For example self.noban_tx_relay. Also, the option shouldn’t be set for tests that previously didn’t have it set. Otherwise they are testing something else. (noban has other effects than just immediate trickle)
    brunoerg commented at 5:07 pm on July 26, 2023:

    Also, the option shouldn’t be set for tests that previously didn’t have it set

    It’s false by default and it has been set up for tests that were already using it.

    maflcko commented at 5:48 am on July 27, 2023:

    It’s false by default and it has been set up for tests that were already using it.

    You are also setting it for tests (all nodes in the test) that only have it set on one node. Not sure about that.

    brunoerg commented at 8:58 pm on August 7, 2023:
    You’re right. I am gonna change it for tests that previously had set it for all nodes.
  47. in src/init.cpp:536 in 1e09c265a9 outdated 
    533+    argsman.AddArg("-whitelist=<[permissions@]IP address or network>", strprintf("Add permission flags to the peers using the given IP address (e.g. 1.2.3.4) or "
534         "CIDR-notated network (e.g. 1.2.3.0/24). Uses the same permissions as "
535-        "-whitebind. Can be specified multiple times." , ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
536+        "-whitebind. "
537+        "Additional flags \"in\" and \"out\" control whether permissions apply to incoming connections and/or outgoing (default: %s). "
538+        "Can be specified multiple times.", "incoming only"), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
    pinheadmz commented at 7:39 pm on July 26, 2023:
    Seems weird to use a string placeholder here for "incoming only" when there isn’t a variable
    pinheadmz commented at 7:41 pm on July 26, 2023:
    Also I think it’s a bit unclear where the “additional flags” go, are they part of [permissions@] ?
    brunoerg commented at 12:16 pm on August 7, 2023:

    Also I think it’s a bit unclear where the “additional flags” go, are they part of [permissions@] ?

    Yes, see the docs of -whitelist:

    0    argsman.AddArg("-whitelist=<[permissions@]IP address or network>", strprintf("Add permission flags to the peers using the given IP address (e.g. 1.2.3.4) or "
1        "CIDR-notated network (e.g. 1.2.3.0/24). Uses the same permissions as "
2        "-whitebind. "
3        "Additional flags \"in\" and \"out\" control whether permissions apply to incoming connections and/or outgoing (default: %s). "
4        "Can be specified multiple times.", "incoming only"), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
5
6    g_wallet_init_interface.AddWalletOptions(argsman);

    We call [permissions@] as permissions flags, so this adds additional ones.

  48. in test/functional/wallet_import_rescan.py:179 in 1e09c265a9 outdated 
    169@@ -170,7 +170,6 @@ def setup_network(self):
170         self.start_nodes(extra_args=[[]] * self.num_nodes)
171         self.import_deterministic_coinbase_privkeys()
172         self.stop_nodes()
173-
    pinheadmz commented at 7:58 pm on July 26, 2023:
    yeah? 😏
    brunoerg commented at 10:08 pm on August 7, 2023:
    Fixed
  49. in test/functional/test_framework/test_framework.py:501 in 1e09c265a9 outdated 
    495@@ -495,6 +496,10 @@ def get_bin_from_version(version, bin_name, bin_default):
496             extra_confs = [[]] * num_nodes
497         if extra_args is None:
498             extra_args = [[]] * num_nodes
499+        # Whitelist peers to speed up tx relay / mempool sync. Don't use it if testing tx relay or timing.
500+        if self.whitelist_peers:
501+            for i in range(len(extra_args)):
502+                extra_args[i] = extra_args[i] + ["-whitelist=noban,in,out@127.0.0.1"]
    pinheadmz commented at 8:14 pm on July 26, 2023:

    I’m not sure this is sufficient functional test coverage. With the exception of the new permissions “in” and “out” which are invalid on master, all tests otherwise pass with this commit (1e09c265a9598df3cc39336e3bcccb993dacf3d8) applied. I was expecting to see a test that makes an outbound connection to a node that misbehaves and doesn’t get banned.

    Or was the motivation more about the tx “trickle”? In that case I can see it being hard to test without waiting for time to pass?

    brunoerg commented at 8:00 pm on August 7, 2023:
    Yea, the motivation here is just to ‘speed up’ some existing tests. But I like the idea of “I was expecting to see a test that makes an outbound connection to a node that misbehaves and doesn’t get banned.” this kind of coverage. I’m gonna add it.
  50. pinheadmz commented at 8:23 pm on July 26, 2023: member

    concept ACK

    Ran all tests locally. Built and reviewed code at each commit. Some questions/comments below. Also I notice this is a fairly old feature request that has been through a few PRs. I wonder if you could address Greg’s comments on this older version

  51. luke-jr changes_requested
  52. luke-jr commented at 1:15 am on July 27, 2023: member
    -blocksonly docs need updating
  53. in src/net.cpp:583 in 1e09c265a9 outdated 
    567@@ -554,6 +568,11 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
568         return nullptr;
569     }
570 
571+    NetPermissionFlags permission_flags = NetPermissionFlags::None;
572+    ServiceFlags node_services = nLocalServices;
573+    AddWhitelistPermissionFlags(permission_flags, addrConnect, vWhitelistedRangeOutgoing);
574+    InitializePermissionFlags(permission_flags, node_services);
    luke-jr commented at 1:48 am on July 27, 2023:

    node_services has been discarded here, likely as an oversight trying to rebase it past 8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67.

    Best approach I see is to export it outward, ensuring callers need to handle the API change.

    brunoerg commented at 5:56 pm on August 7, 2023:
    Yea, I’m going to change it
  54. luke-jr changes_requested
  55. brunoerg force-pushed on Aug 7, 2023
  56. brunoerg commented at 10:10 pm on August 7, 2023: contributor

    Force-pushed:

    1. Renamed whitelist_peers to noban_tx_relay and set up it True for tests that explicity was whitelisting peers with this purpose of speeding up. - #27114 (review)
    2. Added test coverage for an outbound node that misbehaves and doesn’t get banned. - #27114 (review)
    3. Updated -blocksonly documentation.
    4. Changed InitializePermissionFlags to deal with https://github.com/bitcoin/bitcoin/commit/8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67 -https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1275643872
  57. luke-jr commented at 10:51 pm on August 7, 2023: member
    1c1bd6cbc6d96fff7f67186aeb57adfe0ee872c4 doesn’t build
  58. brunoerg commented at 11:02 pm on August 7, 2023: contributor

    https://github.com/bitcoin/bitcoin/commit/1c1bd6cbc6d96fff7f67186aeb57adfe0ee872c4 doesn’t build

    sorry, fixing!

  59. brunoerg force-pushed on Aug 7, 2023
  60. brunoerg commented at 11:13 pm on August 7, 2023: contributor

    https://github.com/bitcoin/bitcoin/commit/1c1bd6cbc6d96fff7f67186aeb57adfe0ee872c4 doesn’t build

    Force-pushed fixing it

  61. in src/net.cpp:462 in 166088d1e7 outdated 
    459+    ServiceFlags node_services = nLocalServices;
460 
461     if (pszDest == nullptr) {
462         if (IsLocal(addrConnect))
463-            return nullptr;
464+            return std::make_pair(nullptr, node_services);
    luke-jr commented at 11:33 pm on August 7, 2023:
    nit: Dislike scattering this all over the place. node_services doesn’t even have a meaning with nullptr. Maybe return std::optional<...> instead?
    brunoerg commented at 11:50 pm on August 7, 2023:
    Seems more elegant.
  62. luke-jr changes_requested
  63. brunoerg force-pushed on Aug 7, 2023
  64. brunoerg force-pushed on Aug 8, 2023
  65. brunoerg commented at 0:01 am on August 8, 2023: contributor
    Thanks, @luke-jr! Force-pushed changing CConnman::ConnectNode to std::optional
  66. in src/init.cpp:532 in 42ad2ea37d outdated 
    528@@ -529,9 +529,11 @@ void SetupServerArgs(ArgsManager& argsman)
529         "Use [host]:port notation for IPv6. Allowed permissions: " + Join(NET_PERMISSIONS_DOC, ", ") + ". "
530         "Specify multiple permissions separated by commas (default: download,noban,mempool,relay). Can be specified multiple times.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
531 
532-    argsman.AddArg("-whitelist=<[permissions@]IP address or network>", "Add permission flags to the peers connecting from the given IP address (e.g. 1.2.3.4) or "
533+    argsman.AddArg("-whitelist=<[permissions@]IP address or network>", strprintf("Add permission flags to the peers using the given IP address (e.g. 1.2.3.4) or "
    pinheadmz commented at 5:18 pm on August 8, 2023:
    I don’t think you need to use sprintf here? Might be left over from the incoming only thing you just updated?
    brunoerg commented at 9:17 pm on September 26, 2023:
    You’re correct! Gonna change it.
    brunoerg commented at 2:12 pm on October 3, 2023:
    done!
  67. in src/net.cpp:378 in d54e7dad75 outdated 
    371+        NetPermissions::AddFlag(flags, NetPermissionFlags::NoBan);
372+    }
373+
374+    if (NetPermissions::HasFlag(flags, NetPermissionFlags::BloomFilter)) {
375+        service_flags = static_cast<ServiceFlags>(service_flags | NODE_BLOOM);
376+    }
    pinheadmz commented at 6:04 pm on August 8, 2023:

    I might be misunderstanding something here, but I think this chunk can be moved to net_processing.cpp InitializeNode() because:

    • the CNode will already have its m_permission_flags set in ConnectNode()
    • m_permission_flags is all that’s needed to set our_services inside InitializeNode()
    • then you won’t need to use std::optional<std::pair<CNode*, ServiceFlags>> which is, hey, a real banger of a type!

    Otherwise I don’t see a reason to compute and then pass around those service flags through all those functions before they are used. (ConnectNode()->OpenNetworkConnection()->InitializeNode())

    As a side note, this looks like a lot of work to allow BLOOM for an outgoing peer, which must be a really weird edge case anyway right?

    brunoerg commented at 5:02 pm on September 27, 2023:

    m_permission_flags is all that’s needed to set our_services inside InitializeNode()

    Are you sure? I think we need to access of nLocalServices for it and we need to do it in CConnman, doesn’t it?

    pinheadmz commented at 7:14 pm on October 16, 2023:

    Hi sorry just getting back to this review. Check out my top two commits here:

    https://github.com/pinheadmz/bitcoin/commits/zip-27114

    I’m still running the tests locally and I could be totally missing something, but this was what I was thinking. I don’t think ConnectNode() needs to return a pair with the service flags, as long as we keep your other changes that pass the permission flags along inside the CNode

    brunoerg commented at 12:12 pm on October 18, 2023:
    Interesting! I’m going to do some tests before addressing it, but it sounds good and much cleaner.
    brunoerg commented at 6:53 pm on October 18, 2023:
    done!
  68. pinheadmz commented at 6:50 pm on August 8, 2023: member

    code review ACK at d54e7dad7513ab4c587456b56d3a838daa151e4d

    built and passed all tests

    However as noted below I think the approach can be simplified in regards to local services.

  69. DrahtBot requested review from naumenkogs on Aug 8, 2023
  70. luke-jr referenced this in commit 82d287e0df on Aug 16, 2023
  71. luke-jr referenced this in commit e0d6813ec3 on Aug 16, 2023
  72. luke-jr referenced this in commit 0487c4a755 on Aug 16, 2023
  73. luke-jr referenced this in commit ee974799fb on Aug 29, 2023
  74. luke-jr referenced this in commit a298689861 on Aug 29, 2023
  75. luke-jr referenced this in commit 47a92035cb on Aug 29, 2023
  76. achow101 requested review from furszy on Sep 20, 2023
  77. RandyMcMillan commented at 10:36 pm on September 27, 2023: contributor
    Concept ACK
  78. DrahtBot added the label Needs rebase on Oct 3, 2023
  79. brunoerg force-pushed on Oct 3, 2023
  80. brunoerg commented at 2:12 pm on October 3, 2023: contributor
    Rebased and addressed #27114 (review).
  81. DrahtBot removed the label Needs rebase on Oct 3, 2023
  82. brunoerg force-pushed on Oct 18, 2023
  83. brunoerg commented at 6:54 pm on October 18, 2023: contributor
    Force-pushed addressing #27114 (review). Thanks, @pinheadmz.
  84. in src/init.cpp:570 in 22e64e8ab5 outdated 
    530@@ -531,9 +531,11 @@ void SetupServerArgs(ArgsManager& argsman)
531         "Use [host]:port notation for IPv6. Allowed permissions: " + Join(NET_PERMISSIONS_DOC, ", ") + ". "
532         "Specify multiple permissions separated by commas (default: download,noban,mempool,relay). Can be specified multiple times.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
533 
534-    argsman.AddArg("-whitelist=<[permissions@]IP address or network>", "Add permission flags to the peers connecting from the given IP address (e.g. 1.2.3.4) or "
535+    argsman.AddArg("-whitelist=<[permissions@]IP address or network>", "Add permission flags to the peers using the given IP address (e.g. 1.2.3.4) or "
    pinheadmz commented at 2:22 pm on October 19, 2023:
    optional nit: I think connecting from is still better than using
    brunoerg commented at 3:46 pm on October 19, 2023:
    perhaps only “from”? “connecting from” seems inbound only.
    pinheadmz commented at 4:53 pm on October 19, 2023:
    “at” ? I dunno
    brunoerg commented at 8:02 pm on October 20, 2023:
    I’ll keep it that way for now
  85. in src/net.cpp:2879 in 22e64e8ab5 outdated 
    2914@@ -2910,9 +2915,9 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
2915         return;
2916 
2917     CNode* pnode = ConnectNode(addrConnect, pszDest, fCountFailure, conn_type, use_v2transport);
2918-
2919     if (!pnode)
2920         return;
2921+
    pinheadmz commented at 2:27 pm on October 19, 2023:
    optional nit: unnecessary whitespace change?
    pablomartin4btc commented at 10:31 pm on November 30, 2023:
    +1 (a7240eb99dc792fdf3a6f5aeb93c40c42a8cdc16)
  86. in src/net.h:1052 in 22e64e8ab5 outdated 
    1070@@ -1071,6 +1071,7 @@ class CConnman
1071         int64_t m_peer_connect_timeout = DEFAULT_PEER_CONNECT_TIMEOUT;
1072         std::vector<std::string> vSeedNodes;
1073         std::vector<NetWhitelistPermissions> vWhitelistedRange;
1074+        std::vector<NetWhitelistPermissions> vWhitelistedRangeOutgoing;
    pinheadmz commented at 2:29 pm on October 19, 2023:
    Might be nice for future devs looking at this code to rename the original vWhitelistedRange to vWhitelistedRangeIncoming, unless there’s a reason like is it ever not just for incoming connections?
    brunoerg commented at 8:00 pm on October 20, 2023:
    Done
  87. in src/net_permissions.cpp:27 in 22e64e8ab5 outdated 
    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+static bool TryParsePermissionFlags(const std::string& str, NetPermissionFlags& output, ConnectionDirection* output_connection_direction, size_t& readen, bilingual_str& error)
26 {
27     NetPermissionFlags flags = NetPermissionFlags::None;
28+    ConnectionDirection connection_direction = ConnectionDirection::None;
    pinheadmz commented at 2:32 pm on October 19, 2023:
    Does this mean that in and out are now required in conf options? If you defaulted to ::In here would that maintain backwards compatibility?
    pinheadmz commented at 2:35 pm on October 19, 2023:
    Update: oh I see the default now on line 75 below… why not set that here when you initialize the variable?
    luke-jr commented at 4:09 pm on October 19, 2023:
    It’s cleaner when/if the default is changed to both.
  88. in test/functional/test_framework/test_framework.py:99 in 38839bea82 outdated 
    95@@ -96,6 +96,7 @@ def __init__(self) -> None:
96         """Sets test framework defaults. Do not override this method. Instead, override the set_test_params() method"""
97         self.chain: str = 'regtest'
98         self.setup_clean_chain: bool = False
99+        self.noban_tx_relay: bool = False
    pinheadmz commented at 2:50 pm on October 19, 2023:

    I really like this clean up commit! Nice feature. I didn’t deeply check all the results from this git grep, but there might still be more to do? Maybe from recent test additions on master?

    0--> git grep whitelist *.py | wc -l
155
    brunoerg commented at 6:21 pm on October 20, 2023:

    there might still be more to do?

    Maybe. I’m checking it again, I only modified the tests that explicity points out that is using it to speedup tx relay. so, there are other tests using it for other purposes.

  89. pinheadmz commented at 2:53 pm on October 19, 2023: member

    code review ACK at 51618c9c8a87f3a1cd7fcb9b4f7d4da96f182138

    I left some more notes below, but recent updates are great. Looking lean and clean

  90. DrahtBot requested review from stratospher on Oct 19, 2023
  91. DrahtBot requested review from jonatack on Oct 19, 2023
  92. DrahtBot removed review request from stratospher on Oct 19, 2023
  93. DrahtBot requested review from stratospher on Oct 19, 2023
  94. pinheadmz commented at 2:54 pm on October 19, 2023: member
    oh one more thing! probably should add a release note :-)
  95. DrahtBot removed review request from stratospher on Oct 19, 2023
  96. DrahtBot requested review from stratospher on Oct 19, 2023
  97. DrahtBot removed review request from stratospher on Oct 19, 2023
  98. DrahtBot requested review from stratospher on Oct 19, 2023
  99. DrahtBot removed review request from stratospher on Oct 19, 2023
  100. DrahtBot requested review from stratospher on Oct 19, 2023
  101. DrahtBot removed review request from stratospher on Oct 19, 2023
  102. DrahtBot requested review from stratospher on Oct 19, 2023
  103. in src/net.cpp:2892 in 51618c9c8a outdated 
    2921+
2922     pnode->grantOutbound = std::move(grant_outbound);
2923 
2924-    m_msgproc->InitializeNode(*pnode, nLocalServices);
2925+    ServiceFlags nodeServices = nLocalServices;
2926+    m_msgproc->InitializeNode(*pnode, nodeServices);
    luke-jr commented at 4:14 pm on October 19, 2023:
    As nodeServices is incorrect after this call, it seems to make more sense to just leave it passing nLocalServices here and the other call site?
    brunoerg commented at 9:09 pm on January 4, 2024:

    I don’t think so because it’s expected to not modify nLocalServices. Doesn’t it?

     0/**
 1 * Services this node offers.
 2 *
 3 * This data is replicated in each Peer instance we create.
 4 *
 5 * This data is not marked const, but after being set it should not
 6 * change.
 7 *
 8 * \sa Peer::our_services
 9 */
10ServiceFlags nLocalServices;
    naumenkogs commented at 8:42 am on January 10, 2024:
    What does \sa mean?
    stratospher commented at 5:34 pm on March 6, 2024:
    a doxygen command :) https://www.doxygen.nl/manual/commands.html#cmdsa
  104. in test/functional/p2p_filter.py:100 in 51618c9c8a outdated 
     92@@ -93,9 +93,9 @@ def merkleblock_received(self, value):
 93 class FilterTest(BitcoinTestFramework):
 94     def set_test_params(self):
 95         self.num_nodes = 1
 96+        self.noban_tx_relay = True
 97         self.extra_args = [[
 98-            '-peerbloomfilters',
 99-            '-whitelist=noban@127.0.0.1',  # immediate tx relay
100+            '-peerbloomfilters'
    luke-jr commented at 4:36 pm on October 19, 2023:
    No reason to delete the commas
    brunoerg commented at 9:09 pm on January 4, 2024:
    I will address it in the next push.
  105. in test/functional/p2p_filter.py:98 in 51618c9c8a outdated 
    92@@ -93,9 +93,9 @@ def merkleblock_received(self, value):
93 class FilterTest(BitcoinTestFramework):
94     def set_test_params(self):
95         self.num_nodes = 1
96+        self.noban_tx_relay = True
    luke-jr commented at 4:39 pm on October 19, 2023:
    Maybe it should be on by default?
    brunoerg commented at 9:10 pm on January 4, 2024:
    I don’t think so because some tests rely on -whitelist for other purposes.
  106. DrahtBot removed review request from stratospher on Oct 19, 2023
  107. DrahtBot requested review from stratospher on Oct 19, 2023
  108. luke-jr referenced this in commit e11c4d433e on Oct 19, 2023
  109. luke-jr referenced this in commit c1bd439b38 on Oct 19, 2023
  110. luke-jr referenced this in commit 7ca26240f1 on Oct 19, 2023
  111. luke-jr referenced this in commit 622ae61e0b on Oct 19, 2023
  112. brunoerg referenced this in commit 31ea683b24 on Oct 20, 2023
  113. brunoerg force-pushed on Oct 20, 2023
  114. brunoerg force-pushed on Oct 20, 2023
  115. brunoerg referenced this in commit d69747ab65 on Oct 20, 2023
  116. DrahtBot added the label CI failed on Oct 20, 2023
  117. brunoerg commented at 8:04 pm on October 20, 2023: contributor

    Force-pushed:

    • added release note
    • renamed vWhitelistedRange to vWhitelistedRangeIncoming
    • addressed self.noban_tx_relay in more tests
  118. brunoerg commented at 9:23 pm on October 22, 2023: contributor
    CI failure is unrelated.
  119. pinheadmz approved
  120. pinheadmz commented at 5:25 pm on October 23, 2023: member

    ACK d69747ab656899d1014483809d22a2c85cb45d78

    Built and ran tests locally on macOS/arm. Confirmed changes since last approval were release note, renaming vWhitelistedRangeIncoming and applying the new feature to more tests.

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK d69747ab656899d1014483809d22a2c85cb45d78
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmU2rDkACgkQ5+KYS2KJ
 7yTrcqw//U4yGOXwkUE6Wh2ieFnh0Sw1bBPUSPe9V5WobchD47C4G0bR//zfVMgUH
 8GOmuqT9nbCtCuVJx4OednoXD7JkEt2PBC4NrZ3xV4uvDZQRFNBSSZpr+AK7ylHIi
 9dn0ayJMKxima0YduqLzrz+FKmwqnNmYFG3Gw7UjM3PZiWxQMsjk7Lk4QWNoPrT+2
10zOuITnkssjYd82qdzuI7w1Elb20knSGpgB+8Iu+NPrXvNGq5fRDtAm4gOzMfHVhY
117YYuDzB/id6WwMMAtEA+Y46GXWJCeowg9gJegADecpa5ghUq19ajxxU03Cxe7L0n
12zkp8DcvJbCe3TT8BHSdJnQlHAa8JRtLnswi1uj2pjYuj0IhkBq49eJCIbwOHaukz
13/g/+QMEZ363IQJaWaydaMACIZhFfxNR9M2QaCZUZN3X/thTlleMl0NyziH0RLVfD
14B6eKDQI8yhielZf1UseyoxX03/HcddPYzSF0hZmnHqkKdge1LiUmQ9+vOKfw+z/p
15d0xPALxj273Mqkzm9gK6YzaZG+U4/eO7yu92sqyrLnUNSbMHf+jtnt+o4bvq1m90
16/bwMhWopX+qpAYuEgELWJPQxg9iL65KdCrGRGTzeHHR4rtqWcoLob14EPyFuK8hX
17NBa75IGYUdBVJojjBFsQXEbBgxFPBrjg2LyIaPFcrAC34bW51JM=
18=PmQV
19-----END PGP SIGNATURE-----

    pinheadmz’s public key is on keybase

  121. DrahtBot removed the label CI failed on Oct 25, 2023
  122. in src/net.h:1051 in 622a829ca1 outdated 
    1057@@ -1058,7 +1058,8 @@ class CConnman
1058         uint64_t nMaxOutboundLimit = 0;
1059         int64_t m_peer_connect_timeout = DEFAULT_PEER_CONNECT_TIMEOUT;
1060         std::vector<std::string> vSeedNodes;
1061-        std::vector<NetWhitelistPermissions> vWhitelistedRange;
1062+        std::vector<NetWhitelistPermissions> vWhitelistedRangeIncoming;
    naumenkogs commented at 11:02 am on October 27, 2023:

    622a829ca1473814928ba32f003480b2382701a2

    what’s the benefit of having two separate vectors?

    brunoerg commented at 2:46 pm on November 3, 2023:
    You can specify different permissions for each connection direction.
    naumenkogs commented at 8:07 am on November 7, 2023:
    perhaps two separate lists (or a mapping to bool) could be added when it’s needed, and not now that it’s unused?
    brunoerg commented at 12:40 pm on November 17, 2023:
    I don’t think it’s unused because you can specify -whitelist=noban,in@ipaddr -whitelist=mempool,out@sameipaddr.
    naumenkogs commented at 9:06 am on November 24, 2023:
    good point.
    naumenkogs commented at 9:07 am on November 24, 2023:
    well, actually….you still can track it with a map<addr, bool>, no?
    brunoerg commented at 10:59 pm on November 24, 2023:
    I think so, but we would have to iterate the whole vector everytime we want to get specific permissions (only outbound or inbound)?
    naumenkogs commented at 9:05 am on January 10, 2024:
    But we don’t do this much anyway? I think the code could be easily modified for this change, you’d only have to pass addr direction to `AddWhitelistPermissionFlags. I won’t insist though.
  123. in test/functional/interface_rest.py:55 in edbc1ae33b outdated 
    51@@ -52,9 +52,7 @@ class RESTTest (BitcoinTestFramework):
52     def set_test_params(self):
53         self.num_nodes = 2
54         self.extra_args = [["-rest", "-blockfilterindex=1"], []]
55-        # whitelist peers to speed up tx relay / mempool sync
    naumenkogs commented at 11:08 am on October 27, 2023:

    edbc1ae33b0a17994331de0660b231e7f00a2bda

    I like saving a few LOC, but dropping these comments here and there… Presumably they were helpful for the reader to understand why whitelist. And noban_tx_relay doesn’t immediately mean speeding up.

    brunoerg commented at 2:30 pm on November 3, 2023:

    I agree it’s helpful, but I am not sure about having it for all occurances. Perhaps we could put in test_framework, e.g.:

     0diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
 1index 220e51df38..aeb9a2691c 100755
 2--- a/test/functional/test_framework/test_framework.py
 3+++ b/test/functional/test_framework/test_framework.py
 4@@ -97,6 +97,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
 5         """Sets test framework defaults. Do not override this method. Instead, override the set_test_params() method"""
 6         self.chain: str = 'regtest'
 7         self.setup_clean_chain: bool = False
 8+        # whitelist peers to speed up tx relay / mempool sync
 9         self.noban_tx_relay: bool = False
10         self.nodes: List[TestNode]
    naumenkogs commented at 8:10 am on November 7, 2023:
    This could yield false positives: sometimes we need just this flags for a different reason than speeding up, right? I also don’t think we can reliably expect a test code reader to figure out his answer (“why noban”) is in the test framework.
    brunoerg commented at 12:38 pm on November 17, 2023:
    Fair enough, gonna add comments.
  124. DrahtBot requested review from naumenkogs on Oct 27, 2023
  125. DrahtBot added the label Needs rebase on Nov 17, 2023
  126. brunoerg force-pushed on Nov 17, 2023
  127. brunoerg referenced this in commit a5c5dde1cc on Nov 17, 2023
  128. brunoerg commented at 12:52 pm on November 17, 2023: contributor
    Rebased
  129. DrahtBot removed the label Needs rebase on Nov 17, 2023
  130. in src/init.cpp:445 in a5c5dde1cc outdated 
    441@@ -442,7 +442,7 @@ void SetupServerArgs(ArgsManager& argsman)
442     argsman.AddArg("-blocknotify=<cmd>", "Execute command when the best block changes (%s in cmd is replaced by block hash)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
443 #endif
444     argsman.AddArg("-blockreconstructionextratxn=<n>", strprintf("Extra transactions to keep in memory for compact block reconstructions (default: %u)", DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
445-    argsman.AddArg("-blocksonly", strprintf("Whether to reject transactions from network peers. Automatic broadcast and rebroadcast of any transactions from inbound peers is disabled, unless the peer has the 'forcerelay' permission. RPC transactions are not affected. (default: %u)", DEFAULT_BLOCKSONLY), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
446+    argsman.AddArg("-blocksonly", strprintf("Whether to reject transactions from network peers. Automatic broadcast and rebroadcast of any transactions from any peer is disabled, unless it has the 'forcerelay' permission. RPC transactions are not affected. (default: %u)", DEFAULT_BLOCKSONLY), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
    jonatack commented at 5:37 pm on November 17, 2023: 
    0    argsman.AddArg("-blocksonly", strprintf("Whether to reject transactions from network peers. Disables automatic broadcast and rebroadcast of transactions, unless the source peer has the 'forcerelay' permission. RPC transactions are not affected. (default: %u)", DEFAULT_BLOCKSONLY), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
    brunoerg commented at 10:13 pm on November 27, 2023:
    Nice suggestion, just addressed it.
  131. in src/net.cpp:339 in a5c5dde1cc outdated 
    334+        if (gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) NetPermissions::AddFlag(flags, NetPermissionFlags::ForceRelay);
335+        if (gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY)) NetPermissions::AddFlag(flags, NetPermissionFlags::Relay);
336+        NetPermissions::AddFlag(flags, NetPermissionFlags::Mempool);
337+        NetPermissions::AddFlag(flags, NetPermissionFlags::NoBan);
338+    }
339+}
    jonatack commented at 8:50 pm on November 27, 2023:

    In the first commit, why not move this initialization into the NetPermissions class, where it makes sense to hold this logic.

      0diff --git a/src/net.cpp b/src/net.cpp
  1index 16771c8c428..64a1c9c30a7 100644
  2--- a/src/net.cpp
  3+++ b/src/net.cpp
  4@@ -328,16 +328,6 @@ bool IsLocal(const CService& addr)
  5     return mapLocalHost.count(addr) > 0;
  6 }
  7 
  8-static void InitializePermissionFlags(NetPermissionFlags& flags) {
  9-    if (NetPermissions::HasFlag(flags, NetPermissionFlags::Implicit)) {
 10-        NetPermissions::ClearFlag(flags, NetPermissionFlags::Implicit);
 11-        if (gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) NetPermissions::AddFlag(flags, NetPermissionFlags::ForceRelay);
 12-        if (gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY)) NetPermissions::AddFlag(flags, NetPermissionFlags::Relay);
 13-        NetPermissions::AddFlag(flags, NetPermissionFlags::Mempool);
 14-        NetPermissions::AddFlag(flags, NetPermissionFlags::NoBan);
 15-    }
 16-}
 17-
 18 CNode* CConnman::FindNode(const CNetAddr& ip)
 19 {
 20     LOCK(m_nodes_mutex);
 21@@ -1744,7 +1734,7 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
 22     int nInbound = 0;
 23 
 24     AddWhitelistPermissionFlags(permission_flags, addr);
 25-    InitializePermissionFlags(permission_flags);
 26+    NetPermissions::Initialize(permission_flags);
 27 
 28     {
 29         LOCK(m_nodes_mutex);
 30diff --git a/src/net.h b/src/net.h
 31index 36c6065b192..6c1672e7599 100644
 32--- a/src/net.h
 33+++ b/src/net.h
 34@@ -53,11 +53,6 @@ class CNode;
 35 class CScheduler;
 36 struct bilingual_str;
 37 
 38-/** Default for -whitelistrelay. */
 39-static const bool DEFAULT_WHITELISTRELAY = true;
 40-/** Default for -whitelistforcerelay. */
 41-static const bool DEFAULT_WHITELISTFORCERELAY = false;
 42-
 43 /** Time after which to disconnect, after waiting for a ping response (or inactivity). */
 44 static constexpr std::chrono::minutes TIMEOUT_INTERVAL{20};
 45 /** Run the feeler connection loop once every 2 minutes. **/
 46diff --git a/src/net_permissions.cpp b/src/net_permissions.cpp
 47index a134a552647..3ed230af0c2 100644
 48--- a/src/net_permissions.cpp
 49+++ b/src/net_permissions.cpp
 50@@ -2,6 +2,7 @@
 51 // Distributed under the MIT software license, see the accompanying
 52 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 53 
 54+#include <common/args.h>
 55 #include <common/system.h>
 56 #include <net_permissions.h>
 57 #include <netbase.h>
 58@@ -18,6 +19,21 @@ const std::vector<std::string> NET_PERMISSIONS_DOC{
 59     "addr (responses to GETADDR avoid hitting the cache and contain random records with the most up-to-date info)"
 60 };
 61 
 62+void NetPermissions::Initialize(NetPermissionFlags& flags)
 63+{
 64+    if (HasFlag(flags, NetPermissionFlags::Implicit)) {
 65+        ClearFlag(flags, NetPermissionFlags::Implicit);
 66+        if (gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) {
 67+            AddFlag(flags, NetPermissionFlags::ForceRelay);
 68+        }
 69+        if (gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY)) {
 70+            AddFlag(flags, NetPermissionFlags::Relay);
 71+        }
 72+        AddFlag(flags, NetPermissionFlags::Mempool);
 73+        AddFlag(flags, NetPermissionFlags::NoBan);
 74+    };
 75+}
 76+
 77 namespace {
 78 
 79 // Parse the following format: "perm1,perm2@xxxxxx"
 80diff --git a/src/net_permissions.h b/src/net_permissions.h
 81index b7f3bffe1c6..c82c5f0e6e5 100644
 82--- a/src/net_permissions.h
 83+++ b/src/net_permissions.h
 84@@ -15,6 +15,11 @@ struct bilingual_str;
 85 
 86 extern const std::vector<std::string> NET_PERMISSIONS_DOC;
 87 
 88+/** Default for -whitelistrelay. */
 89+constexpr bool DEFAULT_WHITELISTRELAY = true;
 90+/** Default for -whitelistforcerelay. */
 91+constexpr bool DEFAULT_WHITELISTFORCERELAY = false;
 92+
 93 enum class NetPermissionFlags : uint32_t {
 94     None = 0,
 95     // Can query bloomfilter even if -peerbloomfilters is false
 96@@ -71,6 +76,7 @@ public:
 97         using t = typename std::underlying_type<NetPermissionFlags>::type;
 98         flags = static_cast<NetPermissionFlags>(static_cast<t>(flags) & ~static_cast<t>(f));
 99     }
100+    static void Initialize(NetPermissionFlags& flags);
101 };
    brunoerg commented at 10:13 pm on November 27, 2023:
    sgtm!
  132. jonatack commented at 9:00 pm on November 27, 2023: member
    WIP review, will finish soon.
  133. brunoerg force-pushed on Nov 27, 2023
  134. brunoerg referenced this in commit e7de6d87cb on Nov 27, 2023
  135. brunoerg referenced this in commit 684fcdf8ea on Nov 27, 2023
  136. brunoerg force-pushed on Nov 27, 2023
  137. brunoerg commented at 10:14 pm on November 27, 2023: contributor

    Force-pushed addressing:

    #27114 (review) #27114 (review)

  138. in src/net_permissions.cpp:34 in 1a47109c2c outdated 
    29+        if (gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY)) {
30+            AddFlag(flags, NetPermissionFlags::Relay);
31+        }
32+        AddFlag(flags, NetPermissionFlags::Mempool);
33+        AddFlag(flags, NetPermissionFlags::NoBan);
34+    };
    furszy commented at 11:07 pm on November 27, 2023:

    In 1a47109c2cd:

    Extra “;” here

    brunoerg commented at 4:40 pm on November 28, 2023:
    done
  139. in src/net_permissions.cpp:30 in 1a47109c2c outdated 
    26+        if (gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) {
27+            AddFlag(flags, NetPermissionFlags::ForceRelay);
28+        }
29+        if (gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY)) {
30+            AddFlag(flags, NetPermissionFlags::Relay);
31+        }
    furszy commented at 11:16 pm on November 27, 2023:
    In 1a47109c: nit: could provide “-whitelistforcerelay” and “-whitelistrelay” as two booleans instead of accessing the global ArgsManager. It would help the path of decreasing the global variables usage and make the function easier to test and use in tests.
    jonatack commented at 11:56 pm on November 27, 2023:
    This would also allow dropping #include <common/args.h> in src/net_permissions.cpp.
    brunoerg commented at 2:52 pm on November 28, 2023:
    Sgtm
    brunoerg commented at 4:40 pm on November 28, 2023:
    done
  140. furszy commented at 11:32 pm on November 27, 2023: member
    left two small things. Nothing important.
  141. brunoerg referenced this in commit e0d068bb50 on Nov 28, 2023
  142. brunoerg force-pushed on Nov 28, 2023
  143. brunoerg commented at 4:40 pm on November 28, 2023: contributor
    Force-pushed addressing #27114 (review)
  144. DrahtBot added the label CI failed on Nov 29, 2023
  145. brunoerg force-pushed on Nov 29, 2023
  146. brunoerg referenced this in commit d0e3c62d41 on Nov 29, 2023
  147. brunoerg commented at 2:23 pm on November 29, 2023: contributor
    rebased and fixed issue pointed out by CI
  148. DrahtBot removed the label CI failed on Nov 29, 2023
  149. in src/net_permissions.cpp:60 in d0e3c62d41 outdated 
    67@@ -52,6 +68,15 @@ bool TryParsePermissionFlags(const std::string& str, NetPermissionFlags& output,
68             else if (permission == "all") NetPermissions::AddFlag(flags, NetPermissionFlags::All);
69             else if (permission == "relay") NetPermissions::AddFlag(flags, NetPermissionFlags::Relay);
70             else if (permission == "addr") NetPermissions::AddFlag(flags, NetPermissionFlags::Addr);
71+            else if (permission == "in") connection_direction |= ConnectionDirection::In;
72+            else if (permission == "out") {
73+                if (output_connection_direction == nullptr) {
74+                    // Only NetWhitebindPermissions() should pass a nullptr.
75+                    error = _("whitebind may only be used for incoming connections (\"out\" was passed)");
    pablomartin4btc commented at 9:16 pm on November 30, 2023:
    Due to this change, should we not update the -whitebind documentation in src/init.cpp? (and relase notes?).
    brunoerg commented at 12:46 pm on December 27, 2023:
    I don’t think so, because we’re not changing the default behaviour of -whitebind. The additional flags are specified only in -whitelist documentation.
  150. in src/init.cpp:568 in d0e3c62d41 outdated 
    535@@ -536,9 +536,11 @@ void SetupServerArgs(ArgsManager& argsman)
536         "Use [host]:port notation for IPv6. Allowed permissions: " + Join(NET_PERMISSIONS_DOC, ", ") + ". "
537         "Specify multiple permissions separated by commas (default: download,noban,mempool,relay). Can be specified multiple times.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
    pablomartin4btc commented at 9:34 pm on November 30, 2023:

    nit, if you have to re-touch and it makes sense to you:

    0        "Specify multiple permissions separated by commas (default: download,noban,mempool,relay,in). Can be specified multiple times.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
    brunoerg commented at 5:32 pm on January 2, 2024:
    Same reason: #27114 (review)
  151. in src/init.cpp:573 in d0e3c62d41 outdated 
    535@@ -536,9 +536,11 @@ void SetupServerArgs(ArgsManager& argsman)
536         "Use [host]:port notation for IPv6. Allowed permissions: " + Join(NET_PERMISSIONS_DOC, ", ") + ". "
537         "Specify multiple permissions separated by commas (default: download,noban,mempool,relay). Can be specified multiple times.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
538 
539-    argsman.AddArg("-whitelist=<[permissions@]IP address or network>", "Add permission flags to the peers connecting from the given IP address (e.g. 1.2.3.4) or "
540+    argsman.AddArg("-whitelist=<[permissions@]IP address or network>", "Add permission flags to the peers using the given IP address (e.g. 1.2.3.4) or "
541         "CIDR-notated network (e.g. 1.2.3.0/24). Uses the same permissions as "
542-        "-whitebind. Can be specified multiple times." , ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
543+        "-whitebind. "
544+        "Additional flags \"in\" and \"out\" control whether permissions apply to incoming connections and/or outgoing (default: incoming only). "
    pablomartin4btc commented at 9:39 pm on November 30, 2023:

    nit, since it was not very clear to me at first sight and I saw another comment related to this (feel free to change it if you agree with the general idea):

    0        "Additional flags \"in\" and \"out\" are part of [permissions@] and control whether permissions apply to incoming connections and/or outgoing (default: incoming only - e.g. noban,in,out@1.2.3.4). "
    brunoerg commented at 12:45 pm on December 27, 2023:

    (default: incoming only - e.g. noban,in,out@1.2.3.4)

    I think it might misconfuse. It seems the example (e.g) refers to the default behaviour.

    pablomartin4btc commented at 4:40 pm on December 29, 2023:
    Yeah, I meant to write how to pass the additional flags in the [permissions@]’s syntax format (the default behaviour should be without the out in it as noban,in@1.2.3.4, perhaps that’s confusing?). Feel free to ignore if doesn’t make sense to you.
    brunoerg commented at 5:47 pm on January 2, 2024:
    I’ll leave it as is for now.
  152. pablomartin4btc approved
  153. pablomartin4btc commented at 10:37 pm on November 30, 2023: member

    tACK d0e3c62d414d91f34bf9c0991a58139ae9365221

    Tested it with bitcoin-qt and I’m wondering if later as a follow-up we could add a feature as a “nice to have” to identify the “whitelisted” peers (and it permissions passed) in the Peers tab on the Node window (we show already the permissions@ on RPC getpeerinfo but not sure about other details on -whitelist or -whitebind and if we want to provide that info via RPC). There’s already a “related” GUI request but more generic to show the options passed on the command line.

    -whitelist=noban@109.201.168.32 -whitelist=noban,out@167.235.110.208 Screenshot from 2023-11-29 19-32-54

    -whitebind=out@1.2.3.4 Screenshot from 2023-11-30 17-16-45

    As @glozow pointed out perhaps you can link the issue #9923 related with this change as “fixes #”.

  154. DrahtBot requested review from furszy on Nov 30, 2023
  155. DrahtBot requested review from jonatack on Nov 30, 2023
  156. DrahtBot requested review from pinheadmz on Nov 30, 2023
  157. brunoerg commented at 12:20 pm on December 27, 2023: contributor

    Tested it with bitcoin-qt and I’m wondering if later as a follow-up we could add a feature as a “nice to have” to identify the “whitelisted” peers (and it permissions passed) in the Peers tab on the Node window (we show already the permissions@ on RPC getpeerinfo but not sure about other details on -whitelist or -whitebind and if we want to provide that info via RPC). There’s already a “related” GUI https://github.com/bitcoin-core/gui/issues/649 but more generic to show the options passed on the command line.

    Sure, will leave it for a follow-up.

  158. brunoerg force-pushed on Jan 2, 2024
  159. brunoerg referenced this in commit 0b8147375d on Jan 2, 2024
  160. brunoerg commented at 6:35 pm on January 2, 2024: contributor
    Force-pushed addressing #27114 (review)
  161. in src/net_permissions.cpp:21 in 0b8147375d outdated 
    17@@ -18,12 +18,28 @@ const std::vector<std::string> NET_PERMISSIONS_DOC{
18     "addr (responses to GETADDR avoid hitting the cache and contain random records with the most up-to-date info)"
19 };
20 
21+void NetPermissions::Initialize(NetPermissionFlags& flags, bool whitelist_forcerelay, bool whitelist_relay)
    luke-jr commented at 6:12 pm on January 3, 2024:

    Passing options individually like this is ugly - worse than just passing const ArgsManager& IMO. If you really want to cache it as a bool, maybe make a struct for connection permission options or something to pass instead.

    Another possibility is to just cache a single NetPermissionFlags for these.

    brunoerg commented at 10:29 am on January 5, 2024:
    I think we can avoid passing them in Initialize by setting them in AddWhitelistPermissionFlags (since it’s from CConnman).
    pablomartin4btc commented at 3:08 pm on January 5, 2024:
    I agree to move it to the CConman::AddWhitelistPermissionFlags (since the obj has both whitelist_forcerelay & whitelist_relay vars already set) and you could have something like CConman:UpdateNetPermissionsFlagswith the logic below.
    brunoerg commented at 5:21 pm on January 5, 2024:
    I will address it in the next push.
  162. brunoerg force-pushed on Jan 5, 2024
  163. brunoerg referenced this in commit eb1ed589fc on Jan 5, 2024
  164. brunoerg force-pushed on Jan 5, 2024
  165. brunoerg referenced this in commit bb4784ee22 on Jan 5, 2024
  166. brunoerg commented at 5:26 pm on January 5, 2024: contributor
    Force-pushed: Rebased and addressed #27114 (review) and #27114 (review) (I moved the -whitelistrelay and -whitelistforcerelay verification to the AddWhitelistPermissionFlags function.
  167. brunoerg referenced this in commit a0dafc1297 on Jan 5, 2024
  168. brunoerg force-pushed on Jan 5, 2024
  169. luke-jr commented at 6:27 pm on January 5, 2024: member
    Why was it rebased? Just seems to make it harder to review the changes…
  170. maflcko commented at 7:42 pm on January 5, 2024: member

    Why was it rebased? Just seems to make it harder to review the changes…

    git range-diff bitcoin-core/master a b does not care about rebases, see the docs in this repo.

  171. brunoerg referenced this in commit 72013c25ad on Jan 5, 2024
  172. brunoerg force-pushed on Jan 5, 2024
  173. brunoerg commented at 7:43 pm on January 5, 2024: contributor
    Pushed to fix CI.
  174. luke-jr commented at 7:51 pm on January 5, 2024: member

    git range-diff bitcoin-core/master a b does not care about rebases, see the docs in this repo.

    It’s also painful to review. If there’s no benefit to a rebase, better to just avoid it.

  175. brunoerg force-pushed on Jan 5, 2024
  176. brunoerg referenced this in commit 6175a2ee09 on Jan 5, 2024
  177. pablomartin4btc commented at 1:58 am on January 7, 2024: member

    addressed #27114 (comment) and #27114 (comment)

    re ACK 6175a2ee09663f7cf20a048b33e537442dfeb81d

    (nit: if you have to touch 2nd and/ or 4th commits, naming could be shorter)

  178. naumenkogs commented at 9:05 am on January 10, 2024: member
    ACK 6175a2ee09663f7cf20a048b33e537442dfeb81d
  179. DrahtBot removed review request from naumenkogs on Jan 10, 2024
  180. in test/functional/test_framework/test_framework.py:499 in e5593fc9bf outdated 
    494@@ -494,6 +495,10 @@ def get_bin_from_version(version, bin_name, bin_default):
495             extra_confs = [[]] * num_nodes
496         if extra_args is None:
497             extra_args = [[]] * num_nodes
498+        # Whitelist peers to speed up tx relay / mempool sync. Don't use it if testing tx relay or timing.
499+        if self.noban_tx_relay:
    glozow commented at 10:13 am on January 12, 2024:
    concept ack to having a TestFramework-wide setting to whitelist everybody for immediate tx relay. e5593fc9bf48b27227786e7f4145b58c849367a3 could be its own PR, as I don’t think there are any instances where we are specifically making outbound connections and need immediate tx relay.
    brunoerg commented at 8:39 pm on January 17, 2024:
    just changed the approach, thanks: #27114 (comment)
  181. in src/init.cpp:574 in 43614707c4 outdated 
    571+    argsman.AddArg("-whitelist=<[permissions@]IP address or network>", "Add permission flags to the peers using the given IP address (e.g. 1.2.3.4) or "
572         "CIDR-notated network (e.g. 1.2.3.0/24). Uses the same permissions as "
573-        "-whitebind. Can be specified multiple times." , ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
574+        "-whitebind. "
575+        "Additional flags \"in\" and \"out\" control whether permissions apply to incoming connections and/or outgoing (default: incoming only). "
576+        "Can be specified multiple times.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
    glozow commented at 10:18 am on January 12, 2024:

    43614707c4fe9ce91bb90b959574ee4dbf6f7b05

    Why is the approach to add “in” and “out” instead of just having specified permissions apply to both? Are there instances where you want to specify different permissions for the same address depending on the connection direction?

    brunoerg commented at 2:22 pm on January 12, 2024:

    Why is the approach to add “in” and “out” instead of just having specified permissions apply to both? Are there instances where you want to specify different permissions for the same address depending on the connection direction?

    For the same address especifically, idk. But since we can use -whitelist to give permissions for “all” nodes (e.g. using 0.0.0.0), it could be worth to specify the direction.

  182. glozow requested review from sr-gi on Jan 12, 2024
  183. glozow requested review from dergoegge on Jan 12, 2024
  184. glozow requested review from mzumsande on Jan 12, 2024
  185. dergoegge commented at 10:33 am on January 12, 2024: member
    I don’t see what this would be useful for, could you elaborate on the motivation?
  186. glozow commented at 10:54 am on January 12, 2024: member
    +1 Could you update the OP with motivation for this PR? I didn’t see much from the linked PRs
  187. brunoerg commented at 1:45 pm on January 12, 2024: contributor

    @dergoegge @glozow I just updated the PR description.

    cc: @sr-gi (asked same in IRC)

  188. sipa commented at 2:50 pm on January 12, 2024: member

    It appears to me that whitelisting of outbound connections really only applies to manual ones: if you have reason to specifically trust another peer or otherwise give them elevated privileges, you probably want to also try actually connecting to them.

    If so, it seems more natural to me to specify outbound net permissions wherever that manual connection is instructed (so inside -addnode, -connect=, or as argument to the addnode RPC)?

  189. mzumsande commented at 3:35 pm on January 12, 2024: contributor

    If so, it seems more natural to me to specify outbound net permissions wherever that manual connection is instructed

    That would mean adding logic / additional options for it in possibly multiple spots, and having yet another place where net permissions are defined. If that becomes too messy, another option would be to leave it in -whitelist but just not apply it to automatic outbound connections.

  190. brunoerg commented at 4:39 pm on January 12, 2024: contributor

    That would mean adding logic / additional options for it in possibly multiple spots, and having yet another place where net permissions are defined. If that becomes too messy, another option would be to leave it in -whitelist but just not apply it to automatic outbound connections.

    I agree. I think the easiest path would be not applying these permissions for the automatic outbound connections, leaving them for the manual ones.

    e.g:

     0diff --git a/src/net.cpp b/src/net.cpp
 1index 698b6c7111..5885e41496 100644
 2--- a/src/net.cpp
 3+++ b/src/net.cpp
 4@@ -520,7 +520,8 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
 5     }
 6 
 7     NetPermissionFlags permission_flags = NetPermissionFlags::None;
 8-    AddWhitelistPermissionFlags(permission_flags, addrConnect, vWhitelistedRangeOutgoing);
 9+    std::vector<NetWhitelistPermissions> whitelist_permissions = conn_type == ConnectionType::MANUAL ? vWhitelistedRangeOutgoing : std::vector<NetWhitelistPermissions>{};
10+    AddWhitelistPermissionFlags(permission_flags, addrConnect, whitelist_permissions);
11     NetPermissions::Initialize(permission_flags);
  191. DrahtBot added the label CI failed on Jan 13, 2024
  192. brunoerg referenced this in commit 1d0216ed32 on Jan 17, 2024
  193. brunoerg force-pushed on Jan 17, 2024
  194. brunoerg renamed this:
    p2p: Allow whitelisting outgoing connections
    p2p: Allow whitelisting manual connections
    on Jan 17, 2024
  195. brunoerg commented at 8:25 pm on January 17, 2024: contributor
    Thanks for the reviews! Since there is a consensus about being useful only for manual connections, I just changed the approach. The flags won’t be applied for automatic connections, only for manual ones. I updated the title and description.
  196. DrahtBot removed the label CI failed on Jan 17, 2024
  197. luke-jr commented at 4:37 pm on January 26, 2024: member
    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.
  198. mzumsande commented at 4:47 pm on January 26, 2024: contributor

    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.

  199. luke-jr commented at 6:45 pm on January 26, 2024: member
    Hmm, it’s still weird, but I guess it’s not terrible then.
  200. in src/net.h:1568 in 1d0216ed32 outdated 
    1555+    /**
1556+     * flag for adding 'relay' permission to whitelisted inbound
1557+     * peers with default permissions.
1558+     */
1559+    bool whitelist_relay;
1560+
    sr-gi commented at 8:51 pm on February 6, 2024:
    Aren’t these flags also used for manuals now?
    brunoerg commented at 12:42 pm on February 8, 2024:
    Yes, I’ll fix it.
  201. in src/net_permissions.cpp:24 in 1d0216ed32 outdated 
    17@@ -18,12 +18,22 @@ const std::vector<std::string> NET_PERMISSIONS_DOC{
18     "addr (responses to GETADDR avoid hitting the cache and contain random records with the most up-to-date info)"
19 };
20 
21+void NetPermissions::Initialize(NetPermissionFlags& flags)
22+{
23+    if (HasFlag(flags, NetPermissionFlags::Implicit)) {
24+        ClearFlag(flags, NetPermissionFlags::Implicit);
    sr-gi commented at 9:11 pm on February 6, 2024:

    Doesn’t this create an unnecessary indirection?

    NetPermissions::Initialize will call NetPermissions::ClearFlag, clear the Implicit flag, and call NetPermissions::Initialize again, which will simply return given the flag is not set anymore

    brunoerg commented at 12:50 pm on February 8, 2024:
    Where is it calling NetPermissions::Initialize again?
    sr-gi commented at 6:09 pm on February 8, 2024:
    Sorry, my bad, looking at the diff I misinterpreted the definition of initialize that is right bellow NetPermissions::ClearFlag as a call to it. Disregard this commet.
  202. in src/net.cpp:575 in 1d0216ed32 outdated 
    573+            NetPermissions::AddFlag(flags, subnet.m_flags);
574+        }
575+    }
576+    if (NetPermissions::HasFlag(flags, NetPermissionFlags::Implicit)) {
577+        if (whitelist_forcerelay) NetPermissions::AddFlag(flags, NetPermissionFlags::ForceRelay);
578+        if (whitelist_relay) NetPermissions::AddFlag(flags, NetPermissionFlags::Relay);
    sr-gi commented at 9:17 pm on February 6, 2024:
    Why are we splitting the parsing of NetPermissionFlags::Implicit between here and NetPermissions::Initialize? I find this a bit weird
    brunoerg commented at 1:45 pm on February 8, 2024:
    I think that was a result of many changes during the path. In fact, we do not need Initialize anymore. We can concentrate the logic into AddWhitelistPermissionFlags.
  203. in src/net.h:1058 in 1d0216ed32 outdated 
    1053@@ -1058,6 +1054,8 @@ class CConnman
1054         std::vector<std::string> m_specified_outgoing;
1055         std::vector<std::string> m_added_nodes;
1056         bool m_i2p_accept_incoming;
1057+        bool whitelist_forcerelay = false;
1058+        bool whitelist_relay = true;
    sr-gi commented at 9:21 pm on February 6, 2024:
    Just curious, is there a reason to initialize this if you are overwriting it during Options::Init? (i.e. is this ever being used without initializing?)
    sr-gi commented at 10:53 pm on February 6, 2024:
    Also, if kept, wouldn’t it be better to set it to the default constant defined in net_permission.h?
    brunoerg commented at 1:21 pm on February 8, 2024:
    Yes, we can call Init without setting all values in Options (e.g. connman fuzz target). I will change it to use DEFAULT_WHITELISTFORCERELAY and DEFAULT_WHITELISTRELAY.
  204. in src/net_permissions.cpp:57 in 1d0216ed32 outdated 
    61@@ -52,6 +62,15 @@ bool TryParsePermissionFlags(const std::string& str, NetPermissionFlags& output,
62             else if (permission == "all") NetPermissions::AddFlag(flags, NetPermissionFlags::All);
63             else if (permission == "relay") NetPermissions::AddFlag(flags, NetPermissionFlags::Relay);
64             else if (permission == "addr") NetPermissions::AddFlag(flags, NetPermissionFlags::Addr);
65+            else if (permission == "in") connection_direction |= ConnectionDirection::In;
66+            else if (permission == "out") {
    sr-gi commented at 10:40 pm on February 6, 2024:
    What is the expected behavior if only the direction is specified? e.g. “in@1234”
    brunoerg commented at 2:27 pm on February 8, 2024:
    Nothing should happen. Should we throw an error in this case?
    sr-gi commented at 6:13 pm on February 8, 2024:

    I guess that depends on what feedback we want to provide the user when an invalid configuration has been set (i.e. silently dropping vs being loud about it).

    This is quite similar to other misconfigurations, such as setting the same permission twice, but with the particularity that it actually cannot be applied by itself, so maybe it’s worth returning an error in this case

    brunoerg commented at 9:41 pm on February 8, 2024:
    Sounds good, I’ll address it.
    stratospher commented at 4:49 am on February 28, 2024:

    47c596b: not entirely convinced that we should throw an error here because empty permissions like ",,@1.2.3.4:32" are allowed (an example). curious to know what other people think.

    regardless, a unit test to document this scenario in netbase_tests.cpp would be nice.

    brunoerg commented at 1:19 pm on February 28, 2024:
    Since we have the all flag, I do not see how useful would be allowing passing only conn direction.
  205. in src/net_permissions.cpp:59 in 1d0216ed32 outdated 
    61@@ -52,6 +62,15 @@ bool TryParsePermissionFlags(const std::string& str, NetPermissionFlags& output,
62             else if (permission == "all") NetPermissions::AddFlag(flags, NetPermissionFlags::All);
63             else if (permission == "relay") NetPermissions::AddFlag(flags, NetPermissionFlags::Relay);
64             else if (permission == "addr") NetPermissions::AddFlag(flags, NetPermissionFlags::Addr);
65+            else if (permission == "in") connection_direction |= ConnectionDirection::In;
66+            else if (permission == "out") {
67+                if (output_connection_direction == nullptr) {
68+                    // Only NetWhitebindPermissions() should pass a nullptr.
    sr-gi commented at 10:49 pm on February 6, 2024:
    It may be worth documenting this in the method docs to prevent potential future footshots
    brunoerg commented at 5:09 pm on February 8, 2024:
    Sounds good
  206. in src/net_processing.cpp:1578 in 1d0216ed32 outdated 
    1549@@ -1550,6 +1550,11 @@ void PeerManagerImpl::InitializeNode(CNode& node, ServiceFlags our_services)
1550         m_node_states.emplace_hint(m_node_states.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(node.IsInboundConn()));
1551         assert(m_txrequest.Count(nodeid) == 0);
1552     }
1553+
1554+    if (NetPermissions::HasFlag(node.m_permission_flags, NetPermissionFlags::BloomFilter)) {
1555+        our_services = static_cast<ServiceFlags>(our_services | NODE_BLOOM);
1556+    }
1557+
    sr-gi commented at 3:39 pm on February 7, 2024:
    Any reason why this was moved from Connman::CreateNodeFromAcceptedSocket to here?
    brunoerg commented at 2:17 pm on February 8, 2024:
    Because we will need it in OpenNetworkConnection.
    sr-gi commented at 6:50 pm on February 8, 2024:
    Ohh ok, that used to only be relevant to inbound but we are now also applying it to manual
  207. in test/functional/feature_fee_estimation.py:137 in 1d0216ed32 outdated 
    131@@ -132,11 +132,13 @@ def make_tx(wallet, utxo, feerate):
132 class EstimateFeeTest(BitcoinTestFramework):
133     def set_test_params(self):
134         self.num_nodes = 3
135+        # whitelist peers to speed up tx relay / mempool sync
136+        self.noban_tx_relay = True
137         # Force fSendTrickle to true (via whitelist.noban)
    sr-gi commented at 4:08 pm on February 7, 2024:
    nit: This comment is not relevant anymore. Either replace it with the one right before self.noban_tx_relay = True or remove it
    brunoerg commented at 1:25 pm on February 8, 2024:
    Nice catch! I’ll address it.
  208. sr-gi commented at 4:23 pm on February 7, 2024: member
    Approach ACK 1d0216e
  209. in src/init.cpp:573 in 1d0216ed32 outdated 
    566@@ -567,9 +567,11 @@ void SetupServerArgs(ArgsManager& argsman)
567         "Use [host]:port notation for IPv6. Allowed permissions: " + Join(NET_PERMISSIONS_DOC, ", ") + ". "
568         "Specify multiple permissions separated by commas (default: download,noban,mempool,relay). Can be specified multiple times.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
569 
570-    argsman.AddArg("-whitelist=<[permissions@]IP address or network>", "Add permission flags to the peers connecting from the given IP address (e.g. 1.2.3.4) or "
571+    argsman.AddArg("-whitelist=<[permissions@]IP address or network>", "Add permission flags to the peers using the given IP address (e.g. 1.2.3.4) or "
572         "CIDR-notated network (e.g. 1.2.3.0/24). Uses the same permissions as "
573-        "-whitebind. Can be specified multiple times." , ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
574+        "-whitebind. "
575+        "Additional flags \"in\" and \"out\" control whether permissions apply to incoming connections and/or outgoing (default: incoming only). "
    sr-gi commented at 6:47 pm on February 8, 2024:
    nit: and/or manual outgoing
    brunoerg commented at 9:42 pm on February 8, 2024:
    Thanks, I’ll fix it.
  210. brunoerg referenced this in commit c10f528350 on Feb 9, 2024
  211. brunoerg force-pushed on Feb 9, 2024
  212. brunoerg commented at 12:58 pm on February 9, 2024: contributor

    Force-pushed addressing:

    Thanks @sr-gi for reviewing!

  213. sr-gi commented at 6:51 pm on February 9, 2024: member
    tACK c10f528
  214. DrahtBot requested review from naumenkogs on Feb 9, 2024
  215. DrahtBot requested review from pablomartin4btc on Feb 9, 2024
  216. naumenkogs commented at 9:18 am on February 20, 2024: member
    ACK c10f528350152ca9248e8c167b67993fc7321ca3
  217. DrahtBot removed review request from naumenkogs on Feb 20, 2024
  218. in src/net.cpp:1794 in c10f528350 outdated 
    1795@@ -1789,9 +1796,6 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
1796     uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize();
1797 
1798     ServiceFlags nodeServices = nLocalServices;
1799-    if (NetPermissions::HasFlag(permission_flags, NetPermissionFlags::BloomFilter)) {
1800-        nodeServices = static_cast<ServiceFlags>(nodeServices | NODE_BLOOM);
1801-    }
    pinheadmz commented at 8:59 pm on February 20, 2024:
    Now that we’re not modifying nLocalServices I don’t think we need the local variable nodeServices any more.
    brunoerg commented at 11:36 am on February 26, 2024:
    Sure, I’ll address it.
  219. in src/net.h:1064 in c10f528350 outdated 
    1059@@ -1064,6 +1060,8 @@ class CConnman
1060         std::vector<std::string> m_specified_outgoing;
1061         std::vector<std::string> m_added_nodes;
1062         bool m_i2p_accept_incoming;
1063+        bool whitelist_forcerelay = DEFAULT_WHITELISTFORCERELAY;
1064+        bool whitelist_relay = DEFAULT_WHITELISTRELAY;
    pinheadmz commented at 9:15 pm on February 20, 2024:
    nit, I think these should get the m_ prefix. When I was reviewing a7240eb99dc792fdf3a6f5aeb93c40c42a8cdc16 I thought whitelist_relay was a local variable and couldn’t tell where it was defined. Maybe just me though 🤷
  220. in doc/release-notes-27114.md:2 in c10f528350 outdated 
    0@@ -0,0 +1,2 @@
1+- Additional flags "in" and "out" have been added to `-whitelist` to control whether
2+  permissions apply to incoming connections and/or manual (default: incoming only).
    pinheadmz commented at 9:47 pm on February 20, 2024:
    Kinda funny that in/out maps to in/manual … perhaps "manual" should’ve been the permission flag?
    brunoerg commented at 12:01 pm on February 26, 2024:
    I’ll leave it as is for while since it is specified that it applies only for manual connections.
  221. pinheadmz approved
  222. pinheadmz commented at 9:49 pm on February 20, 2024: member

    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:

    0git 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
     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK c10f528350152ca9248e8c167b67993fc7321ca3
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmXVHeAACgkQ5+KYS2KJ
 7yTqDMxAApyQUw15xZWgrgO+pvNMlRKensYTKTHvPUZK8MeLpM6MfBVruWAnZwxlC
 82qj6qyAP+j7mOAOYqZqNo5fgwfD01ruNCL/bCpwAa51/hD3yPf9I/h1nA6AFNoVT
 9y9JvyknUKWHRWceNi1vBMa9lrkt5IkJ1o/padFaa52OO01OEBTf+ZJQ7FyHlFcQ2
10nttow4AIXPM37mYFLil9DGOc90qn728fICC6QrUDXjLL2AGFCPJfysl4vv5FwX6E
1192txS6FDh5P8z54DxTOiFSlTVemwOtmSZAtM5Kvs7tgeAiBlOmD9vsMC9BUwRs+c
127Swn9MUiIZ+kveh6oS7SGAQv3uj4kOyhyOPV3iKJKylEaFoivUN2YMtdhNUwUC8i
13bz8p5nbkO397HvHuCQ3mEUcWfYeTXS2ozZfRcbvTXqnZMb5QCuM7CPkH+w5QG/hi
14yGQDp+SbqDFLiEWejmFysIKhDeXuz110jhydgr77UuiCcTVjPTUCvrEw0hZ/PkZi
15ev/tnL0xsC2Nm3gnsjY5ilE+kbkjEerf19qgJ45lweMI2pvT+BNSWxFYxSNbQpYm
16ayYcnsYfHbHdoPlLJoTUO44yOyWSdsqu6tLBdYYuH1nDkoijaTl27O9SI5Cxe2E0
179BdBUPu2c9STB1VZzZLNOTiRgKyXq/rYloSyksiI6Jd4HK48zGI=
18=rbZH
19-----END PGP SIGNATURE-----

    pinheadmz’s public key is on keybase

  223. brunoerg referenced this in commit 231e179702 on Feb 26, 2024
  224. brunoerg force-pushed on Feb 26, 2024
  225. brunoerg commented at 12:00 pm on February 26, 2024: contributor

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

    cc: @naumenkogs @sr-gi @pinheadmz

  226. pinheadmz approved
  227. pinheadmz commented at 4:14 pm on February 26, 2024: member

    ACK 231e1797023c3502e1605b263b24f29eb659ce55

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 231e1797023c3502e1605b263b24f29eb659ce55
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmXcuNkACgkQ5+KYS2KJ
 7yTq5oxAAyNHLUhXS4t6pe+5obxZMLEleSnZvMbq7ILrxCeNc0ke7KQKmy/sMFOT9
 85pBN8Pc3e/LLtxlx+rNuspiooGqBKz3tRV0fEdAdBt4gQhoyaA7+KuFxfOZXjM5z
 9TnimlvgAxZk1bI8RWP/Pd/hvd5rMqWBKu5SLtyycVnZ+lQCObtAyMQVart9KjLpw
10ORTCUPJgSZjc3OqcFvjyKkxKkZEQgmqd67h4j3+k6AoKRsJwauqPZMALYVtLFPb0
11T7xf/6BPk51cyVhflNzv7J3MIOQE4ojlLvPRi+Q3bRZJCoEuPYtfzA8NWz/bKS8o
12M4jMuHanvMiLUbBRWWYZ1nv1AlgB9PBcSODi0iU1C3ZBppAb3uG+AYR3xP0NVnBF
13F2uheUGrKKsGgimdUFg8sjSTjwEZhAJOq0AwN/D1subNOfJl6jCMo3sF9NnECFBn
14plvZoyU3rJSfdfukQOYeF7Q+xF/GxpUUZVSWKtgenpCoc4KvM6c+3aclfDmFKe1s
15jgMal6sXkEGjjWIHeCs/fsdMg2PWK8iQqX0iuTDYc1RsD1XjIwiYdgO3oy5oDVjm
16M7eDUd04ISsqFVDyY6j/F+jxKFZT1cZxzOj2ACZJegaWUW/xQrVBZ1CD64twtT7p
17Ys8svqCF8JiKes0LSWZSG8aHEVz4yx53FukltCljdtbd3CBCM0g=
18=yWJM
19-----END PGP SIGNATURE-----

    pinheadmz’s public key is on keybase

  228. DrahtBot requested review from naumenkogs on Feb 26, 2024
  229. DrahtBot requested review from sr-gi on Feb 26, 2024
  230. in src/init.cpp:1778 in 5883a8911a outdated 
    1774@@ -1775,6 +1775,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1775     connOptions.m_added_nodes = args.GetArgs("-addnode");
1776     connOptions.nMaxOutboundLimit = *opt_max_upload;
1777     connOptions.m_peer_connect_timeout = peer_connect_timeout;
1778+    connOptions.whitelist_forcerelay = gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY);
    stratospher commented at 11:49 am on February 27, 2024:
    5883a89: args instead of gArgs here and below?
    brunoerg commented at 1:38 pm on February 28, 2024:
    Nice find! Done!
  231. in src/net_permissions.h:79 in 1504226002 outdated 
    75@@ -71,6 +76,7 @@ class NetPermissions
76         using t = typename std::underlying_type<NetPermissionFlags>::type;
77         flags = static_cast<NetPermissionFlags>(static_cast<t>(flags) & ~static_cast<t>(f));
78     }
79+    static void Initialize(NetPermissionFlags& flags);
    stratospher commented at 2:53 am on February 28, 2024:
    1504226: leftover from old code :)
    brunoerg commented at 1:38 pm on February 28, 2024:
    Done
  232. DrahtBot requested review from stratospher on Feb 28, 2024
  233. net: store `-whitelist{force}relay` values in `CConnman` 2863d7dddb
  234. net: Move `NetPermissionFlags::Implicit` verification to `AddWhitelistPermissionFlags` 9133fd69a5
  235. net_processing: Move extra service flag into InitializeNode 8e06be347c
  236. Accept "in" and "out" flags to -whitelist to allow whitelisting manual connections 66bc6e2d17
  237. test: add option to speed up tx relay/mempool sync 
    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.
    c985eb854c
  238. test: add coverage for whitelisting manual connections e6b8f19de9
  239. docs: add release notes for #27114 0a533613fb
  240. brunoerg force-pushed on Feb 28, 2024
  241. brunoerg commented at 1:39 pm on February 28, 2024: contributor

    Force-pushed addressing:

    thanks, @stratospher

  242. sr-gi commented at 8:11 pm on February 28, 2024: member
    re-ACK 0a53361
  243. DrahtBot requested review from pinheadmz on Feb 28, 2024
  244. pinheadmz approved
  245. pinheadmz commented at 9:06 pm on February 29, 2024: member

    ACK 0a533613fb44207053796fd01a9f4b523a3153d4

    confirmed minimal diff since last ack

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 0a533613fb44207053796fd01a9f4b523a3153d4
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmXg8cMACgkQ5+KYS2KJ
 7yTqb9BAAlsYlKQ6Umtb5FVctkQebcoO7F4P0cHseOl97Xlkcr+slqDGDnYNs19ft
 86Ct9HJkB/faLzOQ2gyGq3cOxMevkuQEWBxAKti54uoHY5X5Ye51W5ejJz3BLT+Sw
 9pDhwlpXo2hSz7GhgpUeb6mj0+rHyA3pMXNvFGjcrjHCX4AeJdWLZEcau0avZ+Luj
105cl0cDxNztu6xWCSAdoooEDyHcpXifYafPOkMV2L6qefh94rZuHuc7gmCnff5geB
11lb5DZJQm6lJiLLQBHQ6UUw5hLx4Myfn0Vdb3h937amiwZ+2RsTEHTT1RoZ0lT1Q4
12qi/ehtXq+iLcqAA2FzHaiTc1IgZzG3ylJQMGov5rQ2irIpYBKyzPBgZcSloS49oK
13lbMB3a3onyABtmmyCQGLG/ituxp8n1/MawyANaQj+uJF7aJrVoylkNctm7J5gb5T
146YYeYVEV/Dav+OqeJj+LZr+4zxvYHndTgrE7MgTZsGMoam/A7jzjZdS3Y0nceCIZ
15Lm2d57+nzar/36pp2zzfS6yTjdIgK1NxGEYDLDSBSXdWV3YVaCbGezMzghAc2Wb1
16l5JHH+SFvXhOY5WaciuhOUWF7wq726WtaNUeUGldIGHqJB0QrkJOB1IUqpkbaCOU
17d4g4ePAXMkxIeo+m9QXwARLX9iFHr4AkW+qsytQoS3OgMQIQpdQ=
18=N5cR
19-----END PGP SIGNATURE-----

    pinheadmz’s public key is on keybase

  246. in test/functional/mempool_packages.py:31 in c985eb854c outdated 
    26@@ -27,10 +27,11 @@
27 class MempoolPackagesTest(BitcoinTestFramework):
28     def set_test_params(self):
29         self.num_nodes = 2
30+        # whitelist peers to speed up tx relay / mempool sync
31+        self.noban_tx_relay = True
    stratospher commented at 11:34 am on March 6, 2024:

    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.)

    brunoerg commented at 3:26 pm on March 12, 2024:
    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.
  247. stratospher commented at 5:32 pm on March 6, 2024: contributor

    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 (review), #27114 (review))

  248. DrahtBot requested review from stratospher on Mar 6, 2024
  249. achow101 commented at 4:50 pm on March 12, 2024: member
    ACK 0a533613fb44207053796fd01a9f4b523a3153d4
  250. achow101 merged this on Mar 12, 2024
  251. achow101 closed this on Mar 12, 2024
  252. kevkevinpal referenced this in commit c8589c0879 on Mar 13, 2024
  253. kevkevinpal referenced this in commit 9860a035fd on Mar 13, 2024
  254. sr-gi referenced this in commit f8c54c8dee on Mar 25, 2024
  255. Retropex referenced this in commit 7076169c8c on Mar 28, 2024
  256. Retropex referenced this in commit b99db205c4 on Mar 28, 2024
  257. Retropex referenced this in commit 66d5aa3e1d on Mar 28, 2024
  258. Retropex referenced this in commit 96d715f180 on Mar 28, 2024
  259. Retropex referenced this in commit 8d290f4c57 on Mar 28, 2024
  260. Retropex referenced this in commit f33cd8869d on Mar 28, 2024
  261. theStack referenced this in commit 93fae5ae7c on Apr 6, 2024
  262. fanquake referenced this in commit f0794cbd40 on Apr 7, 2024
  263. in src/net_permissions.cpp:77 in 66bc6e2d17 outdated 
    70@@ -61,7 +71,16 @@ bool TryParsePermissionFlags(const std::string& str, NetPermissionFlags& output,
71         readen++;
72     }
73 
74+    // By default, whitelist only applies to incoming connections
75+    if (connection_direction == ConnectionDirection::None) {
76+        connection_direction = ConnectionDirection::In;
77+    } else if (flags == NetPermissionFlags::None) {
    sr-gi commented at 8:26 am on April 10, 2024:

    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

  264. PastaPastaPasta referenced this in commit 38237e84eb on Oct 25, 2024
  265. Fabcien referenced this in commit 0e905bbfc0 on Nov 12, 2024
  266. Fabcien referenced this in commit 7fbc409146 on Nov 12, 2024
  267. roqqit referenced this in commit dd83517b4c on Nov 21, 2024
  268. roqqit referenced this in commit e9574980a0 on Nov 21, 2024
  269. luke-jr referenced this in commit dbbb11e560 on Feb 22, 2025
  270. bitcoin locked this on Apr 10, 2025


brunoerg DrahtBot naumenkogs stratospher jonatack furszy glozow maflcko pinheadmz luke-jr RandyMcMillan pablomartin4btc dergoegge sipa mzumsande sr-gi achow101


jonatack furszy naumenkogs stratospher mzumsande pablomartin4btc

Labels
P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-05-19 00:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me