Abstract:
This BIP defines a peer-to-peer message that can be used for announcements and negotiation related to support of new peer-to-peer features.
Mailing list post: https://gnusha.org/pi/bitcoindev/aUUXLgEUCgGb122o@erisian.com.au/T/#u
Mailing list discussion: https://groups.google.com/g/bitcoindev/c/DFXtbUdCNZE
14 | +``` 15 | + 16 | +## Abstract 17 | + 18 | +This BIP defines a peer-to-peer message that can be used for announcements 19 | +and negotiation related to support of new peer-to-peer features.
and negotiation related to support of new or optional peer-to-peer features.
That seems redundant -- new p2p features have to be optional or they'll break backwards compatibility (and if they break backwards compatibility and are mandatory, maybe there's no need to negotiate), and existing optional features would at least need some sort of new spec in order to use this approach?
Fair enough!
227 | +This approach may be appropriate when making substantial changes to a 228 | +deployed protocol and backwards compatibility is desirable on a short-term 229 | +basis, or when there is disagreement amongst implementations or users 230 | +as to which approach is most desirable. 231 | + 232 | +## Considerations
Nit: I’m interpreting this as the stand-in for the Rationale section.
Some of the rationale is in the motivation section ("what inspired the design"), some is in the spec itself (particularly the "feature negotiation" parts); the "considerations" section focuses on "why these design decisions vs other similarly plausible ones". 49 (and 84), 90, and 348 also use a Considerations section fwiw. I'm mostly following the structure of Suhas's old draft. I didn't rename it to Rationale because it didn't seem sensible to move all the rationale into that section, or to do the footnotes thing like 340, and BIP3 specifies Rationale as optional, and that it's fine as long as you "consider and address as appropriate".
Thanks for your submission. This proposal appears already complete from an editorial perspective, and I have hardly any other comments. It would be great to see some P2P experts weigh in, but otherwise, this seems fairly mature.
18 | +This BIP defines a peer-to-peer message that can be used for announcements 19 | +and negotiation related to support of new peer-to-peer features. 20 | + 21 | +## Motivation 22 | + 23 | +Historically, new peer-to-peer protocol changes have been tied to
It's clear to us, but probably still good to define the acronym used after.
Historically, new peer-to-peer ("P2P") protocol changes have been tied to
20 | + 21 | +## Motivation 22 | + 23 | +Historically, new peer-to-peer protocol changes have been tied to 24 | +bumping the protocol version, so that peers know to only attempt feature 25 | +negotiation with other peers that may have been upgraded to support the
nit, could replace
"peers" and "other peers", with "nodes" and "peers" (as done further below in line 135)
"peers that may have been upgraded to support" with "peers that support"
feel free to ignore
26 | +feature. Coordinating the protocol version across implementations, when 27 | +different clients may have different priorities for features to implement, 28 | +is an unnecessary burden in the upgrade process for P2P features that do 29 | +not require universal support. And at a more philosophical level, having 30 | +the P2P protocol be [permissionlessly extensible][permless-extensible], 31 | +with no coordination required between implementations or developers
with no coordination required between implementations or developers,
30 | +the P2P protocol be [permissionlessly extensible][permless-extensible], 31 | +with no coordination required between implementations or developers 32 | +seems ideal for a decentralized system. 33 | + 34 | +Many earlier P2P protocol upgrades were implemented as new messages 35 | +sent after a peer connection is setup (ie, after receipt of a `verack`
sent after a peer connection is set up (ie, after receipt of a `verack`
32 | +seems ideal for a decentralized system. 33 | + 34 | +Many earlier P2P protocol upgrades were implemented as new messages 35 | +sent after a peer connection is setup (ie, after receipt of a `verack` 36 | +message by both sides). See [BIP 130 (sendheaders)][BIP130], [BIP 133 37 | +(feefilter)][BIP133], [BIP 152 (compact blocks)][BIP152] for some
(feefilter)][BIP133], and [BIP 152 (compact blocks)][BIP152] for
44 | +`verack` to indicate support of the new feature. 45 | + 46 | +In all these cases, sending new messages on the network raises the 47 | +question of what non-implementing software will do with such messages. The 48 | +common behavior observed on the network was for software to ignore 49 | +unknown messages received from a peer, so these proposals posed small
might be clearer to replace "small" with "no", so as not to be read as "a small risk" (if that is the idea here)
117 | +| Type | Name | Description | 118 | +| ----------- | ------------- | ----------- | 119 | +| string | `featureid` | Unique identifier for the feature | 120 | +| byte-vector | `featuredata` | Feature-specific configuration data | 121 | + 122 | +The `featureid` is encoded in the usual way, that is as a `CompactSize`
nit, maybe describe or link to a definition of CompactSize on first use
The `featureid` is encoded in the usual way, that is, as a `CompactSize`
133 | +ie serialized as a `CompactSize` of 0. 134 | + 135 | +Nodes implementing this BIP MAY disconnect peers that send `feature` 136 | +messages where the `feature` message's payload cannot be correctly 137 | +parsed (including having missing or additional data), even if they do 138 | +not recognise the `featureid`.
It may be clearer to place this paragraph after the next one, as the "even if" phrase is confusing before reading the next paragraph.
I think it should be phrased more positively to disambiguate:
Nodes implementing this BIP MAY disconnect peers that send `feature`
messages where the `feature` message's payload cannot be correctly
parsed (including having missing or additional data). In the special case where a node is able to partially parse the message to read the `featureid`, but they do not recognise the `featureid`, they SHOULD NOT disconnect (to preserve upgradability of feature messages).
This is a valid reinterpretation of the earlier bit because the "MAY" disconnect is truly optional, so the addition of a SHOULD NOT doesn't modify the semantic, but makes it more clear what should be implemented, while still retaining valid behavior if a node disconnects because of upgraded feature types.
232 | +## Considerations 233 | + 234 | +The advantage this approach has over bumping the protocol version 235 | +number when introducing new P2P messages or data structures, is that no 236 | +coordination is required (that is, there is no longer a question whether 237 | +version "n+1" belong to Alice's new feature, or Bob's new feature),
version "n+1" belongs to Alice's new feature, or Bob's new feature),
269 | +that prefer to fully validate each message received: 270 | + 271 | + * `feature` messages, even for unknown features, must always be fully 272 | + parseable into a `featureid` and `featuredata` 273 | + * Ignoring unknown messages prior to `verack` is only a recommendation 274 | + nor a requirement, so compliant implementations may disconnect on an
"not" meant here?
, not a requirement, so compliant implementations may disconnect on an
258 | + does not limit the number of features you can accept; and 259 | + * we have experience with negotiating features with individual messages, 260 | + but no experience with doing so via `verack` payload. 261 | + 262 | +A mild disadvantage compared to using a `verack` payload is that this 263 | +approach allows the possibility of interactive feature negotiation prior
I may be confused.
approach does not allow the possibility of interactive feature negotiation prior
The idea is that interactive feature negotiation is a "bad" thing, as it makes negotiation more complex and potentially slower, so allowing it is a disadvantage.
I think the headers will need to be updated to BIP 3 now. Some editorial comments on first read. Agree it is ready for number.
Let’s call this BIP 434.
135 | +Nodes implementing this BIP MAY disconnect peers that send `feature` 136 | +messages where the `feature` message's payload cannot be correctly 137 | +parsed (including having missing or additional data), even if they do 138 | +not recognise the `featureid`. 139 | + 140 | +Nodes implementing this BIP MUST ignore `feature` messages specifying a
this is not a MUST, it's a SHOULD
No, it is a MUST; implementations that do not comply with that requirement do not correctly implement this specification.
The reason it's a requirement is that not ignoring such messages means your implementation is not forwards compatible with new features.
156 | +chosen, such as a URL to the repository where development is taking place, 157 | +or the sha256 digest of some longer reference. 158 | + 159 | +Nodes implementing both this BIP and [BIP 324 (v2 P2P encrypted 160 | +transport)][BIP324] MUST treat a message with a 1-byte `message_type` 161 | +equal to `XXX` that is received prior to `verack` as the `feature` message.
Should synchronise specifying a real number here with updating BIP 324 to claim that number.
It seems reasonable to add the change of BIP 324 to this PR in the same commit. Or alternatively, that could be another separate PR after this document is published before it’s progressed to Complete.
ACK 9630c4c8d076b289e24d7cad818d43440ce613ae