test: p2p: check misbehavior for non-continuous headers messages

theStack commented at 11:53 am on May 21, 2023: contributor

This PR adds missing test coverage for a peer sending a headers message where the headers don’t connect to each other, which should be treated as misbehaving (not disconnecting though, as the score increase is only 20). The relevant code path is PeerManagerImpl::ProcessHeadersMessage -> PeerManagerImpl::CheckHeadersPoW -> PeerManagerImpl::CheckHeadersAreContinuous:

https://github.com/bitcoin/bitcoin/blob/17acb2782a66f033238340e4fb81009e7f79e97a/src/net_processing.cpp#L2415-L2419

https://github.com/bitcoin/bitcoin/blob/17acb2782a66f033238340e4fb81009e7f79e97a/src/net_processing.cpp#L2474-L2484

DrahtBot commented at 11:53 am on May 21, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	instagibbs, sr-gi, achow101

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

Coverage

Coverage	Change (pull 27712, 2a317811edc302daab135ee1ad0fcea3115f4e8a)	Reference (master, a13f3746dccd9c4e)
Lines	+0.0000 %	83.0273 %
Functions	+0.0000 %	79.4882 %
Branches	+0.0000 %	50.4073 %

Updated at: 2023-05-25T07:04:16.691161.

DrahtBot added the label Tests on May 21, 2023

theStack force-pushed on May 21, 2023

DrahtBot added the label CI failed on May 21, 2023

test: p2p: check misbehavior for non-continuous headers messages a97c59f12d

theStack force-pushed on May 21, 2023

theStack commented at 1:25 pm on May 21, 2023: contributor

(Force-pushed with a simpler variant that just deletes a random block header in the middle of a valid headers message instead of messing around with hashPrevBlock, which needed to re-grind the header for valid PoW. The previous version looked like this:

0        # modify previous block hash of an arbitrary sequent block header to break link
1        modified_block_header = block_headers[random.randrange(1, len(block_headers))]
2        modified_block_header.hashPrevBlock ^= 1
3        # need to re-calculate for valid PoW, as this check is done first
4        modified_block_header.rehash()
5        while not modified_block_header.hash.startswith('0'):
6            modified_block_header.nNonce += 1
7            modified_block_header.rehash()

)

DrahtBot removed the label CI failed on May 21, 2023

fanquake requested review from instagibbs on May 22, 2023

fanquake requested review from mzumsande on May 22, 2023

in test/functional/p2p_invalid_messages.py:291 in a97c59f12d

284@@ -283,6 +285,25 @@ def test_invalid_pow_headers_msg(self):
285             peer.send_message(msg_headers([blockheader]))
286             peer.wait_for_disconnect()
287 
288+    def test_noncontinuous_headers_msg(self):
289+        self.log.info("Test headers message with non-continuous headers sequence is logged as misbehaving")
290+        block_hashes = self.generate(self.nodes[0], 10)
291+        block_headers = []

instagibbs commented at 1:37 pm on May 22, 2023:

absolutely don’t have to take, I just like using this pythonism

0        block_headers = [from_hex(CBlockHeader(), self.nodes[0].getblockheader(block_hash, False) for block_hash in block_hashes]

theStack commented at 7:29 pm on May 22, 2023:

Nice indeed, will take it if I have to retouch.

sr-gi commented at 6:38 pm on June 14, 2023:

Agreeing with @instagibbs, the inline for loop init feels more idiomatic, but certainly not a blocker

in test/functional/p2p_invalid_messages.py:305 in a97c59f12d

300+
301+        # delete arbitrary block header somewhere in the middle to break link
302+        del block_headers[random.randrange(1, len(block_headers)-1)]
303+        with self.nodes[0].assert_debug_log(expected_msgs=MISBEHAVING_NONCONTINUOUS_HEADERS_MSGS):
304+            peer.send_and_ping(msg_headers(block_headers))
305+        self.nodes[0].disconnect_p2ps()

instagibbs commented at 1:41 pm on May 22, 2023:

do we want to assert anything about not disconnecting, or the total ban score?

theStack commented at 7:42 pm on May 22, 2023:

As per #19469 and #20755 it is not possible any more to inspect a peer’s ban score via RPC. Could either assert for a more concrete debug log message (containing the expected ‘(0 -> 20)’ banscore before/after string), or even assert for a disconnect after send the problematic message 5 times (100/20), but that all seems excessive and we also don’t do that in other functional tests checking misbehavior. As for “asserting about not disconnecting”, I think that’s already covered by using the send_and_ping call; if the headers message led to a disconnect, our pings after wouldn’t receive a pong anymore (which is internally checked on P2PInterface.sync_with_ping) and the test would fail.

instagibbs approved

instagibbs commented at 1:41 pm on May 22, 2023: member

ACK a97c59f12d50d11d8859f4bbfb9fcf66de667ca0

glozow added the label Good First Review on Jun 7, 2023

sr-gi commented at 6:37 pm on June 14, 2023: member

ACK https://github.com/bitcoin/bitcoin/commit/a97c59f12d50d11d8859f4bbfb9fcf66de667ca0

achow101 commented at 6:44 pm on June 15, 2023: member

ACK a97c59f12d50d11d8859f4bbfb9fcf66de667ca0

achow101 merged this on Jun 15, 2023

achow101 closed this on Jun 15, 2023

theStack deleted the branch on Jun 16, 2023

sidhujag referenced this in commit bb0e58cbc5 on Jun 19, 2023

bitcoin locked this on Jun 15, 2024

test: p2p: check misbehavior for non-continuous headers messages #27712

Reviews

Coverage