Moving the orphan processing into a new function makes it possible to call it from other places. E.g. sendrawtransaction or when a transaction is received via a NetMsgType different from TX.
Also, add test coverage for the function in our ~unit~ functional test framework.
2249 | @@ -2179,9 +2250,6 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2250 | AcceptToMemoryPool(mempool, state, ptx, &fMissingInputs, &lRemovedTxn, false /* bypass_limits */, 0 /* nAbsurdFee */)) {
Looks like lRemovedTxn still needs to be cleared in this method, unless moving it wholly into ProcessOrphans is appropriate.
Sorry, that indeed looks ugly right now. I will rebase this on #11775, which removes lRemovedTxn in whole.
Concept ACK, both for the addition of testing and due to the fact that ProcessMessage is >1300 lines at the moment. Would be great if other applications of the method were ready, to help inform its interface.
This is a move-only commit - so maybe not the right place - but it might be nice to separate the misbehaving peer punishment logic from the re-attempting to accept orphans to the mempool logic - good first step though
Because many listeners (eg wallet) will want both types of events
to be well-ordered, they are parallel and connected on the backend.
However, they are exposed separately to clients to separate the
logic (and because, hopefully, eventually, they can be exposed to
external clients of Bitcoin Core via libconsensus or similar).
This removes the whole ConnectTrace object, which may make it
slightly harder to remove the unbounded-memory-during-reorg bug
by throwing blocks out of memory and re-loading them from disk
later. Comments are added to validationinterface to note where
this should likely happen instead of ConnectTrace.
34 | + 35 | + The node gets disconnected several times in this test. This helper 36 | + method reconnects the p2p and restarts the network thread.""" 37 | + 38 | + network_thread_join() 39 | + self.nodes[0].disconnect_p2ps()
Seems like it woud be more robust (prevent hangs if the node wasn't disconnected) to call network_thread_join after disconnect_p2ps. This is the pattern used most other places (except feature_block for some reason). This would also make shutdown order the reverse of initialization order, which is a good pattern in general.
Agree. Should be fixed in both places.
47 | # Add p2p connection to node0 48 | node = self.nodes[0] # convenience reference to the node 49 | - node.add_p2p_connection(P2PDataStore()) 50 | - 51 | + # reconnect_p2p() expects the network thread to be running 52 | network_thread_start()
It seems not great to immediately start, stop, and restart the network thread. Could just split up the reconnect method up into connect / disconnect methods to avoid this.
36 | + method reconnects the p2p and restarts the network thread.""" 37 | + 38 | + network_thread_join() 39 | + self.nodes[0].disconnect_p2ps() 40 | + self.nodes[0].add_p2p_connection(P2PDataStore()) 41 | + if additional_conn:
Maybe just take a num_connections=1 optional argument and shorten to
for _ in range(num_connections):
self.nodes[0].add_p2p_connection(P2PDataStore())
64 | @@ -44,12 +65,48 @@ def run_test(self):
65 |
66 | # b'\x64' is OP_NOTIF
67 | # Transaction will be rejected with code 16 (REJECT_INVALID)
Check for reject reason below is now dropped (I guess is because the mininode is no longer whitelisted), so might be clearer to update this comment.
572 | @@ -573,7 +573,8 @@ def send_txs_and_test(self, txs, rpc, success=True, reject_code=None, reject_rea 573 | for tx in txs: 574 | self.send_message(msg_tx(tx)) 575 | 576 | - self.sync_with_ping() 577 | + if not expect_disconnect:
Can the expect_disconnect case actually wait and verify the disconnect happened?
64 | @@ -44,12 +65,48 @@ def run_test(self): 65 | 66 | # b'\x64' is OP_NOTIF 67 | # Transaction will be rejected with code 16 (REJECT_INVALID) 68 | + self.log.info('Test a transaction that is rejected') 69 | tx1 = create_transaction(block1.vtx[0], 0, b'\x64', 50 * COIN - 12000) 70 | - node.p2p.send_txs_and_test([tx1], node, success=False, reject_code=16, reject_reason=b'mandatory-script-verify-flag-failed (Invalid OP_IF construction)') 71 | + node.p2p.send_txs_and_test([tx1], node, success=False, expect_disconnect=True) 72 | + self.reconnect_p2p(additional_conn=True) # Add one more (that is later disconnected)
More test description would be helpful here (it took me a while to figure it out): Test confirms that mininode 1 doesn't get disconnected right away after sending the node an invalid orphan transaction. It then confirms mininode 1 is disconnected later when mininode 0 provides the missing parent which makes it possible to detect the orphan was invalid.
At least this explains the tx_withhold and tx_orphan_2_invalid parts of the new test. I still don't know why tx_orphan_1, tx_orphan_2_no_fee, and tx_orphan_2_valid are added and what they are testing for.
94 | + tx_orphan_2_invalid = CTransaction() 95 | + tx_orphan_2_invalid.vin.append(CTxIn(outpoint=COutPoint(tx_orphan_1.sha256, 2))) 96 | + tx_orphan_2_invalid.vout.append(CTxOut(nValue=11 * COIN, scriptPubKey=b'\x51')) 97 | + 98 | + self.log.info('Send the orphans ... ') 99 | + # Send orphan txs that are valid by consensus, but may be rejected by policy from p2ps[0]
What is "may be rejected by policy" referring to and which transactions does it apply to?
103 | + 104 | + self.log.info('Send the withhold tx ... ') 105 | + assert_equal(2, len(node.getpeerinfo())) 106 | + # The tx itself is successfully accepted, but we should get disconnected anyway 107 | + node.p2p.send_txs_and_test([tx_withhold], node, success=True, expect_disconnect=False) 108 | + assert_equal(1, len(node.getpeerinfo()))
Would be nice just for clarity if this could explicitly verify mininode 1 was disconnected and mininode is still connected. I understand the condition is implied from the peer count combined with expect_disconnect=False above, but it's not clear why this can't just be tested directly.
108 | + assert_equal(1, len(node.getpeerinfo())) 109 | 110 | - # Verify valid transaction 111 | - tx1 = create_transaction(block1.vtx[0], 0, b'', 50 * COIN - 12000) 112 | - node.p2p.send_txs_and_test([tx1], node, success=True) 113 | + assert_equal({t.hash for t in [tx_withhold, tx_orphan_1, tx_orphan_2_valid]}, set(node.getrawmempool()))
Could make sense to rebase the test commit before the refactoring commit, to make it clear existing code is supposed to pass the new test.
Or maybe just put the new test in a standalone PR if the refactoring is going to be held up by #11775 anyway (https://github.com/bitcoin/bitcoin/pull/12935#discussion_r180830562).
utACK aaaa4569b1ef49ce6f031325f03d171de55d8419 for both test code and moveonly refactoring code (I didn't have any comments on the moveonly code).