rpc: deprecate banscore field in getpeerinfo

jonatack commented at 11:40 am on July 8, 2020: member

Per #19219 (review) and #19219 (comment), this PR deprecates returning the banscore field in the getpeerinfo RPC, updates the help, adds a test, and updates the release notes. Related to #19464.

rpc: deprecate banscore field in rpc getpeerinfo 8c7647b3fb

test: getpeerinfo banscore deprecation test dd54e3796e

MarcoFalke added this to the milestone 0.21.0 on Jul 8, 2020

in doc/release-notes-19469.md:6 in 454a873624 outdated

0@@ -0,0 +1,6 @@
1+Updated RPCs
2+------------
3+
4+- `getpeerinfo` no longer returns the `banscore` field unless the configuration
5+  option `-deprecatedrpc=banscore` is used. The `banscore` field will be fully
6+  removed in the next release. (#19469)

MarcoFalke commented at 1:02 pm on July 8, 2020:

0  removed in the next major release. (#19469)

jonatack commented at 1:12 pm on July 8, 2020:

thanks – done

MarcoFalke approved

MarcoFalke commented at 1:03 pm on July 8, 2020: member

reviewed on GitHub ACK 454a8736244129b32ff2cd2efee7c913eafb179c

MarcoFalke added the label RPC/REST/ZMQ on Jul 8, 2020

doc: getpeerinfo banscore deprecation release note 41d55d3057

jonatack force-pushed on Jul 8, 2020

laanwj commented at 1:10 pm on July 9, 2020: member

Code review ACK 8c7647b3fbbab03ea84071cf3cd2d0d2bf8be255

amitiuttarwar commented at 8:16 pm on July 9, 2020: contributor

ACK 8c7647b3fbbab03ea84071cf3cd2d0d2bf8be255

fanquake commented at 5:28 am on July 10, 2020: member

@laanwj & @amitiuttarwar: it would seem you’ve both ACK’d the wrong commit hash here?

Locally I’m seeing:

 0
 1bitcoin git:(master) git fetch upstream pull/19469/head:19469 
 2remote: Enumerating objects: 8, done.
 3remote: Counting objects: 100% (8/8), done.
 4remote: Total 15 (delta 8), reused 8 (delta 8), pack-reused 7
 5Unpacking objects: 100% (15/15), 5.72 KiB | 234.00 KiB/s, done.
 6From https://github.com/bitcoin/bitcoin
 7 * [new ref]             refs/pull/19469/head -> 19469
 8bitcoin git:(master) git checkout 19469                      
 9Switched to branch '19469'
10bitcoin git:(19469) git log
11...
12commit 41d55d30579358c805036201664ad6a1c1d48681 (HEAD -> 19469)
13Author: Jon Atack <jon@atack.com>
14Date:   Wed Jul 8 12:54:28 2020 +0200
15
16    doc: getpeerinfo banscore deprecation release note
17
18commit dd54e3796e633cfdf6954af306afd26eadc25116
19Author: Jon Atack <jon@atack.com>
20Date:   Wed Jul 8 12:33:14 2020 +0200
21
22    test: getpeerinfo banscore deprecation test
23
24commit 8c7647b3fbbab03ea84071cf3cd2d0d2bf8be255
25Author: Jon Atack <jon@atack.com>
26Date:   Wed Jul 8 13:00:58 2020 +0200
27
28    rpc: deprecate banscore field in rpc getpeerinfo

Regardless, these changes are straightforward and I’m happy to go ahead and merge this.

Also cc @jnewbery & @sipa. I know you both expressed a desire for the removal of -banscore.

fanquake approved

fanquake commented at 5:28 am on July 10, 2020: member

ACK 41d55d30579358c805036201664ad6a1c1d48681

fanquake merged this on Jul 10, 2020

fanquake closed this on Jul 10, 2020

jonatack deleted the branch on Jul 10, 2020

jnewbery commented at 8:41 am on July 10, 2020: member

This feels a little premature. I think it’s not helped by a confusing collision in terminology:

-banscore is a configuration option that we might better call “configurable discouragement threshold”. That’s what sipa and I were referring to in #19219. It should be removed because we have no good advice for users on how to set it.
banscore in getpeerinfo returns the peer’s current nMisbehavior score. Longer term, it’d be good to replace nMisbehavior with prioritizing/scheduling peer work based on how much resource the peer is using (see https://bitcoincore.reviews/19109.html#l-99 and http://www.erisian.com.au/bitcoin-core-dev/log-2020-07-09.html#l-134).

Until we’ve actually removed the nMisbehavior functionality, it seems useful to be able to check a peer’s nMisbehavior score.

jonatack commented at 9:13 am on July 10, 2020: member

* `-banscore` ... should be removed because we have no good advice for users on how to set it.

Indeed, the first PR in this series, #19464, is a very small change that does just that. Presumably that PR, or another version, will be merged before the next release. I’m happy to work on internal peer prioritisation changes after that first step if people aren’t already doing it.

The confusion around the naming of the banscore field in getpeerinfo was not addressed because it should be removed. In #19219 (review), I suggested it be renamed.

This PR gives users until at least v0.22 to no longer depend on it (if any were). I reckon it will no longer be used internally before that, but in the worst case if this PR does turn out to be premature, it can be reverted before the next release.

sidhujag referenced this in commit 1b2b3e3600 on Jul 10, 2020

MarcoFalke referenced this in commit b93c4244b9 on Jul 14, 2020

sidhujag referenced this in commit b8fc13afe4 on Jul 14, 2020

jnewbery commented at 4:30 pm on August 17, 2020: member

Just realised I never responded to @jonatack’s last comment. I don’t think this should be reverted.

jonatack commented at 4:34 pm on August 17, 2020: member

Thanks @jnewbery. At the next p2p meeting, one topic to discuss could be if someone is working on replacing nMisbehavior with prioritizing/scheduling peer work based on how much resource the peer is using.

jnewbery commented at 4:50 pm on August 17, 2020: member

Sure. @troygiorshev is soon to open a PR that adds tracking for some peer metrics, which would be the start of that work.

deadalnix referenced this in commit a40a553f04 on Jan 6, 2021

DrahtBot locked this on Feb 15, 2022

rpc: deprecate banscore field in getpeerinfo #19469