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.
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?
-enablebip61
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.
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"""
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."""
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
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:
0+++ b/test/functional/p2p_invalid_tx.py
1@@ -68,7 +68,8 @@ class InvalidTxRequestTest(BitcoinTestFramework):
2 # and we get disconnected immediately
3 self.log.info('Test a transaction that is rejected')
4 tx1 = create_tx_with_script(block1.vtx[0], 0, script_sig=b'\x64' * 35, amount=50 * COIN - 12000)
5- node.p2p.send_txs_and_test([tx1], node, success=False, expect_disconnect=True)
6+ node.p2p.send_txs_and_test([tx1], node, success=False, expect_disconnect=True,
7+ 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
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):
None of the calls to this function actually use the optional parameters, so this can be simplified to:
0+++ b/test/functional/feature_csv_activation.py
1@@ -165,11 +165,11 @@ class BIP68_112_113Test(BitcoinTestFramework):
2 block.solve()
3 return block
4
5- def sync_blocks(self, blocks, success=True, *, reject_reason=None, request_block=True):
6+ def sync_blocks(self, blocks, success=True):
7 """Sends blocks to test node. Syncs and verifies that tip has advanced to most recent block.
8
9 Call with success = False if the tip shouldn't advance to the most recent block."""
10- 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)
11+ 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']]
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)
sync_blocks used named argument for reject_reason, but that change should be in a future PR.
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:
0@@ -824,7 +824,7 @@ class FullBlockTest(BitcoinTestFramework):
1 tx.vin.append(CTxIn(COutPoint(b64a.vtx[1].sha256, 0)))
2 b64a = self.update_block("64a", [tx])
3 assert_equal(len(b64a.serialize()), MAX_BLOCK_BASE_SIZE + 8)
4- self.sync_blocks([b64a], success=False, reject_code=1, reject_reason=b'error parsing message')
5+ self.sync_blocks([b64a], success=False, reject_reason='non-canonical ReadCompactSize(): iostream error')
Please help review that first.
Yes, fine (although a note to say that this PR isn’t seeking review would have been helpful!)
