net: Add option `-enablebip61` to configure sending of BIP61 notifications #13134

laanwj · 2018-05-01T13:14:14Z

This commit adds a boolean option -peersendreject, defaulting to 1, that can be used to disable the sending of BIP61 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.

maflcko · 2018-05-01T13:24:30Z

test/functional/p2p_invalid_tx.py

txr seems unused?

yes, it's not necessary, we can re-use tx1 (initially added it because it looked like I needed to generate a different transaction, but persistmempool=0 works too...)

sdaftuar · 2018-05-01T14:49:54Z

Concept ACK

gmaxwell · 2018-05-01T18:12:20Z

Concept ACK

sipa · 2018-05-02T02:48:28Z

Concept ACK

jonasschnelli · 2018-05-02T06:55:54Z

utACK ca4d71559c9d0f4a5a9a1dec105bff827b530a00 (after squashing the squashme)

laanwj · 2018-05-03T12:42:59Z

Squashed ca4d715 → 39d4d10

morcos · 2018-05-03T14:11:26Z

Concept ACK

maflcko

utACK 39d4d10bc961ae939b6bdabfd030140012c8b395. Just two style nits in tests

maflcko · 2018-05-03T14:22:06Z

test/functional/p2p_invalid_tx.py

nit: If you set the reject_code, you might as well set the reject_reason:

reject_reason=b'mandatory-script-verify-flag-failed (Invalid OP_IF construction)'

maflcko · 2018-05-03T14:23:04Z

test/functional/p2p_invalid_tx.py

micro-style-nit: Could add a white space after the comma?

jnewbery

Tested ACK 39d4d10bc961ae939b6bdabfd030140012c8b395

My only micro-nit has been covered by @MarcoFalke

A couple of general points/questions:

I think -enablebip61 is a better argument name than -peersendreject, which I think could be a little ambiguous/confusing.
Should this option also disable logging of received REJECT messages in ProcessMessage() (to shut down the possibility of peers spamming our debug log with REJECT messages).

Possible enhancements for future PRs:

disable REJECT messages by default.
make this configurable per peer (eg for whitelisted peers)

maflcko · 2018-05-03T16:15:48Z

Agree that mentioning bip61 in the option name makes sense.

laanwj · 2018-05-04T08:54:39Z

I think -enablebip61 is a better argument name than -peersendreject, which I think could be a little ambiguous/confusing.

As you might have guessed from the variable name I used that name at first, but thought this name was more direct/clear. But I'm ok with changing it to that.

One of the reasons for switching it around was that -peerbloomfilters was already a thing and I was trying to keep it consistent. Probably we should have used the BIP38 name there as well - even worse, it isnt' even mentioned in the description!

Should this option also disable logging of received REJECT messages in ProcessMessage() (to shut down the possibility of peers spamming our debug log with REJECT messages).

I think that's an orthogonal concern. One might want their node to be silent but still listen to other's reject messages.
Also: The messages are not enabled unless the very noisy ::NET category (which logs multiple things for every packet!) is already enabled. So I don't think this makes log flooding a worse threat in practice. Might be something to keep in mind for #12219 though.

maflcko · 2018-05-09T18:39:47Z

Needs rebase ~~if still relevant~~

laanwj · 2018-05-09T19:07:13Z

Why would this already not be relevant now?

maflcko · 2018-05-09T19:08:43Z

Sorry, this is an automated template reply :)

laanwj · 2018-05-09T19:29:19Z

Rebased and renamed the option.

jnewbery · 2018-05-09T20:02:36Z

Strange. p2p_invalid_tx.py fails in one job in Travis, on the updated line:

        node.p2p.send_txs_and_test([tx1], node, success=False, expect_disconnect=True, reject_code=REJECT_INVALID)

I can't reproduce this locally.

laanwj · 2018-05-10T04:54:22Z

Right - and that's not even with changed -enablebip61 setting, but the default one

2018-05-09T19:56:46.996000Z TestFramework (INFO): Test a transaction that is rejected
2018-05-09T19:57:47.063000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: (['            wait_until(lambda: self.reject_code_received == reject_code, lock=mininode_lock)\n'], 592)

If it gets to that wait_until, the node will have closed the connection, but without sending a reject message.
This could be a rare race condition between sending a message and closing the connection? I would be extremely surprised if it was introduced by this PR.
Can't reproduce it locally either, will restart the job.

laanwj · 2018-05-10T05:34:01Z

I'd missed @MarcoFalke 's comment about testing reject reason, patched and squashed:
47d858ddfc481df811fb9c67a5c105c3dcbdfa10 → 56d8a5b0b58afc978bf47d7f18164d0362a433b0

maflcko · 2018-05-10T14:32:48Z

Yes, the travis failure is unrelated. I will open an issue when I see this again.

maflcko · 2018-05-10T14:38:12Z

re-utACK 56d8a5b0b58afc978bf47d7f18164d0362a433b0 (Only change is rebase and test fixup as well as a change to the option name)

maflcko · 2018-05-13T18:37:55Z

Needs rebase due to merge conflict in tests

…tions This commit adds a boolean option `-enablebip61`, defaulting to `1`, that can be used to disable the sending of BIP61 `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.

laanwj · 2018-05-13T19:06:20Z

Rebased 56d8a5b0b58afc978bf47d7f18164d0362a433b0 → 87fe292

maflcko · 2018-05-15T15:33:20Z

re-utACK 87fe292 (no changes other than rebase)

jnewbery · 2018-05-16T18:21:25Z

utACK 87fe292

maflcko · 2018-05-23T20:23:00Z

@jonasschnelli @sdaftuar Mind to re-ACK?

sipa · 2018-05-23T23:22:01Z

utACK 87fe292

…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

…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

…ding of BIP61 notifications (#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]>

laanwj force-pushed the 2018_05_optional_bip61 branch from f0eb36f to a0dad2b Compare May 1, 2018 13:15

maflcko reviewed May 1, 2018

View reviewed changes

fanquake added the P2P label May 1, 2018

laanwj force-pushed the 2018_05_optional_bip61 branch from ca4d715 to 39d4d10 Compare May 3, 2018 12:42

maflcko reviewed May 3, 2018

View reviewed changes

jnewbery reviewed May 3, 2018

View reviewed changes

laanwj force-pushed the 2018_05_optional_bip61 branch from 39d4d10 to 47d858d Compare May 9, 2018 19:28

laanwj changed the title ~~net: Add option -peersendreject to configure sending of BIP61 notifications~~ net: Add option -enablebip61 to configure sending of BIP61 notifications May 9, 2018

laanwj force-pushed the 2018_05_optional_bip61 branch 2 times, most recently from 0b3ff97 to 56d8a5b Compare May 10, 2018 05:33

laanwj added 2 commits May 13, 2018 21:03

doc: Mention disabling BIP61 in bips.md

87fe292

laanwj force-pushed the 2018_05_optional_bip61 branch from 56d8a5b to 87fe292 Compare May 13, 2018 19:05

laanwj merged commit 87fe292 into bitcoin:master May 29, 2018

Bushstar mentioned this pull request Jun 1, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#340

Merged

sipa added the Needs release note label Aug 14, 2018

fanquake removed the Needs release note label Mar 20, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

net: Add option -enablebip61 to configure sending of BIP61 notifications #13134

net: Add option -enablebip61 to configure sending of BIP61 notifications #13134

Uh oh!

Conversation

laanwj commented May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko May 1, 2018

Choose a reason for hiding this comment

Uh oh!

laanwj May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented May 1, 2018

Uh oh!

gmaxwell commented May 1, 2018

Uh oh!

sipa commented May 2, 2018

Uh oh!

jonasschnelli commented May 2, 2018

Uh oh!

laanwj commented May 3, 2018

Uh oh!

morcos commented May 3, 2018

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko May 3, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko May 3, 2018

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented May 3, 2018

Uh oh!

laanwj commented May 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented May 9, 2018

Uh oh!

maflcko commented May 9, 2018

Uh oh!

laanwj commented May 9, 2018

Uh oh!

jnewbery commented May 9, 2018

Uh oh!

laanwj commented May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented May 10, 2018

Uh oh!

maflcko commented May 10, 2018

Uh oh!

maflcko commented May 10, 2018

Uh oh!

maflcko commented May 13, 2018

Uh oh!

laanwj commented May 13, 2018

Uh oh!

maflcko commented May 15, 2018

Uh oh!

jnewbery commented May 16, 2018

Uh oh!

maflcko commented May 23, 2018

Uh oh!

sipa commented May 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

net: Add option `-enablebip61` to configure sending of BIP61 notifications #13134

net: Add option `-enablebip61` to configure sending of BIP61 notifications #13134

laanwj commented May 1, 2018 •

edited

Loading

laanwj May 1, 2018 •

edited

Loading

laanwj commented May 4, 2018 •

edited

Loading

maflcko commented May 9, 2018 •

edited

Loading

laanwj commented May 10, 2018 •

edited

Loading