rpc/net: Adds misbehaving_score to getpeerinfo

sr-gi commented at 9:46 pm on March 1, 2024: member

This is motivated by the discussions in #29412 (https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1484773525) and #29524(https://github.com/bitcoin/bitcoin/pull/29524#discussion_r1508895425).

Currently, we are asserting logs to check that certain misbehavior has been accounted for, which is far from an ideal interface. Being able to check that the misbehavior_score of a peer has increased as expected seems a better approach.

From an end-user perspective, being able to check whether a peer has been misbehaving also seems useful.

DrahtBot commented at 9:46 pm on March 1, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors, brunoerg
Concept ACK	0xB10C, dergoegge, theStack
Stale ACK	tdb3

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29623 (Simplify network-adjusted time warning logic by stickies-v)
#29575 (net_processing: make any misbehavior trigger immediate discouragement by sipa)
#27052 (test: rpc: add last block announcement time to getpeerinfo result by LarryRuane)

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.

sr-gi force-pushed on Mar 1, 2024

sr-gi commented at 10:11 pm on March 1, 2024: member

In this PR I have only patched rpc_net.py to cover the newly added field, but there is no testing for it being increased.

I’m planning to do a followup patching tests that may be relying on asserting logs to catch misbehavior, but I’m open to adding some tests for this if the current coverage seems insufficient (rpc_net.py doesn’t seem to be the right place to do so though)

brunoerg commented at 10:13 pm on March 1, 2024: contributor

Concept ACK

Suggestion:

 0diff --git a/test/functional/p2p_invalid_messages.py b/test/functional/p2p_invalid_messages.py
 1index 40a69936bc..750447815e 100755
 2--- a/test/functional/p2p_invalid_messages.py
 3+++ b/test/functional/p2p_invalid_messages.py
 4@@ -323,6 +323,8 @@ class InvalidMessagesTest(BitcoinTestFramework):
 5         del block_headers[random.randrange(1, len(block_headers)-1)]
 6         with self.nodes[0].assert_debug_log(expected_msgs=MISBEHAVING_NONCONTINUOUS_HEADERS_MSGS):
 7             peer.send_and_ping(msg_headers(block_headers))
 8+
 9+        assert_equal(self.nodes[0].getpeerinfo()[0]['misbehavior_score'], 20)
10         self.nodes[0].disconnect_p2ps()
11 
12     def test_resource_exhaustion(self):

0xB10C commented at 8:46 am on March 2, 2024: contributor

Concept ACK

in src/net_processing.h:44 in 976d61c974 outdated

40@@ -41,6 +41,7 @@ struct CNodeStateStats {
41     bool m_addr_relay_enabled{false};
42     ServiceFlags their_services;
43     int64_t presync_height{-1};
44+    int m_misbehavior_score{0};

dergoegge commented at 11:02 am on March 2, 2024:

0    int misbehavior_score{0};

nit: I think the m_ only makes sense if there are methods on the struct/class or if there likely will be.

Sjors commented at 9:56 am on March 4, 2024:

At the moment developer-notes.md just says “Class member variables have a m_ prefix.”, and most new variables in this struct already follow that pattern.

dergoegge changes_requested

dergoegge commented at 11:06 am on March 2, 2024: member

Concept ACK

Patch looks good to me but I think you should add some test changes here as wel or even a new p2p_misbehavior.py?

in src/rpc/net.cpp:190 in 976d61c974 outdated

185@@ -186,6 +186,8 @@ static RPCHelpMan getpeerinfo()
186                                                               "best capture connection behaviors."},
187                     {RPCResult::Type::STR, "transport_protocol_type", "Type of transport protocol: \n" + Join(TRANSPORT_TYPE_DOC, ",\n") + ".\n"},
188                     {RPCResult::Type::STR, "session_id", "The session ID for this connection, or \"\" if there is none (\"v2\" transport protocol only).\n"},
189+                    {RPCResult::Type::NUM, "misbehavior_score", "The accumulated misbehavior score for this peer.\n"
190+                                                                 "The peer will be disconnected if 100 is reached.\n"},

1440000bytes commented at 5:02 pm on March 2, 2024:

Disconnected or banned?

brunoerg commented at 7:14 pm on March 2, 2024:

Disconnected and discouraged, no?

sr-gi commented at 11:13 pm on March 2, 2024:

Technically yes, but it feels like the disencoragement is an implementation detail completely transparent to the end user. I feel it’s not worth mentioning, but I won’t oppose it if there’s consensus

brunoerg commented at 11:16 pm on March 2, 2024:

IMO it’s ok to leave as is.

Sjors commented at 10:25 am on March 4, 2024:

Based on my reading of banman.h, we could add something like: “Their IP is not banned, but subsequent connections from the same address are given lower priority (see banman.h)”

“lower priority” is a bit vague, but I think it more clearly captures what “discourage” is trying to do: priotize other peers, but tolerate this ex-peer if it comes back, in case it gives us something useful.

sr-gi commented at 1:18 pm on March 5, 2024:

If we are going to refer them to banman.h, I think just adding and discouraged should be enough. Saying the peer is “discouraged” or that “connections to it are given lower priority” is pretty vague without context, so I’d rather keep the help short if add a reference

1440000bytes commented at 6:25 pm on March 2, 2024: none

I think it should be called banscore: https://github.com/bitcoin/bitcoin/pull/19469

sr-gi commented at 11:09 pm on March 2, 2024: member

Disconnected or banned?

I think it should be called banscore: #19469

The banscore is not a thing anymore afaik, and it hasn’t been for some time. When the misbehavior score reaches 100 peers are disconnected and disencouranged, but not banned anymore.

You can still ban peers manually via rpc IIRC, but this doesn’t happen automatically due to misbehavior anymore.

tdb3 commented at 0:36 am on March 3, 2024: contributor

ACK for 976d61c974ed045c2e2497dcebc4d1fcc4d60a29.

Excellent improvement allowing checking of misbehavior_score directly, rather than through logs. I’m assuming the goal is to first add the capability to check the misbehavior_score in this PR, then update p2p_invalid_messages.py and p2p_filter.py in subsequent PR(s). This approach makes sense to me, and helps tell the story for the commit log.

Built. Ran affected functional test rpc_net.py (test passed).

Exercising RPC

As a quick sanity check, getpeerinfo was called (using bitcoin-cli and curl, bitcoind connected to peers on a signet). misbehavior_score is reported for each peer as expected.

Functional Test Usage

Performed a very quick modification of p2p_filter.py, which currently relies on assert_debug_log(['Misbehaving']) statements. The mod uses the capability added by this PR to obtain misbehavior_score directly, rather than indirectly through log statements. The PR’s additional capability seems to work very nicely.

0    def test_size_limits(self, filter_peer):
1        self.log.info('Check that too large filter is rejected')
2        with self.nodes[0].assert_debug_log(['Misbehaving']):
3            filter_peer.send_and_ping(msg_filterload(data=b'\xbb'*(MAX_BLOOM_FILTER_SIZE+1)))
4        peer_info = self.nodes[0].getpeerinfo()
5        assert_equal(peer_info[0]['misbehavior_score'], 100)

DrahtBot requested review from brunoerg on Mar 3, 2024

DrahtBot requested review from 0xB10C on Mar 3, 2024

DrahtBot requested review from dergoegge on Mar 3, 2024

Sjors commented at 9:58 am on March 4, 2024: member

Concept ACK

ACK 976d61c974ed045c2e2497dcebc4d1fcc4d60a29

If you have to retouch, consider adding @brunoerg’s test patch #29530 (comment) (which I tested as well)

theStack commented at 6:20 pm on March 4, 2024: contributor

Concept ACK

rpc/net: Adds misbehaving_score to getpeerinfo 87efb6f0cf

sr-gi force-pushed on Mar 5, 2024

sr-gi commented at 1:47 pm on March 5, 2024: member

Addressed @brunoerg and @Sjors comments (https://github.com/bitcoin/bitcoin/pull/29530#issuecomment-1974003810, #29530 (review)).

I’ll be adding some tests to address @dergoegge (https://github.com/bitcoin/bitcoin/pull/29530#pullrequestreview-1912642548) in a new commit

Sjors commented at 2:18 pm on March 5, 2024: member

re-utACK 87efb6f0cfde8ebcc21d4cecf20ee31f83c8ac31

DrahtBot requested review from theStack on Mar 5, 2024

brunoerg commented at 2:20 pm on March 5, 2024: contributor

crACK 87efb6f0cfde8ebcc21d4cecf20ee31f83c8ac31

sipa commented at 2:22 pm on March 5, 2024: member

I think some history got misinterpreted here.

The removal of the configuration option, and the removal of the RPC output field, were because of an intent to get rid of the concept of a misbehavior/ban score entirely, though it seems the last step of doing that never got picked up.

Working on a PR to do that. It may need some discussion, but in the mean time, I don’t think we should be walking things backward. If we end up deciding that misbehavior score will stick around for longer, perhaps this can be picked up again.

Sjors commented at 2:52 pm on March 5, 2024: member

@sipa that’s also how I read it, but it’s been a few years since - so I assumed the idea was abandoned. If a new PR is coming soon(tm) we could hold off on merging this. Or just undo it before the v28 branch-off.

sipa commented at 3:47 pm on March 5, 2024: member

@Sjors See https://github.com/sipa/bitcoin/commits/202403_nomisbehave . I’m going to run it for a while myself to see if this practically affects peers on the network (the first commit adds logging for places where behavior changes).

sr-gi commented at 6:04 pm on March 5, 2024: member

@sipa that’s also how I read it, but it’s been a few years since - so I assumed the idea was abandoned. If a new PR is coming soon(tm) we could hold off on merging this. Or just undo it before the v28 branch-off.

I’m happy to hold/close this one if the goal is to get rid of the misbehavior scores

luke-jr referenced this in commit 49815aba19 on Mar 5, 2024

instagibbs commented at 3:06 pm on March 6, 2024: member

I think some history got misinterpreted here.

You have the removal PR/issue/discussion handy for record keeping? I haven’t seen it directly linked here

fanquake commented at 3:09 pm on March 6, 2024: member

You have the removal PR/issue/discussion handy for record keeping? I haven’t seen it directly linked here

Could be starting point: #19219 (comment). Along with #19469 and #20755.

sipa commented at 4:01 pm on March 6, 2024: member

Opened a PR to get rid of misbehavior score entirely: #29575

DrahtBot added the label CI failed on Mar 11, 2024

achow101 commented at 4:10 pm on March 12, 2024: member

Closing as this seems to be superseded by #29575

achow101 closed this on Mar 12, 2024

rpc/net: Adds misbehaving_score to getpeerinfo #29530

Code Coverage

Reviews

Conflicts

Exercising RPC

Functional Test Usage