From 8c8aa19b4b4fa56cd359092ef099bcfc7b26c334 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Thu, 11 Apr 2019 14:37:30 +0000 Subject: [PATCH 1/5] Add BroadcastTransaction utility usage in Chain interface Access through a broadcastTransaction method. Add a wait_callback flag to turn off race protection when wallet already track its tx being in mempool Standardise highfee, absurdfee variable name to max_tx_fee We drop the P2P check in BroadcastTransaction as g_connman is only called by RPCs and the wallet scheduler, both of which are initialized after g_connman is assigned and stopped before g_connman is reset. --- src/interfaces/chain.cpp | 9 +++++++++ src/interfaces/chain.h | 5 +++++ src/node/transaction.cpp | 25 ++++++++++++------------- src/node/transaction.h | 8 +++++--- src/rpc/rawtransaction.cpp | 6 +++--- 5 files changed, 34 insertions(+), 19 deletions(-) diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index 22e4aaedaf..10cf82acdc 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -295,6 +296,14 @@ class ChainImpl : public Chain { RelayTransaction(txid, *g_connman); } + bool broadcastTransaction(const CTransactionRef& tx, std::string& err_string, const CAmount& max_tx_fee, bool relay) override + { + const TransactionError err = BroadcastTransaction(tx, err_string, max_tx_fee, relay, /*wait_callback*/ false); + // Chain clients only care about failures to accept the tx to the mempool. Disregard non-mempool related failures. + // Note: this will need to be updated if BroadcastTransactions() is updated to return other non-mempool failures + // that Chain clients do not need to know about. + return TransactionError::OK == err; + } void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants) override { ::mempool.GetTransactionAncestry(txid, ancestors, descendants); diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index e675defd47..2341506854 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -167,6 +167,11 @@ class Chain //! Relay transaction. virtual void relayTransaction(const uint256& txid) = 0; + //! Transaction is added to memory pool, if the transaction fee is below the + //! amount specified by max_tx_fee, and broadcast to all peers if relay is set to true. + //! Return false if the transaction could not be added due to the fee or for another reason. + virtual bool broadcastTransaction(const CTransactionRef& tx, std::string& err_string, const CAmount& max_tx_fee, bool relay) = 0; + //! Calculate mempool ancestor and descendant counts for the given transaction. virtual void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants) = 0; diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 5cd567a5c4..0cbf645984 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -14,10 +14,12 @@ #include -TransactionError BroadcastTransaction(const CTransactionRef tx, uint256& hashTx, std::string& err_string, const CAmount& highfee) +TransactionError BroadcastTransaction(const CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback) { + assert(g_connman); std::promise promise; - hashTx = tx->GetHash(); + uint256 hashTx = tx->GetHash(); + bool callback_set = false; { // cs_main scope LOCK(cs_main); @@ -33,7 +35,7 @@ TransactionError BroadcastTransaction(const CTransactionRef tx, uint256& hashTx, CValidationState state; bool fMissingInputs; if (!AcceptToMemoryPool(mempool, state, std::move(tx), &fMissingInputs, - nullptr /* plTxnReplaced */, false /* bypass_limits */, highfee)) { + nullptr /* plTxnReplaced */, false /* bypass_limits */, max_tx_fee)) { if (state.IsInvalid()) { err_string = FormatStateMessage(state); return TransactionError::MEMPOOL_REJECTED; @@ -44,7 +46,7 @@ TransactionError BroadcastTransaction(const CTransactionRef tx, uint256& hashTx, err_string = FormatStateMessage(state); return TransactionError::MEMPOOL_ERROR; } - } else { + } else if (wait_callback) { // If wallet is enabled, ensure that the wallet has been made aware // of the new transaction prior to returning. This prevents a race // where a user might call sendrawtransaction with a transaction @@ -53,24 +55,21 @@ TransactionError BroadcastTransaction(const CTransactionRef tx, uint256& hashTx, CallFunctionInValidationInterfaceQueue([&promise] { promise.set_value(); }); + callback_set = true; } } else if (fHaveChain) { return TransactionError::ALREADY_IN_CHAIN; - } else { - // Make sure we don't block forever if re-sending - // a transaction already in mempool. - promise.set_value(); } } // cs_main - promise.get_future().wait(); - - if (!g_connman) { - return TransactionError::P2P_DISABLED; + if (callback_set) { + promise.get_future().wait(); } - RelayTransaction(hashTx, *g_connman); + if (relay) { + RelayTransaction(hashTx, *g_connman); + } return TransactionError::OK; } diff --git a/src/node/transaction.h b/src/node/transaction.h index 51033f94e5..08ceace7f8 100644 --- a/src/node/transaction.h +++ b/src/node/transaction.h @@ -14,11 +14,13 @@ * Broadcast a transaction * * @param[in] tx the transaction to broadcast - * @param[out] &txid the txid of the transaction, if successfully broadcast * @param[out] &err_string reference to std::string to fill with error string if available - * @param[in] highfee Reject txs with fees higher than this (if 0, accept any fee) + * @param[in] max_tx_fee reject txs with fees higher than this (if 0, accept any fee) + * @param[in] relay flag if both mempool insertion and p2p relay are requested + * @param[in] wait_callback, wait until callbacks have been processed to avoid stale result due to a sequentially RPC. + * It MUST NOT be set while cs_main, cs_mempool or cs_wallet are held to avoid deadlock * return error */ -NODISCARD TransactionError BroadcastTransaction(CTransactionRef tx, uint256& txid, std::string& err_string, const CAmount& highfee); +NODISCARD TransactionError BroadcastTransaction(CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback); #endif // BITCOIN_NODE_TRANSACTION_H diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 0ab504de06..b8ec2178d2 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -810,14 +810,14 @@ static UniValue sendrawtransaction(const JSONRPCRequest& request) max_raw_tx_fee = fr.GetFee((weight+3)/4); } - uint256 txid; std::string err_string; - const TransactionError err = BroadcastTransaction(tx, txid, err_string, max_raw_tx_fee); + AssertLockNotHeld(cs_main); + const TransactionError err = BroadcastTransaction(tx, err_string, max_raw_tx_fee, /*relay*/ true, /*wait_callback*/ true); if (TransactionError::OK != err) { throw JSONRPCTransactionError(err, err_string); } - return txid.GetHex(); + return tx->GetHash().GetHex(); } static UniValue testmempoolaccept(const JSONRPCRequest& request) From 611291c198eb2be9bf1aea1bf9b2187b18bdb3aa Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Thu, 11 Apr 2019 15:58:53 +0000 Subject: [PATCH 2/5] Introduce CWalletTx::SubmitMemoryPoolAndRelay Higher wallet-tx method combining RelayWalletTransactions and AcceptToMemoryPool, using new Chain::broadcastTransaction --- src/wallet/wallet.cpp | 62 ++++++++++++++++++------------------------- src/wallet/wallet.h | 7 ++--- 2 files changed, 28 insertions(+), 41 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ef5f0a61e1..9ab347dc8e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2134,8 +2134,7 @@ void CWallet::ReacceptWalletTransactions(interfaces::Chain::Lock& locked_chain) std::map mapSorted; // Sort pending wallet transactions based on their initial wallet insertion order - for (std::pair& item : mapWallet) - { + for (std::pair& item : mapWallet) { const uint256& wtxid = item.first; CWalletTx& wtx = item.second; assert(wtx.GetHash() == wtxid); @@ -2150,12 +2149,12 @@ void CWallet::ReacceptWalletTransactions(interfaces::Chain::Lock& locked_chain) // Try to add wallet transactions to memory pool for (const std::pair& item : mapSorted) { CWalletTx& wtx = *(item.second); - CValidationState state; - wtx.AcceptToMemoryPool(locked_chain, state); + std::string unused_err_string; + wtx.SubmitMemoryPoolAndRelay(unused_err_string, false, locked_chain); } } -bool CWalletTx::RelayWalletTransaction(interfaces::Chain::Lock& locked_chain) +bool CWalletTx::SubmitMemoryPoolAndRelay(std::string& err_string, bool relay, interfaces::Chain::Lock& locked_chain) { // Can't relay if wallet is not broadcasting if (!pwallet->GetBroadcastTransactions()) return false; @@ -2165,17 +2164,21 @@ bool CWalletTx::RelayWalletTransaction(interfaces::Chain::Lock& locked_chain) if (isAbandoned()) return false; // Don't relay conflicted or already confirmed transactions if (GetDepthInMainChain(locked_chain) != 0) return false; - // Don't relay transactions that aren't accepted to the mempool - CValidationState unused_state; - if (!InMempool() && !AcceptToMemoryPool(locked_chain, unused_state)) return false; - // Don't try to relay if the node is not connected to the p2p network - if (!pwallet->chain().p2pEnabled()) return false; - - // Try to relay the transaction - pwallet->WalletLogPrintf("Relaying wtx %s\n", GetHash().ToString()); - pwallet->chain().relayTransaction(GetHash()); - return true; + // Submit transaction to mempool for relay + pwallet->WalletLogPrintf("Submitting wtx %s to mempool for relay\n", GetHash().ToString()); + // We must set fInMempool here - while it will be re-set to true by the + // entered-mempool callback, if we did not there would be a race where a + // user could call sendmoney in a loop and hit spurious out of funds errors + // because we think that this newly generated transaction's change is + // unavailable as we're not yet aware that it is in the mempool. + // + // Irrespective of the failure reason, un-marking fInMempool + // out-of-order is incorrect - it should be unmarked when + // TransactionRemovedFromMempool fires. + bool ret = pwallet->chain().broadcastTransaction(tx, err_string, pwallet->m_default_max_tx_fee, relay); + fInMempool |= ret; + return ret; } std::set CWalletTx::GetConflicts() const @@ -2366,7 +2369,7 @@ void CWallet::ResendWalletTransactions() if (m_best_block_time < nLastResend) return; nLastResend = GetTime(); - int relayed_tx_count = 0; + int submitted_tx_count = 0; { // locked_chain and cs_wallet scope auto locked_chain = chain().lock(); @@ -2378,12 +2381,13 @@ void CWallet::ResendWalletTransactions() // only rebroadcast unconfirmed txes older than 5 minutes before the // last block was found if (wtx.nTimeReceived > m_best_block_time - 5 * 60) continue; - if (wtx.RelayWalletTransaction(*locked_chain)) ++relayed_tx_count; + std::string unused_err_string; + if (wtx.SubmitMemoryPoolAndRelay(unused_err_string, true, *locked_chain)) ++submitted_tx_count; } } // locked_chain and cs_wallet - if (relayed_tx_count > 0) { - WalletLogPrintf("%s: rebroadcast %u unconfirmed transactions\n", __func__, relayed_tx_count); + if (submitted_tx_count > 0) { + WalletLogPrintf("%s: resubmit %u unconfirmed transactions\n", __func__, submitted_tx_count); } } @@ -3322,12 +3326,10 @@ bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve if (fBroadcastTransactions) { - // Broadcast - if (!wtx.AcceptToMemoryPool(*locked_chain, state)) { - WalletLogPrintf("CommitTransaction(): Transaction cannot be broadcast immediately, %s\n", FormatStateMessage(state)); + std::string err_string; + if (!wtx.SubmitMemoryPoolAndRelay(err_string, true, *locked_chain)) { + WalletLogPrintf("CommitTransaction(): Transaction cannot be broadcast immediately, %s\n", err_string); // TODO: if we expect the failure to be long term or permanent, instead delete wtx from the wallet and return failure. - } else { - wtx.RelayWalletTransaction(*locked_chain); } } } @@ -4662,18 +4664,6 @@ bool CWalletTx::IsImmatureCoinBase(interfaces::Chain::Lock& locked_chain) const return GetBlocksToMaturity(locked_chain) > 0; } -bool CWalletTx::AcceptToMemoryPool(interfaces::Chain::Lock& locked_chain, CValidationState& state) -{ - // We must set fInMempool here - while it will be re-set to true by the - // entered-mempool callback, if we did not there would be a race where a - // user could call sendmoney in a loop and hit spurious out of funds errors - // because we think that this newly generated transaction's change is - // unavailable as we're not yet aware that it is in the mempool. - bool ret = locked_chain.submitToMemoryPool(tx, pwallet->m_default_max_tx_fee, state); - fInMempool |= ret; - return ret; -} - void CWallet::LearnRelatedScripts(const CPubKey& key, OutputType type) { if (key.IsCompressed() && (type == OutputType::P2SH_SEGWIT || type == OutputType::BECH32)) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 86c2cf851c..167096d472 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -579,11 +579,8 @@ class CWalletTx int64_t GetTxTime() const; - // Pass this transaction to the node to relay to its peers - bool RelayWalletTransaction(interfaces::Chain::Lock& locked_chain); - - /** Pass this transaction to the mempool. Fails if absolute fee exceeds absurd fee. */ - bool AcceptToMemoryPool(interfaces::Chain::Lock& locked_chain, CValidationState& state); + // Pass this transaction to node for mempool insertion and relay to peers if flag set to true + bool SubmitMemoryPoolAndRelay(std::string& err_string, bool relay, interfaces::Chain::Lock& locked_chain); // TODO: Remove "NO_THREAD_SAFETY_ANALYSIS" and replace it with the correct // annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The annotation From 8753f5652b4710e66b50ce87788bf6f33619b75a Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Thu, 11 Apr 2019 16:01:58 +0000 Subject: [PATCH 3/5] Remove duplicate checks in SubmitMemoryPoolAndRelay IsCoinBase check is already performed early by AcceptToMemoryPoolWorker GetDepthInMainChain check is already perfomed by BroadcastTransaction To avoid deadlock we MUST keep lock order in ResendWalletTransactions and CommitTransaction, even if we lock cs_main again further. in BroadcastTransaction. Lock order will need to be clean at once in a future refactoring --- src/wallet/wallet.cpp | 12 ++++-------- src/wallet/wallet.h | 2 +- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9ab347dc8e..b3269083ec 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2150,20 +2150,16 @@ void CWallet::ReacceptWalletTransactions(interfaces::Chain::Lock& locked_chain) for (const std::pair& item : mapSorted) { CWalletTx& wtx = *(item.second); std::string unused_err_string; - wtx.SubmitMemoryPoolAndRelay(unused_err_string, false, locked_chain); + wtx.SubmitMemoryPoolAndRelay(unused_err_string, false); } } -bool CWalletTx::SubmitMemoryPoolAndRelay(std::string& err_string, bool relay, interfaces::Chain::Lock& locked_chain) +bool CWalletTx::SubmitMemoryPoolAndRelay(std::string& err_string, bool relay) { // Can't relay if wallet is not broadcasting if (!pwallet->GetBroadcastTransactions()) return false; - // Don't relay coinbase transactions outside blocks - if (IsCoinBase()) return false; // Don't relay abandoned transactions if (isAbandoned()) return false; - // Don't relay conflicted or already confirmed transactions - if (GetDepthInMainChain(locked_chain) != 0) return false; // Submit transaction to mempool for relay pwallet->WalletLogPrintf("Submitting wtx %s to mempool for relay\n", GetHash().ToString()); @@ -2382,7 +2378,7 @@ void CWallet::ResendWalletTransactions() // last block was found if (wtx.nTimeReceived > m_best_block_time - 5 * 60) continue; std::string unused_err_string; - if (wtx.SubmitMemoryPoolAndRelay(unused_err_string, true, *locked_chain)) ++submitted_tx_count; + if (wtx.SubmitMemoryPoolAndRelay(unused_err_string, true)) ++submitted_tx_count; } } // locked_chain and cs_wallet @@ -3327,7 +3323,7 @@ bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve if (fBroadcastTransactions) { std::string err_string; - if (!wtx.SubmitMemoryPoolAndRelay(err_string, true, *locked_chain)) { + if (!wtx.SubmitMemoryPoolAndRelay(err_string, true)) { WalletLogPrintf("CommitTransaction(): Transaction cannot be broadcast immediately, %s\n", err_string); // TODO: if we expect the failure to be long term or permanent, instead delete wtx from the wallet and return failure. } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 167096d472..d06d517937 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -580,7 +580,7 @@ class CWalletTx int64_t GetTxTime() const; // Pass this transaction to node for mempool insertion and relay to peers if flag set to true - bool SubmitMemoryPoolAndRelay(std::string& err_string, bool relay, interfaces::Chain::Lock& locked_chain); + bool SubmitMemoryPoolAndRelay(std::string& err_string, bool relay); // TODO: Remove "NO_THREAD_SAFETY_ANALYSIS" and replace it with the correct // annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The annotation From b8eecf8e79dad92ff07b851b1b29c2a66546bbc1 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Thu, 11 Apr 2019 16:04:37 +0000 Subject: [PATCH 4/5] Remove unused submitToMemoryPool and relayTransactions Chain interfaces --- src/interfaces/chain.cpp | 10 ---------- src/interfaces/chain.h | 12 ------------ 2 files changed, 22 deletions(-) diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index 10cf82acdc..1ad4308f29 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -151,12 +151,6 @@ class LockImpl : public Chain::Lock, public UniqueLock LockAssertion lock(::cs_main); return CheckFinalTx(tx); } - bool submitToMemoryPool(const CTransactionRef& tx, CAmount absurd_fee, CValidationState& state) override - { - LockAssertion lock(::cs_main); - return AcceptToMemoryPool(::mempool, state, tx, nullptr /* missing inputs */, nullptr /* txn replaced */, - false /* bypass limits */, absurd_fee); - } using UniqueLock::UniqueLock; }; @@ -292,10 +286,6 @@ class ChainImpl : public Chain auto it = ::mempool.GetIter(txid); return it && (*it)->GetCountWithDescendants() > 1; } - void relayTransaction(const uint256& txid) override - { - RelayTransaction(txid, *g_connman); - } bool broadcastTransaction(const CTransactionRef& tx, std::string& err_string, const CAmount& max_tx_fee, bool relay) override { const TransactionError err = BroadcastTransaction(tx, err_string, max_tx_fee, relay, /*wait_callback*/ false); diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index 2341506854..1d6ed05522 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -43,10 +43,6 @@ class Wallet; //! asynchronously //! (https://github.com/bitcoin/bitcoin/pull/10973#issuecomment-380101269). //! -//! * The relayTransactions() and submitToMemoryPool() methods could be replaced -//! with a higher-level broadcastTransaction method -//! (https://github.com/bitcoin/bitcoin/pull/14978#issuecomment-459373984). -//! //! * The initMessages() and loadWallet() methods which the wallet uses to send //! notifications to the GUI should go away when GUI and wallet can directly //! communicate with each other without going through the node @@ -127,11 +123,6 @@ class Chain //! Check if transaction will be final given chain height current time. virtual bool checkFinalTx(const CTransaction& tx) = 0; - - //! Add transaction to memory pool if the transaction fee is below the - //! amount specified by absurd_fee. Returns false if the transaction - //! could not be added due to the fee or for another reason. - virtual bool submitToMemoryPool(const CTransactionRef& tx, CAmount absurd_fee, CValidationState& state) = 0; }; //! Return Lock interface. Chain is locked when this is called, and @@ -164,9 +155,6 @@ class Chain //! Check if transaction has descendants in mempool. virtual bool hasDescendantsInMempool(const uint256& txid) = 0; - //! Relay transaction. - virtual void relayTransaction(const uint256& txid) = 0; - //! Transaction is added to memory pool, if the transaction fee is below the //! amount specified by max_tx_fee, and broadcast to all peers if relay is set to true. //! Return false if the transaction could not be added due to the fee or for another reason. From fb62f128bbfd8c6cd72ea8e23331a4bae23883ab Mon Sep 17 00:00:00 2001 From: John Newbery Date: Tue, 23 Jul 2019 11:49:53 -0400 Subject: [PATCH 5/5] Tidy up BroadcastTransaction() --- src/node/transaction.cpp | 37 +++++++++++++++++++++++-------------- src/node/transaction.h | 9 +++++++-- 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 0cbf645984..8e56496358 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -23,15 +23,17 @@ TransactionError BroadcastTransaction(const CTransactionRef tx, std::string& err { // cs_main scope LOCK(cs_main); + // If the transaction is already confirmed in the chain, don't do anything + // and return early. CCoinsViewCache &view = *pcoinsTip; - bool fHaveChain = false; - for (size_t o = 0; !fHaveChain && o < tx->vout.size(); o++) { + for (size_t o = 0; o < tx->vout.size(); o++) { const Coin& existingCoin = view.AccessCoin(COutPoint(hashTx, o)); - fHaveChain = !existingCoin.IsSpent(); + // IsSpent doesnt mean the coin is spent, it means the output doesnt' exist. + // So if the output does exist, then this transaction exists in the chain. + if (!existingCoin.IsSpent()) return TransactionError::ALREADY_IN_CHAIN; } - bool fHaveMempool = mempool.exists(hashTx); - if (!fHaveMempool && !fHaveChain) { - // push to local node and sync with wallets + if (!mempool.exists(hashTx)) { + // Transaction is not already in the mempool. Submit it. CValidationState state; bool fMissingInputs; if (!AcceptToMemoryPool(mempool, state, std::move(tx), &fMissingInputs, @@ -46,24 +48,31 @@ TransactionError BroadcastTransaction(const CTransactionRef tx, std::string& err err_string = FormatStateMessage(state); return TransactionError::MEMPOOL_ERROR; } - } else if (wait_callback) { - // If wallet is enabled, ensure that the wallet has been made aware - // of the new transaction prior to returning. 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. + } + + // 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; } - } else if (fHaveChain) { - return TransactionError::ALREADY_IN_CHAIN; } } // cs_main if (callback_set) { + // Wait until Validation Interface clients have been notified of the + // transaction entering the mempool. promise.get_future().wait(); } diff --git a/src/node/transaction.h b/src/node/transaction.h index 08ceace7f8..cf64fc28d9 100644 --- a/src/node/transaction.h +++ b/src/node/transaction.h @@ -11,14 +11,19 @@ #include /** - * Broadcast a transaction + * Submit a transaction to the mempool and (optionally) relay it to all P2P peers. + * + * Mempool submission can be synchronous (will await mempool entry notification + * over the CValidationInterface) or asynchronous (will submit and not wait for + * notification), depending on the value of wait_callback. wait_callback MUST + * NOT be set while cs_main, cs_mempool or cs_wallet are held to avoid + * deadlock. * * @param[in] tx the transaction to broadcast * @param[out] &err_string reference to std::string to fill with error string if available * @param[in] max_tx_fee reject txs with fees higher than this (if 0, accept any fee) * @param[in] relay flag if both mempool insertion and p2p relay are requested * @param[in] wait_callback, wait until callbacks have been processed to avoid stale result due to a sequentially RPC. - * It MUST NOT be set while cs_main, cs_mempool or cs_wallet are held to avoid deadlock * return error */ NODISCARD TransactionError BroadcastTransaction(CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback);