-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Add bip-txrelayv2 #1663
base: master
Are you sure you want to change the base?
Add bip-txrelayv2 #1663
Conversation
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.
Hi @ariard, I was not able to find messages pertaining to this BIP draft on the mailing list. Has this draft been posted to the mailing list?
After a quick read, this document seems to be an early draft. We recommend that BIP authors open an initial pull request against their personal BIPs repository while they are collecting their thoughts and iterating quickly by themselves, and only open a pull request against the main repository after they have shared their proposal with the mailing list and are seeking public review.
<pre> | ||
BIP: XXX | ||
Layer: Peer Services | ||
Title: Transaction Relay V2 |
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.
Could you perhaps reconsider the title? This sounds somewhat similar to the v2 P2P protocol that was recently deployed. Maybe something along the lines of "Behavior regarding unsolicited Transactions", if I’m getting the gist of what you are thinking about?
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 intentional to be similar to BIP24 and the upgrade of the V2 P2P protocol. Before the BIP design and implementation, the peer-to-peer protocol was not versioned, neither really encrypted. Similarly, we’re introducing a versioning of the transaction-relay protocol, which a node can use to selectively support policies and mechanisms.
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 agree that this naming can be easily confused with v2 p2p and v3 transaction relay; differentiated naming would be beneficial.
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 agree that this naming can be easily confused with v2 p2p and v3 transaction relay; differentiated naming would be beneficial.
I’ll let you come with a better name, v2 p2p is the 2nd version of the p2p protocol, of which technically this transaction relay v2 protocol should lay on top. v3 transaction relay if you mean the TRUC thing it’s about the transaction versioning (the nVersion
transaction field).
bip-txrelayv2.mediawiki
Outdated
Title: Transaction Relay V2 | ||
Author: Antoine Riard <[email protected]> | ||
Comments-Summary: No comments yet. | ||
Comments-URI: https://github.com/bitcoin/bips/wiki/Comments:BIP-0339 |
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.
Comments-URI: https://github.com/bitcoin/bips/wiki/Comments:BIP-0339 |
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.
Corrected with BIP-0XXX
, in my memory bip339 is the oldest bip related to p2p tx-relay upgrade (as bip338 was never implemented and deployed). So I copy past bip layout from it.
bip-txrelayv2.mediawiki
Outdated
==Specification== | ||
|
||
Peers supporting the v2 transaction relay protocol signal support by adverstising | ||
the 13th bit service flag in the addr p2p messages (`ADDR` and `ADDRV2`). |
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 Specification seems to define how to advertise a new network service, but it should perhaps elaborate what this service entails.
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’s intentional. The mechanisms and policies supported should be described in a distinct BIP, e.g if some class of messages are disabled like it was proposed by BIP338, which is a distinct mechanism than behavior regarding unsolicited transactions.
==Backward compatibility== | ||
|
||
Older clients remain fully compatible and interoperable after this change. |
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.
Please be sure to include sufficient detail in the spec or compatibility section to explain why this is the case.
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 the case - Earliest version behavior can be applied on the linked implementation code is bitcoin core 29.0.
e7d51e0
to
4c0f0cb
Compare
An email should have been sent linking this BIP draft to the bitcoin dev mail list.
My “offensive” self and my “defensive” self after carefully weighing the technical trade-offs have dialogued for long enough and they have come to consensus that this BIP was ready for more review :) I’m seeking public review. |
4c0f0cb
to
143c993
Compare
Updated at 143c993 with improvements on the BIP. |
processing or alteration in current processing of p2p messages from equivalently upgraded | ||
peers. | ||
|
||
This upgrade mechanism raise issues if new processing of current p2p messages is sanctionned |
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.
Nice to see the new typos CI working (edit: though it doesn't seem to catch everything, hm)
This upgrade mechanism raise issues if new processing of current p2p messages is sanctionned | |
This upgrade mechanism raise issues if new processing of current p2p messages is sanctioned |
(you can run it locally by installing https://github.com/crate-ci/typos and then running typos
in repo root)
|
||
==Motivation== | ||
|
||
Historically, upgrades to the transaction relay protocols have been down by updating |
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.
Historically, upgrades to the transaction relay protocols have been down by updating | |
Historically, upgrades to the transaction relay protocols have been done by updating |
This upgrade mechanism raise issues if new processing of current p2p messages is sanctionned | ||
by an upgraded peer at the peering level by a disconnection of the violating non-upgraded peers. | ||
Non-upgraded peers and clients can see their transaction broadcast silently failing for a subset | ||
of theirs opened connections. |
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.
of theirs opened connections. | |
of their open connections. |
Non-upgraded peers and clients can see their transaction broadcast silently failing for a subset | ||
of theirs opened connections. | ||
|
||
We can improve current upgrade mechanism of the transaction relay protocol by introducing a new |
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.
We can improve current upgrade mechanism of the transaction relay protocol by introducing a new | |
We can improve the current upgrade mechanism of the transaction relay protocol by introducing a |
missing "the", and "introducing" implies new
|
||
==Specification== | ||
|
||
Peers supporting the v2 transaction relay protocol signal support by adverstising |
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.
Peers supporting the v2 transaction relay protocol signal support by adverstising | |
Peers supporting the v2 transaction relay protocol signal support by advertising |
Updated at From reading Jon’s comment on the naming issue, and after brainstorming with myself alone, as this BIP can be used to introducing more compartmentalization in the p2p messages (transactions, blocks, addr, etc), it is good to make explicit which version of the p2p protocol, this compartmentalization is building on. With BIP324 we have a versioning of the protocol, so it can be requested that
|
If we find a better way to implement reject unrequested transactions in a backward compatible fashion, I’ll withdraw it.
ML post: https://groups.google.com/g/bitcoindev/c/nWUcXBQbLGU
Bitcoin Core conceptual discussion: bitcoin/bitcoin#30572