From 2837a9f1eaa2c6bf402d1d9891d9aa84c4a56033 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 16 Jun 2021 10:47:56 +0100 Subject: [mempool] Only add a transaction to the unbroadcast set when it's added to the mempool Currently, if BroadcastTransaction() is called to rebroadcast a transaction (e.g. by ResendWalletTransactions()), then we add the transaction to the unbroadcast set. That transaction has already been broadcast in the past, so peers are unlikely to request it again, meaning RemoveUnbroadcastTx() won't be called and it won't be removed from m_unbroadcast_txids. Net processing will therefore continue to attempt rebroadcast for the transaction every 10-15 minutes. This will most likely continue until the node connects to a new peer which hasn't yet seen the transaction (or perhaps indefinitely). Fix by only adding the transaction to the broadcast set when it's added to the mempool. --- src/node/transaction.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index f21b390915..926dd0e31d 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -71,6 +71,12 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t // Transaction was accepted to the mempool. + if (relay) { + // the mempool tracks locally submitted transactions to make a + // best-effort of initial broadcast + node.mempool->AddUnbroadcastTx(hashTx); + } + if (wait_callback) { // For transactions broadcast from outside the wallet, make sure // that the wallet has been notified of the transaction before @@ -96,9 +102,6 @@ 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()); } -- cgit v1.2.3 From cd48372b67d961fe661990a2c6d3cc3d91478924 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 16 Jun 2021 10:56:07 +0100 Subject: [mempool] Allow rebroadcast for same-txid-different-wtxid transactions This commit fixes some slightly unexpected behaviour when: - there is already transaction in the mempool (the "mempool tx") - BroadcastTransaction() is called for a transaction with the same txid as the mempool transaction but a different witness (the "new tx") Prior to this commit, if BroadcastTransaction() is called with relay=true, then it'll call RelayTransaction() using the txid/wtxid of the new tx, not the txid/wtxid of the mempool tx. For wtxid relay peers, in SendMessages(), the wtxid of the new tx will be taken from setInventoryTxToSend, but will then be filtered out from the vector of wtxids to announce, since m_mempool.info() won't find the transaction (the mempool contains the mempool tx, which has a different wtxid from the new tx). Fix this by calling RelayTransaction() with the wtxid of the mempool transaction in this case. --- src/node/transaction.cpp | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) (limited to 'src') diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 926dd0e31d..0c1295c0b5 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -34,7 +34,8 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t assert(node.peerman); assert(node.mempool); std::promise promise; - uint256 hashTx = tx->GetHash(); + uint256 txid = tx->GetHash(); + uint256 wtxid = tx->GetWitnessHash(); bool callback_set = false; { // cs_main scope @@ -44,12 +45,21 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t // 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)); + 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 (!node.mempool->exists(hashTx)) { + 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 @@ -74,7 +84,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.mempool->AddUnbroadcastTx(txid); } if (wait_callback) { @@ -102,7 +112,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t } if (relay) { - node.peerman->RelayTransaction(hashTx, tx->GetWitnessHash()); + node.peerman->RelayTransaction(txid, wtxid); } return TransactionError::OK; -- cgit v1.2.3 From 5a77abd4e657458852875a07692898982f4b1db5 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 16 Jun 2021 12:02:00 +0100 Subject: [style] Clean up BroadcastTransaction() --- src/node/transaction.cpp | 124 ++++++++++++++++++++++++----------------------- 1 file changed, 63 insertions(+), 61 deletions(-) (limited to 'src') diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 0c1295c0b5..d3bce069b0 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -28,81 +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 promise; 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(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. + { + 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 (relay) { - // the mempool tracks locally submitted transactions to make a - // best-effort of initial broadcast - node.mempool->AddUnbroadcastTx(txid); - } + 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; + 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) { -- cgit v1.2.3