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

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

Merged
merged 2 commits into from May 29, 2018

Conversation

Projects
None yet
9 participants
@laanwj
Copy link
Member

laanwj commented May 1, 2018

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.

@laanwj laanwj force-pushed the laanwj:2018_05_optional_bip61 branch May 1, 2018

test/functional/p2p_invalid_tx.py Outdated
self.restart_node(0, ['-peersendreject=0','-persistmempool=0'])
self.reconnect_p2p(num_connections=1)
txr = create_transaction(block1.vtx[0], 0, b'\x64', 50 * COIN - 12000)
node.p2p.send_txs_and_test([tx1], node, success=False, expect_disconnect=True)

This comment has been minimized.

@MarcoFalke

MarcoFalke May 1, 2018

Member

txr seems unused?

This comment has been minimized.

@laanwj

laanwj May 1, 2018

Author Member

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

This comment has been minimized.

Copy link
Member

sdaftuar commented May 1, 2018

Concept ACK

1 similar comment
@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented May 1, 2018

Concept ACK

@fanquake fanquake added the P2P label May 1, 2018

@sipa

This comment has been minimized.

Copy link
Member

sipa commented May 2, 2018

Concept ACK

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented May 2, 2018

utACK ca4d71559c9d0f4a5a9a1dec105bff827b530a00 (after squashing the squashme)

@laanwj laanwj force-pushed the laanwj:2018_05_optional_bip61 branch May 3, 2018

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented May 3, 2018

Squashed ca4d715 → 39d4d10

@morcos

This comment has been minimized.

Copy link
Member

morcos commented May 3, 2018

Concept ACK

@MarcoFalke
Copy link
Member

MarcoFalke left a comment

utACK 39d4d10bc961ae939b6bdabfd030140012c8b395. Just two style nits in tests

test/functional/p2p_invalid_tx.py Outdated
@@ -71,7 +73,7 @@ def run_test(self):
# and we get disconnected immediately
self.log.info('Test a transaction that is rejected')
tx1 = create_transaction(block1.vtx[0], 0, b'\x64', 50 * COIN - 12000)
node.p2p.send_txs_and_test([tx1], node, success=False, expect_disconnect=True)
node.p2p.send_txs_and_test([tx1], node, success=False, expect_disconnect=True, reject_code=REJECT_INVALID)

This comment has been minimized.

@MarcoFalke

MarcoFalke May 3, 2018

Member

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)'
test/functional/p2p_invalid_tx.py Outdated
@@ -136,6 +138,13 @@ def run_test(self):
wait_until(lambda: 1 == len(node.getpeerinfo()), timeout=12) # p2ps[1] is no longer connected
assert_equal(expected_mempool, set(node.getrawmempool()))

# restart node with sending BIP61 messages disabled, check that it disconnects without sending the reject message
self.log.info('Test a transaction that is rejected, with BIP61 disabled')
self.restart_node(0, ['-peersendreject=0','-persistmempool=0'])

This comment has been minimized.

@MarcoFalke

MarcoFalke May 3, 2018

Member

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

@jnewbery
Copy link
Member

jnewbery left a comment

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)
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented May 3, 2018

Agree that mentioning bip61 in the option name makes sense.

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented May 4, 2018

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.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented May 9, 2018

Needs rebase if still relevant

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented May 9, 2018

Why would this already not be relevant now?

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented May 9, 2018

Sorry, this is an automated template reply :)

@laanwj laanwj force-pushed the laanwj:2018_05_optional_bip61 branch May 9, 2018

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

This comment has been minimized.

Copy link
Member Author

laanwj commented May 9, 2018

Rebased and renamed the option.

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented May 9, 2018

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

This comment has been minimized.

Copy link
Member Author

laanwj commented May 10, 2018

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 laanwj force-pushed the laanwj:2018_05_optional_bip61 branch 2 times, most recently May 10, 2018

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented May 10, 2018

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented May 10, 2018

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented May 10, 2018

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented May 13, 2018

Needs rebase due to merge conflict in tests

laanwj added some commits May 1, 2018

net: Add option `-enablebip61` to configure sending of BIP61 notifica…
…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 laanwj force-pushed the laanwj:2018_05_optional_bip61 branch to 87fe292 May 13, 2018

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented May 13, 2018

Rebased 56d8a5b0b58afc978bf47d7f18164d0362a433b0 → 87fe292

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented May 15, 2018

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

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented May 16, 2018

utACK 87fe292

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented May 23, 2018

@jonasschnelli @sdaftuar Mind to re-ACK?

@sipa

This comment has been minimized.

Copy link
Member

sipa commented May 23, 2018

utACK 87fe292

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request May 29, 2018

Merge #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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.