Add updatepeer RPC

jnewbery commented at 9:48 pm on April 5, 2017: member

Adds an updatepeer RPC to update peer config and carry out actions on the peer by NodeId. At the moment, the actions are just changing whether the peer is whitelisted and whether the peer is a manual_connection (previously called an addnode). Future possible actions would be changing banscore, banning, and so on.

This is designed to be called using named arguments, but due to the RPC infrastructure can also be called with positional arguments (although doing so would be very fiddly).

I’ve set the category to hidden for now. We may want to make the whitelisting behaviour more granular in future and I don’t want to commit us to a public API that we can’t then change.

fanquake added the label RPC/REST/ZMQ on Apr 5, 2017

theuni commented at 6:05 pm on April 6, 2017: member

I’m not sure I like the idea of adding a command that changes the implied state of a peer via its attributes, rather than performing actions explicitly. The whitelist side-effects are especially hazy, see the brief discussion here: #10051 (comment).

jonasschnelli commented at 6:26 pm on April 6, 2017: contributor

@jnewbery: Can you elaborate the use-case for disconnect? IMO setban provides a similar interface, with disconnecting & banning for a specific timespan (1h, etc.) because, a pure disconnect does not prevent the peer from a direct re-connect.

jonasschnelli commented at 6:27 pm on April 6, 2017: contributor

Ah… an there is already the disconnectnode RPC call.

jnewbery commented at 8:22 pm on April 6, 2017: member

@jonasschnelli see #2729 and #6271 for history of the disconnectnode RPC. It’s also useful in testing to be able to control the topology of the test nodes. @theuni - I agree that whitelisting is a mess and should be broken out into bits for controlling individual behaviours. Hence my original comment: “I have a feeling we may want to make the whitelisting behaviour more granular and I don’t want to commit us to a public API that we can’t then change.” I was thinking of your comment in #10051 but couldn’t find the reference.

the idea of adding a command that changes the implied state of a peer via its attributes, rather than performing actions explicitly

I don’t understand what this means in the context of whitelisting. The idea is to update the peer’s whitelist behaviour. What would performing actions explicitly entail?

jnewbery commented at 8:57 pm on April 7, 2017: member

Closing in favour of #10143 for the immediate need (disconnect node by id), but I think this could still be a useful RPC in the future.

jnewbery closed this on Apr 7, 2017

jnewbery reopened this on Oct 26, 2017

jnewbery force-pushed on Oct 26, 2017

jnewbery commented at 10:19 pm on October 26, 2017: member

Reopening with just the ability to update:

fWhitelisted
m_manual_connection

The net_processing functionality for the v0.15.0.2 PRs is disabled for manual connections, so this PR could be helpful for testing those changes. @sdaftuar

in src/rpc/net.cpp:243 in 4a51dbde81 outdated

290 
291-        UniValue recvPerMsgCmd(UniValue::VOBJ);
292-        for (const mapMsgCmdSize::value_type &i : stats.mapRecvBytesPerMsgCmd) {
293-            if (i.second > 0)
294-                recvPerMsgCmd.push_back(Pair(i.first, i.second));
295+    if (request.params.size() >= 3) {

promag commented at 10:25 am on October 27, 2017:

Is null instead.

in src/rpc/net.cpp:255 in 4a51dbde81 outdated

302 
303-        ret.push_back(obj);
304+    UniValue entry(UniValue::VOBJ);
305+    CNodeStats stats;
306+
307+    if (g_connman->GetNodeStats(nodeid, stats)) {

promag commented at 10:29 am on October 27, 2017:

If not then raise invalid parameter?

promag commented at 10:29 am on October 27, 2017:

Missing test for invalid node.

in src/rpc/net.cpp:188 in 4a51dbde81 outdated

186-    g_connman->GetNodeStats(vstats);
187 
188     UniValue ret(UniValue::VARR);
189 
190-    for (const CNodeStats& stats : vstats) {
191+    if (request.params.size() >= 1 && request.params[0].isNum()) {

promag commented at 10:31 am on October 27, 2017:

Not null instead.

promag commented at 10:32 am on October 27, 2017: member

Concept ACK. Will test.

jnewbery force-pushed on Oct 27, 2017

jnewbery renamed this:
~~[WIP] updatepeer RPC~~
updatepeer RPC
on Oct 27, 2017

jnewbery commented at 2:06 pm on October 27, 2017: member

Thanks for the review @promag . I’ve addressed all your comments.

NicolasDorier commented at 2:32 pm on October 27, 2017: contributor

whitelisting via rpc?? BIG BIG concept ACK. I am excited. It will make configuration of services depending on Bitcoin Core RPC and P2P so much easier.

in src/rpc/net.cpp:192 in c799a6c16d outdated

230-            }
231-            obj.push_back(Pair("inflight", heights));
232+        CNodeStats stats;
233+
234+        int node_id = request.params[0].get_int();
235+        if (g_connman->GetNodeStats(node_id, stats)) {

promag commented at 2:35 pm on October 27, 2017:

0if (!...) {
1    throw ...;
2}

in src/rpc/net.cpp:129 in c799a6c16d outdated

125+}
126+
127 UniValue getpeerinfo(const JSONRPCRequest& request)
128 {
129-    if (request.fHelp || request.params.size() != 0)
130+    if (request.fHelp || request.params.size() >= 2)

promag commented at 2:39 pm on October 27, 2017:

Nit, > 1.

promag commented at 2:46 pm on October 27, 2017:

Needs release notes.

in src/rpc/net.cpp:114 in c799a6c16d outdated

109+    }
110+    obj.push_back(Pair("whitelisted", stats.fWhitelisted));
111+
112+    UniValue sendPerMsgCmd(UniValue::VOBJ);
113+    for (const mapMsgCmdSize::value_type &i : stats.mapSendBytesPerMsgCmd) {
114+        if (i.second > 0)

promag commented at 2:39 pm on October 27, 2017:

Nit, same line or { }.

in src/net.cpp:2551 in c799a6c16d outdated

2520@@ -2521,6 +2521,18 @@ size_t CConnman::GetNodeCount(NumConnections flags)
2521     return nNum;
2522 }
2523 
2524+bool CConnman::GetNodeStats(NodeId nodeid, CNodeStats& stats)
2525+{
2526+    LOCK(cs_vNodes);
2527+    for(CNode* pnode : vNodes) {

promag commented at 2:41 pm on October 27, 2017:

Nit, missing spaces after for.

in src/rpc/net.cpp:121 in c799a6c16d outdated

116+    }
117+    obj.push_back(Pair("bytessent_per_msg", sendPerMsgCmd));
118+
119+    UniValue recvPerMsgCmd(UniValue::VOBJ);
120+    for (const mapMsgCmdSize::value_type &i : stats.mapRecvBytesPerMsgCmd) {
121+        if (i.second > 0)

promag commented at 2:41 pm on October 27, 2017:

Same as above.

in src/rpc/net.cpp:215 in c799a6c16d outdated

258+    return ret;
259+}
260+
261+UniValue updatepeer(const JSONRPCRequest& request)
262+{
263+    if (request.fHelp || request.params.size() > 3 || request.params.size() == 0)

promag commented at 2:43 pm on October 27, 2017:

Nit, the most common expression is

0if (request.fHelp || request.params.size() < 1 || request.params.size() > 3)

in src/rpc/net.cpp:229 in c799a6c16d outdated

272+            "\nExamples:\n"
273+            + HelpExampleCli("updatepeer", "0 true")
274+            + HelpExampleRpc("updatepeer", "0, true")
275+        );
276+
277+    if(!g_connman) {

promag commented at 2:44 pm on October 27, 2017:

Nit, space after if.

in src/rpc/net.cpp:236 in c799a6c16d outdated

279+    }
280+
281+    LOCK(cs_main);
282+
283+    RPCTypeCheckArgument(request.params[0], UniValue::VNUM);
284+    int nodeid = request.params[0].get_int();

promag commented at 2:46 pm on October 27, 2017:

Same validation of getpeerinfo.

in src/rpc/net.cpp:198 in c799a6c16d outdated

229-                heights.push_back(height);
230-            }
231-            obj.push_back(Pair("inflight", heights));
232+        CNodeStats stats;
233+
234+        int node_id = request.params[0].get_int();

promag commented at 2:49 pm on October 27, 2017:

Actually I think this is more correct:

raise RPC_INVALID_PARAMETER if node_id < 0;
raise RPC_INVALID_ADDRESS_OR_KEY if !GetNodeStats(...).

Note: if you do this then add a test for 1st case.

jnewbery commented at 7:43 pm on October 27, 2017:

I don’t think it’s too important, but feel free to open a follow-up PR if you think it’s required.

in test/functional/net.py:122 in c799a6c16d outdated

117+        peer_info = self.nodes[0].getpeerinfo()[0]
118+        assert peer_info['whitelisted']
119+        assert not peer_info['addnode']
120+
121+        # Check that updatepeer fails for an invalid peer_id
122+        assert_raises_rpc_error(-8, "not found", self.nodes[0].updatepeer, 5000)

promag commented at 2:50 pm on October 27, 2017:

Missing failure tests for getpeerinfo (maybe unrelated to this PR).

promag commented at 2:56 pm on October 27, 2017: member

Nit, rename PR to Add updatepeer RPC. Squash [RPC] Allow manual_connection to be updated using updatepeer into [RPC] Add updatenode RPC.

jnewbery renamed this:
~~updatepeer RPC~~
Add updatepeer RPC
on Oct 27, 2017

jnewbery force-pushed on Oct 27, 2017

jnewbery commented at 7:43 pm on October 27, 2017: member

@promag nits addressed in latest commit.

jnewbery force-pushed on Oct 27, 2017

MarcoFalke added the label P2P on Oct 28, 2017

in src/net.cpp:2589 in b0da202a28 outdated

2564@@ -2553,6 +2565,30 @@ bool CConnman::DisconnectNode(NodeId id)
2565     return false;
2566 }
2567 
2568+bool CConnman::SetWhitelisted(NodeId id, bool fWhitelisted)
2569+{
2570+    LOCK(cs_vNodes);
2571+    for(CNode* pnode : vNodes) {

promag commented at 2:59 pm on October 31, 2017:

Nit, missing space after for. Same below.

jnewbery commented at 3:09 pm on December 7, 2017:

fixed

in src/rpc/net.cpp:234 in b0da202a28 outdated

282+
283+    if (!g_connman) {
284+        throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
285+    }
286+
287+    LOCK(cs_main);

promag commented at 3:22 pm on October 31, 2017:

Remove lock or am I missing something?

jnewbery commented at 3:11 pm on December 7, 2017:

you’re right - I don’t think this is required

promag commented at 3:25 pm on October 31, 2017: member

utACK after squash.

Nit, how about updatepeer id setting value (setting value ...):

0updatepeer 1 whitelisted true
1updatepeer 1 manual_connection false

It works well without named arguments with the cli, and scales well for other settings.

Note, I always prefer named arguments, specially in these calls. But we do support index arguments and this approach doesn’t help for future settings.

NicolasDorier commented at 7:36 pm on October 31, 2017: contributor

I agree with @promag updatepeer id setting value (setting value ...) would be easier to extend later.

jnewbery commented at 10:40 pm on October 31, 2017: member

Nit, how about updatepeer id setting value (setting value …)

This is a different scheme from all of the existing RPCs. I expect that there would need to be changes to the rpc framework to make this work.

It doesn’t make sense to me have a completely different scheme for just this RPC method, since we already have named arguments.

If you disagree, perhaps you could implement the scheme you’re talking about in a new branch?

NicolasDorier commented at 0:59 am on November 2, 2017: contributor

@jnewbery how so? This is just 3 differents parameters of 3 strings.

The way you are doing now, if there is like 60 settings for one peer, we will have 60 parameters to this function. This does not seem very maintainable. In such case even updatewhitelist id true would be better. We would have 60 functions, which is still better than 60 parameters.

promag commented at 1:15 am on November 2, 2017: member

This is designed to be called using named arguments, but due to the RPC infrastructure can also be called with positional arguments (although doing so would be very fiddly). @jnewbery you too agree that this is by design bad for positional arguments (considering the possible settings can be extended).

Another approach is to use something like sendmany: updatepeer {\"setting\":value, ...}.

FWIW I already gave my utACK 😄

jnewbery force-pushed on Nov 14, 2017

jnewbery commented at 6:34 pm on November 14, 2017: member

@NicolasDorier

how so? This is just 3 differents parameters of 3 strings

Right, but all RPCs currently support positional arguments, where the position of the argument determines its meaning. I don’t necessarily agree that it’s always appropriate, but having a single RPC that doesn’t adhere to that scheme is confusing and inconsistent.

Using named arguments have been around for at least two releases now, so there’s no need to use positional arguments at all.

Rebased

NicolasDorier commented at 12:49 pm on November 20, 2017: contributor

yeah we talked about it, I understand now why named args make sense. Concept ACK.

jnewbery commented at 9:25 pm on November 29, 2017: member

@NicolasDorier - you’ve given this a BIG BIG concept ACK. Mind reviewing? :slightly_smiling_face:

NicolasDorier commented at 1:57 am on November 30, 2017: contributor

Sure will review and test today.

in src/net.cpp:2601 in c072501ac4 outdated

2596+}
2597+
2598+bool CConnman::SetManualConnection(NodeId id, bool manual)
2599+{
2600+    LOCK(cs_vNodes);
2601+    for(CNode* pnode : vNodes) {

NicolasDorier commented at 2:02 am on November 30, 2017:

The loop will get repeated all over the place for each field. (Already two times)

I think a better approach would be to lock at updatepeer level and fetch the right CNode there at the beginning of the call. And change fields directly from there.

in src/rpc/net.cpp:250 in c072501ac4 outdated

295+    }
296+
297+    if (!request.params[1].isNull()) {
298+        RPCTypeCheckArgument(request.params[1], UniValue::VBOOL);
299+        if (!g_connman->SetWhitelisted(node_id, request.params[1].isTrue())) {
300+            throw JSONRPCError(RPC_MISC_ERROR, "Failed to update peer whitelisting");

NicolasDorier commented at 2:04 am on November 30, 2017:

See my above comment, by fetching once the node at the start of this method, you can remove all those if as you already know if the CNode with such id already exist. It makes things a bit easier to test and prevent monkey coding each time we want to add a new field.

This RPC error is also untestable as it makes you reproduce a difficult race condition. (need to drop the node after the previosu GetNodeStates but before this call)

in src/net.cpp:2543 in c072501ac4 outdated

2538@@ -2539,6 +2539,18 @@ size_t CConnman::GetNodeCount(NumConnections flags)
2539     return nNum;
2540 }
2541 
2542+bool CConnman::GetNodeStats(NodeId nodeid, CNodeStats& stats)

NicolasDorier commented at 2:07 am on November 30, 2017:

This method would also be removed if fetching CNode was done at the updatepeer level.

NicolasDorier commented at 7:33 am on December 7, 2017: contributor

@jnewbery do you need more help on this PR? I am really interested into seeing it merged.

jnewbery force-pushed on Dec 7, 2017

jnewbery commented at 4:43 pm on December 7, 2017: member

@NicolasDorier: Sorry for dropping this - I’ve rebased on master and squashed all nits.

I’m not sure about your suggestion for locking in updatepeer and fetching the CNode. cs_vNodes isn’t currently locked anywhere outsdie CConman, which I think is a good property to maintain.

jnewbery force-pushed on Dec 7, 2017

NicolasDorier commented at 5:15 am on December 8, 2017: contributor

@jnewbery in this case, I would suggest to have a method on CConman bool CConman::UpdateSetting(nodeid, str,value) because there is high ratio of ceremonial monkey copy pasta code everytimes we will need to add one property here.

TheBlueMatt commented at 4:00 pm on December 8, 2017: member

I’m still with @theuni on this one, not super happy with the idea of changing properties about peers that net_processing/net both consider “constant”. Would prefer we add some (undocumented?) options to addnode (or a new RPC) which lets you control the flags of a peer as you create a connection.

jnewbery commented at 5:44 pm on December 8, 2017: member

@theuni’s comment was:

the idea of adding a command that changes the implied state of a peer via its attributes, rather than performing actions explicitly

I don’t understand what’s meant by ‘performing actions explicitly’, but I’m happy to modify this PR if there are concrete suggestions.

not super happy with the idea of changing properties about peers that net_processing/net both consider “constant”

Can you articulate what makes you not happy? Being able to change the properties of a connection without having to delete/recreate that object is generally very useful, since cycling a connection has many side-effects. As far as I can see, the only stateful impact of starting with fWhitelisted rather than updating it later is this call to AddLocal() in BindListenPort():

0    if (addrBind.IsRoutable() && fDiscover && !fWhitelisted)
1        AddLocal(addrBind, LOCAL_BIND);

I can’t see any other reason to be concerned about updating fWhitelisted dynamically.

TheBlueMatt commented at 6:43 pm on December 8, 2017: member

I might be slightly more OK with changing fWhitelisted on a peer after its up, as that may be useful “in the real world” outside of tests, but I’m super not a fan of changing manual_connection. One thing @theuni and I have talked about is being more aggressive about CNode representing a “connection handle” where its potentially allowed to have a few constant members which are general information about the connection, eg whether it was automatically or manually added, the remote address, etc. One thing you might imagine is inserting a peer into a set/map (eg compact blocks HB mode peers set, the (implied) set of nPreferredDownload peers (which we currently track with fPreferredDownload, but we could actually take it as implication based on const values in CNode)) or otherwise performing (between-message-)stateful behavior on a peer based on such (constant) information about a connection. In each of these cases, having such constant information change out from under you could potentially introduce races/bugs.

cycling a connection has many side-effects.

I’m not sure what side-effects there are that couldn’t be reproduced on a new connection after cycling the connection. It ends up being an unrealistic test if you dont do that anyway. Maybe I’m just missing the motivation of cases that wouldnt be better tested by cycling a connection?

jnewbery commented at 9:38 pm on December 8, 2017: member

One thing you might imagine…

You’re describing things that may happen in future changes in abstract terms, particularly when you talk about between-message-stateful behaviour.

m_manual_connection is certainly a better name than fAddNode, but I still don’t think it really captures what the property means. Really it’s something like ‘preferred_peer’ - it’s a peer that we don’t want to disconnect or punish for bad behaviour. How it was connected isn’t really relevant. We may want to prefer a peer that has connected inbound to us, and we may want to connect outbound to a peer without preferring it. In that context, I think it makes sense to want to set this property manually after connection, without having to disconnect and go through the version-verack-getheaders-headers handshaking.

This is undoubtedly useful for testing, but I could also imagine it being useful, for example in a business that has more than one bitcoin node and wants to be able to manage their network topology.

Thinking forward a bit more, I think it’d also be beneficial to break out the whitelist behaviour to be more granular, and again be able to update those properties dynamically.

ryanofsky commented at 4:50 pm on December 20, 2017: member

Haven’t been following discussion in much detail but if the problem is fAddNode having both const and non-const interpretations, maybe it should just be split into two variables: m_connection_type that would be const and record origin of connection and m_preferred_peer that would be mutable.

in src/net.cpp:2622 in 0a002002f3 outdated

2610+            return true;
2611+        }
2612+    }
2613+    return false;
2614+}
2615+

kallewoof commented at 8:51 am on February 22, 2018:

Could you maybe make an (optionally inline) method find_node_by_id that the above methods use? Looks a bit copy-pastey.

jnewbery commented at 7:54 pm on May 24, 2018:

refactored out

in src/rpc/net.cpp:141 in 0a002002f3 outdated

138         throw std::runtime_error(
139             "getpeerinfo\n"
140-            "\nReturns data about each connected network node as a json array of objects.\n"
141+            "\nReturns data about connected network nodes as a json array of objects.\n"
142+            "\nArguments:\n"
143+            "1. \"id\"         (numeric, optional) Only return information about the peer with the specified id.\n"

kallewoof commented at 8:53 am on February 22, 2018:

Nit: No period at end (seems to be the norm). Also add this to the first line ("getpeerinfo \"id\"\n")

jnewbery commented at 7:56 pm on May 24, 2018:

fixed

in src/rpc/net.cpp:223 in 0a002002f3 outdated

267+
268+UniValue updatepeer(const JSONRPCRequest& request)
269+{
270+    if (request.fHelp || request.params.size() < 1 || request.params.size() > 3)
271+        throw std::runtime_error(
272+            "updatepeer\n"

kallewoof commented at 8:56 am on February 22, 2018:

"updatepeer \"id\" ( \"whitelisted\" \"manual_connection\" )\n"

in src/rpc/net.cpp:244 in 0a002002f3 outdated

292+    RPCTypeCheckArgument(request.params[0], UniValue::VNUM);
293+    int node_id = request.params[0].get_int();
294+
295+    CNodeStats stats;
296+    if (!g_connman->GetNodeStats(node_id, stats)) {
297+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Node id %u not found", node_id));

kallewoof commented at 8:57 am on February 22, 2018:

Nit: node_id is an int, so %d not %u.

jnewbery commented at 7:57 pm on May 24, 2018:

fixed

kallewoof commented at 8:58 am on February 22, 2018: member

utACK

Needs rebase.

jnewbery force-pushed on May 24, 2018

jnewbery commented at 7:57 pm on May 24, 2018: member

Fixed @kallewoof review comments.

jnewbery force-pushed on May 29, 2018

[RPC] Refactor getpeerinfo RPC to call helper function NodeStatsToJSON().

This commit refactors getpeerinfo() to call a helper function
NodeStatsToJSON() to build a JSON object with peer information.

5f35cdf66b

[net] Add GetNodeByID helper function. 30bed9eaa9

[RPC] Add GetNodeStats() function for individual node.

GetNodeStats() can now be called for a single node.

4fae4a4f63

[RPC] Allow getpeerinfo to be called for a single node.

The getpeerinfo RPC can now be called for a single node by specifying the
node id.

2a764602ea

[RPC] Add updatepeer RPC.

This commit adds an updatepeer RPC for updating a single peer node.

Currently, updatepeer can be used to update the whitelisting and
manual_connection settings for the node. It could potentially be
extended in future to allow updating other attributes of the peer node.

7807d959d5

jnewbery force-pushed on Jul 23, 2018

jnewbery commented at 5:56 pm on July 23, 2018: member

There doesn’t seem to much appetite for this. Closing.

jnewbery closed this on Jul 23, 2018

NicolasDorier commented at 3:23 am on July 24, 2018: contributor

I liked it :(

DrahtBot locked this on Sep 8, 2021

MarcoFalke added the label Up for grabs on May 4, 2022

MarcoFalke commented at 1:23 pm on May 4, 2022: member

Marked “up for grabs”. (Conversation is locked, so a new pull will need to be opened either way).

Add updatepeer RPC #10160