Skip to content

Commit 21209c9

Browse files
laanwjlaanwj
committed
Merge bitcoin#19512: p2p: banscore updates to gui, tests, release notes
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
2 parents 7ebc365 + fa108d6 commit 21209c9

File tree

5 files changed

+44
-106
lines changed

5 files changed

+44
-106
lines changed

‎doc/release-notes.md

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ will trigger BIP 125 (replace-by-fee) opt-in. (#11413)
9898
- The `bumpfee` command now uses `conf_target` rather than `confTarget` in the
9999
options. (#11413)
100100

101-
- `getpeerinfo` no longer returns the `banscore` field unless the configuration
101+
- The `getpeerinfo` RPC no longer returns the `banscore` field unless the configuration
102102
option `-deprecatedrpc=banscore` is used. The `banscore` field will be fully
103103
removed in the next major release. (#19469)
104104

@@ -123,8 +123,9 @@ Updated settings
123123

124124
- The `-banscore` configuration option, which modified the default threshold for
125125
disconnecting and discouraging misbehaving peers, has been removed as part of
126-
changes in this release to the handling of misbehaving peers. Refer to the
127-
section, "Changes regarding misbehaving peers", for details. (#19464)
126+
changes in 0.20.1 and in this release to the handling of misbehaving peers.
127+
Refer to "Changes regarding misbehaving peers" in the 0.20.1 release notes for
128+
details. (#19464)
128129

129130
- The `-debug=db` logging category, which was deprecated in 0.20 and replaced by
130131
`-debug=walletdb` to distinguish it from `coindb`, has been removed. (#19202)
@@ -303,6 +304,11 @@ issue.
303304
GUI changes
304305
-----------
305306

307+
- The GUI Peers window no longer displays a "Ban Score" field. This is part of
308+
changes in 0.20.1 and in this release to the handling of misbehaving
309+
peers. Refer to "Changes regarding misbehaving peers" in the 0.20.1 release
310+
notes for details. (#19512)
311+
306312
Low-level changes
307313
=================
308314

‎src/qt/forms/debugwindow.ui

Lines changed: 20 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1264,36 +1264,13 @@
12641264
</widget>
12651265
</item>
12661266
<item row="8" column="0">
1267-
<widget class="QLabel" name="label_24">
1268-
<property name="text">
1269-
<string>Ban Score</string>
1270-
</property>
1271-
</widget>
1272-
</item>
1273-
<item row="8" column="1">
1274-
<widget class="QLabel" name="peerBanScore">
1275-
<property name="cursor">
1276-
<cursorShape>IBeamCursor</cursorShape>
1277-
</property>
1278-
<property name="text">
1279-
<string>N/A</string>
1280-
</property>
1281-
<property name="textFormat">
1282-
<enum>Qt::PlainText</enum>
1283-
</property>
1284-
<property name="textInteractionFlags">
1285-
<set>Qt::LinksAccessibleByMouse|Qt::TextSelectableByKeyboard|Qt::TextSelectableByMouse</set>
1286-
</property>
1287-
</widget>
1288-
</item>
1289-
<item row="9" column="0">
12901267
<widget class="QLabel" name="label_22">
12911268
<property name="text">
12921269
<string>Connection Time</string>
12931270
</property>
12941271
</widget>
12951272
</item>
1296-
<item row="9" column="1">
1273+
<item row="8" column="1">
12971274
<widget class="QLabel" name="peerConnTime">
12981275
<property name="cursor">
12991276
<cursorShape>IBeamCursor</cursorShape>
@@ -1309,14 +1286,14 @@
13091286
</property>
13101287
</widget>
13111288
</item>
1312-
<item row="10" column="0">
1289+
<item row="9" column="0">
13131290
<widget class="QLabel" name="label_15">
13141291
<property name="text">
13151292
<string>Last Send</string>
13161293
</property>
13171294
</widget>
13181295
</item>
1319-
<item row="10" column="1">
1296+
<item row="9" column="1">
13201297
<widget class="QLabel" name="peerLastSend">
13211298
<property name="cursor">
13221299
<cursorShape>IBeamCursor</cursorShape>
@@ -1332,14 +1309,14 @@
13321309
</property>
13331310
</widget>
13341311
</item>
1335-
<item row="11" column="0">
1312+
<item row="10" column="0">
13361313
<widget class="QLabel" name="label_19">
13371314
<property name="text">
13381315
<string>Last Receive</string>
13391316
</property>
13401317
</widget>
13411318
</item>
1342-
<item row="11" column="1">
1319+
<item row="10" column="1">
13431320
<widget class="QLabel" name="peerLastRecv">
13441321
<property name="cursor">
13451322
<cursorShape>IBeamCursor</cursorShape>
@@ -1355,14 +1332,14 @@
13551332
</property>
13561333
</widget>
13571334
</item>
1358-
<item row="12" column="0">
1335+
<item row="11" column="0">
13591336
<widget class="QLabel" name="label_18">
13601337
<property name="text">
13611338
<string>Sent</string>
13621339
</property>
13631340
</widget>
13641341
</item>
1365-
<item row="12" column="1">
1342+
<item row="11" column="1">
13661343
<widget class="QLabel" name="peerBytesSent">
13671344
<property name="cursor">
13681345
<cursorShape>IBeamCursor</cursorShape>
@@ -1378,14 +1355,14 @@
13781355
</property>
13791356
</widget>
13801357
</item>
1381-
<item row="13" column="0">
1358+
<item row="12" column="0">
13821359
<widget class="QLabel" name="label_20">
13831360
<property name="text">
13841361
<string>Received</string>
13851362
</property>
13861363
</widget>
13871364
</item>
1388-
<item row="13" column="1">
1365+
<item row="12" column="1">
13891366
<widget class="QLabel" name="peerBytesRecv">
13901367
<property name="cursor">
13911368
<cursorShape>IBeamCursor</cursorShape>
@@ -1401,14 +1378,14 @@
14011378
</property>
14021379
</widget>
14031380
</item>
1404-
<item row="14" column="0">
1381+
<item row="13" column="0">
14051382
<widget class="QLabel" name="label_26">
14061383
<property name="text">
14071384
<string>Ping Time</string>
14081385
</property>
14091386
</widget>
14101387
</item>
1411-
<item row="14" column="1">
1388+
<item row="13" column="1">
14121389
<widget class="QLabel" name="peerPingTime">
14131390
<property name="cursor">
14141391
<cursorShape>IBeamCursor</cursorShape>
@@ -1424,7 +1401,7 @@
14241401
</property>
14251402
</widget>
14261403
</item>
1427-
<item row="15" column="0">
1404+
<item row="14" column="0">
14281405
<widget class="QLabel" name="peerPingWaitLabel">
14291406
<property name="toolTip">
14301407
<string>The duration of a currently outstanding ping.</string>
@@ -1434,7 +1411,7 @@
14341411
</property>
14351412
</widget>
14361413
</item>
1437-
<item row="15" column="1">
1414+
<item row="14" column="1">
14381415
<widget class="QLabel" name="peerPingWait">
14391416
<property name="cursor">
14401417
<cursorShape>IBeamCursor</cursorShape>
@@ -1450,14 +1427,14 @@
14501427
</property>
14511428
</widget>
14521429
</item>
1453-
<item row="16" column="0">
1430+
<item row="15" column="0">
14541431
<widget class="QLabel" name="peerMinPingLabel">
14551432
<property name="text">
14561433
<string>Min Ping</string>
14571434
</property>
14581435
</widget>
14591436
</item>
1460-
<item row="16" column="1">
1437+
<item row="15" column="1">
14611438
<widget class="QLabel" name="peerMinPing">
14621439
<property name="cursor">
14631440
<cursorShape>IBeamCursor</cursorShape>
@@ -1473,14 +1450,14 @@
14731450
</property>
14741451
</widget>
14751452
</item>
1476-
<item row="17" column="0">
1453+
<item row="16" column="0">
14771454
<widget class="QLabel" name="label_timeoffset">
14781455
<property name="text">
14791456
<string>Time Offset</string>
14801457
</property>
14811458
</widget>
14821459
</item>
1483-
<item row="17" column="1">
1460+
<item row="16" column="1">
14841461
<widget class="QLabel" name="timeoffset">
14851462
<property name="cursor">
14861463
<cursorShape>IBeamCursor</cursorShape>
@@ -1496,7 +1473,7 @@
14961473
</property>
14971474
</widget>
14981475
</item>
1499-
<item row="18" column="0">
1476+
<item row="17" column="0">
15001477
<widget class="QLabel" name="peerMappedASLabel">
15011478
<property name="toolTip">
15021479
<string>The mapped Autonomous System used for diversifying peer selection.</string>
@@ -1506,7 +1483,7 @@
15061483
</property>
15071484
</widget>
15081485
</item>
1509-
<item row="18" column="1">
1486+
<item row="17" column="1">
15101487
<widget class="QLabel" name="peerMappedAS">
15111488
<property name="cursor">
15121489
<cursorShape>IBeamCursor</cursorShape>
@@ -1522,7 +1499,7 @@
15221499
</property>
15231500
</widget>
15241501
</item>
1525-
<item row="19" column="0">
1502+
<item row="18" column="0">
15261503
<spacer name="verticalSpacer_3">
15271504
<property name="orientation">
15281505
<enum>Qt::Vertical</enum>

‎src/qt/rpcconsole.cpp

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1126,9 +1126,6 @@ void RPCConsole::updateNodeDetail(const CNodeCombinedStats *stats)
11261126
// This check fails for example if the lock was busy and
11271127
// nodeStateStats couldn't be fetched.
11281128
if (stats->fNodeStateStatsAvailable) {
1129-
// Ban score is init to 0
1130-
ui->peerBanScore->setText(QString("%1").arg(stats->nodeStateStats.nMisbehavior));
1131-
11321129
// Sync height is init to -1
11331130
if (stats->nodeStateStats.nSyncHeight > -1)
11341131
ui->peerSyncHeight->setText(QString("%1").arg(stats->nodeStateStats.nSyncHeight));

‎src/test/denialofservice_tests.cpp

Lines changed: 5 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
217217
connman->ClearNodes();
218218
}
219219

220-
BOOST_AUTO_TEST_CASE(DoS_banning)
220+
BOOST_AUTO_TEST_CASE(peer_discouragement)
221221
{
222222
auto banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
223223
auto connman = MakeUnique<CConnman>(0x1337, 0x1337);
@@ -249,7 +249,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
249249
dummyNode2.fSuccessfullyConnected = true;
250250
{
251251
LOCK(cs_main);
252-
Misbehaving(dummyNode2.GetId(), 50);
252+
Misbehaving(dummyNode2.GetId(), DISCOURAGEMENT_THRESHOLD - 1);
253253
}
254254
{
255255
LOCK2(cs_main, dummyNode2.cs_sendProcessing);
@@ -259,64 +259,20 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
259259
BOOST_CHECK(banman->IsDiscouraged(addr1)); // ... but 1 still should be
260260
{
261261
LOCK(cs_main);
262-
Misbehaving(dummyNode2.GetId(), 50);
262+
Misbehaving(dummyNode2.GetId(), 1); // 2 reaches discouragement threshold
263263
}
264264
{
265265
LOCK2(cs_main, dummyNode2.cs_sendProcessing);
266266
BOOST_CHECK(peerLogic->SendMessages(&dummyNode2));
267267
}
268-
BOOST_CHECK(banman->IsDiscouraged(addr2));
268+
BOOST_CHECK(banman->IsDiscouraged(addr1)); // Expect both 1 and 2
269+
BOOST_CHECK(banman->IsDiscouraged(addr2)); // to be discouraged now
269270

270271
bool dummy;
271272
peerLogic->FinalizeNode(dummyNode1.GetId(), dummy);
272273
peerLogic->FinalizeNode(dummyNode2.GetId(), dummy);
273274
}
274275

275-
BOOST_AUTO_TEST_CASE(DoS_banscore)
276-
{
277-
auto banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
278-
auto connman = MakeUnique<CConnman>(0x1337, 0x1337);
279-
auto peerLogic = MakeUnique<PeerLogicValidation>(connman.get(), banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool);
280-
281-
banman->ClearBanned();
282-
CAddress addr1(ip(0xa0b0c001), NODE_NONE);
283-
CNode dummyNode1(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr1, 3, 1, CAddress(), "", true);
284-
dummyNode1.SetSendVersion(PROTOCOL_VERSION);
285-
peerLogic->InitializeNode(&dummyNode1);
286-
dummyNode1.nVersion = 1;
287-
dummyNode1.fSuccessfullyConnected = true;
288-
{
289-
LOCK(cs_main);
290-
Misbehaving(dummyNode1.GetId(), DISCOURAGEMENT_THRESHOLD - 11);
291-
}
292-
{
293-
LOCK2(cs_main, dummyNode1.cs_sendProcessing);
294-
BOOST_CHECK(peerLogic->SendMessages(&dummyNode1));
295-
}
296-
BOOST_CHECK(!banman->IsDiscouraged(addr1));
297-
{
298-
LOCK(cs_main);
299-
Misbehaving(dummyNode1.GetId(), 10);
300-
}
301-
{
302-
LOCK2(cs_main, dummyNode1.cs_sendProcessing);
303-
BOOST_CHECK(peerLogic->SendMessages(&dummyNode1));
304-
}
305-
BOOST_CHECK(!banman->IsDiscouraged(addr1));
306-
{
307-
LOCK(cs_main);
308-
Misbehaving(dummyNode1.GetId(), 1);
309-
}
310-
{
311-
LOCK2(cs_main, dummyNode1.cs_sendProcessing);
312-
BOOST_CHECK(peerLogic->SendMessages(&dummyNode1));
313-
}
314-
BOOST_CHECK(banman->IsDiscouraged(addr1));
315-
316-
bool dummy;
317-
peerLogic->FinalizeNode(dummyNode1.GetId(), dummy);
318-
}
319-
320276
BOOST_AUTO_TEST_CASE(DoS_bantime)
321277
{
322278
auto banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);

‎test/functional/p2p_leak.py

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -65,9 +65,10 @@ def on_blocktxn(self, message): self.bad_message(message)
6565

6666
# Node that never sends a version. We'll use this to send a bunch of messages
6767
# anyway, and eventually get disconnected.
68-
class CNodeNoVersionBan(CLazyNode):
69-
# send a bunch of veracks without sending a message. This should get us disconnected.
70-
# NOTE: implementation-specific check here. Remove if bitcoind ban behavior changes
68+
class CNodeNoVersionMisbehavior(CLazyNode):
69+
# Send enough veracks without a message to reach the peer discouragement
70+
# threshold. This should get us disconnected. NOTE: implementation-specific
71+
# test; update if our discouragement policy for peer misbehavior changes.
7172
def on_open(self):
7273
super().on_open()
7374
for _ in range(DISCOURAGEMENT_THRESHOLD):
@@ -108,15 +109,16 @@ def set_test_params(self):
108109
self.num_nodes = 1
109110

110111
def run_test(self):
111-
no_version_bannode = self.nodes[0].add_p2p_connection(CNodeNoVersionBan(), send_version=False, wait_for_verack=False)
112+
no_version_disconnect_node = self.nodes[0].add_p2p_connection(
113+
CNodeNoVersionMisbehavior(), send_version=False, wait_for_verack=False)
112114
no_version_idlenode = self.nodes[0].add_p2p_connection(CNodeNoVersionIdle(), send_version=False, wait_for_verack=False)
113115
no_verack_idlenode = self.nodes[0].add_p2p_connection(CNodeNoVerackIdle(), wait_for_verack=False)
114116

115117
# Wait until we got the verack in response to the version. Though, don't wait for the other node to receive the
116118
# verack, since we never sent one
117119
no_verack_idlenode.wait_for_verack()
118120

119-
wait_until(lambda: no_version_bannode.ever_connected, timeout=10, lock=mininode_lock)
121+
wait_until(lambda: no_version_disconnect_node.ever_connected, timeout=10, lock=mininode_lock)
120122
wait_until(lambda: no_version_idlenode.ever_connected, timeout=10, lock=mininode_lock)
121123
wait_until(lambda: no_verack_idlenode.version_received, timeout=10, lock=mininode_lock)
122124

@@ -126,13 +128,13 @@ def run_test(self):
126128
#Give the node enough time to possibly leak out a message
127129
time.sleep(5)
128130

129-
#This node should have been banned
130-
assert not no_version_bannode.is_connected
131+
# Expect this node to be disconnected for misbehavior
132+
assert not no_version_disconnect_node.is_connected
131133

132134
self.nodes[0].disconnect_p2ps()
133135

134136
# Make sure no unexpected messages came in
135-
assert no_version_bannode.unexpected_msg == False
137+
assert no_version_disconnect_node.unexpected_msg == False
136138
assert no_version_idlenode.unexpected_msg == False
137139
assert no_verack_idlenode.unexpected_msg == False
138140

0 commit comments

Comments
 (0)