-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Add BIP 338: Disable transaction relay message #1052
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 338: Disable transaction relay message #1052
Conversation
This message, valid between version/verack for peers with version >= 70017, would allow establishing at the time of connection that no transactions will be announced/requested between those peers.
I sent an email to the bitcoin-dev list about this today (https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-January/018340.html) -- would like to request a BIP number be assigned. |
Concept ACK |
Also change the phrasing to more clearly indicate when block-relay-only peering was deployed.
Anything else to be done here? |
ping @luke-jr |
Can you add something to Motivation that actually explains why this is useful? |
@luke-jr Happy to edit if you have specific feedback about language I can include. Can I get a BIP number in the meantime so that I have a way to refer to this work elsewhere? |
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.
Approach ACK
# A new disabletx message is added, which is defined as an empty message where pchCommand == "disabletx". | ||
# The protocol version of nodes implementing this BIP must be set to 70017 or higher. | ||
# If a node sets the transaction relay field in the version message to a peer to false, then the disabletx message MAY also be sent in response to a version message from that peer if the peer's protocol version is >= 70017. If sent, the disabletx message MUST be sent prior to sending a verack. | ||
# A node that has sent or received a disabletx message to/from a peer MUST NOT send any of these messages to the peer: |
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 disconnection (as opposed to ignoring said messages) be explicitly specified here for the MUST NOT messages (or is that implied?)
Disconnection is mentioned line 68 below, "Nodes MAY decide to not remain connected..."
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 how software handles a peer that violates a "MUST NOT" behavior is totally up to the implementation and disconnecting is an implied acceptable outcome.
Calling out that nodes might disconnect a peer further down was just to make it clear that even nodes that faithfully implement the BIP might disconnect each other for a legitimate reason (ie that a node is looking for a full relay connection, but only could establish a block-relay-only connection).
@luke-jr I'm happy to include some of the explanation that @sipa provided on IRC the other day in this draft -- I genuinely don't know what is confusing about this proposal but I'm happy to iterate on the reasoning in the motivation until it makes sense to everyone. However, even before this is merged to the repo it would be helpful for me to have a BIP number assigned, so that I can continue work on the implementation and having that reviewed, and not be held up for the trivial fact that there is no BIP 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.
ACK 55a31eb I've reviewed the BIP and concluded that it can be merged according to BIP 2
(feel free to ignore the unrelated style nit)
bip-disable-tx.mediawiki
Outdated
|
||
==Specification== | ||
|
||
# A new disabletx message is added, which is defined as an empty message where pchCommand == "disabletx". |
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.
# A new disabletx message is added, which is defined as an empty message where pchCommand == "disabletx". | |
# A new disabletx message is added, which is defined as an empty message with message type "disabletx". |
unrelated style nit: I know previous bips like to reference Bitcoin Core internal implementation details, such as variable naming. Though, I think it would be good if bips avoided implementation-specific details where possible to avoid confusion when the internals change. Just saying "message type" might even be clearer and the audience that is interested in Bitcoin Core internals, can still follow the link in the Implementation section.
re-ACK 332b9a4 only change is explicit mention of |
re-ACK 31f61e7 only change is adding a sentence |
This message, valid between version/verack for peers with version >= 70017,
would allow establishing at the time of connection that no transactions will be
announced/requested between those peers.