rpc: deprecate banscore field in getpeerinfo #19469

pull jonatack wants to merge 3 commits into bitcoin:master from jonatack:deprecate-getpeerinfo-banscore changing 4 files +36 −2

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:

  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:


bitcoin git:(master) git fetch upstream pull/19469/head:19469 
remote: Enumerating objects: 8, done.
remote: Counting objects: 100% (8/8), done.
remote: Total 15 (delta 8), reused 8 (delta 8), pack-reused 7
Unpacking objects: 100% (15/15), 5.72 KiB | 234.00 KiB/s, done.
From https://github.com/bitcoin/bitcoin
 * [new ref]             refs/pull/19469/head -> 19469
bitcoin git:(master) git checkout 19469                      
Switched to branch '19469'
bitcoin git:(19469) git log
...
commit 41d55d30579358c805036201664ad6a1c1d48681 (HEAD -> 19469)
Author: Jon Atack <jon@atack.com>
Date:   Wed Jul 8 12:54:28 2020 +0200

    doc: getpeerinfo banscore deprecation release note

commit dd54e3796e633cfdf6954af306afd26eadc25116
Author: Jon Atack <jon@atack.com>
Date:   Wed Jul 8 12:33:14 2020 +0200

    test: getpeerinfo banscore deprecation test

commit 8c7647b3fbbab03ea84071cf3cd2d0d2bf8be255
Author: Jon Atack <jon@atack.com>
Date:   Wed Jul 8 13:00:58 2020 +0200

    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

Contributors

Labels

Milestone
0.21.0

Linked (view graph)

#19219 Replace automatic bans with discouragement filter #19464 net: remove -banscore configuration option