Skip to content

Commit

Permalink
[backport#19512] p2p: banscore updates to gui, tests, release notes
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@laanwj @majcosta
laanwj authored and majcosta committed Jan 6, 2021
1 parent 91502b3 commit 59fd163
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 115 deletions.
7 changes: 6 additions & 1 deletion doc/release-notes.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,15 @@ Bitcoin ABC version 0.22.11 is now available from:

This release includes the following features and fixes:

- `getpeerinfo` no longer returns the `banscore` field unless the configuration
- The `getpeerinfo` RPC no longer returns the `banscore` field unless the configuration
option `-deprecatedrpc=banscore` is used. The `banscore` field will be fully
removed in the next major release.

- The `-banscore` configuration option, which modified the default threshold for
disconnecting and discouraging misbehaving peers, has been removed as part of
changes in this release to the handling of misbehaving peers.

GUI changes
-----------

- The GUI Peers window no longer displays a "Ban Score" field.
57 changes: 17 additions & 40 deletions src/qt/forms/debugwindow.ui
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1270,36 +1270,13 @@
</widget>
</item>
<item row="8" column="0">
<widget class="QLabel" name="label_24">
<property name="text">
<string>Ban Score</string>
</property>
</widget>
</item>
<item row="8" column="1">
<widget class="QLabel" name="peerBanScore">
<property name="cursor">
<cursorShape>IBeamCursor</cursorShape>
</property>
<property name="text">
<string>N/A</string>
</property>
<property name="textFormat">
<enum>Qt::PlainText</enum>
</property>
<property name="textInteractionFlags">
<set>Qt::LinksAccessibleByMouse|Qt::TextSelectableByKeyboard|Qt::TextSelectableByMouse</set>
</property>
</widget>
</item>
<item row="9" column="0">
<widget class="QLabel" name="label_22">
<property name="text">
<string>Connection Time</string>
</property>
</widget>
</item>
<item row="9" column="1">
<item row="8" column="1">
<widget class="QLabel" name="peerConnTime">
<property name="cursor">
<cursorShape>IBeamCursor</cursorShape>
Expand All @@ -1315,14 +1292,14 @@
</property>
</widget>
</item>
<item row="10" column="0">
<item row="9" column="0">
<widget class="QLabel" name="label_15">
<property name="text">
<string>Last Send</string>
</property>
</widget>
</item>
<item row="10" column="1">
<item row="9" column="1">
<widget class="QLabel" name="peerLastSend">
<property name="cursor">
<cursorShape>IBeamCursor</cursorShape>
Expand All @@ -1338,14 +1315,14 @@
</property>
</widget>
</item>
<item row="11" column="0">
<item row="10" column="0">
<widget class="QLabel" name="label_19">
<property name="text">
<string>Last Receive</string>
</property>
</widget>
</item>
<item row="11" column="1">
<item row="10" column="1">
<widget class="QLabel" name="peerLastRecv">
<property name="cursor">
<cursorShape>IBeamCursor</cursorShape>
Expand All @@ -1361,14 +1338,14 @@
</property>
</widget>
</item>
<item row="12" column="0">
<item row="11" column="0">
<widget class="QLabel" name="label_18">
<property name="text">
<string>Sent</string>
</property>
</widget>
</item>
<item row="12" column="1">
<item row="11" column="1">
<widget class="QLabel" name="peerBytesSent">
<property name="cursor">
<cursorShape>IBeamCursor</cursorShape>
Expand All @@ -1384,14 +1361,14 @@
</property>
</widget>
</item>
<item row="13" column="0">
<item row="12" column="0">
<widget class="QLabel" name="label_20">
<property name="text">
<string>Received</string>
</property>
</widget>
</item>
<item row="13" column="1">
<item row="12" column="1">
<widget class="QLabel" name="peerBytesRecv">
<property name="cursor">
<cursorShape>IBeamCursor</cursorShape>
Expand All @@ -1407,14 +1384,14 @@
</property>
</widget>
</item>
<item row="14" column="0">
<item row="13" column="0">
<widget class="QLabel" name="label_26">
<property name="text">
<string>Ping Time</string>
</property>
</widget>
</item>
<item row="14" column="1">
<item row="13" column="1">
<widget class="QLabel" name="peerPingTime">
<property name="cursor">
<cursorShape>IBeamCursor</cursorShape>
Expand All @@ -1430,7 +1407,7 @@
</property>
</widget>
</item>
<item row="15" column="0">
<item row="14" column="0">
<widget class="QLabel" name="peerPingWaitLabel">
<property name="toolTip">
<string>The duration of a currently outstanding ping.</string>
Expand All @@ -1440,7 +1417,7 @@
</property>
</widget>
</item>
<item row="15" column="1">
<item row="14" column="1">
<widget class="QLabel" name="peerPingWait">
<property name="cursor">
<cursorShape>IBeamCursor</cursorShape>
Expand All @@ -1456,14 +1433,14 @@
</property>
</widget>
</item>
<item row="16" column="0">
<item row="15" column="0">
<widget class="QLabel" name="peerMinPingLabel">
<property name="text">
<string>Min Ping</string>
</property>
</widget>
</item>
<item row="16" column="1">
<item row="15" column="1">
<widget class="QLabel" name="peerMinPing">
<property name="cursor">
<cursorShape>IBeamCursor</cursorShape>
Expand All @@ -1479,14 +1456,14 @@
</property>
</widget>
</item>
<item row="17" column="0">
<item row="16" column="0">
<widget class="QLabel" name="label_timeoffset">
<property name="text">
<string>Time Offset</string>
</property>
</widget>
</item>
<item row="17" column="1">
<item row="16" column="1">
<widget class="QLabel" name="timeoffset">
<property name="cursor">
<cursorShape>IBeamCursor</cursorShape>
Expand Down
4 changes: 0 additions & 4 deletions src/qt/rpcconsole.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1307,10 +1307,6 @@ void RPCConsole::updateNodeDetail(const CNodeCombinedStats *stats) {
// This check fails for example if the lock was busy and
// nodeStateStats couldn't be fetched.
if (stats->fNodeStateStatsAvailable) {
// Ban score is init to 0
ui->peerBanScore->setText(
QString("%1").arg(stats->nodeStateStats.nMisbehavior));

// Sync height is init to -1
if (stats->nodeStateStats.nSyncHeight > -1) {
ui->peerSyncHeight->setText(
Expand Down
65 changes: 6 additions & 59 deletions src/test/denialofservice_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) {
connman->ClearNodes();
}

BOOST_AUTO_TEST_CASE(DoS_banning) {
BOOST_AUTO_TEST_CASE(peer_discouragement) {
const Config &config = GetConfig();
std::atomic<bool> interruptDummy(false);

Expand Down Expand Up @@ -277,7 +277,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning) {
dummyNode2.fSuccessfullyConnected = true;
{
LOCK(cs_main);
Misbehaving(dummyNode2.GetId(), 50, "");
Misbehaving(dummyNode2.GetId(), DISCOURAGEMENT_THRESHOLD - 1);
}
{
LOCK2(cs_main, dummyNode2.cs_sendProcessing);
Expand All @@ -290,75 +290,22 @@ BOOST_AUTO_TEST_CASE(DoS_banning) {
BOOST_CHECK(banman->IsDiscouraged(addr1));
{
LOCK(cs_main);
Misbehaving(dummyNode2.GetId(), 50, "");
// 2 reaches discouragement threshold
Misbehaving(dummyNode2.GetId(), 1);
}
{
LOCK2(cs_main, dummyNode2.cs_sendProcessing);
BOOST_CHECK(
peerLogic->SendMessages(config, &dummyNode2, interruptDummy));
}
BOOST_CHECK(banman->IsDiscouraged(addr2));
BOOST_CHECK(banman->IsDiscouraged(addr1)); // Expect both 1 and 2
BOOST_CHECK(banman->IsDiscouraged(addr2)); // to be discouraged now

bool dummy;
peerLogic->FinalizeNode(config, dummyNode1.GetId(), dummy);
peerLogic->FinalizeNode(config, dummyNode2.GetId(), dummy);
}

BOOST_AUTO_TEST_CASE(DoS_banscore) {
const Config &config = GetConfig();
std::atomic<bool> interruptDummy(false);

auto banman = std::make_unique<BanMan>(GetDataDir() / "banlist.dat",
config.GetChainParams(), nullptr,
DEFAULT_MISBEHAVING_BANTIME);
auto connman = std::make_unique<CConnman>(config, 0x1337, 0x1337);
auto peerLogic = std::make_unique<PeerLogicValidation>(
connman.get(), banman.get(), *m_node.scheduler, *m_node.chainman,
*m_node.mempool);

banman->ClearBanned();
CAddress addr1(ip(0xa0b0c001), NODE_NONE);
CNode dummyNode1(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr1, 3, 1,
CAddress(), "", ConnectionType::INBOUND);
dummyNode1.SetSendVersion(PROTOCOL_VERSION);
peerLogic->InitializeNode(config, &dummyNode1);
dummyNode1.nVersion = 1;
dummyNode1.fSuccessfullyConnected = true;
{
LOCK(cs_main);
Misbehaving(dummyNode1.GetId(), DISCOURAGEMENT_THRESHOLD - 11);
}
{
LOCK2(cs_main, dummyNode1.cs_sendProcessing);
BOOST_CHECK(
peerLogic->SendMessages(config, &dummyNode1, interruptDummy));
}
BOOST_CHECK(!banman->IsDiscouraged(addr1));
{
LOCK(cs_main);
Misbehaving(dummyNode1.GetId(), 10, "");
}
{
LOCK2(cs_main, dummyNode1.cs_sendProcessing);
BOOST_CHECK(
peerLogic->SendMessages(config, &dummyNode1, interruptDummy));
}
BOOST_CHECK(!banman->IsDiscouraged(addr1));
{
LOCK(cs_main);
Misbehaving(dummyNode1.GetId(), 1, "");
}
{
LOCK2(cs_main, dummyNode1.cs_sendProcessing);
BOOST_CHECK(
peerLogic->SendMessages(config, &dummyNode1, interruptDummy));
}
BOOST_CHECK(banman->IsDiscouraged(addr1));

bool dummy;
peerLogic->FinalizeNode(config, dummyNode1.GetId(), dummy);
}

BOOST_AUTO_TEST_CASE(DoS_bantime) {
const Config &config = GetConfig();
std::atomic<bool> interruptDummy(false);
Expand Down
24 changes: 13 additions & 11 deletions test/functional/p2p_leak.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,10 @@ def on_blocktxn(self, message): self.bad_message(message)
# anyway, and eventually get disconnected.


class CNodeNoVersionBan(CLazyNode):
# send a bunch of veracks without sending a message. This should get us disconnected.
# NOTE: implementation-specific check here. Remove if bitcoind ban
# behavior changes
class CNodeNoVersionMisbehavior(CLazyNode):
# Send enough veracks without a message to reach the peer discouragement
# threshold. This should get us disconnected. NOTE: implementation-specific
# test; update if our discouragement policy for peer misbehavior changes.
def on_open(self):
super().on_open()
for _ in range(DISCOURAGEMENT_THRESHOLD):
Expand Down Expand Up @@ -133,8 +133,8 @@ def set_test_params(self):
self.num_nodes = 1

def run_test(self):
no_version_bannode = self.nodes[0].add_p2p_connection(
CNodeNoVersionBan(), send_version=False, wait_for_verack=False)
no_version_disconnect_node = self.nodes[0].add_p2p_connection(
CNodeNoVersionMisbehavior(), send_version=False, wait_for_verack=False)
no_version_idlenode = self.nodes[0].add_p2p_connection(
CNodeNoVersionIdle(), send_version=False, wait_for_verack=False)
no_verack_idlenode = self.nodes[0].add_p2p_connection(
Expand All @@ -144,8 +144,10 @@ def run_test(self):
# verack, since we never sent one
no_verack_idlenode.wait_for_verack()

wait_until(lambda: no_version_bannode.ever_connected,
timeout=10, lock=mininode_lock)
wait_until(
lambda: no_version_disconnect_node.ever_connected,
timeout=10,
lock=mininode_lock)
wait_until(lambda: no_version_idlenode.ever_connected,
timeout=10, lock=mininode_lock)
wait_until(lambda: no_verack_idlenode.version_received,
Expand All @@ -158,16 +160,16 @@ def run_test(self):
# Give the node enough time to possibly leak out a message
time.sleep(5)

# This node should have been banned
assert not no_version_bannode.is_connected
# Expect this node to be disconnected for misbehavior
assert not no_version_disconnect_node.is_connected

self.nodes[0].disconnect_p2ps()

# Wait until all connections are closed
wait_until(lambda: len(self.nodes[0].getpeerinfo()) == 0)

# Make sure no unexpected messages came in
assert not no_version_bannode.unexpected_msg
assert not no_version_disconnect_node.unexpected_msg
assert not no_version_idlenode.unexpected_msg
assert not no_verack_idlenode.unexpected_msg

Expand Down

0 comments on commit 59fd163

Please sign in to comment.