RPC/Net: Allow changing the connection_type for addnode onetry

[luke-jr]

No description provided.

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• rpc: Remove duplicate name and argNames from CRPCCommand #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.

[DrahtBot]

🐙 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]

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]

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]

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]

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]

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]

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]

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]

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.

--- a/src/net.h
+++ b/src/net.h
@@ -543,7 +543,9 @@ private:
      */
     ServiceFlags nLocalServices;
 
+public: // hax0r
     std::unique_ptr<CSemaphore> semOutbound;
+private:
     std::unique_ptr<CSemaphore> semAddnode;
     int nMaxConnections;
 
--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -301,8 +301,12 @@ static RPCHelpMan addnode()
 
     if (strCommand == "onetry")
     {
+        CSemaphoreGrant grant(*node.connman->semOutbound, true);
+        if (!grant) {
+            throw JSONRPCError(RPC_CLIENT_NODE_NOT_ADDED, "Error: No free outbound connection slots.");
+        }
         CAddress addr;
-        node.connman->OpenNetworkConnection(addr, false, nullptr, strNode.c_str(), ConnectionType::MANUAL);
+        node.connman->OpenNetworkConnection(addr, false, &grant, strNode.c_str(), ConnectionType::OUTBOUND_FULL_RELAY);
         return NullUniValue;
     }

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]

@ajtowns I like that.

[ajtowns]

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

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

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.

[maflcko]

@ajtowns Your patch forgets to update the docs:

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

[luke-jr]

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

[rebroad]

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]

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.