net_processing: fix BIP152 first integer interpretation #35550

pull brunoerg wants to merge 2 commits into bitcoin:master from brunoerg:2026-06-addrman-branch changing 2 files +21 −1

brunoerg commented at 5:58 PM on June 17, 2026: contributor

Fixes #35542

According to the BIP152, the first integer in sendcmpct message shall be interpreted as a boolean (and MUST have a value of either 1 or 0). We currently correctly interpret it as boolean, however, we accept any value >=1 and treat it as true, deviating from the specification. This PR fixes it.
net_processing: fix BIP152 first integer interpretation 2d0dce0af5
DrahtBot commented at 5:59 PM on June 17, 2026: contributor



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



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35550.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK edilmedeiros, davidgumberg, w0xlt, Sjors, jonatack

Concept ACK yancyribbens

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Type	Reviewers
ACK	edilmedeiros, davidgumberg, w0xlt, Sjors, jonatack
Concept ACK	yancyribbens

in test/functional/p2p_compactblocks.py:275 in 2e6276bcb3

 270 | +        node = self.nodes[0]
 271 | +        bad_peer = node.add_p2p_connection(TestP2PConn())
 272 | +        msg = msg_sendcmpct(announce=2, version=2)
 273 | +        with node.assert_debug_log(['invalid sendcmpct announce field']):
 274 | +            bad_peer.send_without_ping(msg)
 275 | +            bad_peer.wait_for_disconnect()

jonatack commented at 8:17 PM on June 17, 2026:

nit, can probably use the helper

@@ -271,8 +271,7 @@ class CompactBlocksTest(BitcoinTestFramework):
         bad_peer = node.add_p2p_connection(TestP2PConn())
         msg = msg_sendcmpct(announce=2, version=2)
         with node.assert_debug_log(['invalid sendcmpct announce field']):
-            bad_peer.send_without_ping(msg)
-            bad_peer.wait_for_disconnect()
+            bad_peer.send_await_disconnect(msg)

brunoerg commented at 10:50 PM on June 17, 2026:

Done.

in src/net_processing.cpp:3945 in 2e6276bcb3 outdated

3941 |          vRecv >> sendcmpct_hb >> sendcmpct_version;
3942 |  
3943 | +        // BIP152: the first integer is interpreted as a boolean and MUST have a
3944 | +        // value of either 1 or 0.
3945 | +        if (sendcmpct_hb > 1) {
3946 | +            Misbehaving(peer, "invalid sendcmpct announce field");

jonatack commented at 8:22 PM on June 17, 2026:

Musing, perhaps "byte" or "message" instead of "field" here but no strong opinion.

brunoerg commented at 10:55 PM on June 17, 2026:

I'm fine with "byte" or "field", no strong opinion on this. Will keep as is for now. Thanks.

jonatack commented at 8:28 PM on June 17, 2026: member

ACK 2e6276bcb3560bf07f6daa689a052200a4eb0a54, might be good to verify that other implementations would not be affected by this tightening. Sanity checked getpeerinfo and -netinfo output.
test: announce field must be 0 or 1 in sendcmpct abc33ff043
brunoerg force-pushed on Jun 17, 2026
brunoerg commented at 10:51 PM on June 17, 2026: contributor

Force-pushed addressing #35550 (review)
yancyribbens commented at 11:29 PM on June 17, 2026: contributor

Concept ACK
edilmedeiros commented at 12:33 PM on June 18, 2026: contributor

utACK abc33ff043f07226d1f1f88a1d1ddce8dccdd930

might be good to verify that other implementations would not be affected by this tightening.

This was found during the review of the draft implementation for btcd. Floresta doesn't implement bip152.
DrahtBot requested review from jonatack on Jun 18, 2026
davidgumberg commented at 11:34 PM on June 18, 2026: contributor

crACK https://github.com/bitcoin/bitcoin/commit/abc33ff043f07226d1f1f88a1d1ddce8dccdd930 Seems reasonable to comply with BIP152 strictly, test looks good as well.
w0xlt commented at 6:13 AM on June 19, 2026: contributor

ACK abc33ff043f07226d1f1f88a1d1ddce8dccdd930
fanquake commented at 7:24 AM on June 19, 2026: member

cc @instagibbs
Sjors commented at 12:18 PM on June 19, 2026: member
ACK abc33ff043f07226d1f1f88a1d1ddce8dccdd930 @edilmedeiros wrote:

This was found during the review of the draft implementation for btcd.

Glad to see more adoption.

The other independent implementations (not code forks) that implement BIP152 support (usually just the message encoding), as far as I'm aware, seem fine:
- bcoin: https://github.com/bcoin-org/bcoin/blob/v2.2.0/lib/net/pool.js#L3774-L3776 (blockMode is a number, not forced a bool, but it defaults to 0 and afaik is never changed)
- gocoin: https://github.com/piotrnar/gocoin/blob/1.11.0/client/network/tick.go#L813-L814
- libbitcoin: https://github.com/libbitcoin/libbitcoin-network/blob/master/include/bitcoin/network/messages/peer/detail/send_compact.hpp#L52 / https://github.com/libbitcoin/libbitcoin-network/blob/master/src/messages/peer/detail/send_compact.cpp#L91 (not sure if it ever actually sends the message, but encodes it correctly).
0xB10C commented at 12:54 PM on June 19, 2026: contributor

Happy to run this for a few days on a node and see if anything that connects to me triggers the misbehavior.

edit: Dashboard https://demo.peer.observer/monitoring/d/bcrn2q4/misbehavior-of-peers-for-23-35550
jonatack commented at 8:47 PM on June 19, 2026: member

re-ACK abc33ff043f07226d1f1f88a1d1ddce8dccdd930

Sanity-checked that Knots' code is the same as current master.

Thanks @Sjors and @0xB10C for looking at other impls and the network.