-
Notifications
You must be signed in to change notification settings - Fork 5.9k
BIP 434: Peer Feature Negotiation #2076
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
base: master
Are you sure you want to change the base?
Conversation
murchandamus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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), even if they do | ||
| not recognise the `featureid`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be clearer to place this paragraph after the next one, as the "even if" phrase is confusing before reading the next paragraph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| but no experience with doing so via `verack` payload. | ||
|
|
||
| A mild disadvantage compared to using a `verack` payload is that this | ||
| approach allows the possibility of interactive feature negotiation prior |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be confused.
| approach allows the possibility of interactive feature negotiation prior | |
| approach does not allow the possibility of interactive feature negotiation prior |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
Let’s call this BIP 434. |
bip-peer-feature-negotiation.md
Outdated
| parsed (including having missing or additional data), even if they do | ||
| not recognise the `featureid`. | ||
|
|
||
| Nodes implementing this BIP MUST ignore `feature` messages specifying a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not a MUST, it's a SHOULD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
9697b2b to
9630c4c
Compare
|
|
||
| Nodes implementing both this BIP and [BIP 324 (v2 P2P encrypted | ||
| transport)][BIP324] MUST treat a message with a 1-byte `message_type` | ||
| equal to `XXX` that is received prior to `verack` as the `feature` message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should synchronise specifying a real number here with updating BIP 324 to claim that number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
Updated for number assignment, BIP3 headers, and @jonatack's phrasing suggestions |
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/[email protected]/T/#u
Mailing list discussion: https://groups.google.com/g/bitcoindev/c/DFXtbUdCNZE