Skip to content
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-halt-unrequested-txn-processing #1664

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ariard
Copy link

@ariard ariard commented Sep 6, 2024

This introducing a BIP for the unrequested transaction processing for the mechanism itself.

This is building on top of bip-txrelayv2 proposal.

Bitcoin Core conceptual discussion: bitcoin/bitcoin#30572
Partially related ML post: https://groups.google.com/g/bitcoindev/c/nWUcXBQbLGU

Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

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

Hey @ariard, as I pointed out already a few days ago on #1663, it would be preferable if a draft had matured a bit before a pull request is opened to this repository. This document appears to be more of a sketch than a draft.

The document should both elaborate on the perceived issue that is being addressed, as well as on convincing the audience that this approach is viable and effects a significant improvement of the situation. Overall, this proposal needs more substance for further review to be warranted.

@murchandamus murchandamus marked this pull request as draft September 12, 2024 17:52
@murchandamus murchandamus added New BIP PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author labels Sep 12, 2024
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.

A conceptual discussion is taking place in bitcoin/bitcoin#30572.

Agree with @murchandamus that this pull ought to be in draft in its current state.

==Abstract==

This BIP proposes a new mechanism to halt the processing of unrequested transations
received by a node from its bitcoin networks peers.
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
received by a node from its bitcoin networks peers.
received by a node from its bitcoin network peers.


==Abstract==

This BIP proposes a new mechanism to halt the processing of unrequested transations
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
This BIP proposes a new mechanism to halt the processing of unrequested transations
This BIP proposes a new mechanism to halt the processing of unrequested transactions

==Motivation==

Historically, nodes have been exchanging transactions on the Bitcoin peer-to-peer
network by sending an inv, and if the transaction has not been discovered and processed
Copy link
Member

Choose a reason for hiding this comment

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

Suggest wrapping p2p message types throughout this draft either in double quotes (i.e. "inv" in BIPs 33, 35, 130, 133, 330, 331), or in code blocks (i.e. inv in BIPs 37, 140, 157), and appending "message" when it helps clarity.

Suggested change
network by sending an inv, and if the transaction has not been discovered and processed
network by sending an inv message, and if the transaction has not been discovered and processed

yet by the other peer, sending a dedicated tx message.

Sending an unnannounced tx message has always been considered in conformity with
the protocol, however this behavior creates a denanonymization vector if leveraged
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
the protocol, however this behavior creates a denanonymization vector if leveraged
the protocol; however, this behavior can create a de-anonymization vector if leveraged

@ariard
Copy link
Author

ariard commented Sep 13, 2024

if a draft had matured a bit

Hey @murchandamus, sorry but running the following unix command yields nothing about draft bip "maturity".

curl -L https://raw.githubusercontent.com/bitcoin/bips/master/bip-0002.mediawiki > bip-process && cat bip-process | grep "mature"

The document should both elaborate on the perceived issue that is being addressed, as well as on convincing the audience that this approach is viable and effects a significant improvement of the situation. Overall, this proposal needs more substance for further review to be warranted.

The perceived issued addressed is already marked in the bip by mentionning the deanoynimization vector. On convincing the audience, this is a non-sense, a BIP author cannot take responsibility of convincing an undefined audience, especially in a decentralized ecosystem like bitcoin, where there is no formally defined domain experts.

I'll let potential reviewers judge by themselves if this proposal needs more substance to be already reviewed or not. If they wish more context there is a already an email on the list and an open PR on bitcoin core, as pointed out by Jon.

@murchandamus
Copy link
Contributor

You may read "matured a bit" as me trying to convey that this proposal seems to fall short of several requirements stated in BIP 2, including "has been submitted to the mailing list", "clear and complete description", "proper motivation", "explains design decisions", "describes alternate designs", "addresses backwards compatibility", "specification should be detailed enough to allow competing, interoperable implementations", and "high quality".

I’m happy to agree to disagree on this, but it seems obvious to me that this PR will not make significant progress until some effort goes towards addressing these shortcomings.

@ariard
Copy link
Author

ariard commented Sep 17, 2024

You may read "matured a bit" as me trying to convey that this proposal seems to fall short of several requirements stated in BIP 2, including "has been submitted to the mailing list", "clear and complete description", "proper motivation", "explains design decisions", "describes alternate designs", "addresses backwards compatibility", "specification should be detailed enough to allow competing, interoperable implementations", and "high quality”.
I’m happy to agree to disagree on this, but it seems obvious to me that this PR will not make significant progress until
some effort goes towards addressing these shortcomings.

Thanks for the intent clarification.

About the requirements you're listing as stated in BIP2:

  • "has been submitted to the mailing list": the BIP has been submitted here on the mailing list on Sep 6 2024 (i.e 10 days ago, before you're opening comment)
  • "clear and complete description": the BIP describe a behavior change about tx processing using IETF's RFC 2119 (you can precise what is not clear...)
  • "proper motivation": the motivation is pointing out how this can constitute a deanonymization vector
  • "explains design decisions": emerging from conversations on the historical PRs on bitcoin core, it sounds the most minimal change not disrupting other softwares
  • "describes alternate designs": i'm not aware of any sound competing idea, with at least a bip draft + implem (if you know some, i'll add them...and this meet by very few existent bips)
  • "addresses backwards compatibility": this is addressed by non-upgraded softwares to NODE_TXRELAYV2_V2
  • "specification should be detailed enough to allow competing, interoperable implementations": here, i'll let a non bitcoin core contributor working on another inter-operable implemation comment what is not clear
  • "high quality": you're free to do a technical review and point out what is not high quality in this BIP

If you wish to engage in a real technical review of this draft BIP, there are 2 TODOs open:

TODO: segment the protocol version by sub-categories of traffic class (e.g tx, addr, block ?)

and

TODO: add a policy rule to disconnect protocol violation ?

Looking forward if you have technical thoughts, beyond editorial remarks as a BIP maintainer.

@murchandamus
Copy link
Contributor

Looking forward if you have technical thoughts, beyond editorial remarks as a BIP maintainer.

I’m not sure I see a benefit in an opt-in mechanism for not-processing unrequested transactions. Perhaps you could elaborate how it would improve the situation.

As I said, I’m happy to agree to disagree on the quality of this proposal, but given that you appear keener on rejecting my feedback than improving the quality of your proposal, providing feedback does not seem to be a good use of my time.

@ariard
Copy link
Author

ariard commented Sep 29, 2024

I’m not sure I see a benefit in an opt-in mechanism for not-processing unrequested transactions. Perhaps you could elaborate how it would improve the situation.

Adding an opt-in mechanism allows for gradual deployment of the not-processing unrequested transactions among all the classes of transaction-relay bitcoin softwares and clients. How many inbound slots are reserved for non-upgraded peers is a matter leave to the clients, as not all clients are similarly exposed in face of DoS or deanonymization risks.

For more of the technical rational, I can invite you to have a look on the arguments raised by many contributors on the PR (bitcoin core 30572). So far no other proposal has been brought to discussion, with the same trade-offs. Adding an opt-in mechanism is also compounding on some learnings from the mempoolfullrbf deployment option.

@bitcoin bitcoin deleted a comment from Bloodsworth24 Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New BIP PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants