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

pull laanwj wants to merge 2 commits into bitcoin:master from laanwj:2018_05_optional_bip61 changing 5 files +40 −11
  1. laanwj commented at 1:14 pm on May 1, 2018: member

    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.

  2. laanwj force-pushed on May 1, 2018
  3. in test/functional/p2p_invalid_tx.py:146 in a0dad2bfeb outdated 
    137@@ -136,6 +138,14 @@ def run_test(self):
138         wait_until(lambda: 1 == len(node.getpeerinfo()), timeout=12)  # p2ps[1] is no longer connected
139         assert_equal(expected_mempool, set(node.getrawmempool()))
140 
141+        # restart node with sending BIP61 messages disabled, check that it disconnects without sending the reject message
142+        self.log.info('Test a transaction that is rejected, with BIP61 disabled')
143+        self.restart_node(0, ['-peersendreject=0','-persistmempool=0'])
144+        self.reconnect_p2p(num_connections=1)
145+        txr = create_transaction(block1.vtx[0], 0, b'\x64', 50 * COIN - 12000)
146+        node.p2p.send_txs_and_test([tx1], node, success=False, expect_disconnect=True)
    MarcoFalke commented at 1:24 pm on May 1, 2018:
    txr seems unused?
    laanwj commented at 1:28 pm on May 1, 2018:
    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…)
  4. sdaftuar commented at 2:49 pm on May 1, 2018: member
    Concept ACK
  5. gmaxwell commented at 6:12 pm on May 1, 2018: contributor
    Concept ACK
  6. fanquake added the label P2P on May 1, 2018
  7. sipa commented at 2:48 am on May 2, 2018: member
    Concept ACK
  8. jonasschnelli commented at 6:55 am on May 2, 2018: contributor
    utACK ca4d71559c9d0f4a5a9a1dec105bff827b530a00 (after squashing the squashme)
  9. laanwj force-pushed on May 3, 2018
  10. laanwj commented at 12:42 pm on May 3, 2018: member
    Squashed ca4d715 → 39d4d10
  11. morcos commented at 2:11 pm on May 3, 2018: member
    Concept ACK
  12. in test/functional/p2p_invalid_tx.py:76 in 39d4d10bc9 outdated 
    72@@ -71,7 +73,7 @@ def run_test(self):
73         # and we get disconnected immediately
74         self.log.info('Test a transaction that is rejected')
75         tx1 = create_transaction(block1.vtx[0], 0, b'\x64', 50 * COIN - 12000)
76-        node.p2p.send_txs_and_test([tx1], node, success=False, expect_disconnect=True)
77+        node.p2p.send_txs_and_test([tx1], node, success=False, expect_disconnect=True, reject_code=REJECT_INVALID)
    MarcoFalke commented at 2:22 pm on May 3, 2018:

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

    0reject_reason=b'mandatory-script-verify-flag-failed (Invalid OP_IF construction)'
  13. in test/functional/p2p_invalid_tx.py:143 in 39d4d10bc9 outdated 
    137@@ -136,6 +138,13 @@ def run_test(self):
138         wait_until(lambda: 1 == len(node.getpeerinfo()), timeout=12)  # p2ps[1] is no longer connected
139         assert_equal(expected_mempool, set(node.getrawmempool()))
140 
141+        # restart node with sending BIP61 messages disabled, check that it disconnects without sending the reject message
142+        self.log.info('Test a transaction that is rejected, with BIP61 disabled')
143+        self.restart_node(0, ['-peersendreject=0','-persistmempool=0'])
    MarcoFalke commented at 2:23 pm on May 3, 2018:
    micro-style-nit: Could add a white space after the comma?
  14. MarcoFalke commented at 2:26 pm on May 3, 2018: member
    utACK 39d4d10bc961ae939b6bdabfd030140012c8b395. Just two style nits in tests
  15. jnewbery commented at 4:00 pm on May 3, 2018: member

    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)
  16. MarcoFalke commented at 4:15 pm on May 3, 2018: member
    Agree that mentioning bip61 in the option name makes sense.
  17. laanwj commented at 8:54 am on May 4, 2018: member

    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.

  18. MarcoFalke commented at 6:39 pm on May 9, 2018: member
    Needs rebase if still relevant
  19. laanwj commented at 7:07 pm on May 9, 2018: member
    Why would this already not be relevant now?
  20. MarcoFalke commented at 7:08 pm on May 9, 2018: member
    Sorry, this is an automated template reply :)
  21. laanwj force-pushed on May 9, 2018
  22. laanwj renamed this:
    net: Add option `-peersendreject` to configure sending of BIP61 notifications
    net: Add option `-enablebip61` to configure sending of BIP61 notifications
    on May 9, 2018
  23. laanwj commented at 7:29 pm on May 9, 2018: member
    Rebased and renamed the option.
  24. jnewbery commented at 8:02 pm on May 9, 2018: member

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

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

    I can’t reproduce this locally.

  25. laanwj commented at 4:54 am on May 10, 2018: member

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

    02018-05-09T19:56:46.996000Z TestFramework (INFO): Test a transaction that is rejected
12018-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.

  26. laanwj force-pushed on May 10, 2018
  27. laanwj force-pushed on May 10, 2018
  28. laanwj commented at 5:34 am on May 10, 2018: member
    I’d missed @MarcoFalke ’s comment about testing reject reason, patched and squashed: 47d858ddfc481df811fb9c67a5c105c3dcbdfa10 → 56d8a5b0b58afc978bf47d7f18164d0362a433b0
  29. MarcoFalke commented at 2:32 pm on May 10, 2018: member
    Yes, the travis failure is unrelated. I will open an issue when I see this again.
  30. MarcoFalke commented at 2:38 pm on May 10, 2018: member
    re-utACK 56d8a5b0b58afc978bf47d7f18164d0362a433b0 (Only change is rebase and test fixup as well as a change to the option name)
  31. MarcoFalke commented at 6:37 pm on May 13, 2018: member
    Needs rebase due to merge conflict in tests
  32. net: Add option `-enablebip61` to configure sending of BIP61 notifications 
    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.
    fe16dd8226
  33. doc: Mention disabling BIP61 in bips.md 87fe292d89
  34. laanwj force-pushed on May 13, 2018
  35. laanwj commented at 7:06 pm on May 13, 2018: member
    Rebased 56d8a5b0b58afc978bf47d7f18164d0362a433b0 → 87fe292d897e09e176ac7e254144466c319cc9ac
  36. MarcoFalke commented at 3:33 pm on May 15, 2018: member
    re-utACK 87fe292 (no changes other than rebase)
  37. jnewbery commented at 6:21 pm on May 16, 2018: member
    utACK 87fe292d897e09e176ac7e254144466c319cc9ac
  38. MarcoFalke commented at 8:23 pm on May 23, 2018: member
    @jonasschnelli @sdaftuar Mind to re-ACK?
  39. sipa commented at 11:22 pm on May 23, 2018: member
    utACK 87fe292d897e09e176ac7e254144466c319cc9ac
  40. laanwj merged this on May 29, 2018
  41. laanwj closed this on May 29, 2018
  42. laanwj referenced this in commit 70d3541313 on May 29, 2018
  43. sipa added the label Needs release notes on Aug 14, 2018
  44. fanquake removed the label Needs release note on Mar 20, 2019
  45. PastaPastaPasta referenced this in commit df67c17c11 on Apr 15, 2020
  46. PastaPastaPasta referenced this in commit e336169af2 on Apr 16, 2020
  47. PastaPastaPasta referenced this in commit 51e0616149 on Apr 18, 2020
  48. PastaPastaPasta referenced this in commit 9025a957ad on Apr 18, 2020
  49. UdjinM6 referenced this in commit d804a753af on Apr 19, 2020
  50. ckti referenced this in commit f6c28cf14f on Mar 28, 2021
  51. DrahtBot locked this on Dec 16, 2021


laanwj MarcoFalke sdaftuar gmaxwell sipa jonasschnelli morcos jnewbery

Labels
P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me