Merge bitcoin#13134: net: Add option -enablebip61 to configure sending of BIP61 notifications
#3414
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merged
PastaPastaPasta
force-pushed
the
backport-13134
branch
from
9b01e14 to
6745df4
Compare
UdjinM6
requested changes
Apr 16, 2020
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
See inline comments + should apply the same logic to NetMsgType::REJECT messages in governance.cpp and privatesend-client/server.cpp
thephez
reviewed
Apr 16, 2020
src/net_processing.h
Outdated
Collaborator
There was a problem hiding this comment.
General comment regarding future BIP61-related backports. IIRC, Bitcoin eventually sets this to false and then entirely removes BIP61 support. The removal of BIP61 from Bitcoin Core was extremely controversial from the standpoint of mobile wallets.
Not sure what our plan is for when we get to that point, but wanted to bring it up so we do not blindly follow Bitcoin. I imagine @HashEngineering and @QuantumExplorer will want to provide input on that.
Member
Author
There was a problem hiding this comment.
Correct, I've seen this, but these backports at this point are needed for resolving conflicts. Also, I do think their reasoning for removing it made senes
Member
There was a problem hiding this comment.
Yeah I strongly encourage the continued use of BIP61, the reasoning to remove it was too weak in Bitcoin.
UdjinM6
requested changes
Apr 17, 2020
|
Needs rebase |
PastaPastaPasta
force-pushed
the
backport-13134
branch
from
a46483e to
9d4423b
Compare
Member
Author
|
rebased on develop |
…ing of BIP61 notifications 87fe292 doc: Mention disabling BIP61 in bips.md (Wladimir J. van der Laan) fe16dd8 net: Add option `-enablebip61` to configure sending of BIP61 notifications (Wladimir J. van der Laan) Pull request description: This commit adds a boolean option `-peersendreject`, defaulting to `1`, that can be used to disable the sending of [BIP61](https://github.com/bitcoin/bips/blob/master/bip-0061.mediawiki) `reject` messages. This functionality has been requested for various reasons: - security (DoS): reject messages can reveal internal state that can be used to target certain resources such as the mempool more easily. - bandwidth: a typical node sends lots of reject messages; this counts against upstream bandwidth. Also the reject messages tend to be larger than the message that was rejected. On the other hand, reject messages can be useful while developing client software (I found them indispensable while creating bitcoin-submittx), as well as for our own test cases, so whatever the default becomes on the long run, IMO the functionality should be retained as option. But that's a discussion for later, for now it's simply a node operator decision. Also adds a RPC test that checks the functionality. Tree-SHA512: 9488cc53e13cd8e5c6f8eb472a44309572673405c1d1438c3488f627fae622c95e2198bde5ed7d29e56b948e2918bf1920239e9f865889f4c37c097c37a4d7a9
Signed-off-by: Pasta <[email protected]>
Signed-off-by: Pasta <[email protected]>
Signed-off-by: Pasta <[email protected]>
Signed-off-by: Pasta <[email protected]>
PastaPastaPasta
force-pushed
the
backport-13134
branch
from
9d4423b to
eb76a92
Compare
Member
Author
|
rebased again to (hopefully) resolve tests |
PastaPastaPasta
force-pushed
the
backport-13134
branch
from
eb76a92 to
7ab99df
Compare
PastaPastaPasta
force-pushed
the
backport-13134
branch
from
7ab99df to
3c491ab
Compare
gades
pushed a commit
to cosanta/cosanta-core
that referenced
this pull request
Feb 5, 2022
…ding of BIP61 notifications (dashpay#3414) * Merge bitcoin#13134: net: Add option `-enablebip61` to configure sending of BIP61 notifications 87fe292 doc: Mention disabling BIP61 in bips.md (Wladimir J. van der Laan) fe16dd8 net: Add option `-enablebip61` to configure sending of BIP61 notifications (Wladimir J. van der Laan) Pull request description: This commit adds a boolean option `-peersendreject`, defaulting to `1`, that can be used to disable the sending of [BIP61](https://github.com/bitcoin/bips/blob/master/bip-0061.mediawiki) `reject` messages. This functionality has been requested for various reasons: - security (DoS): reject messages can reveal internal state that can be used to target certain resources such as the mempool more easily. - bandwidth: a typical node sends lots of reject messages; this counts against upstream bandwidth. Also the reject messages tend to be larger than the message that was rejected. On the other hand, reject messages can be useful while developing client software (I found them indispensable while creating bitcoin-submittx), as well as for our own test cases, so whatever the default becomes on the long run, IMO the functionality should be retained as option. But that's a discussion for later, for now it's simply a node operator decision. Also adds a RPC test that checks the functionality. Tree-SHA512: 9488cc53e13cd8e5c6f8eb472a44309572673405c1d1438c3488f627fae622c95e2198bde5ed7d29e56b948e2918bf1920239e9f865889f4c37c097c37a4d7a9 * 0.17 -> 0.16 Signed-off-by: Pasta <[email protected]> * tx1 -> base_ tx fixing 13134 Signed-off-by: Pasta <[email protected]> * move added bip61 message checking up Signed-off-by: Pasta <[email protected]> * Dash specific code, only send reject messages if bip61 is enabled Signed-off-by: Pasta <[email protected]> * Fix invalidtxrequest.py Co-authored-by: Wladimir J. van der Laan <[email protected]> Co-authored-by: UdjinM6 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tests currently fail. I did my best to fix them, this is a dependency for further p2p refactoring and changes I have done.