RPC/Net: Allow changing the connection

luke-jr commented at 11:07 pm on December 2, 2020: member

luke-jr force-pushed on Dec 2, 2020

DrahtBot added the label RPC/REST/ZMQ on Dec 2, 2020

DrahtBot commented at 2:40 am on December 3, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20012 (rpc: Remove duplicate name and argNames from CRPCCommand by MarcoFalke)

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.

RPC/Net: Allow changing the connection_type for addnode onetry f947d91c3d

luke-jr force-pushed on Dec 3, 2020

DrahtBot added the label Needs rebase on Dec 3, 2020

DrahtBot commented at 3:58 am on December 3, 2020: member

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

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

jnewbery commented at 10:18 am on December 3, 2020: member

This still hasn’t addressed the review comment here: #12674 (comment) from 2.5 years ago.

#19315 adds this functionality in the correct way, and doesn’t break the outbound connection counting logic.

luke-jr commented at 4:07 pm on December 3, 2020: member

This still hasn’t addressed the review comment here: #12674 (comment) from 2.5 years ago.

There is nothing to address. It is the expected behaviour.

amitiuttarwar commented at 6:43 pm on December 3, 2020: contributor

I don’t understand the context for these proposed changes (why no PR description?), and the changes are problematic as they stand. For example, passing ConnectionType::INBOUND to OpenNetworkConnection immediately hits an assertion error, because it doesn’t conceptually make sense (how does a node open an inbound connection?).

luke-jr commented at 6:51 pm on December 3, 2020: member

I don’t understand the context for these proposed changes

Two use cases I’m aware of:

Tests
Connecting to a node without giving that node special privileges

and the changes are problematic as they stand. For example, passing ConnectionType::INBOUND to OpenNetworkConnection immediately hits an assertion error, because it doesn’t conceptually make sense (how does a node open an inbound connection?).

I would expect it to simply treat it as an inbound connection (but yes, the bug needs to be fixed - not sure the best way, any suggestions?).

ajtowns commented at 5:32 pm on December 4, 2020: member

I think #19315 handles tests more thoroughly than this (given that PR actually makes the tests use the functionality).

For mainnet, I don’t think feeler or addrfetch connections would have much benefit, and block-relay-only connections should already be maintained where possible via #17428 and trying to manually tweak them probably doesn’t make much sense. I think that mostly leaves privileged-vs-non-priviliged, ie manual vs outbound-full-relay which seems plausible.

Is there actually any reason why onetry shouldn’t always be outbound-full-relay instead of manual? If you actually want to preserve the connection, then reconnecting if it drops out seems like a must, but onetry doesn’t do that.

Matt’s point stands though – creating an additional OUTBOUND_FULL_RELAY connection will make semOutbound inconsistent with the nOutboundFullRelay count in ThreadOpenConnections which leads to misbehaviour. I think it’s along the lines of repeatedly opening a 9th outbound connection, then closing an outbound connection because it only wants to keep 8 around, but there might be some further nuance to it.

fanquake marked this as a draft on Dec 6, 2020

fanquake commented at 3:39 am on December 6, 2020: member

I’ve made this a draft, because it’s not ready for review.

You need to add a PR description, which addresses any outstanding questions & explains the motivation for this change. Does it fix a bug, if so which one, does it need backporting? Even if you don’t think there is anything new information to add from #12674, you should at least be pointing out that there was a previous PR, and associated discussion. I’m not really sure why a new PR was required over #12674.

Please rebase the branch so it will actually compile and the CI will run. There’s also at least one bug that needs fixing.

As much as you don’t seem to believe that issues raised in the previous version of this PR are relevant, clearly you need to better explain why that is the case. This is the kind of thing you’d summarize and make clear in the PR description, rather than leaving it empty and making reviewers chase down the changes history, just to have them re-asked the same questions as in previous PR.

gmaxwell commented at 4:20 am on December 6, 2020: contributor

I agree with Luke-jr that it’s weird that onetry gets privileged treatment.

When I split addnodes into their own connection pool I would have probably treated onetrys like ordinary automatic connections if I’d even given the matter any thought. If I did give it thought perhaps I didn’t do it because it’s not instantly obvious how to handle the outbound connection limit. I think under the original behaviour of onetry (and the original purpose of it, to help a node that was having problems getting good peers get a good peer so that it could learn peers) that if outbounds were full that the onetry would just do nothing. I think that behaviour is still defensible, but I simultaneously see how it could be a little surprising.

I agree with aj that the default behaviour of onetry should just be like an automatic connection. If that were accomplished (correctly) then I think there would be little need for other additions.

I don’t think exposing connection types directly is a good change, except perhaps as some kind of hidden testing feature: Their meaning depends on complex/subtle internal behaviour that will change from release to release. Also being able to handle a user introduced connection of arbitrary type may require excessive generality for little gain. I also doubt that exotic usage of addnode types is likely to get adequate testing or review– so this is a functionality that I would expect to bitrot, or– to the extent that it’s useful at all– have its usefulness broken by future developers that don’t understand the usage (or just by accident, because it’s not being used at all by people who review the software).

ajtowns commented at 6:58 am on December 7, 2020: member

I think the following would allow onetry to be a normal outbound-full-relay connection while keeping the maximums correctly limited. If there’s already 8 outbounds, this will use the “feeler” slot to bump that up to 9 outbounds, and sometime later one outbound will be evicted – probably the one you just added unless a new block appears in the meantime. If desired, you could call disconnectnode Y; addnode X onetry to reduce to 7 outbounds first so that you’ll likely keep the onetry.

 0--- a/src/net.h
 1+++ b/src/net.h
 2@@ -543,7 +543,9 @@ private:
 3      */
 4     ServiceFlags nLocalServices;
 5 
 6+public: // hax0r
 7     std::unique_ptr<CSemaphore> semOutbound;
 8+private:
 9     std::unique_ptr<CSemaphore> semAddnode;
10     int nMaxConnections;
11 
12--- a/src/rpc/net.cpp
13+++ b/src/rpc/net.cpp
14@@ -301,8 +301,12 @@ static RPCHelpMan addnode()
15 
16     if (strCommand == "onetry")
17     {
18+        CSemaphoreGrant grant(*node.connman->semOutbound, true);
19+        if (!grant) {
20+            throw JSONRPCError(RPC_CLIENT_NODE_NOT_ADDED, "Error: No free outbound connection slots.");
21+        }
22         CAddress addr;
23-        node.connman->OpenNetworkConnection(addr, false, nullptr, strNode.c_str(), ConnectionType::MANUAL);
24+        node.connman->OpenNetworkConnection(addr, false, &grant, strNode.c_str(), ConnectionType::OUTBOUND_FULL_RELAY);
25         return NullUniValue;
26     }

Conceivably could also make -connect be outbound-full-relay connections rather than manual – though given if you drop one of them for misbehaving you won’t try reconnecting to anywhere else, that might be a bad idea. If you did do that though, it would mean OpenNetworkConnection could be changed to accept a CSemaphoreGrant& instead of a pointer, removing some special cases. (EDIT: grant, not semaphore)

gmaxwell commented at 8:21 pm on December 12, 2020: contributor

@ajtowns I like that.

ajtowns commented at 8:04 am on January 13, 2021: member

Now that #19315 is merged, a simpler patch is:

0     if (strCommand == "onetry")
1     {
2-        CAddress addr;
3-        node.connman->OpenNetworkConnection(addr, false, nullptr, strNode.c_str(), ConnectionType::MANUAL);
4+        const auto conn_type = ConnectionType::OUTBOUND_FULL_RELAY;
5+        if (!node.connman->AddConnection(strNode, conn_type)) {
6+            throw JSONRPCError(RPC_CLIENT_NODE_CAPACITY_REACHED, strprintf("Error: Already at capacity for %s.", ConnectionTypeAsString(conn_type)));
7+        }
8         return NullUniValue;
9     }

Note that this approach fails the attempt if existing_connections >= max_connections, so will just give an immediate error if you already have 8 outbounds and attempt a onetry, rather than using the feeler slot.

MarcoFalke commented at 9:23 am on January 13, 2021: member

@ajtowns Your patch forgets to update the docs:

 0diff --git a/src/net.h b/src/net.h
 1index 4f1a6b89a9..03c2f4dba6 100644
 2--- a/src/net.h
 3+++ b/src/net.h
 4@@ -939,7 +939,7 @@ public:
 5     std::vector<AddedNodeInfo> GetAddedNodeInfo();
 6 
 7     /**
 8-     * Attempts to open a connection. Currently only used from tests.
 9+     * Attempts to open a connection.
10      *
11      * [@param](/bitcoin-bitcoin/contributor/param/)[in]   address     Address of node to try connecting to
12      * [@param](/bitcoin-bitcoin/contributor/param/)[in]   conn_type   ConnectionType::OUTBOUND or ConnectionType::BLOCK_RELAY

luke-jr commented at 11:30 pm on July 6, 2021: member

Since this was just for tests, addconnection serves the same purpose in theory. Closing.

luke-jr closed this on Jul 6, 2021

rebroad commented at 7:49 pm on September 4, 2021: contributor

Since this was just for tests, addconnection serves the same purpose in theory. Closing.

but only when in regtest mode…. why not in regular mode also?

luke-jr commented at 8:26 pm on September 4, 2021: member

I don’t have a use case for that. If you want to pursue that, I suggest just opening up addconnection to non-regtest instead.

DrahtBot locked this on Sep 4, 2022

RPC/Net: Allow changing the connection_type for addnode onetry #20551

Conflicts