p2p, rpc: Manual block-relay-only connections with addnode #24170

mzumsande commented at 8:32 PM on January 26, 2022: contributor

This implements the suggestion from #23763 to introduce an option to establish block-relay-connections manually with the -addnode RPC. Adding these can make sense for a node operator that wants to be connected to a anonymity networks like Tor or I2P, but also wants to have additional protection against eclipse attacks: Following the best chain can be more of an issue on anonymity networks because these are smaller and it can be easier to create a lot of sybil nodes there.

In that situation, manual block-relay-only connections to peers on clearnet networks can help us staying connected to the best chain, but in contrast to normal manual connections, transactions and addresses aren't transmitted on these links - in particular not our own address or transaction from our wallet. This increases privacy and will also make it harder to perform fingerprinting attacks (connecting our identities over different networks).

Manual Block-Relay connections:

can be specified with -addnode RPC, both with the add and onetry command
can be specified with the -addnode bitcoind arg (or in bitcoin.conf) with <IP>=manual-block-relay
don't participate in transaction and address relay
don't get discouraged / punished for misbehavior (but will still get disconnected for sending TX/tx-INV as automatic block-relay-only peers do)
are not subject to outbound eviction logic (unlike automatic block-relay-only-peers)

mzumsande marked this as a draft on Jan 26, 2022

mzumsande force-pushed on Jan 26, 2022

DrahtBot added the label GUI on Jan 26, 2022

DrahtBot added the label P2P on Jan 26, 2022

DrahtBot added the label RPC/REST/ZMQ on Jan 26, 2022

DrahtBot commented at 11:08 PM on January 26, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	sipa, brunoerg
Stale ACK	jnewbery, willcl-ark, aureleoules, rajarshimaitra, LarryRuane

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:

#27509 (Relay own transactions only via short-lived Tor or I2P connections by vasild)
#27467 (p2p: skip netgroup diversity follow-up by jonatack)
#27385 (net, refactor: extract Network and BIP155Network logic to node/network by jonatack)
#26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)
#26366 (p2p, rpc, test: addnode improv + add test coverage for invalid command by brunoerg)
#25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
#25268 (refactor: Introduce EvictionManager by dergoegge)

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.

in src/net_processing.cpp:1276 in f2fc01fd5e outdated

1272 | @@ -1273,7 +1273,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node)
1273 |      }
1274 |      } // cs_main
1275 |      if (node.fSuccessfullyConnected && misbehavior == 0 &&
1276 | -        !node.IsBlockOnlyConn() && !node.IsInboundConn()) {
1277 | +        !node.IsBlockOnlyConn() && !node.IsManualBlockOnlyConn() && !node.IsInboundConn()) {

luke-jr commented at 3:06 AM on January 31, 2022:

IsBlockOnlyConn and IsManualConn (below) should just return true for MANUAL_BLOCK_RELAY rather than additional checks every place

mzumsande commented at 2:15 PM on January 31, 2022:

I didn't do that because IsBlockOnlyConn() is used in several places (e.g. PeerManagerImpl::EvictExtraOutboundPeers, CConnman::GetExtraBlockRelayCount(), GetCurrentBlockRelayOnlyConns()) which would not apply to manual block-relay-only connections.

in src/rpc/net.cpp:281 in f2fc01fd5e outdated

 277 | @@ -278,6 +278,7 @@ static RPCHelpMan addnode()
 278 |                  {
 279 |                      {"node", RPCArg::Type::STR, RPCArg::Optional::NO, "The node (see getpeerinfo for nodes)"},
 280 |                      {"command", RPCArg::Type::STR, RPCArg::Optional::NO, "'add' to add a node to the list, 'remove' to remove a node from the list, 'onetry' to try a connection to the node once"},
 281 | +                    {"block_relay_only", RPCArg::Type::BOOL, RPCArg::Default{false}, "If set, treat as block-relay-only connection (deactivating transaction and addr relay on this connection)."},

luke-jr commented at 3:07 AM on January 31, 2022:

This should be a connection_type String param, not a Boolean (see also #20551).

(Throw an exception if an unsupported value is passed, for this PR)

mzumsande commented at 10:53 PM on April 13, 2022:

Done with the latest push.

in src/net.h:906 in f2fc01fd5e outdated

 902 | @@ -877,7 +903,7 @@ class CConnman
 903 |      // Count the number of block-relay-only peers we have over our limit.
 904 |      int GetExtraBlockRelayCount() const;
 905 |  
 906 | -    bool AddNode(const std::string& node);
 907 | +    bool AddNode(const std::string& node, bool block_relay_only);

luke-jr commented at 3:08 AM on January 31, 2022:

Rather use ConnectionType here

mzumsande commented at 10:54 PM on April 13, 2022:

done

luke-jr changes_requested

mzumsande commented at 2:17 PM on January 31, 2022: contributor

I'd be really interested in opinions whether the use case (as outlined in OP and linked issue) is important enough to justify the added complexity of adding another connection type (and also adding more complexity to -addnode).

DrahtBot added the label Needs rebase on Feb 4, 2022

mzumsande force-pushed on Apr 13, 2022

mzumsande commented at 10:55 PM on April 13, 2022: contributor

Rebased and addressed feedback.

mzumsande force-pushed on Apr 13, 2022

DrahtBot removed the label Needs rebase on Apr 14, 2022

mzumsande force-pushed on Apr 17, 2022

mzumsande marked this as ready for review on Apr 17, 2022

in src/net.h:1132 in 99154c5c31 outdated

1113 | @@ -1087,7 +1114,7 @@ class CConnman
1114 |      AddrMan& addrman;
1115 |      std::deque<std::string> m_addr_fetches GUARDED_BY(m_addr_fetches_mutex);
1116 |      Mutex m_addr_fetches_mutex;
1117 | -    std::vector<std::string> m_added_nodes GUARDED_BY(m_added_nodes_mutex);
1118 | +    std::vector<AddedNodeEntry> m_added_nodes GUARDED_BY(m_added_nodes_mutex);

www222fff commented at 2:31 AM on April 28, 2022:

Why not try std::map<std::string, ConnectionType> m_added_nodes, then we can avoid to define AddedNodeEntry?

mzumsande commented at 5:20 PM on May 2, 2022:

Hmm, I'm a bit undecided about that: I think avoiding a struct is not really a good argument for switching form vector to map: We would need to switch to a pair or struct if we'd ever need to add another member, plus there is a case for introducing a struct even if we'd switch to a map, as was recently done in PruneLocks example. Performance-wise, this doesn't seem important at all since the container cannot have more than 8 entries (MAX_ADDNODE_CONNECTIONS).

in src/net_processing.cpp:1233 in 99154c5c31 outdated

1229 | @@ -1230,7 +1230,7 @@ void PeerManagerImpl::InitializeNode(CNode *pnode)
1230 |          mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(pnode->IsInboundConn()));
1231 |          assert(m_txrequest.Count(nodeid) == 0);
1232 |      }
1233 | -    PeerRef peer = std::make_shared<Peer>(nodeid, /*tx_relay=*/ !pnode->IsBlockOnlyConn());
1234 | +    PeerRef peer = std::make_shared<Peer>(nodeid, /*tx_relay=*/!pnode->IsBlockOnlyConn() && !pnode->IsManualBlockRelayConn());

www222fff commented at 2:33 AM on April 28, 2022:

Why not directly change IsBlockOnlyConn() directly as below, then can avoid to define new function IsManualBlockRelayConn()?

 bool IsBlockOnlyConn() const {

   return m_conn_type == ConnectionType::BLOCK_RELAY;

   return m_conn_type == ConnectionType::BLOCK_RELAY || m_conn_type == ConnectionType::MANUAL_BLOCK_RELAY;

}

mzumsande commented at 9:20 AM on April 28, 2022:

See #24170 (review) - there are several placed that use IsBlockOnlyConn() with logic specific to automatic block-relay-only connections that have logic that does not apply to manual ones. So if I did that, we'd need additional logic in these places to distinguish between manual and automatic connections.

luke-jr commented at 7:13 PM on April 29, 2022:

That seems preferable?

mzumsande commented at 5:09 PM on May 2, 2022:

I have changed the logic now such that IsBlockRelayConn is used when both connection types are meant, where as IsManualBlockRelayConn and IsAutomaticBlockRelayConn are used in spots where only one is meant.

in src/net.cpp:2209 in 99154c5c31 outdated

2205 | @@ -2202,9 +2206,9 @@ std::vector<AddedNodeInfo> CConnman::GetAddedNodeInfo() const
2206 |          }
2207 |      }
2208 |  
2209 | -    for (const std::string& strAddNode : lAddresses) {
2210 | -        CService service(LookupNumeric(strAddNode, Params().GetDefaultPort(strAddNode)));
2211 | -        AddedNodeInfo addedNode{strAddNode, CService(), false, false};
2212 | +    for (auto nodes_to_add : lAddresses) {

jnewbery commented at 8:28 AM on April 29, 2022:

    for (auto node_to_add : lAddresses) {

(since you're iterating over the items individually, node_to_add is singular)

mzumsande commented at 10:21 PM on May 1, 2022:

done

in src/net.cpp:2211 in 99154c5c31 outdated

2205 | @@ -2202,9 +2206,9 @@ std::vector<AddedNodeInfo> CConnman::GetAddedNodeInfo() const
2206 |          }
2207 |      }
2208 |  
2209 | -    for (const std::string& strAddNode : lAddresses) {
2210 | -        CService service(LookupNumeric(strAddNode, Params().GetDefaultPort(strAddNode)));
2211 | -        AddedNodeInfo addedNode{strAddNode, CService(), false, false};
2212 | +    for (auto nodes_to_add : lAddresses) {
2213 | +        CService service(LookupNumeric(nodes_to_add.m_node_address, Params().GetDefaultPort(nodes_to_add.m_node_address)));
2214 | +        AddedNodeInfo addedNode{nodes_to_add.m_node_address, CService(), false, false, nodes_to_add.conn_type};

jnewbery commented at 8:28 AM on April 29, 2022:

Since you're touching this line, can you comment the boolean arguments?

mzumsande commented at 10:21 PM on May 1, 2022:

done

in src/net.h:487 in 99154c5c31 outdated

 540 | @@ -523,10 +541,16 @@ class CNode
 541 |          return m_conn_type == ConnectionType::INBOUND;
 542 |      }
 543 |  
 544 | +    bool IsManualBlockRelayConn() const

jnewbery commented at 8:32 AM on April 29, 2022:

Since you're adding IsManualBlockRelayConn(), consider renaming IsBlockOnlyConn() to IsAutomaticBlockRelayConn(), otherwise that function name is misleading (it returns false for connections that are block relay only connections but were created manually).

mzumsande commented at 5:10 PM on May 2, 2022:

Done, see also #24170 (review).

in src/net.h:206 in 99154c5c31 outdated

 200 | +    bool fConnected;
 201 | +    bool fInbound;
 202 | +    ConnectionType conn_type;
 203 | +};
 204 | +
 205 | +struct AddedNodeEntry {

jnewbery commented at 8:34 AM on April 29, 2022:

I know that this is maybe a bit of a stretch for this PR, but I really don't like the addnode/addednode terminology, and would much prefer this to be named something like ManualConnectionEntry, which much more precisely describes the nature of these connections.

mzumsande commented at 10:31 PM on May 1, 2022:

The downside of ManualConnectionEntry is that it is not a connection, just a name of a node we'll try to connect to (which may succeed or not). I didn't really like the name AddedNodeEntry either though, but couldn't think of anything better. Maybe ManualNodeEntry?

jnewbery commented at 9:13 AM on May 3, 2022:

Or maybe ManualConnectionTarget or DesiredManualConnection? I'm not sure they're great either.

in src/net.h:207 in 99154c5c31 outdated

 202 | +    ConnectionType conn_type;
 203 | +};
 204 | +
 205 | +struct AddedNodeEntry {
 206 | +    std::string m_node_address;
 207 | +    ConnectionType conn_type;

jnewbery commented at 8:35 AM on April 29, 2022:

Be consistent with the m_ prefix (either m_node_address and m_conn_type or node_address and conn_type). I prefer naming all members of structs with the m_ prefix, but opinions vary.

mzumsande commented at 10:22 PM on May 1, 2022:

changed to m_conn_type

in src/rpc/net.cpp:301 in 99154c5c31 outdated

 293 | @@ -293,6 +294,20 @@ static RPCHelpMan addnode()
 294 |          throw std::runtime_error(
 295 |              self.ToString());
 296 |      }
 297 | +    std::string connection_type_name = "manual";
 298 | +    if (!request.params[2].isNull()) {
 299 | +        connection_type_name = request.params[2].get_str();
 300 | +    }
 301 | +    if (connection_type_name != "manual" && connection_type_name != "manual-block-relay") {

jnewbery commented at 8:46 AM on April 29, 2022:

Is it possible to use ConnectionTypeAsString() here to avoid defining these strings in multiple places in the source?

mzumsande commented at 10:23 PM on May 1, 2022:

Good idea! Used ConnectionTypeAsString() here and in the last commit (also rearranged the rpc processing code a bit).

jnewbery commented at 8:54 AM on April 29, 2022: contributor

Concept ACK. This seems like a reasonable feature request.

I've left a few comments, mostly around naming.

I wonder if in future, we could change the net/net_processing responsibilities such that net_processing is unaware of whether a connection is manual or not. The only places that net_processing uses that information are in MaybeDiscourageAndDisconnect() (before this PR), and EvictExtraOutboundPeers() (in this PR). If disconnection and eviction logic is moved out of net_processing (cc @cfields), then net_processing won't need to be aware of the difference between manual and automatic connections.

in src/init.cpp:469 in 99154c5c31 outdated

 439 | @@ -440,7 +440,7 @@ void SetupServerArgs(ArgsManager& argsman)
 440 |                   " If <type> is not supplied or if <type> = 1, indexes for all known types are enabled.",
 441 |                   ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 442 |  
 443 | -    argsman.AddArg("-addnode=<ip>", strprintf("Add a node to connect to and attempt to keep the connection open (see the addnode RPC help for more info). This option can be specified multiple times to add multiple nodes; connections are limited to %u at a time and are counted separately from the -maxconnections limit.", MAX_ADDNODE_CONNECTIONS), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
 444 | +    argsman.AddArg("-addnode=<ip>[=manual-block-relay]", strprintf("Add a node to connect to and attempt to keep the connection open (see the addnode RPC help for more info). This option can be specified multiple times to add multiple nodes; connections are limited to %u at a time and are counted separately from the -maxconnections limit. Append =manual-block-relay to disable transaction and address relay.", MAX_ADDNODE_CONNECTIONS), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);

luke-jr commented at 7:14 PM on April 29, 2022:

Prefer =blocksonly to be clearer and match the existing -blocksonly option.

jnewbery commented at 9:37 PM on April 29, 2022:

-blocksonly mode is different from block relay peers.

luke-jr commented at 11:10 PM on April 29, 2022:

How so? Maybe it needs better docs on the difference...

jnewbery commented at 7:07 PM on April 30, 2022:

Here you go: https://bitcoin.stackexchange.com/questions/112828/what-is-a-block-relay-only-connection-what-is-it-used-for

luke-jr commented at 7:22 PM on April 30, 2022:

That doesn't contrast.

mzumsande commented at 8:30 PM on April 30, 2022:

Some differences:

-blocksonly is a global startup option (applying to all connections) -block-relay-only is connection-specific
A node in -blocksonly mode participates in address relay, we never send out addresses on -block-relay-only connections (and ignore incoming ones).
A node in -blocksonly signals not to receive any transactions (fRelay) and will disconnect if that is breached, but it may in special cases send out transactions itself (if submitted via RPC). We'll never send a tx over a block-relay-only connection.
A node in -blocksonly mode still has two of their outbound connections as -block-relay-only (for which then the block-relay-only rules apply).

luke-jr changes_requested

in src/net.cpp:2585 in 99154c5c31 outdated

2821 |  {
2822 |      LOCK(m_added_nodes_mutex);
2823 | -    for (const std::string& it : m_added_nodes) {
2824 | -        if (strNode == it) return false;
2825 | +    for (const auto& it : m_added_nodes) {
2826 | +        if (strNode == it.m_node_address) return false;

www222fff commented at 10:14 PM on April 29, 2022:

Supposed option modify case need not be handled? If add node firstly with setting manual and then setting manual block relay.

mzumsande commented at 9:56 PM on May 1, 2022:

No, a modify option is not supported, especially since it is not possible to change the connection connection type of an existing connection. A user could instead use the 'remove' option and then re-add with another connection type (which would not change existing connections though).

willcl-ark commented at 1:29 PM on January 16, 2023:

It might be an option to use std::any_of here if using standard library algorithms is preferable, but personally I think it might end up a little less readable.

in src/net.cpp:1082 in 99154c5c31 outdated

1259 | @@ -1258,6 +1260,7 @@ bool CConnman::AddConnection(const std::string& address, ConnectionType conn_typ
1260 |      switch (conn_type) {
1261 |      case ConnectionType::INBOUND:
1262 |      case ConnectionType::MANUAL:
1263 | +    case ConnectionType::MANUAL_BLOCK_RELAY:

www222fff commented at 10:16 PM on April 29, 2022:

Addconnection rpc should not support manual block relay, so seems here need not check?

mzumsande commented at 9:52 PM on May 1, 2022:

It's true that it doesn't support this, just as it doesn't support INBOUND or MANUAL. But leaving it out her would result in a compiler warning for the switch, see comment below: // no default case, so the compiler can warn about missing cases

mzumsande force-pushed on May 1, 2022

mzumsande force-pushed on May 2, 2022

mzumsande commented at 5:24 PM on May 2, 2022: contributor

Thanks for the reviews @www222fff @luke-jr @jnewbery I pushed an update, addressing the feedback.

I wonder if in future, we could change the net/net_processing responsibilities such that net_processing is unaware of whether a connection is manual or not. The only places that net_processing uses that information are in MaybeDiscourageAndDisconnect() (before this PR), and EvictExtraOutboundPeers() (in this PR). If disconnection and eviction logic is moved out of net_processing (cc @cfields), then net_processing won't need to be aware of the difference between manual and automatic connections.

Yes, that sounds like a nice side-effect of moving the eviction logic out of net_processing.

in src/net.h:818 in 4ad2689916 outdated

 814 | @@ -786,7 +815,9 @@ class CConnman
 815 |          vWhitelistedRange = connOptions.vWhitelistedRange;
 816 |          {
 817 |              LOCK(m_added_nodes_mutex);
 818 | -            m_added_nodes = connOptions.m_added_nodes;
 819 | +            for (auto addedNode : connOptions.m_added_nodes) {

jnewbery commented at 9:14 AM on May 3, 2022:

            for (auto added_node : connOptions.m_added_nodes) {

mzumsande commented at 3:51 PM on May 4, 2022:

changed (and also changed the argument of GetAddedNodeEntry with the same name)

in src/net.cpp:2580 in 4ad2689916 outdated

2839 | @@ -2836,22 +2840,22 @@ std::vector<CAddress> CConnman::GetAddresses(CNode& requestor, size_t max_addres
2840 |      return cache_entry.m_addrs_response_cache;
2841 |  }
2842 |  
2843 | -bool CConnman::AddNode(const std::string& strNode)
2844 | +bool CConnman::AddNode(const std::string& strNode, ConnectionType conn_type)

jnewbery commented at 9:27 AM on May 3, 2022:

Should this assert or return false if conn_type != ConnectionType::MANUAL && conn_type != ConnectionType::MANUAL_BLOCK_RELAY? What would it mean to have a non-manual connection type in the m_added_nodes container?

If this is changed to an assert you'd need to also update the test in fuzz/connman.

mzumsande commented at 3:53 PM on May 4, 2022:

I added an assert and updated the fuzz test to comply with that.

in src/rpc/net.cpp:297 in 4ad2689916 outdated

 293 | @@ -293,6 +294,22 @@ static RPCHelpMan addnode()
 294 |          throw std::runtime_error(
 295 |              self.ToString());
 296 |      }
 297 | +    std::string manual_name{ConnectionTypeAsString(ConnectionType::MANUAL)};

jnewbery commented at 9:29 AM on May 3, 2022:

These can be const.

mzumsande commented at 3:51 PM on May 4, 2022:

done

jnewbery commented at 9:34 AM on May 3, 2022: contributor

Code review ACK 4ad26899169c45a6295efe8f1798e91ac0810e00

mzumsande force-pushed on May 4, 2022

mzumsande commented at 3:54 PM on May 4, 2022: contributor

4ad2689 to 6d75df7: addressed feedback by @jnewbery

jnewbery commented at 5:30 PM on May 4, 2022: contributor

Code review ACK 6d75df749d

jonatack commented at 5:59 AM on May 9, 2022: contributor

It seems to me the linked issue would be solved in the general case by #24545 (or alternatively since v22 by adding manual connections to other non-clearnet networks).

In that situation, manual block-relay-only connections to peers on clearnet networks can help us staying connected to the best chain, but in contrast to normal manual connections, transactions and addresses aren't transmitted on these links - in particular not our own address or transaction from our wallet. This increases privacy and will also make it harder to perform fingerprinting attacks (connecting our identities over different networks).

Manual connections are to trusted peers, so if I understand correctly, the specific threat model here is that of a trusted peer fingerprinting us?

vasild commented at 9:02 AM on May 10, 2022: contributor

Manual connections are to trusted peers, so if I understand correctly, the specific threat model here is that of a trusted peer fingerprinting us?

My understanding is that the problem being addressed here is a passive MITM observing that a particular transaction originated from our node when we send it to our trusted peer, added with -addnode (over clearnet).

jonatack commented at 9:50 AM on May 10, 2022: contributor

Manual connections are to trusted peers, so if I understand correctly, the specific threat model here is that of a trusted peer fingerprinting us?

My understanding is that the problem being addressed here is a passive MITM observing that a particular transaction originated from our node when we send it to our trusted peer, added with -addnode (over clearnet).

If so, my understanding was that BIP324 is designed to address this: https://drive.google.com/file/d/1o6PaA-vAWXhpCHHbBkUGZJ-6bmde6gEY/view

sipa commented at 2:26 PM on May 10, 2022: member

Overall I think this is a useful addition, if it ends up being used. I'm a bit concerned it's too obscure for people to decide to configure links like this. Weak concept ACK. @mzumsande / @dhruv It seems both this PR and #24545 add a way to associate flags with manually configured connections. Perhaps it's worth seeing if the interfaces can be made the same? @vasild Maybe just a terminology note, but a MitM is by definition an active attacker that inserts themselves in the connection. I think perhaps you mean more something like an eavesdropper / large scale surveillance (which just observes without interfering)? @jonatack Even BIP324, and even when combined with some mechanism of authentication, cannot fully prevent information about transaction origin to a global surveillant due to traffic patterns. We do intend to include affordances for dummy traffic / traffic shaping in BIP324, but actually making use of that to hide transaction propagation patterns is out of scope.

vasild commented at 6:33 AM on May 11, 2022: contributor

@sipa, yes, by passive man in the middle, I mean eavesdropping or spying. Btw, wikipedia MITM reads "... attacker secretly relays and possibly alters the communications between two parties ..."

mzumsande commented at 12:39 PM on May 11, 2022: contributor

Manual connections are to trusted peers, so if I understand correctly, the specific threat model here is that of a trusted peer fingerprinting us?

There are different levels of trust and I think that it is quite typical that users would just choose a peer with a good uptime and no past misbehaviour as a "trusted" addnode peer without actually being able to vouch for its operator completely.

But even if the peer is indeed good, there is still the possibility of indirect attempts to infer this connection, where a third party would connect to us (on the privacy network where we accept connections) and also to our suspected addnode peer (via clearnet). In this situation, I think it would be possible to infer the existence of this connection with a reasonable degree of confidence using addr relay probing, and probably some tricks exist within tx relay as well. I think that there is a much larger attack surface if you participate in addr and tx relay, and part of the reason block-relay-only connections were introduced is to just disable these attack vectors completely on these connections instead of trying to solve all relevant issues with addr and tx relay.

It seems both this PR and #24545 add a way to associate flags with manually configured connections. Perhaps it's worth seeing if the interfaces can be made the same?

Yes, this should be easily possible. I had a look at an older version of this (#23900), and I think that the changes in net are quite similar already. I will soon take a closer look and possibly change some nomenclature/details here to make them more compatible.

I'm a bit concerned it's too obscure for people to decide to configure links like this.

Yes, that's definitely a valid point, I was unsure about this myself. It seems really hard to gauge what features users will actually use.

DrahtBot added the label Needs rebase on May 16, 2022

mzumsande force-pushed on May 18, 2022

mzumsande commented at 3:19 PM on May 18, 2022: contributor

6d75df7 to c348172: Rebased and made the approach analogous to the one by @dhruv in #24545 by renaming AddedNodeEntry to AddedNodeParams and and making it a member of AddedNodeInfo.

DrahtBot removed the label Needs rebase on May 18, 2022

DrahtBot added the label Needs rebase on May 19, 2022

mzumsande force-pushed on May 19, 2022

mzumsande commented at 2:44 PM on May 19, 2022: contributor

c348172 to 8af5283:

Rebased after the merge of #22778 - this meant adding a couple more calls to IsBlockRelayConn(), that used to be m_tx_relay == nullptr checks before #22778 and therefore apply for both manual and automatic block-relay-only connections.

DrahtBot removed the label Needs rebase on May 19, 2022

jnewbery commented at 7:37 AM on May 20, 2022: contributor

reACK 8af5283992

maflcko removed the label GUI on May 20, 2022

jonatack commented at 2:21 PM on May 20, 2022: contributor

Attempted a couple of week-long Twitter surveys related to this topic at https://twitter.com/jonatack/status/1524377548062351360 and https://twitter.com/jonatack/status/1524380125126660096.

LarryRuane commented at 12:07 AM on June 15, 2022: contributor

6d75df7 to c348172: Rebased and made the approach analogous to the one by @dhruv in #24545 by renaming AddedNodeEntry to AddedNodeParams and and making it a member of AddedNodeInfo.

Suggestion, if possible do the rebase and PR changes in separate force-pushes, to make it easier for reviewers to see just the PR changes. I think I've seen GitHub combine the two force-pushes if they're close in time, so it may be advisable to wait a bit between the force-pushes.

DrahtBot added the label Needs rebase on Jun 15, 2022

mzumsande force-pushed on Jun 15, 2022

mzumsande commented at 2:22 PM on June 15, 2022: contributor

8af5283 to 629a6f4: Rebased due to conflict with #25156.

Suggestion, if possible do the rebase and PR changes in separate force-pushes, to make it easier for reviewers to see just the PR changes.

Ok, I'll try to think of that next time!

DrahtBot removed the label Needs rebase on Jun 15, 2022

in src/rpc/net.cpp:291 in 629a6f4040 outdated

 277 | @@ -278,6 +278,7 @@ static RPCHelpMan addnode()
 278 |                  {
 279 |                      {"node", RPCArg::Type::STR, RPCArg::Optional::NO, "The node (see getpeerinfo for nodes)"},
 280 |                      {"command", RPCArg::Type::STR, RPCArg::Optional::NO, "'add' to add a node to the list, 'remove' to remove a node from the list, 'onetry' to try a connection to the node once"},
 281 | +                    {"connection_type", RPCArg::Type::STR, RPCArg::Default{"manual"}, "Type of connection to open ('manual' or 'manual-block-relay')"},

LarryRuane commented at 8:40 PM on June 15, 2022:

If this argument is provided with "remove", throw an exception?

mzumsande commented at 6:04 PM on June 20, 2022:

Done.

in src/net.cpp:2234 in 629a6f4040 outdated

2230 | @@ -2227,9 +2231,9 @@ std::vector<AddedNodeInfo> CConnman::GetAddedNodeInfo() const
2231 |          }
2232 |      }
2233 |  
2234 | -    for (const std::string& strAddNode : lAddresses) {
2235 | -        CService service(LookupNumeric(strAddNode, Params().GetDefaultPort(strAddNode)));
2236 | -        AddedNodeInfo addedNode{strAddNode, CService(), false, false};
2237 | +    for (auto node_to_add : lAddresses) {

LarryRuane commented at 8:58 PM on June 15, 2022:

    for (const auto& node_to_add : lAddresses) {

in src/net.cpp:2238 in 629a6f4040 outdated

2236 | -        AddedNodeInfo addedNode{strAddNode, CService(), false, false};
2237 | +    for (auto node_to_add : lAddresses) {
2238 | +        CService service(LookupNumeric(node_to_add.m_node_address, Params().GetDefaultPort(node_to_add.m_node_address)));
2239 | +        AddedNodeInfo addedNode{node_to_add, CService(), /*fConnected=*/false, /*fInbound=*/false};
2240 |          if (service.IsValid()) {
2241 |              // strAddNode is an IP:port

LarryRuane commented at 9:04 PM on June 15, 2022:

            // node_to_add.m_node_address is an IP:port

mzumsande commented at 6:06 PM on June 20, 2022:

I changed it to "node address", so I don't have to use a variable name here.

in src/net.cpp:2246 in 629a6f4040 outdated

2243 | @@ -2240,7 +2244,7 @@ std::vector<AddedNodeInfo> CConnman::GetAddedNodeInfo() const
2244 |              }
2245 |          } else {
2246 |              // strAddNode is a name

LarryRuane commented at 9:04 PM on June 15, 2022:

            // node_to_add.m_node_address is a name

mzumsande commented at 6:06 PM on June 20, 2022:

as above

in src/net.h:816 in 629a6f4040 outdated

 812 | @@ -785,7 +813,9 @@ class CConnman
 813 |          vWhitelistedRange = connOptions.vWhitelistedRange;
 814 |          {
 815 |              LOCK(m_added_nodes_mutex);
 816 | -            m_added_nodes = connOptions.m_added_nodes;
 817 | +            for (auto added_node : connOptions.m_added_nodes) {

LarryRuane commented at 3:57 PM on June 16, 2022:

            for (const auto& added_node : connOptions.m_added_nodes) {

mzumsande commented at 6:07 PM on June 20, 2022:

done, thanks.

LarryRuane commented at 4:36 PM on June 16, 2022: contributor

ACK 629a6f404056e3228f49fd03e6c682d8f2820e37 comments are suggestions

mzumsande force-pushed on Jun 20, 2022

mzumsande commented at 6:10 PM on June 20, 2022: contributor

629a6f4 to 00ea370: addressed suggestions by @LarryRuane - thank you!

in src/net.cpp:3172 in 00ea370661 outdated

3167 | +    const size_t index = added_node.rfind('=');
3168 | +    if (index != std::string::npos) {
3169 | +        if (added_node.substr(index + 1) == ConnectionTypeAsString(ConnectionType::MANUAL_BLOCK_RELAY)) {
3170 | +            added_node = added_node.substr(0, index);
3171 | +            conn_type = ConnectionType::MANUAL_BLOCK_RELAY;
3172 | +        }

LarryRuane commented at 11:28 PM on June 21, 2022:

We're not checking that the connection type string is valid here, as we are in the addnode rpc handler. It's understandable because it's actually too late to check it here. I recommend checking it in AppInitMain() where many other config / arg parameters are checked. If it's not one of the valid types, call InitError(). You may want to refactor the connection type string checking logic out of the rpc handler into a separate function (probably should be a static CConnman method), and then call it from there and from AppInitMain().

mzumsande commented at 8:40 PM on July 5, 2022:

I added a check to init - in doing so, I realized that if I check for invalid connection types anyway, I might as well do therest of the parsing of the string there as well - so I changed it to pass a vector of AddedNodeParams structs already in CConnman::Options and removed the CConnman::GetAddedNodeParams function. Let me know if that makes sense to you!

in src/net.cpp:3164 in 00ea370661 outdated

3160 | @@ -3156,6 +3161,19 @@ uint64_t CConnman::CalculateKeyedNetGroup(const CAddress& address) const
3161 |      return GetDeterministicRandomizer(RANDOMIZER_ID_NETGROUP).Write(vchNetGroup.data(), vchNetGroup.size()).Finalize();
3162 |  }
3163 |  
3164 | +AddedNodeParams CConnman::GetAddedNodeParams(std::string added_node)

LarryRuane commented at 1:38 AM on June 22, 2022:

AddedNodeParams CConnman::GetAddedNodeParams(std::string added_node) const

mzumsande commented at 8:40 PM on July 5, 2022:

This became obsolete, since GetAddedNodeParams was removed.

in src/net.h:1087 in 00ea370661 outdated

1083 | @@ -1054,6 +1084,7 @@ class CConnman
1084 |  
1085 |      size_t SocketSendData(CNode& node) const EXCLUSIVE_LOCKS_REQUIRED(node.cs_vSend);
1086 |      void DumpAddresses();
1087 | +    AddedNodeParams GetAddedNodeParams(std::string addedNode);

LarryRuane commented at 1:39 AM on June 22, 2022:

    AddedNodeParams GetAddedNodeParams(std::string addedNode) const;

mzumsande commented at 8:40 PM on July 5, 2022:

This became obsolete, since GetAddedNodeParams was removed.

LarryRuane commented at 8:08 PM on June 22, 2022: contributor

Should this have release notes?

mzumsande force-pushed on Jul 5, 2022

mzumsande commented at 8:45 PM on July 5, 2022: contributor

Sorry, @LarryRuane it took me some time to address your comments. 00ea370 to 20b6e75:

Addressed feedback to validate the connection type when passing via bitcoind arg. Note that the node name is not being validated, like before.
Added a commit with a relase note.

I also extended the test coverage of the RPC -addnode to cover manual-block-relay connections (rpc_net.py).

DrahtBot added the label Needs rebase on Jul 7, 2022

in src/net.h:188 in 20b6e75ddc outdated

 179 | @@ -188,6 +180,30 @@ enum class ConnectionType {
 180 |       * AddrMan is empty.
 181 |       */
 182 |      ADDR_FETCH,
 183 | +
 184 | +    /**
 185 | +     * Manual-block-relay connections are manually added nodes (via -addnode)
 186 | +     * that behave like BLOCK_RELAY connections in that they don't participate in
 187 | +     * transaction and address relay, and also share the properties of MANUAL
 188 | +     * connections. When connected to a privacy network such as

jnewbery commented at 11:40 AM on July 8, 2022:

grammar nit:

     * connections. When the node is connected to a privacy network such as

Otherwise, the "connected to..." clause seems like it's attached to the "manual block-relay-only connection" (equivalent to "when the manual block-relay-only connections are connected to a privacy network...)

mzumsande commented at 8:56 PM on July 11, 2022:

Done, thanks!

in src/net_processing.cpp:421 in 20b6e75ddc outdated

 412 | @@ -413,7 +413,7 @@ struct CNodeState {
 413 |        * Both are only in effect for outbound, non-manual, non-protected connections.
 414 |        * Any peer protected (m_protect = true) is not chosen for eviction. A peer is
 415 |        * marked as protected if all of these are true:
 416 | -      *   - its connection type is IsBlockOnlyConn() == false
 417 | +      *   - its connection type is IsAutomaticBlockRelayConn() == false

jnewbery commented at 12:00 PM on July 8, 2022:

I think it's very strange to say "connection type is IsAutomaticBlockRelayConn() == false", when the logic for setting m_protect is pfrom.IsFullOutboundConn(). I know this line is only touched by the scripted diff, but since it's being touched perhaps updated to say "its connection type is OUTBOUND_FULL_RELAY.

(in general I don't think it's useful to have comments that simply restate the programming logic in english so would prefer to see this removed, but that's out of scope for this PR).

mzumsande commented at 8:56 PM on July 11, 2022:

Good point, I changed this in the commit where the manual-block-relay connections are introduced.

in doc/release-notes.md:114 in 20b6e75ddc outdated

 103 | @@ -104,6 +104,11 @@ Low-level changes
 104 |  RPC
 105 |  ---
 106 |  
 107 | +- The RPC `addnode` and the `-addnode` bitcoind startup option now
 108 | +optionally support adding manual peers that are block-relay-only.
 109 | +On these connections, the node doesn't participate in transaction or
 110 | +address relay. (#24170)

jnewbery commented at 2:09 PM on July 8, 2022:

Perhaps add "see bitcoind help documentation and RPC help documentation for parameter semantics" or similar

mzumsande commented at 8:58 PM on July 11, 2022:

Done, in a slightly shortened form.

jnewbery commented at 2:10 PM on July 8, 2022: contributor

utACK 20b6e75ddc88a8d11aaae32700e606b5eada24f5

Happy to re-review once this has been rebased.

mzumsande force-pushed on Jul 11, 2022

mzumsande commented at 8:33 PM on July 11, 2022: contributor

20b6e75 to 40d996e: rebased due to conflict with #25500 40d996e to d37c267: Addressed comments by @jnewbery

mzumsande force-pushed on Jul 11, 2022

DrahtBot removed the label Needs rebase on Jul 11, 2022

DrahtBot added the label Needs rebase on Jul 12, 2022

mzumsande force-pushed on Jul 12, 2022

mzumsande commented at 4:10 PM on July 12, 2022: contributor

d37c267 to 70595a5: rebased (conflict with #25591)

DrahtBot removed the label Needs rebase on Jul 12, 2022

jnewbery commented at 9:31 AM on July 13, 2022: contributor

utACK 70595a585483539b19e3fe77f07e32d06063266b

Verified git range-diff

DrahtBot added the label Needs rebase on Jul 19, 2022

mzumsande force-pushed on Jul 19, 2022

mzumsande commented at 7:34 PM on July 19, 2022: contributor

70595a5 to 690b040 : rebased due to conflict with #25514

DrahtBot removed the label Needs rebase on Jul 19, 2022

DrahtBot added the label Needs rebase on Jul 26, 2022

mzumsande force-pushed on Jul 28, 2022

mzumsande commented at 8:40 PM on July 28, 2022: contributor

690b040 to 894486a: rebased (conflict with #25699)

DrahtBot removed the label Needs rebase on Jul 28, 2022

DrahtBot added the label Needs rebase on Sep 16, 2022

mzumsande force-pushed on Sep 19, 2022

mzumsande commented at 5:20 PM on September 19, 2022: contributor

894486a to 9bc3178:

Rebased (moved release note into its own file)

DrahtBot removed the label Needs rebase on Sep 19, 2022

DrahtBot added the label Needs rebase on Oct 13, 2022

mzumsande force-pushed on Oct 18, 2022

DrahtBot removed the label Needs rebase on Oct 18, 2022

brunoerg commented at 6:25 PM on October 18, 2022: contributor

Concept ACK

Should we consider the manual block-relay-only connections as anchors? E.g I added 2 manual block-relay-only connections in addition to the 2 outbound block-relay-only ones and I'd like to consider the manual ones as my anchors. so, when my node starts it would connect to that 2 peers. Could it make sense?

DrahtBot added the label Needs rebase on Oct 21, 2022

mzumsande force-pushed on Oct 21, 2022

mzumsande commented at 6:43 PM on October 21, 2022: contributor

Rebased

Should we consider the manual block-relay-only connections as anchors? E.g I added 2 manual block-relay-only connections in addition to the 2 outbound block-relay-only ones and I'd like to consider the manual ones as my anchors. so, when my node starts it would connect to that 2 peers. Could it make sense?

This PR allows to add the manual block-relay-only connections as addnode peers to bitcoin.conf - I think if we do this, it's essentially the same behavior as if it was an anchor, we'll connect to them with each new restart.

DrahtBot removed the label Needs rebase on Oct 21, 2022

DrahtBot added the label Needs rebase on Nov 2, 2022

mzumsande force-pushed on Nov 21, 2022

DrahtBot removed the label Needs rebase on Nov 21, 2022

willcl-ark approved

willcl-ark commented at 2:07 PM on January 16, 2023: member

ACK 3eb8c42368a6e059b1842cbcec478815d947a341

If you touch again (to kick the CI.) you might consider also amending this comment to clarify that that manual-block-relay peers are not excluded from the protection, only automatic-block-relay peers are?

in src/init.cpp:1735 in 401f9c57bb outdated

1722 | +        if (index != std::string::npos) {
1723 | +            const std::string conn_name = added_node.substr(index + 1);
1724 | +            if (conn_name == ConnectionTypeAsString(ConnectionType::MANUAL_BLOCK_RELAY)) {
1725 | +                added_node = added_node.substr(0, index);
1726 | +                conn_type = ConnectionType::MANUAL_BLOCK_RELAY;
1727 | +            } else {

aureleoules commented at 4:05 PM on January 16, 2023:

Maybe also add the case for ConnectionType::MANUAL?

./src/bitcoind -addnode='test.local=manual'

2023-01-16T16:04:19Z Error: Invalid -addnode connection type: manual Error: Invalid -addnode connection type: manual

mzumsande commented at 10:00 PM on February 17, 2023:

Not sure about this: Currently, the help for "addnode" states that only =manual-block-relay is possible (manual is the default). Adding this to the help might only complicate things for the user, because there would be no difference between =manual and no argument at all?

brunoerg commented at 7:05 PM on March 2, 2023:

I prefer not having =manual, it would add more stuff to the code and I believe it might complicate for the user, it's the default behavior, not sure why someone would want to specify it.

aureleoules approved

aureleoules commented at 4:08 PM on January 16, 2023: member

Light ACK 3eb8c42368a6e059b1842cbcec478815d947a341

The code looks good and I verified that the tests correctly test the new behavior. Be wary I am not familiar with net processing.

brunoerg commented at 4:28 PM on January 16, 2023: contributor

This PR allows to add the manual block-relay-only connections as addnode peers to bitcoin.conf - I think if we do this, it's essentially the same behavior as if it was an anchor, we'll connect to them with each new restart.

Not sure if I was clear, just to clarify. If I use -addnode=ip=manual-block-relay in CLI, not putting it in bitcoin.conf, when I shutdown my node, this peer is not gonna be dump into anchors.dat, right?

in src/net.h:989 in 3eb8c42368 outdated

 987 | @@ -972,7 +988,7 @@ class CConnman
 988 |      /**
 989 |       * Return vector of current BLOCK_RELAY peers.

rajarshimaitra commented at 1:11 PM on January 19, 2023:

If we are changing the name of the functions to AutomaticBlockRelayConns would it make sense to call the connection BLOCK_RELAY type also to AUTOMATIC_BLOCK_RELAY?

Then comments like this and the connection type returned would be consistent and could be less confusing for future readers.

For example this call only returns automatic block relays, not all block relays..

LarryRuane commented at 7:13 PM on February 10, 2023:

https://github.com/bitcoin/bitcoin/pull/24170/files#r1081240881

If we are changing the name of the functions to AutomaticBlockRelayConns would it make sense to call the connection BLOCK_RELAY type also to AUTOMATIC_BLOCK_RELAY?

Doing this rename would touch 20 lines of code in 8 files, but could be done with a scripted diff. I agree it would be more consistent, so personally, I'm in favor, but I'd also understand not wanting to increase the diff that much.

mzumsande commented at 10:01 PM on February 17, 2023:

I agree that it would be more consistent and added a commit to do this - I'll check how many additional conflicts this introduces...

in src/net.h:457 in 3eb8c42368 outdated

 427 | @@ -424,6 +428,7 @@ class CNode
 428 |              case ConnectionType::MANUAL:
 429 |              case ConnectionType::ADDR_FETCH:
 430 |              case ConnectionType::FEELER:
 431 | +            case ConnectionType::MANUAL_BLOCK_RELAY:
 432 |                  return false;

rajarshimaitra commented at 1:35 PM on January 19, 2023:

I might be confused, but why don't we wanna consider MANUAL_BLOCK_RELAY as Outbound and BlockRelay?

mzumsande commented at 10:03 PM on February 17, 2023:

It's used in net_processing in places suchas ConsiderEviction() where we wouldn't want to consider manual connections. I think the name "IsOutboundOrBlockRelayConn" isn't ideal (even before this PR, because manual connections are also outbound connections), maybe the name "IsAutomaticOutboundOrBlockRelayConn" would be better. I added a commit to do that.

in test/functional/rpc_net.py:236 in 3eb8c42368 outdated

 234 | +        self.nodes[0].addnode(node=dummy_node, command='add', connection_type='manual-block-relay')
 235 | +        added_nodes = self.nodes[0].getaddednodeinfo(dummy_node)
 236 | +        assert_equal(len(added_nodes), 1)
 237 | +        assert_equal(added_nodes[0]['addednode'], dummy_node)
 238 | +        assert_equal(added_nodes[0]['connection_type'], 'manual-block-relay')
 239 | +        self.nodes[0].addnode(node=dummy_node, command='remove')

rajarshimaitra commented at 4:26 PM on January 19, 2023:

We had this question in our review club:

along with testing addnode, does it also makes sense to connect with self.nodes[1] in manual-blocks-only mode and assert that only block data is received and not other types of data?

If that makes sense, maybe it could be a nice follow-up after adding the new category. perhaps can be done in p2p_add_connections.py?

mzumsande commented at 10:05 PM on February 17, 2023:

Yes, I think it makes sense as a follow-up! Though, if I'm not missing something we don't even support regular MANUAL connections there, so adding support for these would make sense as well?

rajarshimaitra commented at 5:02 PM on January 19, 2023: contributor

Concept + tACK 3eb8c42368a6e059b1842cbcec478815d947a341

I think this is a reasonable addition, though share the concern about how much it would actually be used in public. Mostly seems to be not hurting much, modulo the code complexity added due to a new type.

Below are a few nonblocking comments and questions.

achow101 commented at 5:18 PM on January 31, 2023: member

Any net people want to take a look here?

jonatack commented at 5:46 PM on January 31, 2023: contributor

As mentioned in the issue #23763, perhaps I'm missing something but this seems somewhat redundant with

using a VPN, and/or
running bitcoind over multiple encrypted networks (Tor, I2P, CJDNS), and/or
BIP324 v2 encrypted p2p in the future

achow101 commented at 6:03 PM on January 31, 2023: member

As mentioned in the issue #23763, perhaps I'm missing something but this seems somewhat redundant with

using a VPN, and/or

running bitcoind over multiple encrypted networks (Tor, I2P, CJDNS), and/or

I don't see how any of those are relevant. Privacy networks and VPNs are generally bandwidth constrained and have a much smaller pool of nodes to connect to, therefore making it easier for an attacker to eclipse you. Being able to add manual blocksonly connections allows nodes to add connections to clearnet nodes for public data that they are unlikely to originate - blocks. Any transactions would still be relayed through connections made on the privacy networks so their origins are masked.

BIP324 v2 encrypted p2p in the future

Since we're talking about active attackers rather than passive listening, BIP 324 doesn't help.

jonatack commented at 6:56 PM on January 31, 2023: contributor

Privacy networks and VPNs are generally bandwidth constrained

I've been running bitcoind over VPNs and all of our encrypted networks since years without bandwidth issues, and currently since 3 months from rural Central America on slow/flakey connections. Even when GitHub takes a long time to load a page, bitcoind thankfully runs well in making peer connections under adverse conditions from what I can see between the debug log and a live -netinfo 4 dashboard. Most of the time it's running onlynet=tor/i2p/cjdns, e.g. all three without clearnet.

and have a much smaller pool of nodes to connect to, therefore making it easier for an attacker to eclipse you

This may have evolved with a significant increase in Tor and I2P nodes. As one example, my little dev node on my laptop out here currently knows 14k Tor peers and 1100 I2P peers after the IsTerrible filter. 15k seems like a pretty decent amount of peers to connect to -- there are significantly more of them nowadays relative to a year ago; for I2P in particular the number seems to have grown quite a lot recently.

in src/init.cpp:1720 in 3eb8c42368 outdated

1712 | @@ -1713,7 +1713,23 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1713 |      connOptions.m_msgproc = node.peerman.get();
1714 |      connOptions.nSendBufferMaxSize = 1000 * args.GetIntArg("-maxsendbuffer", DEFAULT_MAXSENDBUFFER);
1715 |      connOptions.nReceiveFloodSize = 1000 * args.GetIntArg("-maxreceivebuffer", DEFAULT_MAXRECEIVEBUFFER);
1716 | -    connOptions.m_added_nodes = args.GetArgs("-addnode");
1717 | +
1718 | +    std::vector<AddedNodeParams> added_nodes;
1719 | +    for (auto added_node : args.GetArgs("-addnode")) {
1720 | +        ConnectionType conn_type = ConnectionType::MANUAL;
1721 | +        const size_t index = added_node.rfind('=');

LarryRuane commented at 6:34 PM on February 10, 2023:

if you retouch

        ConnectionType conn_type{ConnectionType::MANUAL};
        const size_t index{added_node.rfind('=')};

in src/init.cpp:1722 in 3eb8c42368 outdated

1718 | +    std::vector<AddedNodeParams> added_nodes;
1719 | +    for (auto added_node : args.GetArgs("-addnode")) {
1720 | +        ConnectionType conn_type = ConnectionType::MANUAL;
1721 | +        const size_t index = added_node.rfind('=');
1722 | +        if (index != std::string::npos) {
1723 | +            const std::string conn_name = added_node.substr(index + 1);

LarryRuane commented at 6:34 PM on February 10, 2023:

            const std::string conn_name{added_node.substr(index + 1)};

in src/net.cpp:1956 in 3eb8c42368 outdated

1900 | @@ -1899,20 +1901,20 @@ std::vector<AddedNodeInfo> CConnman::GetAddedNodeInfo() const
1901 |          }
1902 |      }
1903 |  
1904 | -    for (const std::string& strAddNode : lAddresses) {
1905 | -        CService service(LookupNumeric(strAddNode, Params().GetDefaultPort(strAddNode)));
1906 | -        AddedNodeInfo addedNode{strAddNode, CService(), false, false};
1907 | +    for (const auto& node_to_add : lAddresses) {
1908 | +        CService service(LookupNumeric(node_to_add.m_node_address, Params().GetDefaultPort(node_to_add.m_node_address)));
1909 | +        AddedNodeInfo addedNode{node_to_add, CService(), /*fConnected=*/false, /*fInbound=*/false};

LarryRuane commented at 6:43 PM on February 10, 2023:

            AddedNodeInfo addedNode{
            .m_params{node_to_add},
            .resolvedAddress{CService()},
            .fConnected{false},
            .fInbound{false}};

mzumsande commented at 10:06 PM on February 17, 2023:

I think I prefer the more concise version here, especially when most of the args are just defaults that are set to their actual values in the following lines. Also see below.

in src/net.cpp:2588 in 3eb8c42368 outdated

2535 | +    for (const auto& it : m_added_nodes) {
2536 | +        if (strNode == it.m_node_address) return false;
2537 |      }
2538 |  
2539 | -    m_added_nodes.push_back(strNode);
2540 | +    m_added_nodes.push_back({strNode, conn_type});

LarryRuane commented at 7:02 PM on February 10, 2023:

    m_added_nodes.push_back(AddedNodeParams{
        .m_node_address{strNode},
        .m_conn_type{conn_type}});

mzumsande commented at 10:06 PM on February 17, 2023:

tried it, but the CI doesn't like it: designated initializers are C++20-only?

in src/rpc/net.cpp:308 in 3eb8c42368 outdated

 299 | @@ -299,6 +300,25 @@ static RPCHelpMan addnode()
 300 |          throw std::runtime_error(
 301 |              self.ToString());
 302 |      }
 303 | +    const std::string manual_name{ConnectionTypeAsString(ConnectionType::MANUAL)};
 304 | +    const std::string manual_block_relay_name{ConnectionTypeAsString(ConnectionType::MANUAL_BLOCK_RELAY)};
 305 | +    if (strCommand == "remove" && !request.params[2].isNull()) {
 306 | +        throw JSONRPCError(RPC_INVALID_PARAMS, "Cannot specify a connection type with 'remove'");
 307 | +    }
 308 | +    std::string connection_type_name = manual_name;

LarryRuane commented at 7:18 PM on February 10, 2023:

    std::string connection_type_name{manual_name};

in src/rpc/net.cpp:316 in 3eb8c42368 outdated

 311 | +    }
 312 | +    ConnectionType connection_type;
 313 | +    if (connection_type_name == manual_block_relay_name){
 314 | +        connection_type = ConnectionType::MANUAL_BLOCK_RELAY;
 315 | +    }
 316 | +    else if (connection_type_name == manual_name) {

LarryRuane commented at 7:21 PM on February 10, 2023:

clang-format says (also below)

    } else if (connection_type_name == manual_name) {

mzumsande commented at 10:06 PM on February 17, 2023:

done

LarryRuane commented at 7:24 PM on February 10, 2023: contributor

re-ACK 9e953f1134fe4b752868f519f12d8dee5c1cef52

Code suggestions are advisory.

Suggestion for PR description, maybe include an RPC (or config) example so reviewers can try it out manually quickly without having to look at the code or help to figure out the syntax, something like

bitcoin-cli addnode <IP>:8333 add manual-block-relay

The results of manual testing look good. After enabling logging category "net" and adding a manual-block-relay peer, this appears in debug.log:

New outbound peer connected: version: 70016, blocks=775913, peer=24 (manual-block-relay)

After that, the only logging for peer=24 is sending and receiving ping and pong, and also:

[net] received: headers (82 bytes) peer=24
[net] received: addrv2 (44 bytes) peer=24
[net] ignoring addrv2 message from manual-block-relay peer=24

DrahtBot requested review from jnewbery on Feb 10, 2023

DrahtBot requested review from LarryRuane on Feb 10, 2023

mzumsande commented at 7:18 PM on February 13, 2023: contributor

I was busy with other things in the last weeks, but I will get back to address feedback later this week!

mzumsande force-pushed on Feb 17, 2023

mzumsande commented at 10:13 PM on February 17, 2023: contributor

Sorry for the delay I addressed the outstanding comments, rebased and pushed an update.

Major changes are two scripted-diff commits renaming the function. IsOutboundOrBlockRelayConn to IsAutomaticOutboundOrBlockRelayConn and the connection type BLOCK_RELAY to AUTOMATIC_BLOCK_RELAY

If you touch again (to kick the CI.) you might consider also amending this comment to clarify that that manual-block-relay peers are not excluded from the protection, only automatic-block-relay peers are?

Since the protection only applies to Full Outbound connections (and nothing else), manual block-relay peers are also excluded from it (just like all other manual peers). It just makes most sense naming (automatic) block-relay-only peers here.

Not sure if I was clear, just to clarify. If I use -addnode=ip=manual-block-relay in CLI, not putting it in bitcoin.conf, when I shutdown my node, this peer is not gonna be dump into anchors.dat, right?

yes, that is correct, anchors are only chosen from automatic block-relay-only connections. I think it makes sense not to mix automatic and manual connections that way: If the node operator wants to be permanently connected to a given manual connection, they can add this to their permanent config.

mzumsande force-pushed on Feb 17, 2023

mzumsande commented at 10:50 PM on February 17, 2023: contributor

As mentioned in the issue #23763, perhaps I'm missing something but this seems somewhat redundant with

using a VPN, and/or

running bitcoind over multiple encrypted networks (Tor, I2P, CJDNS), and/or

BIP324 v2 encrypted p2p in the future

I'm not only thinking about a third party monitoring your connections, but also about about the information you'd give the clearnet outbound peers you connect to. I think there are good reasons to have a connection to clearnet (getting blocks quickly, eclipse attack resistance), and a way to do that which ensures you don't send your wallet transactions over these connections makes sense to me
(especially since rebroadcast still has privacy issues).

For me, the critical question is still if this opt-in feature will find enough users to justify the added complexity in net - I honestly don't know that, and I wouldn't have a problem closing this for now if people think there isn't enough demand for this use case currently.

stratospher commented at 10:02 AM on February 22, 2023: contributor

For me, the critical question is still if this opt-in feature will find enough users to justify the added complexity in net - I honestly don't know that, and I wouldn't have a problem closing this for now if people think there isn't enough demand for this use case currently.

if it helps, i found some related issues: (but i don’t know this either)

there’s an issue in raspiblitz where they talk about wanting to connect their tor node to clearnet to speed up IBD and and not lose privacy. they ended up using i2p peers.
there’s also https://github.com/bitcoin/bitcoin/issues/19042 and https://github.com/bitcoin/bitcoin/issues/14692 where they talk about wanting to connect to clearnet peers without relaying own transactions.

in the very unlikely scenario that everyone likes this connection and all bitcoin nodes on tor connect with clearnet only using this option - that wouldn't be good for the network right? (assuming an average tor user wouldn’t want their addresses/transactions relayed to their clearnet peers)

mzumsande commented at 9:23 PM on February 23, 2023: contributor

in the very unlikely scenario that everyone likes this connection and all bitcoin nodes on tor connect with clearnet only using this option - that wouldn't be good for the network right? (assuming an average tor user wouldn’t want their addresses/transactions relayed to their clearnet peers)

We'd need enough inbound capacity on clearnet to accomodate the additional manual connections to it, and also some users relaying transactions on both networks, otherwise txns submitted through tor wouldn't make it into clearnet and vice versa. I think that there are many nodes with a lesser need for privacy (e.g. because they don't submit transactions regularly) that don't have the need for something like and would be fine having full connections to both networks.

in src/net.h:126 in a1d6516d02 outdated

 120 | +    ConnectionType m_conn_type;
 121 | +};
 122 | +
 123 | +struct AddedNodeInfo {
 124 | +    AddedNodeParams m_params;
 125 | +    CService resolvedAddress;

brunoerg commented at 7:09 PM on March 2, 2023:

What would be the difference between resolvedAddress and m_node_address?

mzumsande commented at 8:14 PM on May 15, 2023:

m_node_address is a string, which is part of AddedNodeParams, which has just the unvalidated user input.

resolvedAddress is a CService built from m_node_address, which is generated in GetAddedNodeInfo and used in ThreadOpenConnection / rpc code.

in src/rpc/net.cpp:307 in a1d6516d02 outdated

 303 | @@ -303,6 +304,23 @@ static RPCHelpMan addnode()
 304 |          throw std::runtime_error(
 305 |              self.ToString());
 306 |      }
 307 | +    const std::string manual_name{ConnectionTypeAsString(ConnectionType::MANUAL)};

brunoerg commented at 7:16 PM on March 2, 2023:

nit: you could create manual_name and manual_block_relay_name after checking the connection type with "remove":

diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 6155af323..9d87d89e3 100644
--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -304,11 +304,14 @@ static RPCHelpMan addnode()
         throw std::runtime_error(
             self.ToString());
     }
-    const std::string manual_name{ConnectionTypeAsString(ConnectionType::MANUAL)};
-    const std::string manual_block_relay_name{ConnectionTypeAsString(ConnectionType::MANUAL_BLOCK_RELAY)};
+
     if (strCommand == "remove" && !request.params[2].isNull()) {
         throw JSONRPCError(RPC_INVALID_PARAMS, "Cannot specify a connection type with 'remove'");
     }
+
+    const std::string manual_name{ConnectionTypeAsString(ConnectionType::MANUAL)};
+    const std::string manual_block_relay_name{ConnectionTypeAsString(ConnectionType::MANUAL_BLOCK_RELAY)};
+
     std::string connection_type_name{manual_name};
     if (!request.params[2].isNull()) {
         connection_type_name = request.params[2].get_str();

mzumsande commented at 8:47 PM on May 15, 2023:

made obsolete by the next comment.

in src/rpc/net.cpp:312 in a1d6516d02 outdated

 303 | @@ -303,6 +304,23 @@ static RPCHelpMan addnode()
 304 |          throw std::runtime_error(
 305 |              self.ToString());
 306 |      }
 307 | +    const std::string manual_name{ConnectionTypeAsString(ConnectionType::MANUAL)};
 308 | +    const std::string manual_block_relay_name{ConnectionTypeAsString(ConnectionType::MANUAL_BLOCK_RELAY)};
 309 | +    if (strCommand == "remove" && !request.params[2].isNull()) {
 310 | +        throw JSONRPCError(RPC_INVALID_PARAMS, "Cannot specify a connection type with 'remove'");
 311 | +    }
 312 | +    std::string connection_type_name{manual_name};

brunoerg commented at 8:21 PM on March 2, 2023:

perhaps there is a way to simplify it:

diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 6155af323..b9e082a63 100644
--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -304,22 +304,19 @@ static RPCHelpMan addnode()
         throw std::runtime_error(
             self.ToString());
     }
-    const std::string manual_name{ConnectionTypeAsString(ConnectionType::MANUAL)};
-    const std::string manual_block_relay_name{ConnectionTypeAsString(ConnectionType::MANUAL_BLOCK_RELAY)};
+
     if (strCommand == "remove" && !request.params[2].isNull()) {
         throw JSONRPCError(RPC_INVALID_PARAMS, "Cannot specify a connection type with 'remove'");
     }
-    std::string connection_type_name{manual_name};
+
+    const std::string manual_block_relay_name{ConnectionTypeAsString(ConnectionType::MANUAL_BLOCK_RELAY)};
+    ConnectionType connection_type{ConnectionType::MANUAL};
     if (!request.params[2].isNull()) {
-        connection_type_name = request.params[2].get_str();
-    }
-    ConnectionType connection_type;
-    if (connection_type_name == manual_block_relay_name) {
-        connection_type = ConnectionType::MANUAL_BLOCK_RELAY;
-    } else if (connection_type_name == manual_name) {
-        connection_type = ConnectionType::MANUAL;
-    }  else {
-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Unsupported connection type: " + connection_type_name);
+        const std::string connection_type_name{request.params[2].get_str()};
+        if (connection_type_name == manual_block_relay_name)
+            connection_type = ConnectionType::MANUAL_BLOCK_RELAY;
+        else
+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Unsupported connection type: " + connection_type_name);
     }
 
     NodeContext& node = EnsureAnyNodeContext(request.context);

mzumsande commented at 8:46 PM on May 15, 2023:

Yes, thanks, that's a bit simpler and should be equivalent, so I took your suggestion!

DrahtBot added the label Needs rebase on Mar 19, 2023

scripted-diff: Rename IsBlockOnlyConn to IsAutomaticBlockRelayConn

This is in preparation for the addition of manual block-relay-only
connections.

-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }

ren IsBlockOnlyConn IsAutomaticBlockRelayConn
ren GetCurrentBlockRelayOnlyConns GetCurrentAutomaticBlockRelayConns

-END VERIFY SCRIPT-

2fc6e2c9ef

p2p: add MANUAL_BLOCK_RELAY Connection type

This commit implements the p2p behaviour, but the new
connection type is not used yet.

53be444ffd

mzumsande force-pushed on Apr 26, 2023

mzumsande commented at 11:53 AM on April 26, 2023: contributor

b36db717b50de318e22461141f5c0135263beee4 to c13c986: rebased (will address the oustanding review comments soon!)

DrahtBot removed the label Needs rebase on Apr 26, 2023

net, rpc: add block-relay-only option to addnode rpc

This applies to both add mode and onetry mode

Co-authored-by: dhruv <856960+dhruv@users.noreply.github.com>
Co-authored-by: brunoerg <brunoely.gc@gmail.com>

e1aec804fb

net, init: Add manual-block-relay option to bitcoind addnode

This allows to specify manual-block-relay peers per
command arg or bitcoin.conf

7de6f89e50

scripted-diff: Rename IsOutboundOrBlockRelayConn to IsAutomaticOutboundOrBlockRelayConn

to make it clearer that manual connections are not included

-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }

ren IsOutboundOrBlockRelayConn IsAutomaticOutboundOrBlockRelayConn

-END VERIFY SCRIPT-

826643287b

scripted-diff: rename BLOCK_RELAY to AUTOMATIC_BLOCK_RELAY

to better distinguish from MANUAL_BLOCK_RELAY

-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }

ren BLOCK_RELAY AUTOMATIC_BLOCK_RELAY

-END VERIFY SCRIPT-

bbd047b69b

doc: add release note for manual block-relay connections 41ce31289f

mzumsande force-pushed on May 15, 2023

mzumsande commented at 8:48 PM on May 15, 2023: contributor

c13c986 to 41ce312: Incorporated feedback by @brunoerg

DrahtBot added the label CI failed on May 31, 2023

DrahtBot removed the label CI failed on May 31, 2023

mzumsande commented at 7:11 PM on June 4, 2023: contributor

Seems like other proposals such as #27213 and #27509 that rely on improving automatic behavior have more support compared to this one (which needs manual setup by the node operator). Closing for now.

mzumsande closed this on Jun 4, 2023

bitcoin locked this on Jun 3, 2024