Skip to content

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

Merged
merged 5 commits into from
Feb 12, 2021

Conversation

sdaftuar
Copy link
Member

@sdaftuar sdaftuar commented Jan 6, 2021

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.

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.
@sdaftuar
Copy link
Member Author

sdaftuar commented Jan 6, 2021

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.

@jonatack
Copy link
Member

Concept ACK

Also change the phrasing to more clearly indicate when block-relay-only peering
was deployed.
@sdaftuar
Copy link
Member Author

Anything else to be done here?

@maflcko
Copy link
Member

maflcko commented Jan 26, 2021

@luke-jr

@sdaftuar
Copy link
Member Author

sdaftuar commented Feb 1, 2021

ping @luke-jr

@luke-jr
Copy link
Member

luke-jr commented Feb 3, 2021

Can you add something to Motivation that actually explains why this is useful?

@luke-jr luke-jr added the New BIP label Feb 3, 2021
@sdaftuar
Copy link
Member Author

sdaftuar commented Feb 4, 2021

@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?

Copy link
Member

@jonatack jonatack left a 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:
Copy link
Member

@jonatack jonatack Feb 5, 2021

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..."

Copy link
Member Author

@sdaftuar sdaftuar Feb 11, 2021

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).

@sdaftuar
Copy link
Member Author

sdaftuar commented Feb 9, 2021

@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.

562 2021-02-04T21:59:49  <sipa> sdaftuar, luke-jr: the second paragraph of sdaftuar's ML post about it (https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-January/018340.html) is probably a good start
563 2021-02-04T22:00:39  <luke-jr> ah
564 2021-02-04T22:01:07  <luke-jr> so the point is to allow N*M light connections where being unsure means only N could be accepted?
565 2021-02-04T22:01:24  <luke-jr> (though IIRC our limits are due to FD set sizes and open fd limits currently?)
566 2021-02-04T22:03:56  <sipa> luke-jr: my understanding is this: we have a default inbound connection limit (125) which is based on the observation that every "fully fledged" connection has a significant impact on processing costs and bandwidth (among other things), so we can't just raise that without impacting worst-case performance of average nodes
569 2021-02-04T22:04:58  <sipa> if we instead knew for block-only connections (on the incoming side) that they'd *never* need transactions, the worst part is reduced, and perhaps it becomes acceptable to have more incoming connections per peer, given that some percentage of them are reserved for non-block-relaying connections
570 2021-02-04T22:05:19  <sipa> right now, the block-only nature of a connection is only known on the outbound side
571 2021-02-04T22:05:21  <luke-jr> I see

Copy link
Member

@maflcko maflcko left a 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)


==Specification==

# A new disabletx message is added, which is defined as an empty message where pchCommand == "disabletx".
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# 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.

@maflcko
Copy link
Member

maflcko commented Feb 12, 2021

re-ACK 332b9a4 only change is explicit mention of tx message type behaviour

@maflcko
Copy link
Member

maflcko commented Feb 12, 2021

re-ACK 31f61e7 only change is adding a sentence

@luke-jr luke-jr changed the title p2p: Add disabletx message Add BIP 338: Disable transaction relay message Feb 12, 2021
@luke-jr luke-jr merged commit 31f61e7 into bitcoin:master Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants