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	fjahr
Concept ACK	sedited, w0xlt

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:

#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

serialize: string_view serialization

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

98370d272d

serialize: add LimitedVectorFormatter 0658cc4e52

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

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.

112ea1cdc2

test_framework: BIP 434 support 95f4d2f08d

test: Add functional test for BIP434 db94443535

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});

in src/net_processing.cpp:3985 in db94443535

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

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

ACK db94443535b63497b9ead90efca01ab09df99d60

DrahtBot requested review from sedited on May 16, 2026