p2p: Disable BIP 61 by default #14054

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

jnewbery commented at 2:53 PM on August 27, 2018:

s/reject_code and reject_reason are/reject_reason is

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

jnewbery commented at 2:56 PM on August 27, 2018:

s/reject_code and reject_reason are/reject_reason is

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

jnewbery commented at 3:13 PM on August 27, 2018:

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

 128 | -            if self.nodes[0].p2p.last_message["reject"].code == REJECT_INVALID:
 129 | -                # Generic rejection when a block is invalid
 130 | -                assert_equal(self.nodes[0].p2p.last_message["reject"].reason, b'block-validation-failed')
 131 | -            else:
 132 | -                assert b'Non-canonical DER signature' in self.nodes[0].p2p.last_message["reject"].reason
 133 | +            assert b'Non-canonical DER signature' in self.nodes[0].p2p.last_message["reject"].reason

jnewbery commented at 3:14 PM on August 27, 2018:

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.

 164 | @@ -165,11 +165,11 @@ def create_test_block(self, txs, version=536870912):
 165 |          block.solve()
 166 |          return block
 167 |  
 168 | -    def sync_blocks(self, blocks, success=True, reject_code=None, reject_reason=None, request_block=True):
 169 | +    def sync_blocks(self, blocks, success=True, *, reject_reason=None, request_block=True):

jnewbery commented at 3:18 PM on August 27, 2018:

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)

  54 | @@ -51,10 +55,11 @@ def cltv_validate(node, tx, height):
  55 |                                    list(CScript(new_tx.vin[0].scriptSig)))
  56 |      return new_tx
  57 |  
  58 | +
  59 |  class BIP65Test(BitcoinTestFramework):
  60 |      def set_test_params(self):
  61 |          self.num_nodes = 1
  62 | -        self.extra_args = [['-whitelist=127.0.0.1']]
  63 | +        self.extra_args = [['-whitelist=127.0.0.1', '-par=1']]

jnewbery commented at 3:25 PM on August 27, 2018:

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

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

jnewbery commented at 3:30 PM on August 27, 2018:

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