p2p: Implement `encoding` traits for `AddrV2Payload` by rustaceanrob · Pull Request #5762 · rust-bitcoin/rust-bitcoin

rustaceanrob · 2026-03-03T10:15:15Z

Last prerequisite before implementing encoding for NetworkMessage

nyonson · 2026-03-03T20:42:41Z

Andrew just wrote up a good summary of the CompactSize restrictions which I think kinda explains your question: #5752 (comment). Basically, we have designed CompactSize codecs around collection lengths. Maybe this service flags use case is showing a gap in the interface? I am not quiet sure the best way forward yet.

tcharding · 2026-03-09T00:53:39Z

Is this ready for review @rustaceanrob?

rustaceanrob · 2026-03-09T07:17:22Z

I was deciding what to do about the service flag oddities, but I think this should actually be ready to go.

In protocol.h, although the service flags run to uint64_t, they mention 1 << 24 to 1 << 31 as the reserved bits for experimentation. Given we are only at 1 << 11, in both experimentation and normal operation we don't go over 32 bit max. I don't think we will have a case where a signaled flag is silently cut off by the u32 cast until we are old and gray. I can add an in-line comment or mention this in the commit.

@luisschwab what service bits are used for Utreexo?

yancyribbens · 2026-03-09T11:41:30Z

+            Encoder4::new(
+                ArrayEncoder::without_length_prefix(self.time.to_le_bytes()),
+                // Silent high-bits cutoff on 32-bit arch
+                CompactSizeEncoder::new(self.services.to_u64() as usize),


Can you provide maybe a better description of why 64 bits is needed for the services?

It's usually not preferable to use as to silently truncate bits. Ideally you want to use into(), and if that's not an option, consider an explicit saturate so there's less surprise. Although, I'm having trouble finding a good description of what the upper bits for the services portion of AddrV2 could be. It seems silly that this services field needs 64 bits, so I wonder why there is a need to declare that many different service possibilities..

Lastly, other crates in this workspace do a specific // cast OK comment, for example "cast ok, 64 fits into a u8".

Can you provide maybe a better description of why 64 bits is needed for the services?

No, I don't know the history and the documentation states experimental bits only go to 1 << 31 so I am unsure why 64 bits was used.

It seems silly that this services field needs 64 bits

Yes it does.

// cast OK comment, for example

I will adopt that, thanks.

Updated the commit message and in-line code comment

thanks for posting the link to protocol.h, that makes more sense now, sort of. ha, "temporary experiments"..

Maybe it would be better though to just explicitly panic with some such message like "temporary service experiments can not be run on 32bit architectures". It's such a rare case though that I think this is unlikely to ever be encountered. Just that it's much better to avoid using as so people don't need to think too hard about what could happen.

I agree with @yancyribbens' comment above

luisschwab · 2026-03-09T16:53:37Z

@rustaceanrob utreexod and Floresta currently use 1 << 24 (NODE_UTREEXO) and 1 << 25 (NODE_UTREEXO_ARCHIVE), which are in the experimental range. BIP-0183 defines 1 << 12 (NODE_UTREEXO) and 1 << 13 (NODE_UTREEXO_ARCHIVE), which we will migrate to in the near future. I do have a draft PR here to add P2P Utreexo stuff, but I'm waiting for the BIPs to become final to update it and mark it as ready for review.

yancyribbens · 2026-03-09T19:23:11Z

@luisschwab

@rustaceanrob utreexod and Floresta currently use 1 << 24 (NODE_UTREEXO) and 1 << 25 (NODE_UTREEXO_ARCHIVE), which are in the experimental range. BIP-0183 defines 1 << 12 (NODE_UTREEXO) and 1 << 13 (NODE_UTREEXO_ARCHIVE), which we will migrate to in the near future.

Interesting. How is this supposed to work on 32 bit machines, though?

I do have a draft PR here to add P2P Utreexo stuff, but I'm waiting for the BIPs to become final to update it and mark it as ready for review.

Oh cool, Where?

luisschwab · 2026-03-09T19:35:54Z

Interesting. How is this supposed to work on 32 bit machines, though?

Not sure what you mean by this, but 32-bit machines can just use two 32-bit registers to represent an u64. That would only be an issue if we used usize for it, but that's not the case:

/// Flags to indicate which network services a node supports.
#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct ServiceFlags(u64);

Oh cool, Where?

#5009

yancyribbens · 2026-03-09T20:19:29Z

That would only be an issue if we used usize for it, but that's not the case:

Well, we are talking about this call: CompactSizeEncoder::new(self.services.to_u64() as usize) to here which does indeed take a usize.

#5009

Ok yes I've seen that. nice.

luisschwab · 2026-03-09T20:53:24Z

Well, we are talking about this call: CompactSizeEncoder::new(self.services.to_u64() as usize) to here which does indeed take a usize.

Oh, I hadn't looked at the changes here. Utreexo stuff will be fine on 32-bit once we move to the bits defined in BIP-0183.

rustaceanrob · 2026-03-10T09:25:11Z

Interestingly there are nodes advertising u64::MAX, probably as a sort of fingerprinting or explicit attack: bitcoin/bitcoin#34768

As acknowledged in that thread, these services are unassigned and are meaningless. I updated the first commit to panic if the services are greater than u32::MAX, which is almost certainly malicious and beyond the experimental range. Likewise when decoding, if bits higher than experimental range are assigned, they should be ignored IMO.

yancyribbens · 2026-03-10T19:02:14Z

I updated the first commit to panic if the services are greater than u32::MAX, which is almost certainly malicious and beyond the experimental range.

Thanks. I'm not sure if others have the same opinion, although I would prefer knowing something experimental I'm working on isn't supported by a panic instead of silently succeeding and passing a service as u32::MAX which is what values would be truncated to. I guess this could be debug_assert! which would only panic then in debug mode. Probably not worth worrying about though unless there is an issue that arises?

Sadly we still need the pesky as to do the conversion - When I had suggested adding a panic, I had thought that we could not use as but I guess it's still required.

yancyribbens · 2026-03-10T19:10:08Z

Actually, I'm wondering now if debug_assert would really be the better way to go. The consequence of this change is that in the rare case that someone is running a arch that's 32 bits would have their software blow up if someone on the network was running experimental services. That would be good to know about in debug mode, but in release mode that could be annoying if you just don't care about the service some malicious or experimental node is advertising.

luisschwab · 2026-03-10T19:30:48Z

The consequence of this change is that in the rare case that someone is running a arch that's 32 bits would have their software blow up if someone on the network was running experimental services.

Yes, that's an unnaceptable DoS vector. Floresta and utreexod would make a lot of nodes crash if this was the case.

yancyribbens · 2026-03-10T20:03:02Z

Yes, that's an unnaceptable DoS vector. Floresta and utreexod would make a lot of nodes crash if this was the case.

To be fair, you'd have to be running some pretty ancient setup - 32 bits, no PAE kernel. Still, I think given what you said, debug_assert sounds better IMO

yancyribbens · 2026-03-11T23:30:21Z

It looks like it would be better to wait for #5784 to merge. Since Services is a wrapped u64 no ugly as cast is required then. That means all the comments can be removed. Not sure why nobody else commented here about that. Sorry if I created more work for you.

rustaceanrob · 2026-03-12T14:11:45Z

Putting on ice since #5784 is making good progress

85a6da3 api: update for CompactSizeU64Decoder (Nick Johnson) bb7bf48 consensus_encoding: const the compact size prefix values (Nick Johnson) 4614515 consensus_encoding: add round trip test for compact size codecs (Nick Johnson) 77a60ae consensus_encoding: add a u64 decoder for compact size (Nick Johnson) 773b4a3 consensus_encoding: add a u64 constructor to compact size encoder (Nick Johnson) a06bf03 consensus_encoding: move compact size into its own module (Nick Johnson) Pull request description: Add u64 support to the Compact Size codec since there are non prefix length use cases (#5762). Kind of related to #5752. I believe this is the simplest way to add support, but the asymmetry kind of annoys me. On the encoding side, only a new constructor is necessary. But on the decoding side a new type is required since there is a 1:1 with the associated output. I don't think symmetry is a strong enough reason to put a type on the encoding side though. I didn't put much though into the names, focusing on minimal change. But maybe it would be worth it to name each encoder/decoder after its use case instead of impl? So the existing one would be called something like `LengthPrefixDecoder` and so on. The first commit moves the existing compact size stuff into its own `::compact_size` module, to kind of corner this weirdness. This creates kind of a funky dependency graph with `::decode` and `::decode::decoders` (and the equivalent encoders side), but none of that is exposed from the crate since it is a private module. ACKs for top commit: tcharding: ACK 85a6da3 apoelstra: ACK 85a6da3; successfully ran local tests Tree-SHA512: ba3dca8f698ede3538664e29edba3ba2cb5190515233a0b3afef93af656d6ef7538efad7f7173cad38e53263073d5ac6a69be5c940c907cbe5b20181786e23b2

yancyribbens · 2026-03-23T09:38:06Z

Putting on ice since #5784 is making good progress

5784 recently merged, so this work can be resumed.

Member of `AddrV2Payload` which is ultimately apart of `NetworkMessage`. Some non-zero number of clients advertise services outside of 32 bits of precision, which requires decoding arbitrarily large compact sizes. As these integers do not represent collection lengths and will not be used for allocation, this is acceptable. ref: https://github.com/bitcoin/bitcoin/blob/4d7d5f6b79d4c11c47e7a828d81296918fd11d4d/src/protocol.h#L332 ref: bitcoin/bitcoin#34768

Vector counterpart of the previous commit.

rustaceanrob · 2026-03-23T19:14:15Z

Rebased cbfdb80 with the compact size encoding changes

tcharding

ACK cbfdb80

apoelstra

ACK cbfdb80; successfully ran local tests

github-actions Bot added the C-p2p label Mar 3, 2026

nyonson reviewed Mar 3, 2026

View reviewed changes

Comment thread p2p/src/address.rs Outdated

nyonson mentioned this pull request Mar 3, 2026

consensus_encoding: Add range check to when decoding a compact size #5752

Closed

rustaceanrob mentioned this pull request Mar 4, 2026

Remaining bitcoin dependencies in p2p #5411

Closed

6 tasks

nyonson mentioned this pull request Mar 5, 2026

consensus_encoding: add u64 support to compact size #5784

Merged

rustaceanrob force-pushed the 26-3-3-addrv2-pl branch from 8714b85 to b49b9a7 Compare March 8, 2026 14:37

rustaceanrob marked this pull request as ready for review March 9, 2026 07:21

yancyribbens reviewed Mar 9, 2026

View reviewed changes

rustaceanrob force-pushed the 26-3-3-addrv2-pl branch 2 times, most recently from 41127aa to 1ce00fa Compare March 9, 2026 12:24

rustaceanrob force-pushed the 26-3-3-addrv2-pl branch from 1ce00fa to 20b1e3f Compare March 10, 2026 09:22

rustaceanrob force-pushed the 26-3-3-addrv2-pl branch from 20b1e3f to 98c213f Compare March 11, 2026 16:10

rustaceanrob marked this pull request as draft March 12, 2026 14:11

rustaceanrob force-pushed the 26-3-3-addrv2-pl branch from 98c213f to 648f2a5 Compare March 17, 2026 13:00

rustaceanrob added 3 commits March 23, 2026 17:40

p2p: Implement encoding traits for AddrV2Payload

514f36b

Vector counterpart of the previous commit.

test(p2p): Update deser tests for AddrV2Payload

cbfdb80

rustaceanrob force-pushed the 26-3-3-addrv2-pl branch from 648f2a5 to cbfdb80 Compare March 23, 2026 17:49

rustaceanrob marked this pull request as ready for review March 23, 2026 19:14

tcharding approved these changes Mar 24, 2026

View reviewed changes

apoelstra approved these changes Mar 26, 2026

View reviewed changes

apoelstra merged commit 0741d87 into rust-bitcoin:master Mar 26, 2026
28 checks passed

Uh oh!

Conversation

rustaceanrob commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nyonson commented Mar 3, 2026

Uh oh!

tcharding commented Mar 9, 2026

Uh oh!

rustaceanrob commented Mar 9, 2026

Uh oh!

yancyribbens Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

rustaceanrob Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustaceanrob Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yancyribbens Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

luisschwab Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

luisschwab commented Mar 9, 2026

Uh oh!

yancyribbens commented Mar 9, 2026

Uh oh!

luisschwab commented Mar 9, 2026

Uh oh!

yancyribbens commented Mar 9, 2026

Uh oh!

luisschwab commented Mar 9, 2026

Uh oh!

rustaceanrob commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yancyribbens commented Mar 10, 2026

Uh oh!

yancyribbens commented Mar 10, 2026

Uh oh!

luisschwab commented Mar 10, 2026

Uh oh!

yancyribbens commented Mar 10, 2026

Uh oh!

yancyribbens commented Mar 11, 2026

Uh oh!

rustaceanrob commented Mar 12, 2026

Uh oh!

yancyribbens commented Mar 23, 2026

Uh oh!

rustaceanrob commented Mar 23, 2026

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rustaceanrob commented Mar 3, 2026 •

edited

Loading

rustaceanrob Mar 9, 2026 •

edited

Loading

rustaceanrob Mar 9, 2026 •

edited

Loading

rustaceanrob commented Mar 10, 2026 •

edited

Loading