Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BIP 155: addrv2 BIP proposal #766

Merged
merged 4 commits into from Jul 23, 2019

Conversation

@laanwj
Copy link
Member

commented Feb 27, 2019

Proposal for wider v2 address messages.

Last time this went around (as a github gist) there were some considerations still to be discussed, for lack of a working mailing list I'll post them here:

Considerations

  • ''Client MAY store and gossip address formats that they do not know about'': does it ever make sense to gossip addresses outside a certain overlay network? Say, I2P addresses to Tor? I'm not sure. Especially for networks that have no exit nodes as there is no overlap with the globally routed internet at all.

  • Lower precision of time field? seconds precision seems overkill, and can even be harmful, there have been attacks that exploited high precision timestamps for mapping the current network topology.

    • (gmaxwell) If you care about space time field could be reduced to 16 bits easily. Turn it into a "time ago seen" quantized to 1 hour precision. (IIRC we quantize times to 2hrs regardless).
  • Rolling port into addr, or making the port optional, would make it possible to shave off two bytes for address types that don't have ports (however, all of the currently listed formats have a concept of port.). It could also be an optional data item (see below).

  • (gmaxwell) Optional (per-service) data could be useful for various things:

    • Node-flavors for striping (signalling which slice of the blocks the node has in selective pruning)

    • Payload for is alternative ports for other transports (e.g. UDP ports)

    • If we want optional flags. I guess the best thing would just be a byte to include the count of them, then a byte "type" for each one where the type also encodes if the payload is 0/8/16/32 bits. (using the two MSB of the type to encode the length). And then bound the count of them so that the total is still reasonably sized.

@luke-jr luke-jr added the New BIP label Mar 29, 2019

@luke-jr luke-jr changed the title addrv2 BIP proposal BIP 155: addrv2 BIP proposal Mar 29, 2019

@luke-jr

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

Assigned BIP 155

@laanwj laanwj referenced this pull request Jun 15, 2019

laanwj added some commits Jul 17, 2019

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

Thanks!
Filled in the BIP number and fixed the travis issues, this should be ready for merge I think?

<ref>[https://bitcoin.org/en/developer-reference#addr Bitcoin Developer Reference: addr message]</ref>, with the difference that the
fixed 16-byte IP address is replaced by a network ID and a variable-length address, and the time and services format has been changed to VARINT.

This means that the message contains a serialized <code>std::vector</code> of the following structure:

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 18, 2019

Member

I know this is how bips were written in the past, but I'd slightly prefer not to leak cpp details or Bitcoin Core implementation details such as std::vector (can be replaced by "list" or "vector"), pchCommand (can be replaced by "message type"), and static const int GOSSIP_ADDRV2_VERSION (can be omitted without loss of information).

If someone is eager to look at the cpp implementation, there is already a section that links to it.

This comment has been minimized.

Copy link
@laanwj

laanwj Jul 18, 2019

Author Member

fine with me

This comment has been minimized.

Copy link
@laanwj

laanwj Jul 18, 2019

Author Member

I think I went with explicit C++ types here for clarity as to how things would be serialized, after all there is not really a spec of the serialization format (and how the certain structures are named, like "how to serialize a list", wouldn't "a list" be vague and ambigious?) besides the implementation

This comment has been minimized.

Copy link
@MarcoFalke

This comment has been minimized.

Copy link
@laanwj

laanwj Jul 31, 2019

Author Member

string seems to be worse as it implies a human-readable text

Send <code>addrv2</code> messages only, and exclusively, when the peer has a certain protocol version (or higher):
<source lang="c++">
//! gossiping using `addrv2` messages starts with this version
static const int GOSSIP_ADDRV2_VERSION = 70016;

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 18, 2019

Member

I haven't put much thought into this, but could you explain the tradeoffs here a bit? I think there are two ways to announce support for a new p2p feature:

  • Bump the protocol version
  • Send a p2p message to announce your support for the feature

Bumping the minimum protocol version seems to imply that nodes would have to support all protocol features that were introduced prior (like compactblocks, feefilter, ...)

Whereas with a new protocol message, even mobile wallets could make use of the new feature?

This comment has been minimized.

Copy link
@laanwj

laanwj Jul 31, 2019

Author Member

it does add a new protocol message (that's the addrv2), but the version tells it that the other side understands them

would you prefer to add another protocol message to signal support, which is always sent after version?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 31, 2019

Member

No, you can keep this as is. mobile wallets tend to get their addresses from dns seeds, so this shouldn't matter.

|-
| <code>VARINT</code> (unsigned)
| <code>time</code>
| Time that this node was last seen as connected to the network. A time in Unix epoch time format, up to 64 bits wide.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 18, 2019

Member

Would help to explain what "last seen" means, since it appears easy to get wrong. E.g. bitcoin/bitcoin#5860 (comment)

This comment has been minimized.

Copy link
@laanwj

laanwj Jul 31, 2019

Author Member

The meaning of this field doesn't change from the current implementation. I'm fine with adding an explanation though, if you supply it, but I'm not sure what to add here.

@luke-jr luke-jr merged commit 095087b into bitcoin:master Jul 23, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laanwj

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2019

Came up during discussion on IRC, need to be explicit about representing IPv4 as network ID IPV4, that the old wrapped representation is no longer allowed:

==Appendix X: IPv4 address encoding==
Clients MUST send IPv4 addresses with this network ID, with the 32-bit address in the address field.

Clients SHOULD ignore IPv4 Mapped Address (::FFFF:0:0/96) addresses on receive if they come with the IPV6 network ID.

(thanks @dongcarl)

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2019

As for changing the signaling from protocol version-based to sending a message. I think this message, when sent from one peer to another, would signal support for the first peer to receive addrv2 messages from the other one. So something like sendaddrv2? (like bip130)

I wish we had some kind of standard message for this.

I think for simplicy of implementation we'd want to mandate that this should be sent after version but before any other (non-BIP-signalling) messages, so a connection won't switch to v2 suddenly halfway other things.

Edit2: This sendaddrv2 message could also contain wider addresses that doesn't fit in the version message. e.g. addrMe, the TorV3 address that we used to connect to the peer. The version message would contain a dummy address (localhost) then.

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

There was some discussion on IRC that there should be a max cap for network addresses, but it should not be 256 bits, but more something like 512 bytes, to accommodate future extensions but have some kind of limit for DoS. Currently it says:

Field addr has a variable length, with a maximum of 32 bytes (256 bits). Clients SHOULD reject
longer addresses.

Change this to: a maximum of 512 bytes, and the whole message (not just the single address) SHOULD be rejected if it contains any larger item.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.