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
:
test: p2p: check misbehavior for non-continuous headers messages #27712
pull theStack wants to merge 1 commits into bitcoin:master from theStack:202305-test-check_for_noncontinuous_header_msg changing 1 files +21 −0-
theStack commented at 11:53 am on May 21, 2023: contributor
-
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 withhashPrevBlock
, 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 blockerin 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 thesend_and_ping
call; if the headers message led to a disconnect, our pings after wouldn’t receive a pong anymore (which is internally checked onP2PInterface.sync_with_ping
) and the test would fail.instagibbs approvedinstagibbs commented at 1:41 pm on May 22, 2023: memberACK a97c59f12d50d11d8859f4bbfb9fcf66de667ca0glozow added the label Good First Review on Jun 7, 2023sr-gi commented at 6:37 pm on June 14, 2023: memberachow101 commented at 6:44 pm on June 15, 2023: memberACK a97c59f12d50d11d8859f4bbfb9fcf66de667ca0achow101 merged this on Jun 15, 2023achow101 closed this on Jun 15, 2023
theStack deleted the branch on Jun 16, 2023sidhujag referenced this in commit bb0e58cbc5 on Jun 19, 2023bitcoin locked this on Jun 15, 2024
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-31 03:12 UTC
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-31 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me
More mirrored repositories can be found on mirror.b10c.me