RPC: Support addnode onetry without making the connection priviliged

luke-jr commented at 6:03 am on March 12, 2018: member

The behaviour of addnode onetry changed without notice. This restores the ability to at least get the original behaviour.

Needed for properly testing DoS rules.

fanquake added the label RPC/REST/ZMQ on Mar 12, 2018

MarcoFalke commented at 1:11 pm on March 12, 2018: member

rpc_net.py fails

in src/rpc/net.cpp:201 in c21d55ac91 outdated

194@@ -195,17 +195,16 @@ UniValue addnode(const JSONRPCRequest& request)
195     std::string strCommand;
196     if (!request.params[1].isNull())
197         strCommand = request.params[1].get_str();
198-    if (request.fHelp || request.params.size() != 2 ||
199+    if (request.fHelp || request.params.size() < 2 || request.params.size() > 3 ||
200         (strCommand != "onetry" && strCommand != "add" && strCommand != "remove"))
201         throw std::runtime_error(
202-            "addnode \"node\" \"add|remove|onetry\"\n"
203+            "addnode \"node\" \"add|remove|onetry\" (privileged)\n"

promag commented at 2:34 pm on March 12, 2018:

Missing spaces inside ().

in src/rpc/net.cpp:226 in c21d55ac91 outdated

222-        g_connman->OpenNetworkConnection(addr, false, nullptr, strNode.c_str(), false, false, true);
223+        g_connman->OpenNetworkConnection(addr, false, nullptr, strNode.c_str(), false, false, privileged);
224         return NullUniValue;
225     }
226 
227+    if (privileged) {

promag commented at 2:39 pm on March 12, 2018:

Should be if (!request.params[2].isNull()) {? Furthermore, if the default is true then this will always fail.

luke-jr commented at 4:27 pm on March 12, 2018:

request.params[2].isNull() is checked when setting the bool above. But yes, it’s inverted here.

promag commented at 4:31 pm on March 12, 2018:

I mean, it doesn’t matter the value, at this point command is not onetry, so privileged should not be specified.

luke-jr commented at 4:45 pm on March 12, 2018:

privileged=true is required for non-onetry. (Maybe some day we will support false too.)

meshcollider commented at 9:07 pm on March 13, 2018:

The error should be thrown for (!privileged) then, it’s backwards?

in src/rpc/net.cpp:253 in c21d55ac91 outdated

213@@ -215,14 +214,19 @@ UniValue addnode(const JSONRPCRequest& request)
214         throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
215 
216     std::string strNode = request.params[0].get_str();
217+    const bool privileged = request.params[2].isNull() ? true : request.params[2].get_bool();

promag commented at 4:32 pm on March 12, 2018:

WDYT moving this to onetry if block below.

luke-jr commented at 9:13 pm on March 13, 2018:

It’s needed for all cases.

in src/rpc/net.cpp:263 in c21d55ac91 outdated

223+        g_connman->OpenNetworkConnection(addr, false, nullptr, strNode.c_str(), false, false, privileged);
224         return NullUniValue;
225     }
226 
227+    if (privileged) {
228+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Unprivileged connections are only supported for the \"onetry\" command for now");

meshcollider commented at 9:08 pm on March 13, 2018:

This error seems ambiguous to me, could either mean onetry only supports unprivileged, or unprivileged is only supported by onetry and not add

luke-jr commented at 9:13 pm on March 13, 2018:

I don’t see how it could mean the former in standard English.

meshcollider commented at 9:09 pm on March 13, 2018: contributor

Concept ACK

luke-jr force-pushed on Mar 13, 2018

TheBlueMatt commented at 7:17 pm on March 28, 2018: member

This breaks our outbound-connection-counting logic in the aut-connect loop as it uses the manual connection flag to figure out how many other connections to make.

in src/rpc/net.cpp:207 in 0670e4fffc outdated

206-            "Nodes added using addnode (or -connect) are protected from DoS disconnection and are not required to be\n"
207-            "full nodes/support SegWit as other outbound peers are (though such peers will not be synced from).\n"
208             "\nArguments:\n"
209             "1. \"node\"     (string, required) The node (see getpeerinfo for nodes)\n"
210             "2. \"command\"  (string, required) '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\n"
211+            "3. privileged (boolean, optional, default=true) If true, nodes added will be protected from DoS disconnection and not required to be full nodes or support segwit as other outbound peers are (though such peers will not be synced from). Only supported for command \"onetry\" for now.\n"

jnewbery commented at 8:08 pm on March 28, 2018:

This setting is called ‘manual_connection’ internally. I don’t think it makes sense to add new terminology here.

luke-jr commented at 7:30 am on March 31, 2018:

manual_connection does not express the nature of the flag.

jnewbery commented at 8:09 pm on March 28, 2018: member

Why no PR description? What’s the use case for this?

luke-jr commented at 8:38 am on March 31, 2018: member

Added a PR description, and kicked Travis.

DrahtBot closed this on Jul 21, 2018

DrahtBot reopened this on Jul 21, 2018

sipa commented at 9:21 pm on July 21, 2018: member

Can you address @TheBlueMatt’s comment here: #12674 (comment) ?

luke-jr commented at 9:29 pm on July 21, 2018: member

@TheBlueMatt @sipa I don’t consider that broken. A non-privileged connection should be treated the same as any other normal outgoing connection.

DrahtBot commented at 11:08 am on October 20, 2018: 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:

#18740 (Remove g_rpc_node global by ryanofsky)
#18606 (test: checks that bitcoin-cli autocomplete is in sync by pierreN)
#18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones 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 added the label Needs rebase on Nov 13, 2018

luke-jr commented at 2:16 pm on February 12, 2019: member

Rebased

luke-jr force-pushed on Feb 12, 2019

DrahtBot removed the label Needs rebase on Feb 12, 2019

MarcoFalke added the label Needs rebase on Feb 12, 2019

in src/rpc/net.cpp:224 in 73891da8b7 outdated

223-                "full nodes/support SegWit as other outbound peers are (though such peers will not be synced from).\n",
224+                "Or try a connection to a node once.\n",
225                 {
226                     {"node", RPCArg::Type::STR, /* opt */ false, /* default_val */ "", "The node (see getpeerinfo for nodes)"},
227                     {"command", RPCArg::Type::STR, /* opt */ false, /* default_val */ "", "'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"},
228+                    {"privileged", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "true", "If true, nodes added will be protected from DoS disconnection and not required to be full nodes or support segwit as other outbound peers are (though such peers will not be synced from). Only supported for command \"onetry\" for now."},

MarcoFalke commented at 11:56 pm on February 12, 2019:

0                    {"privileged", RPCArg::Type::BOOL, /* default */ "true", "If true, nodes added will be protected from DoS disconnection and not required to be full nodes or support segwit as other outbound peers are (though such peers will not be synced from). Only supported for command \"onetry\" for now."},

luke-jr commented at 1:22 pm on February 13, 2019:

I don’t see how that would compile.

MarcoFalke commented at 4:04 pm on February 13, 2019:

I understand your frustration you recently raised on IRC that pull requests frequently need to be rebased and that you prefer not to rebase unless the pull request is ready for merge. However, a pull request that needs rebase is less likely to receive review, since reviewers can’t trivially run the latest tests on it (without having to solve a merge conflict). Also, they’d need to redo the solution of the merge conflict after you solve it to re-ACK, so that drives back reviewers even more. I think solving trivial merge conflicts is a nice courtesy to reviewers. If the pull request in general does not attract reviewers that seems like a separate problem.

MarcoFalke deleted a comment on Feb 12, 2019

luke-jr force-pushed on May 2, 2019

DrahtBot removed the label Needs rebase on May 2, 2019

DrahtBot added the label Needs rebase on Oct 30, 2019

RPC: Support addnode onetry without making the connection priviliged 7711302913

luke-jr force-pushed on Mar 27, 2020

DrahtBot removed the label Needs rebase on Mar 27, 2020

in src/rpc/net.cpp:231 in 7711302913

227@@ -228,17 +228,16 @@ static UniValue addnode(const JSONRPCRequest& request)
228     std::string strCommand;
229     if (!request.params[1].isNull())
230         strCommand = request.params[1].get_str();
231-    if (request.fHelp || request.params.size() != 2 ||
232+    if (request.fHelp || request.params.size() < 2 || request.params.size() > 3 ||

pierreN commented at 12:55 pm on April 14, 2020:

#18606 tries to unify the coding-style of all non-hidden RPC calls by using RPCHelpMan::Check

If #18606 gets merged before this PR, this change shouldn’t be necessary (should be automatically handled by RPCHelpMan::IsValidNumArgs). Only the line adding the parameter privileged in RPCHelpMan below will be necessary.

And thanks to #18606 the new bash auto-completion file will be generated automatically by running test/functional/tool_cli_completion.py --overwrite before committing :smiley:

If this PR gets merged before #18606 I’ll update my PR accordingly.

DrahtBot commented at 12:12 pm on May 21, 2020: member

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

DrahtBot added the label Needs rebase on May 21, 2020

luke-jr closed this on Dec 2, 2020

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

Superceding with #20551

DrahtBot locked this on Feb 15, 2022

RPC: Support addnode onetry without making the connection priviliged #12674

Conflicts