per #19219 (comment), #19219 (review) and #19219 (comment). Edit: now split into 3 PRs:
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
0@@ -0,0 +1,7 @@
1+Updated settings
2+----------------
3+
4+- The `-banscore` configuration option, which modified the default threshold for
5+ disconnecting and discouraging misbehaving peers, has been removed as part of
6+ changes in this release to the handling of misbehaving peers. Refer to the
7+ section, "Changes regarding misbehaving peers", for details. (#19464)
git range-diff 505b4ed f926882 b71eac1 for easy re-ACK.
22@@ -23,6 +23,7 @@ static const unsigned int DEFAULT_MAX_ORPHAN_TRANSACTIONS = 100;
23 static const unsigned int DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN = 100;
24 static const bool DEFAULT_PEERBLOOMFILTERS = false;
25 static const bool DEFAULT_PEERBLOCKFILTERS = false;
26+static const int BANSCORE_THRESHOLD = 100;
can be moved to the cpp file and made constexpr?
good idea
Do the unit and functional tests pass on ff43c0c49eb087d27cd6ecf6b30fe6f6f96df650
If not, the following two commits should be squashed into it. Or maybe just squash all commit? :man_shrugging:
Review-only ACK
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.
0In general, [commits should be atomic](https://en.wikipedia.org/wiki/Atomic_commit#Atomic_commit_convention)
1and diffs should be easy to read. For this reason, do not mix any formatting
2fixes or code moves with actual code changes.
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.
But AFAIK it’s not what is requested in this project’s documentation
Ok, then our documentation is wrong and should be fixed.
and move it from validation to net processing.
98@@ -99,6 +99,11 @@ Build System
99 Updated settings
100 ----------------
101
102+- The `-banscore` configuration option, which modified the default threshold for
103+ disconnecting and discouraging misbehaving peers, has been removed as part of
104+ changes in this release to the handling of misbehaving peers. Refer to the
105+ section, "Changes regarding misbehaving peers", for details. (#19464)
106+
Shouldn’t this be in doc/release-notes-19464.md? doc/developer-notes.md says:
Release notes should be added to a PR-specific release note file at
/doc/release-notes-<PR number>.md
22@@ -23,6 +23,8 @@ static const unsigned int DEFAULT_MAX_ORPHAN_TRANSACTIONS = 100;
23 static const unsigned int DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN = 100;
24 static const bool DEFAULT_PEERBLOOMFILTERS = false;
25 static const bool DEFAULT_PEERBLOCKFILTERS = false;
26+/** Threshold for marking a node to be discouraged, e.g. disconnected and added to the discouragement filter. */
27+static const int DISCOURAGEMENT_THRESHOLD{100};
Misbehaving in net_processing.cpp), and expected to be removed (see the links in the PR description).
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?
Thanks for reviewing, @vasild!
Should
doc/man/bitcoin-qt.1anddoc/man/bitcoind.1be 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
-banscorecommand 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.
98@@ -99,6 +99,11 @@ Build System
99 Updated settings
100 ----------------
101
102+- The `-banscore` configuration option, which modified the default threshold for
103+ disconnecting and discouraging misbehaving peers, has been removed as part of
104+ changes in this release to the handling of misbehaving peers. Refer to the
105+ section, "Changes regarding misbehaving peers", for details. (#19464)
0 section, "Changes regarding misbehaving peers" in the 0.20.1 release notes, for details. (#19464)
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 06059b0c2a6c2db70c87a7715f8a344a13400fa1 📙
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3review ACK 06059b0c2a6c2db70c87a7715f8a344a13400fa1 📙
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUjlzAv+Mu1M+kc3EzVlpVHUNyR9vN531cYpgnWG+gct6oTejWWTBb8snhDrkAID
8+J4QbpAVd7LBg6CV2NJAl2I0/mTbcSuA66z9DhPwhptKnzocbxf4qXvj5sBnIxg8
9MOOcRAz5mGVqTom64t2EOme4qKnGvHQROY4srg/Iaf87+Xeur4qBFLuwezGOG9VH
10xEXSJlaZ0MM9zhLfuJpAIOE0NYiAwPRWnihCofbspg7Mv3duXiOna4gsF7TXV3LA
11uR/tLwZaZJkQRugKA0Dd5tBFu4+tPqZQ/blyFyGdbSunHVIJnyvLwA7T/nnz0YSO
12vcgJn8nFYO/MObhyBgWST0Bv6I/MsaB9it7cQnbXQjDtAAd4oUqeOzH5gbZYRSxy
13MqSRf/4eKcefvx0o5oIuwlva4HjUIwm68t2rn8lK/nSdERRGILCXE//SaIT9NUCc
14nbOIVLiQvVembkFWEZMWatZm9ty93a5ImbAY7YvyCBSK6/1vqtUClIXAHLU8+6oM
152HQc/Pec
16=/Sro
17-----END PGP SIGNATURE-----
Timestamp of file with hash 7468957afe68f0b446600d6fe8fc55ac3e9656a9dfb3300c2ac5b7e9c089a92c -
69@@ -70,7 +70,7 @@ class CNodeNoVersionBan(CLazyNode):
70 # NOTE: implementation-specific check here. Remove if bitcoind ban behavior changes
71 def on_open(self):
72 super().on_open()
73- for i in range(banscore):
74+ for _ in range(DISCOURAGEMENT_THRESHOLD):
Because we can no longer use -banscore to change the default threshold (see removed line in test: self.extra_args = [['-banscore=' + str(banscore)]]), so we have to use DISCOURAGEMENT_THRESHOLD here for the number of veracks sans version to send.
Note to self: the words “ban” in this functional test should be updated to “discourage”.
286@@ -288,7 +287,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)
287 dummyNode1.fSuccessfullyConnected = true;
288 {
289 LOCK(cs_main);
290- Misbehaving(dummyNode1.GetId(), 100);
291+ Misbehaving(dummyNode1.GetId(), DISCOURAGEMENT_THRESHOLD - 11);
100 to 89 here? :)
-banscore lines of this test, particularly the first one: gArgs.ForceSetArg("-banscore", "111"); // because 11 is my favorite number.
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.
Milestone
0.21.0