rpc/net: Adds misbehaving_score to getpeerinfo #29530

sr-gi · 2024-03-01T21:46:22Z

This is motivated by the discussions in #29412 (#29412 (comment)) and #29524(#29524 (comment)).

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 · 2024-03-01T21:46:25Z

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 · 2024-03-01T22:11:49Z

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 · 2024-03-01T22:13:35Z

Concept ACK

Suggestion:

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

0xB10C · 2024-03-02T08:46:51Z

Concept ACK

dergoegge

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?

dergoegge · 2024-03-02T11:02:13Z

src/net_processing.h

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


Suggested change

int m_misbehavior_score{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.

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.

1440000bytes · 2024-03-02T17:02:40Z

src/rpc/net.cpp

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


Disconnected or banned?

Disconnected and discouraged, no?

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

IMO it's ok to leave as is.

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.

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 · 2024-03-02T18:25:37Z

I think it should be called banscore: #19469

sr-gi · 2024-03-02T23:09:03Z

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 · 2024-03-03T00:36:05Z

ACK for 976d61c.

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.

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

Sjors · 2024-03-04T09:58:28Z

Concept ACK

ACK 976d61c

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

theStack · 2024-03-04T18:20:34Z

Concept ACK

sr-gi · 2024-03-05T13:47:20Z

Addressed @brunoerg and @Sjors comments (#29530 (comment), #29530 (comment)).

I'll be adding some tests to address @dergoegge (#29530 (review)) in a new commit

Sjors · 2024-03-05T14:18:50Z

re-utACK 87efb6f

brunoerg

crACK 87efb6f

sipa · 2024-03-05T14:22:55Z

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 · 2024-03-05T14:52:10Z

@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 · 2024-03-05T15:47:39Z

@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 · 2024-03-05T18:04:45Z

@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

Github-Pull: bitcoin#29530 Rebased-From: 976d61c

instagibbs · 2024-03-06T15:06:36Z

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 · 2024-03-06T15:09:12Z

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 · 2024-03-06T16:01:42Z

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

achow101 · 2024-03-12T16:10:30Z

Closing as this seems to be superseded by #29575

sr-gi force-pushed the 2024-03-01-getpeerinfo-misbehaving branch from 020a8ef to 976d61c Compare March 1, 2024 21:46

sr-gi mentioned this pull request Mar 1, 2024

p2p: Don't consider blocks mutated if they don't connect to known prev block #29524

Merged

dergoegge suggested changes Mar 2, 2024

View reviewed changes

DrahtBot mentioned this pull request Mar 2, 2024

test: rpc: add last block announcement time to getpeerinfo result #27052

Open

1440000bytes reviewed Mar 2, 2024

View reviewed changes

DrahtBot requested review from brunoerg, 0xB10C and dergoegge March 3, 2024 00:36

rpc/net: Adds misbehaving_score to getpeerinfo

87efb6f

sr-gi force-pushed the 2024-03-01-getpeerinfo-misbehaving branch from 976d61c to 87efb6f Compare March 5, 2024 13:28

DrahtBot requested a review from theStack March 5, 2024 14:18

brunoerg reviewed Mar 5, 2024

View reviewed changes

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 5, 2024

rpc/net: Adds misbehaving_score to getpeerinfo

49815ab

Github-Pull: bitcoin#29530 Rebased-From: 976d61c

DrahtBot mentioned this pull request Mar 7, 2024

net_processing: make any misbehavior trigger immediate discouragement #29575

Open

DrahtBot added the CI failed label Mar 11, 2024

DrahtBot mentioned this pull request Mar 11, 2024

Simplify network-adjusted time warning logic #29623

Merged

glozow mentioned this pull request Mar 12, 2024

refactor: consolidate MempoolAcceptResult processing #29619

Merged

achow101 closed this Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc/net: Adds misbehaving_score to getpeerinfo #29530

rpc/net: Adds misbehaving_score to getpeerinfo #29530

sr-gi commented Mar 1, 2024

DrahtBot commented Mar 1, 2024 •

edited

sr-gi commented Mar 1, 2024 •

edited

brunoerg commented Mar 1, 2024 •

edited

0xB10C commented Mar 2, 2024

dergoegge left a comment

dergoegge Mar 2, 2024

Sjors Mar 4, 2024 •

edited

1440000bytes Mar 2, 2024

brunoerg Mar 2, 2024

sr-gi Mar 2, 2024

brunoerg Mar 2, 2024

Sjors Mar 4, 2024 •

edited

sr-gi Mar 5, 2024

1440000bytes commented Mar 2, 2024

sr-gi commented Mar 2, 2024 •

edited

tdb3 commented Mar 3, 2024 •

edited

Sjors commented Mar 4, 2024 •

edited

theStack commented Mar 4, 2024

sr-gi commented Mar 5, 2024

Sjors commented Mar 5, 2024

brunoerg left a comment

sipa commented Mar 5, 2024 •

edited

Sjors commented Mar 5, 2024 •

edited

sipa commented Mar 5, 2024

sr-gi commented Mar 5, 2024

instagibbs commented Mar 6, 2024

fanquake commented Mar 6, 2024

sipa commented Mar 6, 2024

achow101 commented Mar 12, 2024

rpc/net: Adds misbehaving_score to getpeerinfo #29530

rpc/net: Adds misbehaving_score to getpeerinfo #29530

Conversation

sr-gi commented Mar 1, 2024

DrahtBot commented Mar 1, 2024 • edited

Code Coverage

Reviews

Conflicts

sr-gi commented Mar 1, 2024 • edited

brunoerg commented Mar 1, 2024 • edited

0xB10C commented Mar 2, 2024

dergoegge left a comment

Choose a reason for hiding this comment

dergoegge Mar 2, 2024

Choose a reason for hiding this comment

Sjors Mar 4, 2024 • edited

Choose a reason for hiding this comment

1440000bytes Mar 2, 2024

Choose a reason for hiding this comment

brunoerg Mar 2, 2024

Choose a reason for hiding this comment

sr-gi Mar 2, 2024

Choose a reason for hiding this comment

brunoerg Mar 2, 2024

Choose a reason for hiding this comment

Sjors Mar 4, 2024 • edited

Choose a reason for hiding this comment

sr-gi Mar 5, 2024

Choose a reason for hiding this comment

1440000bytes commented Mar 2, 2024

sr-gi commented Mar 2, 2024 • edited

tdb3 commented Mar 3, 2024 • edited

Exercising RPC

Functional Test Usage

Sjors commented Mar 4, 2024 • edited

theStack commented Mar 4, 2024

sr-gi commented Mar 5, 2024

Sjors commented Mar 5, 2024

brunoerg left a comment

Choose a reason for hiding this comment

sipa commented Mar 5, 2024 • edited

Sjors commented Mar 5, 2024 • edited

sipa commented Mar 5, 2024

sr-gi commented Mar 5, 2024

instagibbs commented Mar 6, 2024

fanquake commented Mar 6, 2024

sipa commented Mar 6, 2024

achow101 commented Mar 12, 2024

DrahtBot commented Mar 1, 2024 •

edited

sr-gi commented Mar 1, 2024 •

edited

brunoerg commented Mar 1, 2024 •

edited

Sjors Mar 4, 2024 •

edited

Sjors Mar 4, 2024 •

edited

sr-gi commented Mar 2, 2024 •

edited

tdb3 commented Mar 3, 2024 •

edited

Sjors commented Mar 4, 2024 •

edited

sipa commented Mar 5, 2024 •

edited

Sjors commented Mar 5, 2024 •

edited