diff options
-rw-r--r-- | src/node/transaction.cpp | 119 | ||||
-rwxr-xr-x | test/functional/mempool_accept_wtxid.py | 17 | ||||
-rwxr-xr-x | test/functional/mempool_unbroadcast.py | 6 |
3 files changed, 89 insertions, 53 deletions
diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index f21b390915..d3bce069b0 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -28,65 +28,83 @@ static TransactionError HandleATMPError(const TxValidationState& state, std::str TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback) { - // BroadcastTransaction can be called by either sendrawtransaction RPC or wallet RPCs. - // node.peerman is assigned both before chain clients and before RPC server is accepting calls, - // and reset after chain clients and RPC sever are stopped. node.peerman should never be null here. - assert(node.peerman); + // BroadcastTransaction can be called by either sendrawtransaction RPC or the wallet. + // chainman, mempool and peerman are initialized before the RPC server and wallet are started + // and reset after the RPC sever and wallet are stopped. + assert(node.chainman); assert(node.mempool); + assert(node.peerman); + std::promise<void> promise; - uint256 hashTx = tx->GetHash(); + uint256 txid = tx->GetHash(); + uint256 wtxid = tx->GetWitnessHash(); bool callback_set = false; - { // cs_main scope - assert(node.chainman); - LOCK(cs_main); - // If the transaction is already confirmed in the chain, don't do anything - // and return early. - CCoinsViewCache &view = node.chainman->ActiveChainstate().CoinsTip(); - for (size_t o = 0; o < tx->vout.size(); o++) { - const Coin& existingCoin = view.AccessCoin(COutPoint(hashTx, o)); - // IsSpent doesn't mean the coin is spent, it means the output doesn't exist. - // So if the output does exist, then this transaction exists in the chain. - if (!existingCoin.IsSpent()) return TransactionError::ALREADY_IN_CHAIN; - } - if (!node.mempool->exists(hashTx)) { - // Transaction is not already in the mempool. - if (max_tx_fee > 0) { - // First, call ATMP with test_accept and check the fee. If ATMP - // fails here, return error immediately. + { + LOCK(cs_main); + + // If the transaction is already confirmed in the chain, don't do anything + // and return early. + CCoinsViewCache &view = node.chainman->ActiveChainstate().CoinsTip(); + for (size_t o = 0; o < tx->vout.size(); o++) { + const Coin& existingCoin = view.AccessCoin(COutPoint(txid, o)); + // IsSpent doesn't mean the coin is spent, it means the output doesn't exist. + // So if the output does exist, then this transaction exists in the chain. + if (!existingCoin.IsSpent()) return TransactionError::ALREADY_IN_CHAIN; + } + + if (auto mempool_tx = node.mempool->get(txid); mempool_tx) { + // There's already a transaction in the mempool with this txid. Don't + // try to submit this transaction to the mempool (since it'll be + // rejected as a TX_CONFLICT), but do attempt to reannounce the mempool + // transaction if relay=true. + // + // The mempool transaction may have the same or different witness (and + // wtxid) as this transaction. Use the mempool's wtxid for reannouncement. + wtxid = mempool_tx->GetWitnessHash(); + } else { + // Transaction is not already in the mempool. + if (max_tx_fee > 0) { + // First, call ATMP with test_accept and check the fee. If ATMP + // fails here, return error immediately. + const MempoolAcceptResult result = AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, tx, false /* bypass_limits */, + true /* test_accept */); + if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { + return HandleATMPError(result.m_state, err_string); + } else if (result.m_base_fees.value() > max_tx_fee) { + return TransactionError::MAX_FEE_EXCEEDED; + } + } + // Try to submit the transaction to the mempool. const MempoolAcceptResult result = AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, tx, false /* bypass_limits */, - true /* test_accept */); + false /* test_accept */); if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { return HandleATMPError(result.m_state, err_string); - } else if (result.m_base_fees.value() > max_tx_fee) { - return TransactionError::MAX_FEE_EXCEEDED; } - } - // Try to submit the transaction to the mempool. - const MempoolAcceptResult result = AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, tx, false /* bypass_limits */, - false /* test_accept */); - if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { - return HandleATMPError(result.m_state, err_string); - } - // Transaction was accepted to the mempool. + // Transaction was accepted to the mempool. - if (wait_callback) { - // For transactions broadcast from outside the wallet, make sure - // that the wallet has been notified of the transaction before - // continuing. - // - // This prevents a race where a user might call sendrawtransaction - // with a transaction to/from their wallet, immediately call some - // wallet RPC, and get a stale result because callbacks have not - // yet been processed. - CallFunctionInValidationInterfaceQueue([&promise] { - promise.set_value(); - }); - callback_set = true; - } - } + if (relay) { + // the mempool tracks locally submitted transactions to make a + // best-effort of initial broadcast + node.mempool->AddUnbroadcastTx(txid); + } + if (wait_callback) { + // For transactions broadcast from outside the wallet, make sure + // that the wallet has been notified of the transaction before + // continuing. + // + // This prevents a race where a user might call sendrawtransaction + // with a transaction to/from their wallet, immediately call some + // wallet RPC, and get a stale result because callbacks have not + // yet been processed. + CallFunctionInValidationInterfaceQueue([&promise] { + promise.set_value(); + }); + callback_set = true; + } + } } // cs_main if (callback_set) { @@ -96,10 +114,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t } if (relay) { - // the mempool tracks locally submitted transactions to make a - // best-effort of initial broadcast - node.mempool->AddUnbroadcastTx(hashTx); - node.peerman->RelayTransaction(hashTx, tx->GetWitnessHash()); + node.peerman->RelayTransaction(txid, wtxid); } return TransactionError::OK; diff --git a/test/functional/mempool_accept_wtxid.py b/test/functional/mempool_accept_wtxid.py index dd1f8997ad..63ecc8ee2a 100755 --- a/test/functional/mempool_accept_wtxid.py +++ b/test/functional/mempool_accept_wtxid.py @@ -16,6 +16,7 @@ from test_framework.messages import ( CTxOut, sha256, ) +from test_framework.p2p import P2PTxInvStore from test_framework.script import ( CScript, OP_0, @@ -62,6 +63,8 @@ class MempoolWtxidTest(BitcoinTestFramework): parent_txid = node.sendrawtransaction(hexstring=raw_parent, maxfeerate=0) node.generate(1) + peer_wtxid_relay = node.add_p2p_connection(P2PTxInvStore()) + # Create a new transaction with witness solving first branch child_witness_script = CScript([OP_TRUE]) child_witness_program = sha256(child_witness_script) @@ -87,10 +90,13 @@ class MempoolWtxidTest(BitcoinTestFramework): assert_equal(child_one_txid, child_two_txid) assert child_one_wtxid != child_two_wtxid - self.log.info("Submit one child to the mempool") + self.log.info("Submit child_one to the mempool") txid_submitted = node.sendrawtransaction(child_one.serialize().hex()) assert_equal(node.getrawmempool(True)[txid_submitted]['wtxid'], child_one_wtxid) + peer_wtxid_relay.wait_for_broadcast([child_one_wtxid]) + assert_equal(node.getmempoolinfo()["unbroadcastcount"], 0) + # testmempoolaccept reports the "already in mempool" error assert_equal(node.testmempoolaccept([child_one.serialize().hex()]), [{ "txid": child_one_txid, @@ -108,9 +114,18 @@ class MempoolWtxidTest(BitcoinTestFramework): # sendrawtransaction will not throw but quits early when the exact same transaction is already in mempool node.sendrawtransaction(child_one.serialize().hex()) + + self.log.info("Connect another peer that hasn't seen child_one before") + peer_wtxid_relay_2 = node.add_p2p_connection(P2PTxInvStore()) + + self.log.info("Submit child_two to the mempool") # sendrawtransaction will not throw but quits early when a transaction with the same non-witness data is already in mempool node.sendrawtransaction(child_two.serialize().hex()) + # The node should rebroadcast the transaction using the wtxid of the correct transaction + # (child_one, which is in its mempool). + peer_wtxid_relay_2.wait_for_broadcast([child_one_wtxid]) + assert_equal(node.getmempoolinfo()["unbroadcastcount"], 0) if __name__ == '__main__': MempoolWtxidTest().main() diff --git a/test/functional/mempool_unbroadcast.py b/test/functional/mempool_unbroadcast.py index b475b65e68..7d9e6c306d 100755 --- a/test/functional/mempool_unbroadcast.py +++ b/test/functional/mempool_unbroadcast.py @@ -92,6 +92,12 @@ class MempoolUnbroadcastTest(BitcoinTestFramework): self.disconnect_nodes(0, 1) node.disconnect_p2ps() + self.log.info("Rebroadcast transaction and ensure it is not added to unbroadcast set when already in mempool") + rpc_tx_hsh = node.sendrawtransaction(txFS["hex"]) + mempool = node.getrawmempool(True) + assert rpc_tx_hsh in mempool + assert not mempool[rpc_tx_hsh]['unbroadcast'] + def test_txn_removal(self): self.log.info("Test that transactions removed from mempool are removed from unbroadcast set") node = self.nodes[0] |