p2p: Disable BIP 61 by default #14054

maflcko · 2018-08-24T20:23:41Z

The live p2p network should not be used for debugging or as development aid when implementing the p2p protocol. Instead, applications should be tested locally (e.g. by inspecting the debug log of a validating node on the local network)

Using the p2p network for this purpose seems wasteful and even dangerous, as peers can not be trusted to send the correct reject messages or a reject message at all.

jnewbery · 2018-08-24T20:41:42Z

Concept ACK!

The changes to the test framework remove all testing of the p2p REJECT message. Shouldn't we maintain at least some minor testing of REJECT, unless(/until) REJECT is removed entirely?

gmaxwell · 2018-08-24T22:19:02Z

Concept ACK.

maflcko · 2018-08-24T22:37:45Z

@jnewbery Added back one test case for -enablebip61

laanwj · 2018-08-27T09:16:22Z

Concept ACK

I'm not convinced about removing REJECT completely, I think it is useful for testing and development (and have used it for that purpose a few times, for example during development of bitcoin-submittx).

However I fully agree on the reasoning behind disabling it by default.

jnewbery

Looks good. A few comments inline.

jnewbery · 2018-08-27T14:53:45Z

test/functional/test_framework/mininode.py

@@ -492,66 +485,59 @@ def send_blocks_and_test(self, blocks, rpc, success=True, request_block=True, re
           ensure that any getdata messages are responded to
         - if success is True: assert that the node's tip advances to the most recent block
         - if success is False: assert that the node's tip doesn't advance
-         - if reject_code and reject_reason are set: assert that the correct reject message is received"""
+         - if reject_code and reject_reason are set: assert that the correct reject message is logged"""


s/reject_code and reject_reason are/reject_reason is

jnewbery · 2018-08-27T14:56:28Z

test/functional/test_framework/mininode.py

        """Send txs to test node and test whether they're accepted to the mempool.

         - add all txs to our tx_store
         - send tx messages for all txs
         - if success is True/False: assert that the txs are/are not accepted to the mempool
         - if expect_disconnect is True: Skip the sync with ping
-         - if reject_code and reject_reason are set: assert that the correct reject message is received."""
+         - if reject_code and reject_reason are set: assert that the correct reject message is logged."""


s/reject_code and reject_reason are/reject_reason is

jnewbery · 2018-08-27T15:13:01Z

test/functional/p2p_invalid_tx.py

@@ -143,9 +143,6 @@ def run_test(self):
                "disconnecting peer=0",
        ]):
            node.p2p.send_txs_and_test([tx1], node, success=False, expect_disconnect=True)
-        # send_txs_and_test will have waited for disconnect, so we can safely check that no reject has been received


This whole sub-test can be removed (everything from # restart node with sending BIP61 messages disabled, check that it disconnects without sending the reject message onward)

You could also update L71 higher up to test the reject reason:

+++ b/test/functional/p2p_invalid_tx.py @@ -68,7 +68,8 @@ class InvalidTxRequestTest(BitcoinTestFramework): # and we get disconnected immediately self.log.info('Test a transaction that is rejected') tx1 = create_tx_with_script(block1.vtx[0], 0, script_sig=b'\x64' * 35, amount=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_reason="{} from peer=0 was not accepted: mandatory-script-verify-flag-failed (Invalid OP_IF construction) (code 16)".format(tx1.hash))

jnewbery · 2018-08-27T15:14:41Z

test/functional/feature_dersig.py

-                assert_equal(self.nodes[0].p2p.last_message["reject"].reason, b'block-validation-failed')
-            else:
-                assert b'Non-canonical DER signature' in self.nodes[0].p2p.last_message["reject"].reason
+            assert b'Non-canonical DER signature' in self.nodes[0].p2p.last_message["reject"].reason


I have a mild preference to test reject messages in their own test case (p2p_reject.py), but not strongly enough to block this PR.

jnewbery · 2018-08-27T15:18:11Z

test/functional/feature_csv_activation.py

@@ -165,11 +165,11 @@ def create_test_block(self, txs, version=536870912):
        block.solve()
        return block

-    def sync_blocks(self, blocks, success=True, reject_code=None, reject_reason=None, request_block=True):
+    def sync_blocks(self, blocks, success=True, *, reject_reason=None, request_block=True):


None of the calls to this function actually use the optional parameters, so this can be simplified to:

+++ b/test/functional/feature_csv_activation.py @@ -165,11 +165,11 @@ class BIP68_112_113Test(BitcoinTestFramework): block.solve() return block - def sync_blocks(self, blocks, success=True, *, reject_reason=None, request_block=True): + def sync_blocks(self, blocks, success=True): """Sends blocks to test node. Syncs and verifies that tip has advanced to most recent block. Call with success = False if the tip shouldn't advance to the most recent block.""" - self.nodes[0].p2p.send_blocks_and_test(blocks, self.nodes[0], success=success, reject_reason=reject_reason, request_block=request_block, expect_disconnect=False, timeout=60) + self.nodes[0].p2p.send_blocks_and_test(blocks, self.nodes[0], success=success)

jnewbery · 2018-08-27T15:25:16Z

test/functional/feature_cltv.py

 class BIP65Test(BitcoinTestFramework):
    def set_test_params(self):
        self.num_nodes = 1
-        self.extra_args = [['-whitelist=127.0.0.1']]
+        self.extra_args = [['-whitelist=127.0.0.1', '-par=1']]


Please add a comment to explain why par=1 is needed.

jnewbery · 2018-08-27T15:30:37Z

test/functional/feature_block.py

@@ -169,7 +169,7 @@ def run_test(self):
        self.log.info("Reject a block where the miner creates too much coinbase reward")
        self.move_tip(6)
        b9 = self.next_block(9, spend=out[4], additional_coinbase_value=1)
-        self.sync_blocks([b9], False, 16, b'bad-cb-amount', reconnect=True)
+        self.sync_blocks([b9], False, 'coinbase pays too much (actual=10000000000 vs limit=9999999999)', reconnect=True)


Would be nice if all calls to sync_blocks used named argument for reject_reason, but that change should be in a future PR.

DrahtBot · 2018-08-29T17:32:38Z

No more conflicts as of last run.

luke-jr · 2018-08-30T12:48:09Z

Reject may be useful (necessary) for tracking the best-block of peers and making sure our "essential" peers are using the same consensus rules as we are. Until we have reliable peer best-block tracking implemented, I suggest we leave reject alone.

jnewbery · 2018-09-06T20:51:08Z

Reject may be useful (necessary) for tracking the best-block of peers and making sure our "essential" peers are using the same consensus rules as we are.

bitcoind does not track peers' best blocks or consensus rules using REJECT messages. In fact, all it does with REJECT messages is log (if NET logging is enabled) and drop.

I don't think it makes sense to continue sending REJECT messages on the basis that they 'may' be used for something in future.

This breaks on master in feature_block.py. You need this change:

@@ -824,7 +824,7 @@ class FullBlockTest(BitcoinTestFramework):
         tx.vin.append(CTxIn(COutPoint(b64a.vtx[1].sha256, 0)))
         b64a = self.update_block("64a", [tx])
         assert_equal(len(b64a.serialize()), MAX_BLOCK_BASE_SIZE + 8)
-        self.sync_blocks([b64a], success=False, reject_code=1, reject_reason=b'error parsing message')
+        self.sync_blocks([b64a], success=False, reject_reason='non-canonical ReadCompactSize(): iostream error')

maflcko · 2018-09-06T21:40:14Z

@jnewbery I will rebase this on

qa: Read reject reasons from debug log, not p2p messages #14119 (qa: Read reject reasons from debug log, not p2p messages by MarcoFalke)

to get rid of all test changes and leave this a one-line change. Please help review that first.

jnewbery · 2018-09-07T11:42:37Z

Please help review that first.

Yes, fine (although a note to say that this PR isn't seeking review would have been helpful!)

ken2812221 · 2018-09-10T02:15:39Z

utACK faea5bf

jnewbery · 2018-09-10T14:26:45Z

tested ACK faea5bf

faea5bf doc: release notes for -enablebip61 default change (MarcoFalke) fa14b54 p2p: Disable BIP 61 by default (MarcoFalke) Pull request description: The live p2p network should not be used for debugging or as development aid when implementing the p2p protocol. Instead, applications should be tested locally (e.g. by inspecting the debug log of a validating node on the local network) Using the p2p network for this purpose seems wasteful and even dangerous, as peers can not be trusted to send the correct reject messages or a reject message at all. Tree-SHA512: 9c91ad035b5110942172a3b4b8a332a84e0c0aa9ee80f8134aeab63e66ac604841e68b04038681c288b716e5e0cbe38065eb5c7eb63fa72c3bdb3255a4b2d99d

a756363 [docs] document BIP 61 deprecation (John Newbery) da14d90 [p2p] Enable BIP 61 REJECT messages by default (John Newbery) Pull request description: This PR reverts #14054 following discussion on the bitcoin-dev mailing list. It also adds release notes to clearly document that the `enablebip61` option will be disabled by default in a future release before being removed entirely. Tree-SHA512: 0c9162045a4fb95689a0cb2de19f98a83636c9a6fb7ffa6809773b593c6c00b14e0480683e4d1c48e9b7f90e89cf7c2dca18bff42f5d2da2a43c522039e9f1ee

…fault 92f3e80 [docs] release note for disabling reject messages by default (John Newbery) Pull request description: v0.18 deprecated BIP 61 REJECT messages. v0.19 disables them by default (#14054). This adds a release note to document that. BIP 61 REJECT messages will be removed entirely in a future version. Tree-SHA512: 575b7e2800c40cd47b8704abb3ab1e5acdd266ece7209a629e47fed1a88ca94bc0858591e8707b157e913385360a43f2695ecaae81e9881dc2a9b3c9391c80c2

Summary: faea5bfc5a975874acf763082852ed532ed81a95 doc: release notes for -enablebip61 default change (MarcoFalke) fa14b54a872ef0ff755e3c1c0775904ca33cb5ff p2p: Disable BIP 61 by default (MarcoFalke) Pull request description: The live p2p network should not be used for debugging or as development aid when implementing the p2p protocol. Instead, applications should be tested locally (e.g. by inspecting the debug log of a validating node on the local network) Using the p2p network for this purpose seems wasteful and even dangerous, as peers can not be trusted to send the correct reject messages or a reject message at all. Tree-SHA512: 9c91ad035b5110942172a3b4b8a332a84e0c0aa9ee80f8134aeab63e66ac604841e68b04038681c288b716e5e0cbe38065eb5c7eb63fa72c3bdb3255a4b2d99d Backport of Core [[bitcoin/bitcoin#14054 | PR14054]] Test Plan: ninja ninja check ninja check-functional Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6455

…s by default 92f3e80 [docs] release note for disabling reject messages by default (John Newbery) Pull request description: v0.18 deprecated BIP 61 REJECT messages. v0.19 disables them by default (bitcoin#14054). This adds a release note to document that. BIP 61 REJECT messages will be removed entirely in a future version. Tree-SHA512: 575b7e2800c40cd47b8704abb3ab1e5acdd266ece7209a629e47fed1a88ca94bc0858591e8707b157e913385360a43f2695ecaae81e9881dc2a9b3c9391c80c2

faea5bf doc: release notes for -enablebip61 default change (MarcoFalke) fa14b54 p2p: Disable BIP 61 by default (MarcoFalke) Pull request description: The live p2p network should not be used for debugging or as development aid when implementing the p2p protocol. Instead, applications should be tested locally (e.g. by inspecting the debug log of a validating node on the local network) Using the p2p network for this purpose seems wasteful and even dangerous, as peers can not be trusted to send the correct reject messages or a reject message at all. Tree-SHA512: 9c91ad035b5110942172a3b4b8a332a84e0c0aa9ee80f8134aeab63e66ac604841e68b04038681c288b716e5e0cbe38065eb5c7eb63fa72c3bdb3255a4b2d99d # Conflicts: # doc/release-notes.md

…s by default 92f3e80 [docs] release note for disabling reject messages by default (John Newbery) Pull request description: v0.18 deprecated BIP 61 REJECT messages. v0.19 disables them by default (bitcoin#14054). This adds a release note to document that. BIP 61 REJECT messages will be removed entirely in a future version. Tree-SHA512: 575b7e2800c40cd47b8704abb3ab1e5acdd266ece7209a629e47fed1a88ca94bc0858591e8707b157e913385360a43f2695ecaae81e9881dc2a9b3c9391c80c2

maflcko added the P2P label Aug 24, 2018

maflcko force-pushed the Mf1808-noBIP61 branch from fad16a6 to 558bb75 Compare August 24, 2018 20:29

maflcko added the Needs release note label Aug 24, 2018

maflcko force-pushed the Mf1808-noBIP61 branch 2 times, most recently from e7f5785 to d3abdc2 Compare August 24, 2018 21:00

maflcko force-pushed the Mf1808-noBIP61 branch from d3abdc2 to fa17b5a Compare August 24, 2018 22:36

jnewbery reviewed Aug 27, 2018

View reviewed changes

maflcko force-pushed the Mf1808-noBIP61 branch from fa17b5a to ec1b32d Compare August 27, 2018 19:55

DrahtBot mentioned this pull request Aug 29, 2018

qa: Use named args in validation acceptance tests #14101

Merged

maflcko force-pushed the Mf1808-noBIP61 branch from ec1b32d to d3b57b2 Compare August 30, 2018 01:05

maflcko force-pushed the Mf1808-noBIP61 branch 2 times, most recently from ecb53e8 to 7d46692 Compare August 31, 2018 12:59

This was referenced Aug 31, 2018

qa: Read reject reasons from debug log, not p2p messages #14119

Merged

Minor style enhancement in documentation #14091

Closed

p2p: Disable BIP 61 by default

fa14b54

maflcko force-pushed the Mf1808-noBIP61 branch 2 times, most recently from 29d8e45 to fa14b54 Compare September 8, 2018 20:44

doc: release notes for -enablebip61 default change

faea5bf

maflcko removed the Needs release note label Sep 9, 2018

laanwj merged commit faea5bf into bitcoin:master Sep 10, 2018

maflcko deleted the Mf1808-noBIP61 branch September 10, 2018 17:12

Bushstar mentioned this pull request Oct 11, 2018

Updates from bitcoin/master FeatherCoin/Feathercoin#366

Merged

jnewbery mentioned this pull request Mar 14, 2019

0.18: [p2p] Enable reject messages by default #15602

Merged

jnewbery mentioned this pull request Mar 14, 2019

[docs] release note for disabling reject messages by default #15604

Merged

ysangkok mentioned this pull request Oct 11, 2020

Obsolete 61 (feature was removed from Core) bitcoin/bips#1010

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

p2p: Disable BIP 61 by default #14054

p2p: Disable BIP 61 by default #14054

maflcko commented Aug 24, 2018 •

edited

Loading

jnewbery commented Aug 24, 2018

gmaxwell commented Aug 24, 2018

maflcko commented Aug 24, 2018

laanwj commented Aug 27, 2018

jnewbery left a comment

jnewbery Aug 27, 2018

jnewbery Aug 27, 2018

jnewbery Aug 27, 2018

jnewbery Aug 27, 2018

jnewbery Aug 27, 2018

jnewbery Aug 27, 2018

jnewbery Aug 27, 2018

DrahtBot commented Aug 29, 2018 •

edited

Loading

luke-jr commented Aug 30, 2018

jnewbery commented Sep 6, 2018

maflcko commented Sep 6, 2018

jnewbery commented Sep 7, 2018

ken2812221 commented Sep 10, 2018

jnewbery commented Sep 10, 2018

p2p: Disable BIP 61 by default #14054

p2p: Disable BIP 61 by default #14054

Conversation

maflcko commented Aug 24, 2018 • edited Loading

jnewbery commented Aug 24, 2018

gmaxwell commented Aug 24, 2018

maflcko commented Aug 24, 2018

laanwj commented Aug 27, 2018

jnewbery left a comment

Choose a reason for hiding this comment

jnewbery Aug 27, 2018

Choose a reason for hiding this comment

jnewbery Aug 27, 2018

Choose a reason for hiding this comment

jnewbery Aug 27, 2018

Choose a reason for hiding this comment

jnewbery Aug 27, 2018

Choose a reason for hiding this comment

jnewbery Aug 27, 2018

Choose a reason for hiding this comment

jnewbery Aug 27, 2018

Choose a reason for hiding this comment

jnewbery Aug 27, 2018

Choose a reason for hiding this comment

DrahtBot commented Aug 29, 2018 • edited Loading

luke-jr commented Aug 30, 2018

jnewbery commented Sep 6, 2018

maflcko commented Sep 6, 2018

jnewbery commented Sep 7, 2018

ken2812221 commented Sep 10, 2018

jnewbery commented Sep 10, 2018

maflcko commented Aug 24, 2018 •

edited

Loading

DrahtBot commented Aug 29, 2018 •

edited

Loading