BIP 434 Support: Peer feature negotiation #35221

ajtowns commented at 11:48 AM on May 6, 2026: contributor

Adds support for BIP 434.

DrahtBot commented at 11:48 AM on May 6, 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/35221.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	darosior, fjahr, w0xlt
Concept ACK	sedited

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35410 (net: use the proxy if overriden when doing v2->v1 reconnections by vasild)
#28463 (p2p: Increase inbound capacity for block-relay only connections by mzumsande)

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.

ajtowns force-pushed on May 6, 2026

DrahtBot added the label CI failed on May 6, 2026

fjahr commented at 12:23 PM on May 6, 2026: contributor

Concept ACK

sedited commented at 12:43 PM on May 6, 2026: contributor

Concept ACK

ajtowns force-pushed on May 6, 2026

DrahtBot added the label Needs rebase on May 6, 2026

ajtowns force-pushed on May 6, 2026

DrahtBot removed the label Needs rebase on May 6, 2026

dergoegge added this to a project on May 11, 2026

dergoegge changed the project status on May 11, 2026

in src/net.h:829 in 88efda2743 outdated

 824 | @@ -825,6 +825,13 @@ class CNode
 825 |          return m_conn_type == ConnectionType::PRIVATE_BROADCAST;
 826 |      }
 827 |  
 828 | +    /** Protocol version advertised in our VERSION message.
 829 | +     *  Private broadcast connections use a fixed version to maximise anonymity. */

fjahr commented at 12:04 PM on May 12, 2026:

I would like a little more documentation on this, here or where private broadcast documentation mainly lives. Doesn't have to be in this PR though. For example, should this be bumped in the future or will this just stay at this version forever?

ajtowns commented at 1:47 PM on May 13, 2026:

Presumably it will be bumped if private broadcast connections need some new feature that's gated in some way. But it shouldn't be bumped without a need, aiui. See #27509 (comment) perhaps.

in src/net.cpp:952 in f51cb08f70 outdated

 951 | -    "",
 952 | -    "",
 953 | -    ""
 954 | +    "", "", "", // 29-31
 955 | +    "", "", "", "", // 32-35
 956 | +    "", NetMsgType::FEATURE, // 36-37

fjahr commented at 12:05 PM on May 12, 2026:

What's the reasoning for picking this number?

ajtowns commented at 1:49 PM on May 13, 2026:

See https://github.com/bitcoin/bips/pull/2092 and https://github.com/bitcoin/bips/blob/master/bip-0324/message_type_ids.md

sipa commented at 5:56 PM on May 15, 2026:

In commit "BIP434: FEATURE message support"

Nit: the "// Unimplemented message types that are assigned in BIP324:" comment above doesn't really apply to this.

w0xlt commented at 4:53 PM on May 12, 2026: contributor

Concept ACK

fjahr commented at 8:52 PM on May 12, 2026: contributor

Approach ACK

I really haven't found much to complain about so far. IMO this can be taken out of draft.

Instead I spent some time to create a functional test (LLM scaffolded, human edited): https://github.com/fjahr/bitcoin/commit/f7e256f9f841bb0f55fc29ea37415f99a882cf11 Feel free to ignore if you'd rather roll your own instead, of course.

in src/serialize.h:812 in cad323f630 outdated

 808 | +template<typename Stream, typename C>
 809 | +void Serialize(Stream& os, const std::basic_string_view<C>& str)
 810 | +{
 811 | +    WriteCompactSize(os, str.size());
 812 | +    if (!str.empty())
 813 | +        os.write(MakeByteSpan(str));

sipa commented at 5:44 PM on May 15, 2026:

In commit "serialize: string_view serialization"

Style nit: please use braces when indenting.

in src/serialize.h:497 in cda4d113d2 outdated

 493 | @@ -494,6 +494,7 @@ static inline Wrapper<Formatter, T&> Using(T&& t) { return Wrapper<Formatter, T&
 494 |  #define VARINT(obj) Using<VarIntFormatter<VarIntMode::DEFAULT>>(obj)
 495 |  #define COMPACTSIZE(obj) Using<CompactSizeFormatter<true>>(obj)
 496 |  #define LIMITED_STRING(obj,n) Using<LimitedStringFormatter<n>>(obj)
 497 | +#define LIMITED_VECTOR(obj,n) Using<LimitedVectorFormatter<n>>(obj)

sipa commented at 5:51 PM on May 15, 2026:

In commit "serialize: add LimitedVectorFormatter"

Add some basic roundtrip tests for this?

ajtowns commented at 10:05 PM on May 15, 2026:

Added a basic unit test

ajtowns force-pushed on May 15, 2026

ajtowns commented at 10:06 PM on May 15, 2026: contributor

Addressed nits, added the test, removed the DUMMY feature and replaced it with commented-out placeholders.

ajtowns marked this as ready for review on May 15, 2026

DrahtBot removed the label CI failed on May 16, 2026

in src/net_processing.cpp:3751 in db94443535

3745 | @@ -3737,6 +3746,11 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
3746 |              }
3747 |          }
3748 |  
3749 | +        if (greatest_common_version >= FEATURE_VERSION) {
3750 | +            // announce supported features
3751 | +            //MakeAndPushFeature(pfrom, NetMsgFeature::FOO, uint32_t{1});

fjahr commented at 11:30 AM on May 16, 2026:

nit:

            // MakeAndPushFeature(pfrom, NetMsgFeature::FOO, uint32_t{1});

ajtowns commented at 10:55 PM on May 21, 2026:

Presumably the first PR implementing a feature will remove this comment anyway, so will ignore unless i retouch for other reasons

in src/net_processing.cpp:3985 in db94443535 outdated

3980 | +            vRecv >> LIMITED_STRING(feature_id, MAX_FEATUREID_LENGTH);
3981 | +            std::vector<unsigned char> feature_data_vec;
3982 | +            vRecv >> LIMITED_VECTOR(feature_data_vec, MAX_FEATUREDATA_LENGTH);
3983 | +            feature_data = DataStream(feature_data_vec);
3984 | +        } catch (const std::exception&) {
3985 | +            feature_id.clear(); // use empty feature_id as error indicator

fjahr commented at 11:33 AM on May 16, 2026:

nit: I guess making feature_id an optional would a bit more idiomatic but no strong opinion

ajtowns commented at 10:56 PM on May 21, 2026:

0-3 byte feature_id's are also invalid, so this would just make it more complicated to check validity (if (!feature_id || feature_id.size() < 4)) and use the value later (*feature_id vs feature_id), I think.

fjahr commented at 11:38 AM on May 16, 2026: contributor

ACK db94443535b63497b9ead90efca01ab09df99d60

DrahtBot requested review from sedited on May 16, 2026

in test/functional/p2p_bip434_feature.py:100 in db94443535 outdated

  95 | +        self.test_features_announced_to_modern_peer()
  96 | +        self.test_feature_after_verack_disconnects()
  97 | +        self.test_feature_before_version_ignored()
  98 | +        self.test_feature_id_length_boundaries()
  99 | +        self.test_feature_data_length_boundaries()
 100 | +        self.test_trailing_bytes_disconnect()

w0xlt commented at 9:29 PM on May 20, 2026:

nit: a test for non-canonical CompactSize encodings can be added

diff --git a/test/functional/p2p_bip434_feature.py b/test/functional/p2p_bip434_feature.py
index 4f4f292a67..2e41d6851c 100755
--- a/test/functional/p2p_bip434_feature.py
+++ b/test/functional/p2p_bip434_feature.py
@@ -51,6 +51,12 @@ def feature_wire(feature_id, feature_data, *, trailing=b""):
             + trailing)
 
 
+def noncanonical_compact_size(n):
+    """Encode n using a non-minimal CompactSize uint16 representation."""
+    assert n < 253
+    return b"\xfd" + n.to_bytes(2, "little")
+
+
 def _version_msg(nversion):
     v = msg_version()
     v.nVersion = nversion
@@ -97,6 +103,7 @@ class P2PBIP434FeatureTest(BitcoinTestFramework):
         self.test_feature_before_version_ignored()
         self.test_feature_id_length_boundaries()
         self.test_feature_data_length_boundaries()
+        self.test_noncanonical_compact_size_disconnect()
         self.test_trailing_bytes_disconnect()
         self.test_truncated_feature_id_disconnect()
         self.test_truncated_feature_data_disconnect()
@@ -196,6 +203,19 @@ class P2PBIP434FeatureTest(BitcoinTestFramework):
             else:
                 self._expect_disconnect(payload)
 
+    def test_noncanonical_compact_size_disconnect(self):
+        self.log.info("Test that non-canonical CompactSize encodings trigger disconnect")
+        feature_id = b"abcd"
+        feature_data = b"data"
+        self._expect_disconnect(
+            noncanonical_compact_size(len(feature_id)) + feature_id
+            + ser_compact_size(len(feature_data)) + feature_data,
+        )
+        self._expect_disconnect(
+            ser_compact_size(len(feature_id)) + feature_id
+            + noncanonical_compact_size(len(feature_data)) + feature_data,
+        )
+
     def test_trailing_bytes_disconnect(self):
         self.log.info("Test that trailing bytes after data triggers disconnect")
         self._expect_disconnect(

</details>

ajtowns commented at 11:05 PM on May 21, 2026:

Isn't that a bit overkill for functional tests?

I think the only non-canonical compactsize test we currently have is in feature_block.py. The language from bip 434 is copied from 152 (compact blocks), so if we're adding a test for that behaviour here, presumably there should be a similar test there and in other places?

Doesn't really feel like the additional coverage is very meaningful?

w0xlt commented at 9:32 PM on May 20, 2026: contributor

ACK db94443535b63497b9ead90efca01ab09df99d60

brunoerg commented at 1:45 PM on May 21, 2026: contributor

I ran mutation testing for this PR with the addition of the functional test - the result can be seen at https://bitcoincore.space/ [Per PR > 36221]. All good, 100% of mutation score for net_processing and serialize.

ajtowns commented at 10:52 PM on May 21, 2026: contributor

FYI, the second-most-recent commit on https://github.com/ajtowns/bitcoin/commits/202605-bip324-id/ has an example of implementing a new bip434 feature on top of this PR.

darosior commented at 7:13 PM on May 27, 2026: member

Concept ACK

in src/net_processing.cpp:3974 in 112ea1cdc2 outdated

3965 | @@ -3952,6 +3966,40 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
3966 |          return;
3967 |      }
3968 |  
3969 | +    if (msg_type == NetMsgType::FEATURE) {
3970 | +        if (pfrom.fSuccessfullyConnected) {
3971 | +            // Disconnect peers that send a FEATURE message after VERACK.
3972 | +            LogDebug(BCLog::NET, "feature received after verack, %s", pfrom.DisconnectMsg());
3973 | +            pfrom.fDisconnect = true;
3974 | +            return;

darosior commented at 3:05 PM on May 28, 2026:

Should we also return if the greatest common version is <= FEATURE_VERSION? That seems preferable.

ajtowns commented at 5:49 AM on May 29, 2026:

I think ignoring only makes sense if we want to leave having some different logic for FEATURE messages with prior protocol versions as a feature upgrade option? Always disconnecting or making a best effort attempt to parse and support seem better otherwise.

No objection to switching to always-disconnect, ignoring just seems a weird middle ground that I don't think really makes sense?

ajtowns commented at 5:59 AM on May 29, 2026:

FWIW, WTXIDRELAY is ignored (with a log message) if we receive it with an early protocol version, ADDRV2, SENDCMPCT and SENDTXRCNCL are all accepted for any announced protocol version (BIP 155 doesn't limit SENDADDRV2 to any particular protocol version, but the other messages are limited).

darosior commented at 2:16 PM on May 29, 2026:

Disconnecting wouldn't be shocking, but quite inconsistent with how we normally ignore this type of misbehaviour from our peers.

sipa commented at 2:48 PM on May 29, 2026:

I think it's a good practice to outright forbid (by disconnecting) well-defined invalid behavior. We aren't easily able to retroactively start disconnecting things for condoned but ignored in the past for fear of breaking other software, but this isn't a concern when introducing a new feature, especially one that can be described through a BIP.

darosior commented at 3:59 PM on May 29, 2026:

No objection to disconnecting. I think disconnecting is preferable to accepting the features from a peer with version < 70017.

ajtowns commented at 11:06 AM on June 2, 2026:

Changed to disconnect.

darosior commented at 5:27 PM on May 28, 2026: member

Looks good to me, i just think we should ignore FEATURE messages from peers advertising protocol versions <= 70016. I think that would be cleaner because it would be symmetric with our own behaviour, and because having connections with features enabled via the FEATURE message but a common greatest protocol version <= 70016 would be a bit surprising.

I'm also not a big fan of the arbitrary minimum size limit on the feature_id field, but i don't think it's important enough to object.

ajtowns force-pushed on Jun 2, 2026

fjahr commented at 12:21 PM on June 2, 2026: contributor

re-ACK 3da32c8d158622cd0bf6510b55804fe471d362f7

Only change since last review is disconnecting from older version peers where feature is not expected.

DrahtBot requested review from w0xlt on Jun 2, 2026

DrahtBot requested review from darosior on Jun 2, 2026

DrahtBot added the label Needs rebase on Jun 4, 2026

serialize: string_view serialization

Allows `stream << sv` to serialize from a string_view directly, without
converting to a string or similar first.

1b3f776ebb

serialize: add LimitedVectorFormatter 94ed45427c

net: Add AdvertisedVersion() for protocol version advertised to a peer 3210fc477a

BIP434: FEATURE message support

BIP434 defines FEATURE messages which are sent between VERSION and VERACK
to indicate support for new P2P protocol features. This commit provides
the infrastructure for easily using BIP434 negotiation when implementing
such new P2P protocol features. Note that advertised protocol version
is bumped to 70017, as per BIP434's specification.

6a129983c9

test_framework: BIP 434 support 01b8a117d2

test: Add functional test for BIP434 da74ff9ca4

ajtowns force-pushed on Jun 4, 2026

DrahtBot removed the label Needs rebase on Jun 4, 2026

darosior approved

darosior commented at 8:50 PM on June 4, 2026: member

ACK da74ff9ca49ef5f6e6a06b31b039f1aa0300d11e

DrahtBot requested review from fjahr on Jun 4, 2026

fjahr commented at 10:02 PM on June 4, 2026: contributor

ACK da74ff9ca49ef5f6e6a06b31b039f1aa0300d11e

Verified latest push was only rebase.

w0xlt commented at 10:42 PM on June 4, 2026: contributor

reACK da74ff9ca49ef5f6e6a06b31b039f1aa0300d11e