net: remove -banscore configuration option #19464

jonatack · 2020-07-08T08:44:00Z

per #19219 (comment), #19219 (comment) and #19219 (comment). Edit: now split into 3 PRs:

net: remove -banscore configuration option (this PR)
rpc: deprecate banscore field in getpeerinfo (rpc: deprecate banscore field in getpeerinfo #19469)
gui, doc, doc: no longer display "Ban Score" in GUI Peers window + test/doc fixups (p2p: banscore updates to gui, tests, release notes #19512)

DrahtBot · 2020-07-08T11:28:40Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

Add address-based index (attempt 4?) #14053 (Add address-based index (attempt 4?) by marcinja)

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.

41d55d3 doc: getpeerinfo banscore deprecation release note (Jon Atack) dd54e37 test: getpeerinfo banscore deprecation test (Jon Atack) 8c7647b rpc: deprecate banscore field in rpc getpeerinfo (Jon Atack) Pull request description: Per #19219 (comment) 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. ACKs for top commit: fanquake: ACK 41d55d3 Tree-SHA512: 8eca08332581e2fe191a2aafff6ba89ce39413f0491ed0de8b86577739f0ec430b1a8fbff2914b0f3138a229563dfcc1981c0cf5b7dd6061b5c48680a28423bc

maflcko

Concept aCK

doc/release-notes-19464.md

41d55d3 doc: getpeerinfo banscore deprecation release note (Jon Atack) dd54e37 test: getpeerinfo banscore deprecation test (Jon Atack) 8c7647b rpc: deprecate banscore field in rpc getpeerinfo (Jon Atack) Pull request description: Per bitcoin#19219 (comment) and bitcoin#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 bitcoin#19464. ACKs for top commit: fanquake: ACK 41d55d3 Tree-SHA512: 8eca08332581e2fe191a2aafff6ba89ce39413f0491ed0de8b86577739f0ec430b1a8fbff2914b0f3138a229563dfcc1981c0cf5b7dd6061b5c48680a28423bc

fjahr · 2020-07-10T19:01:04Z

Concept ACK but the commits are cluttered up with quite a bit of edits that produce unnecessary noise.

jonatack · 2020-07-11T08:38:49Z

Thanks @MarcoFalke and @fjahr for reviewing! Updated with your feedback per git range-diff 505b4ed f926882 b71eac1 for easy re-ACK.

maflcko

Do the unit and functional tests pass on ff43c0c

If not, the following two commits should be squashed into it. Or maybe just squash all commit? 🤷‍♂️

Review-only ACK

src/net_processing.h

jonatack · 2020-07-11T09:32:33Z

Do the unit and functional tests pass on ff43c0c

The tests had to be updated for the config removal.

jonatack · 2020-07-11T09:40:03Z

If not, the following two commits should be squashed into it. Or maybe just squash all commit? man_shrugging

Review-only ACK

Thanks for ACKing. I took the time to made atomic commits for different actions, step by step, as seen in other contributors' changesets and as described in CONTRIBUTING.md and in the wikipedia link it contains.

In general, [commits should be atomic](https://en.wikipedia.org/wiki/Atomic_commit#Atomic_commit_convention)
and diffs should be easy to read. For this reason, do not mix any formatting
fixes or code moves with actual code changes.

maflcko · 2020-07-11T09:52:57Z

Please see the second section of https://www.codewithjason.com/atomic-commits-testing/

jonatack · 2020-07-11T10:11:26Z

Please see the second section of https://www.codewithjason.com/atomic-commits-testing/

Yes, I agree with this and it has been my standard practice. But AFAIK it's not what is requested in this project's documentation, and maybe not the most-observed practice in this project -- I had to unlearn my practices to adapt to it.

maflcko · 2020-07-11T10:13:04Z

But AFAIK it's not what is requested in this project's documentation

Ok, then our documentation is wrong and should be fixed.

jonatack · 2020-07-11T10:15:11Z

I'll make the commits hygienic.

and move it from validation to net processing.

Noticed this new component label when it was added to bitcoin/bitcoin#19464

vasild

Should doc/man/bitcoin-qt.1 and doc/man/bitcoind.1 be updated in this PR (they contain references to -banscore)?

If this PR gets merged now, it means that Bitcoin Core 0.21 will not recognize the -banscore command line option, is this the intention?

doc/release-notes.md

src/net_processing.h

jonatack · 2020-07-13T14:35:39Z

Thanks for reviewing, @vasild!

Should doc/man/bitcoin-qt.1 and doc/man/bitcoind.1 be updated in this PR (they contain references to -banscore)?

AFAIK the manpages are auto-generated; we don't edit them in PRs.

If this PR gets merged now, it means that Bitcoin Core 0.21 will not recognize the -banscore command line option, is this the intention?

Correct. See the links in the PR description for intention and reasoning behind the removal.

AFAIK there isn't a deprecation process here like for the RPC API, so either it's removed as proposed here, or we add a deprecation warning in the help (which everyone will ignore until the option is removed) for a release or two. Presumably this discouragement scoring will disappear anyway by 0.21 or 0.22 which means the -banscore option won't do anything.

maflcko · 2020-07-13T17:28:01Z

Ideally everything would go in one swish (including state->nMisbehavior), but that can't happen because it is still needed for RPC. This is a necessary first step.

review ACK 06059b0 📙

Show signature and timestamp

Signature:

Timestamp of file with hash 7468957afe68f0b446600d6fe8fc55ac3e9656a9dfb3300c2ac5b7e9c089a92c -

vasild

ACK 06059b0

practicalswift

Concept ACK: giving users knobs without a recommendation about how/when to use them is not friendly :)

test/functional/p2p_leak.py

src/test/denialofservice_tests.cpp

maflcko

Oh, I see this section won't end up in the 0.21 release notes, so might want to clarify at some point

doc/release-notes.md

jnewbery · 2020-07-14T08:39:42Z

post-merge ACK.

The test BOOST_AUTO_TEST_CASE(DoS_banscore) should be removed entirely. It only existed to test manually testing the -banscore config. Nothing else in the test is untested by the previous DoS_banning test.

jonatack · 2020-07-14T12:17:26Z

post-merge ACK.

The test BOOST_AUTO_TEST_CASE(DoS_banscore) should be removed entirely. It only existed to test manually testing the -banscore config. Nothing else in the test is untested by the previous DoS_banning test.

Good point @jnewbery. Added the removal to the test change commit in #19512.

06059b0 net: rename DEFAULT_BANSCORE_THRESHOLD to DISCOURAGEMENT_THRESHOLD (Jon Atack) 1d4024b net: remove -banscore configuration option (Jon Atack) Pull request description: per bitcoin#19219 (comment), bitcoin#19219 (comment) and bitcoin#19219 (comment). Edit: now split into 3 straightforward PRs: - net: remove -banscore configuration option (this PR) - rpc: deprecate banscore field in getpeerinfo (bitcoin#19469, *merged*) - gui: no longer display banscores (TBA in the gui repo) ACKs for top commit: MarcoFalke: review ACK 06059b0 📙 vasild: ACK 06059b0 Tree-SHA512: 03fad249986e0896697033fbb8ba2cbfaae7d7603b1fb2a38b3d41db697630d238623f4d732b9098c82af249ce5a1767dd432b7ca0fec10544e23d24fbd57c50

fa108d6 test: update tests for peer discouragement (Jon Atack) 1a9f462 gui, doc: rm Ban Score in GUI Peers window/release notes updates (Jon Atack) Pull request description: This is the third `-banscore` PR in the mini-series described in #19464. See that PR for the intention and reasoning. - no longer display "Ban Score" in the GUI peers window and add a release note, plus release note fixups per #19464 (review) - update tests (`src/test/denialofservice_tests.cpp` and `test/functional/p2p_leak.py`) from banning to discouragement and per #19464 (comment) ACKs for top commit: jnewbery: ACK fa108d6 laanwj: ACK fa108d6 Tree-SHA512: 58a449b3f47b8cb5490b34e4442ee8675bfad1ce48af4e4fd5c67715b0c1a596fb8e731d42e576b4c3b64627f76e0a68cbb1da9ea9f588a5932fe119baf40d50

fa108d6 test: update tests for peer discouragement (Jon Atack) 1a9f462 gui, doc: rm Ban Score in GUI Peers window/release notes updates (Jon Atack) Pull request description: This is the third `-banscore` PR in the mini-series described in bitcoin#19464. See that PR for the intention and reasoning. - no longer display "Ban Score" in the GUI peers window and add a release note, plus release note fixups per bitcoin#19464 (review) - update tests (`src/test/denialofservice_tests.cpp` and `test/functional/p2p_leak.py`) from banning to discouragement and per bitcoin#19464 (comment) ACKs for top commit: jnewbery: ACK fa108d6 laanwj: ACK fa108d6 Tree-SHA512: 58a449b3f47b8cb5490b34e4442ee8675bfad1ce48af4e4fd5c67715b0c1a596fb8e731d42e576b4c3b64627f76e0a68cbb1da9ea9f588a5932fe119baf40d50

Summary: 06059b0c2a6c2db70c87a7715f8a344a13400fa1 net: rename DEFAULT_BANSCORE_THRESHOLD to DISCOURAGEMENT_THRESHOLD (Jon Atack) 1d4024bca8086cceff7539dd8c15e0b7fe1cc5ea net: remove -banscore configuration option (Jon Atack) Pull request description: per bitcoin/bitcoin#19219 (comment), bitcoin/bitcoin#19219 (comment) and bitcoin/bitcoin#19219 (comment). Edit: now split into 3 straightforward PRs: - net: remove -banscore configuration option (this PR) - rpc: deprecate banscore field in getpeerinfo (#19469, *merged*) - gui: no longer display banscores (TBA in the gui repo) --- Backport of Core [[bitcoin/bitcoin#19464 | PR19464]] Depends on D8798 Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8799

Summary: fa108d6a757838225179a8df942cfb6d99c98c90 test: update tests for peer discouragement (Jon Atack) 1a9f462caa63fa16d7b4415312d2032a42b3fe0b gui, doc: rm Ban Score in GUI Peers window/release notes updates (Jon Atack) Pull request description: This is the third `-banscore` PR in the mini-series described in #19464. See that PR for the intention and reasoning. - no longer display "Ban Score" in the GUI peers window and add a release note, plus release note fixups per bitcoin/bitcoin#19464 (review) - update tests (`src/test/denialofservice_tests.cpp` and `test/functional/p2p_leak.py`) from banning to discouragement and per bitcoin/bitcoin#19464 (comment) --- Backport of Core [[bitcoin/bitcoin#19512 | PR19512]] Depends on D8799 Test Plan: ninja all check check-functional run bitcoin-qt on testnet mode, open Peers window, click a peer and see that Banscore is no longer reported Reviewers: #bitcoin_abc, PiRK, Fabien Reviewed By: #bitcoin_abc, PiRK, Fabien Subscribers: Fabien, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8800

DrahtBot added GUI P2P RPC/REST/ZMQ Tests Validation labels Jul 8, 2020

fanquake removed GUI Tests Validation labels Jul 8, 2020

jonatack mentioned this pull request Jul 8, 2020

rpc: deprecate banscore field in getpeerinfo #19469

Merged

jonatack force-pushed the remove-banscore-option branch from c0ba1e8 to f926882 Compare July 8, 2020 12:47

jonatack changed the title ~~net, rpc: remove -banscore option, deprecate banscore in getpeerinfo~~ net: remove -banscore configuration option Jul 8, 2020

jonatack marked this pull request as ready for review July 8, 2020 13:05

maflcko added Settings P2P and removed RPC/REST/ZMQ P2P labels Jul 8, 2020

DrahtBot mentioned this pull request Jul 8, 2020

Add address-based index (attempt 4?) #14053

Closed

maflcko reviewed Jul 10, 2020

View reviewed changes

doc/release-notes-19464.md Outdated Show resolved Hide resolved

jonatack force-pushed the remove-banscore-option branch from f926882 to b71eac1 Compare July 11, 2020 08:35

maflcko approved these changes Jul 11, 2020

View reviewed changes

src/net_processing.h Outdated Show resolved Hide resolved

jonatack force-pushed the remove-banscore-option branch from b71eac1 to de15350 Compare July 11, 2020 10:04

maflcko added this to the 0.21.0 milestone Jul 11, 2020

jonatack force-pushed the remove-banscore-option branch 3 times, most recently from 78395cd to 3cdb66d Compare July 11, 2020 11:16

jonatack added 2 commits July 11, 2020 19:41

net: remove -banscore configuration option

1d4024b

net: rename DEFAULT_BANSCORE_THRESHOLD to DISCOURAGEMENT_THRESHOLD

06059b0

and move it from validation to net processing.

jonatack force-pushed the remove-banscore-option branch from 3cdb66d to 06059b0 Compare July 11, 2020 17:42

jonatack added a commit to bitcoin-core-review-club/website that referenced this pull request Jul 13, 2020

Rakefile: add new Settings Bitcoin Core component

93a7b7d

Noticed this new component label when it was added to bitcoin/bitcoin#19464

vasild reviewed Jul 13, 2020

View reviewed changes

doc/release-notes.md Show resolved Hide resolved

src/net_processing.h Show resolved Hide resolved

vasild approved these changes Jul 13, 2020

View reviewed changes

practicalswift reviewed Jul 13, 2020

View reviewed changes

test/functional/p2p_leak.py Show resolved Hide resolved

src/test/denialofservice_tests.cpp Show resolved Hide resolved

maflcko merged commit b93c424 into bitcoin:master Jul 14, 2020

maflcko reviewed Jul 14, 2020

View reviewed changes

doc/release-notes.md Show resolved Hide resolved

jonatack deleted the remove-banscore-option branch July 14, 2020 06:58

jonatack mentioned this pull request Jul 14, 2020

p2p: banscore updates to gui, tests, release notes #19512

Merged

fanquake mentioned this pull request Aug 17, 2020

Replace -banscore with -ignoremisbehaviour #16683

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: remove -banscore configuration option #19464

net: remove -banscore configuration option #19464

jonatack commented Jul 8, 2020 •

edited

Loading

DrahtBot commented Jul 8, 2020

maflcko left a comment

fjahr commented Jul 10, 2020

jonatack commented Jul 11, 2020

maflcko left a comment

jonatack commented Jul 11, 2020

jonatack commented Jul 11, 2020

maflcko commented Jul 11, 2020

jonatack commented Jul 11, 2020 •

edited

Loading

maflcko commented Jul 11, 2020

jonatack commented Jul 11, 2020

vasild left a comment

jonatack commented Jul 13, 2020 •

edited

Loading

maflcko commented Jul 13, 2020

vasild left a comment

practicalswift left a comment

maflcko left a comment

jnewbery commented Jul 14, 2020

jonatack commented Jul 14, 2020

net: remove -banscore configuration option #19464

net: remove -banscore configuration option #19464

Conversation

jonatack commented Jul 8, 2020 • edited Loading

DrahtBot commented Jul 8, 2020

Conflicts

maflcko left a comment

Choose a reason for hiding this comment

fjahr commented Jul 10, 2020

jonatack commented Jul 11, 2020

maflcko left a comment

Choose a reason for hiding this comment

jonatack commented Jul 11, 2020

jonatack commented Jul 11, 2020

maflcko commented Jul 11, 2020

jonatack commented Jul 11, 2020 • edited Loading

maflcko commented Jul 11, 2020

jonatack commented Jul 11, 2020

vasild left a comment

Choose a reason for hiding this comment

jonatack commented Jul 13, 2020 • edited Loading

maflcko commented Jul 13, 2020

vasild left a comment

Choose a reason for hiding this comment

practicalswift left a comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

jnewbery commented Jul 14, 2020

jonatack commented Jul 14, 2020

jonatack commented Jul 8, 2020 •

edited

Loading

jonatack commented Jul 11, 2020 •

edited

Loading

jonatack commented Jul 13, 2020 •

edited

Loading