Add updatepeer RPC #10160

jnewbery · 2017-04-05T21:48:52Z

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.

theuni · 2017-04-06T18:05:47Z

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 · 2017-04-06T18:26:31Z

@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 · 2017-04-06T18:27:34Z

Ah... an there is already the disconnectnode RPC call.

jnewbery · 2017-04-06T20:22:37Z

@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 · 2017-04-07T20:57:08Z

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 · 2017-10-26T22:19:00Z

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

promag

Concept ACK. Will test.

promag · 2017-10-27T10:25:49Z

src/rpc/net.cpp

Is null instead.

promag · 2017-10-27T10:29:03Z

src/rpc/net.cpp

If not then raise invalid parameter?

Missing test for invalid node.

promag · 2017-10-27T10:31:55Z

src/rpc/net.cpp

Not null instead.

jnewbery · 2017-10-27T14:06:33Z

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

NicolasDorier · 2017-10-27T14:32:40Z

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.

promag

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

promag · 2017-10-27T14:35:38Z

src/rpc/net.cpp

if (!...) { throw ...; }

promag · 2017-10-27T14:39:24Z

src/rpc/net.cpp

Nit, > 1.

promag · 2017-10-27T14:39:49Z

src/rpc/net.cpp

Nit, same line or { }.

promag · 2017-10-27T14:41:09Z

src/net.cpp

Nit, missing spaces after for.

promag · 2017-10-27T14:41:47Z

src/rpc/net.cpp

Same as above.

promag · 2017-10-27T14:44:17Z

src/rpc/net.cpp

Nit, space after if.

promag · 2017-10-27T14:46:17Z

src/rpc/net.cpp

Same validation of getpeerinfo.

promag · 2017-10-27T14:46:52Z

src/rpc/net.cpp

Needs release notes.

promag · 2017-10-27T14:49:28Z

src/rpc/net.cpp

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.

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

promag · 2017-10-27T14:50:26Z

test/functional/net.py

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

jnewbery · 2017-10-27T19:43:49Z

@promag nits addressed in latest commit.

promag

utACK after squash.

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

updatepeer 1 whitelisted true
updatepeer 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.

promag · 2017-10-31T14:59:01Z

src/net.cpp

Nit, missing space after for. Same below.

promag · 2017-10-31T15:22:19Z

src/rpc/net.cpp

Remove lock or am I missing something?

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

NicolasDorier · 2017-10-31T19:36:38Z

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

jnewbery · 2017-10-31T22:40:00Z

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 · 2017-11-02T00:59:09Z

@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 · 2017-11-02T01:15:55Z

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 😄

NicolasDorier · 2017-11-30T02:04:15Z

src/rpc/net.cpp

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)

NicolasDorier · 2017-11-30T02:07:33Z

src/net.cpp

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

NicolasDorier · 2017-12-07T07:33:49Z

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

jnewbery · 2017-12-07T16:43:41Z

@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.

NicolasDorier · 2017-12-08T05:15:11Z

@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 · 2017-12-08T16:00:35Z

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 · 2017-12-08T17:44:06Z

@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():

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

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

TheBlueMatt · 2017-12-08T18:43:30Z

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 · 2017-12-08T21:38:34Z

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

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.

kallewoof

utACK

Needs rebase.

kallewoof · 2018-02-22T08:51:34Z

src/net.cpp

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

refactored out

kallewoof · 2018-02-22T08:53:22Z

src/rpc/net.cpp

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

kallewoof · 2018-02-22T08:56:08Z

src/rpc/net.cpp

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

kallewoof · 2018-02-22T08:57:04Z

src/rpc/net.cpp

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

jnewbery · 2018-05-24T19:57:52Z

Fixed @kallewoof review comments.

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

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

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

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.

jnewbery · 2018-07-23T17:56:35Z

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

NicolasDorier · 2018-07-24T03:23:34Z

I liked it :(

maflcko · 2022-05-04T13:23:20Z

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

jnewbery mentioned this pull request Apr 5, 2017

[net] Allow disconnectnode RPC to be called with node id #10143

Merged

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

jnewbery closed this Apr 7, 2017

jnewbery reopened this Oct 26, 2017

jnewbery force-pushed the updatepeer branch from 9792a9a to 4a51dbd Compare October 26, 2017 22:17

promag reviewed Oct 27, 2017

View reviewed changes

jnewbery force-pushed the updatepeer branch 2 times, most recently from 172e302 to c799a6c Compare October 27, 2017 14:05

jnewbery changed the title ~~[WIP] updatepeer RPC~~ updatepeer RPC Oct 27, 2017

promag reviewed Oct 27, 2017

View reviewed changes

jnewbery changed the title ~~updatepeer RPC~~ Add updatepeer RPC Oct 27, 2017

jnewbery force-pushed the updatepeer branch from c799a6c to 91fab00 Compare October 27, 2017 19:42

jnewbery mentioned this pull request Oct 27, 2017

Disconnect outbound peers on invalid chains #11568

Merged

jnewbery force-pushed the updatepeer branch from 91fab00 to b0da202 Compare October 27, 2017 20:45

maflcko added the P2P label Oct 28, 2017

promag reviewed Oct 31, 2017

View reviewed changes

jnewbery force-pushed the updatepeer branch from b0da202 to c072501 Compare November 14, 2017 18:33

NicolasDorier reviewed Nov 30, 2017

View reviewed changes

src/net.cpp Outdated

Copy link

Contributor

NicolasDorier Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jnewbery force-pushed the updatepeer branch from c072501 to 0259014 Compare December 7, 2017 16:41

jnewbery force-pushed the updatepeer branch from 0259014 to 0a00200 Compare December 7, 2017 16:44

ryanofsky reviewed Dec 20, 2017

View reviewed changes

ajtowns mentioned this pull request Jan 16, 2018

[tests] Rename functional tests #11774

Merged

kallewoof reviewed Feb 22, 2018

View reviewed changes

jnewbery force-pushed the updatepeer branch 2 times, most recently from 16900c5 to fd18f4e Compare May 24, 2018 19:57

jnewbery force-pushed the updatepeer branch from fd18f4e to bd0182c Compare May 29, 2018 15:26

jnewbery added 5 commits July 23, 2018 10:37

[RPC] Refactor getpeerinfo RPC to call helper function NodeStatsToJSO…

5f35cdf

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

[net] Add GetNodeByID helper function.

30bed9e

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

4fae4a4

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

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

2a76460

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

jnewbery force-pushed the updatepeer branch from bd0182c to 7807d95 Compare July 23, 2018 15:25

jnewbery closed this Jul 23, 2018

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

maflcko added the Up for grabs label May 4, 2022

Add updatepeer RPC #10160

Add updatepeer RPC #10160

Uh oh!

Conversation

jnewbery commented Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni commented Apr 6, 2017

Uh oh!

jonasschnelli commented Apr 6, 2017

Uh oh!

jonasschnelli commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Apr 6, 2017

Uh oh!

jnewbery commented Apr 7, 2017

Uh oh!

jnewbery commented Oct 26, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Oct 27, 2017

Uh oh!

NicolasDorier commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Oct 27, 2017

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasDorier commented Oct 31, 2017

Uh oh!

jnewbery commented Oct 31, 2017

Uh oh!

NicolasDorier commented Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

jnewbery commented Apr 5, 2017 •

edited

Loading

jonasschnelli commented Apr 6, 2017 •

edited

Loading

NicolasDorier commented Oct 27, 2017 •

edited

Loading

promag left a comment •

edited

Loading

NicolasDorier commented Nov 2, 2017 •

edited

Loading

promag commented Nov 2, 2017 •

edited

Loading

NicolasDorier Nov 30, 2017 •

edited

Loading

NicolasDorier commented Dec 8, 2017 •

edited

Loading

TheBlueMatt commented Dec 8, 2017 •

edited

Loading

kallewoof left a comment •

edited

Loading