merge bitcoin#15713: Replace chain relayTransactions/submitMemoryPool by higher method

src/interfaces/chain.cpp

@@ -11,6 +11,7 @@
 #include <interfaces/wallet.h>
 #include <net.h>
 #include <node/coin.h>
+#include <node/transaction.h>
 #include <policy/fees.h>
 #include <policy/policy.h>
 #include <primitives/block.h>
@@ -170,12 +171,6 @@ class LockImpl : public Chain::Lock
         LockAnnotation lock(::cs_main);
         return CheckFinalTx(tx);
     }
-    bool submitToMemoryPool(const CTransactionRef& tx, CAmount absurd_fee, CValidationState& state) override
-    {
-        LockAnnotation lock(::cs_main);
-        return AcceptToMemoryPool(::mempool, state, tx, nullptr /* missing inputs */,
-            false /* bypass limits */, absurd_fee);
-    }
 };
 
 class LockingStateImpl : public LockImpl, public UniqueLock<CCriticalSection>
@@ -318,10 +313,13 @@ public:
         auto it = ::mempool.GetIter(txid);
         return it && (*it)->GetCountWithDescendants() > 1;
     }
-    void relayTransaction(const uint256& txid) override
+    bool broadcastTransaction(const CTransactionRef& tx, std::string& err_string, const CAmount& max_tx_fee, bool relay) override
     {
-        CInv inv(CCoinJoin::GetDSTX(txid) ? MSG_DSTX : MSG_TX, txid);
+        const TransactionError err = BroadcastTransaction(tx, err_string, max_tx_fee, relay, /*wait_callback*/ false);
-        g_connman->ForEachNode([&inv](CNode* node) { node->PushInventory(inv); });
+        // 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
     {

src/interfaces/chain.h

@@ -52,10 +52,6 @@ class Handler;
 //!   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
@@ -145,11 +141,6 @@ public:
 
         //! 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
@@ -184,8 +175,10 @@ public:
     //! Check if transaction has descendants in mempool.
     virtual bool hasDescendantsInMempool(const uint256& txid) = 0;
 
-    //! Relay transaction.
+    //! Transaction is added to memory pool, if the transaction fee is below the
-    virtual void relayTransaction(const uint256& txid) = 0;
+    //! 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;

src/node/transaction.cpp

@@ -13,26 +13,30 @@
 
 #include <future>
 
-TransactionError BroadcastTransaction(const CTransactionRef tx, uint256& hashTx, std::string& err_string, const CAmount& highfee, const bool bypass_limits)
+TransactionError BroadcastTransaction(const CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback, bool bypass_limits)
 {
+    assert(g_connman);
     std::promise<void> promise;
-    hashTx = tx->GetHash();
+    uint256 hashTx = tx->GetHash();
+    bool callback_set = false;
 
     { // cs_main scope
     LOCK(cs_main);
+    // If the transaction is already confirmed in the chain, don't do anything
+    // and return early.
     CCoinsViewCache &view = ::ChainstateActive().CoinsTip();
-    bool fHaveChain = false;
+    for (size_t o = 0; o < tx->vout.size(); o++) {
-    for (size_t o = 0; !fHaveChain && 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 (!mempool.exists(hashTx)) {
-    if (!fHaveMempool && !fHaveChain) {
+        // Transaction is not already in the mempool. Submit it.
-        // push to local node and sync with wallets
         CValidationState state;
         bool fMissingInputs;
         if (!AcceptToMemoryPool(mempool, state, std::move(tx), &fMissingInputs,
-                                bypass_limits, highfee)) {
+                                bypass_limits, max_tx_fee)) {
             if (state.IsInvalid()) {
                 err_string = FormatStateMessage(state);
                 return TransactionError::MEMPOOL_REJECTED;
@@ -43,33 +47,37 @@ TransactionError BroadcastTransaction(const CTransactionRef tx, uint256& hashTx,
                 err_string = FormatStateMessage(state);
                 return TransactionError::MEMPOOL_ERROR;
             }
-        } else {
+        }
-            // If wallet is enabled, ensure that the wallet has been made aware
+
-            // of the new transaction prior to returning. This prevents a race
+        // Transaction was accepted to the mempool.
-            // where a user might call sendrawtransaction with a transaction
+
-            // to/from their wallet, immediately call some wallet RPC, and get
+        if (wait_callback) {
-            // a stale result because callbacks have not yet been processed.
+            // 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;
-    } 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 (callback_set) {
-
+        // Wait until Validation Interface clients have been notified of the
-    if (!g_connman) {
+        // transaction entering the mempool.
-        return TransactionError::P2P_DISABLED;
+        promise.get_future().wait();
     }
 
-    g_connman->RelayTransaction(*tx);
+    if (relay) {
+        g_connman->RelayTransaction(*tx);
+    }
 
     return TransactionError::OK;
 }

src/node/transaction.h

@@ -11,14 +11,21 @@
 #include <util/error.h>
 
 /**
- * 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] &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.
  * return error
  */
-[[nodiscard]] TransactionError BroadcastTransaction(CTransactionRef tx, uint256& txid, std::string& err_string, const CAmount& highfee, const bool bypass_limits = false);
+[[nodiscard]] TransactionError BroadcastTransaction(CTransactionRef tx, std::string& err_string, const CAmount& highfee, bool relay, bool wait_callback, bool bypass_limits = false);
 
 #endif // BITCOIN_NODE_TRANSACTION_H

src/rpc/rawtransaction.cpp

@@ -809,14 +809,14 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
 
     bool bypass_limits = false;
     if (!request.params[3].isNull()) bypass_limits = request.params[3].get_bool();
-    uint256 txid;
     std::string err_string;
-    const TransactionError err = BroadcastTransaction(tx, txid, err_string, max_raw_tx_fee, bypass_limits);
+    AssertLockNotHeld(cs_main);
+    const TransactionError err = BroadcastTransaction(tx, err_string, max_raw_tx_fee, /* relay */ true, /* wait_callback */ true, bypass_limits);
     if (TransactionError::OK != err) {
         throw JSONRPCTransactionError(err, err_string);
     }
 
-    return txid.GetHex();
+    return tx->GetHash().GetHex();
 }
 
 static UniValue testmempoolaccept(const JSONRPCRequest& request)

src/wallet/wallet.cpp

@@ -2265,8 +2265,7 @@ void CWallet::ReacceptWalletTransactions()
     std::map<int64_t, CWalletTx*> mapSorted;
 
     // Sort pending wallet transactions based on their initial wallet insertion order
-    for (std::pair<const uint256, CWalletTx>& item : mapWallet)
+    for (std::pair<const uint256, CWalletTx>& item : mapWallet) {
-    {
         const uint256& wtxid = item.first;
         CWalletTx& wtx = item.second;
         assert(wtx.GetHash() == wtxid);
@@ -2281,32 +2280,31 @@ void CWallet::ReacceptWalletTransactions()
     // Try to add wallet transactions to memory pool
     for (const std::pair<const int64_t, CWalletTx*>& item : mapSorted) {
         CWalletTx& wtx = *(item.second);
-        CValidationState state;
+        std::string unused_err_string;
-        wtx.AcceptToMemoryPool(*locked_chain, state);
+        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;
-    // 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
+    // Submit transaction to mempool for relay
-    if (GetDepthInMainChain(locked_chain) != 0) return false;
+    pwallet->WalletLogPrintf("Submitting wtx %s to mempool for relay\n", GetHash().ToString());
-    // Don't relay transactions that aren't accepted to the mempool
+    // We must set fInMempool here - while it will be re-set to true by the
-    CValidationState unused_state;
+    // entered-mempool callback, if we did not there would be a race where a
-    if (!InMempool() && !AcceptToMemoryPool(locked_chain, unused_state)) return false;
+    // user could call sendmoney in a loop and hit spurious out of funds errors
-    // Don't try to relay if the node is not connected to the p2p network
+    // because we think that this newly generated transaction's change is
-    if (!pwallet->chain().p2pEnabled()) return false;
+    // unavailable as we're not yet aware that it is in the mempool.
-
+    //
-    // Try to relay the transaction
+    // Irrespective of the failure reason, un-marking fInMempool
-    pwallet->WalletLogPrintf("Relaying wtx %s\n", GetHash().ToString());
+    // out-of-order is incorrect - it should be unmarked when
-    pwallet->chain().relayTransaction(GetHash());
+    // TransactionRemovedFromMempool fires.
-
+    bool ret = pwallet->chain().broadcastTransaction(tx, err_string, pwallet->m_default_max_tx_fee, relay);
-    return true;
+    fInMempool |= ret;
+    return ret;
 }
 
 std::set<uint256> CWalletTx::GetConflicts() const
@@ -2628,7 +2626,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, mempool.cs and cs_wallet scope
         auto locked_chain = chain().lock();
@@ -2640,12 +2638,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, mempool.cs and cs_wallet
 
-    if (relayed_tx_count > 0) {
+    if (submitted_tx_count > 0) {
-        WalletLogPrintf("%s: rebroadcast %u unconfirmed transactions\n", __func__, relayed_tx_count);
+        WalletLogPrintf("%s: resubmit %u unconfirmed transactions\n", __func__, submitted_tx_count);
     }
 }
 
@@ -3950,12 +3949,10 @@ bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve
 
         if (fBroadcastTransactions)
         {
-            // Broadcast
+            std::string err_string;
-            if (!wtx.AcceptToMemoryPool(*locked_chain, state)) {
+            if (!wtx.SubmitMemoryPoolAndRelay(err_string, true, *locked_chain)) {
-                WalletLogPrintf("CommitTransaction(): Transaction cannot be broadcast immediately, %s\n", FormatStateMessage(state));
+                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);
             }
         }
     }
@@ -5544,18 +5541,6 @@ bool CMerkleTx::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;
-}
-
 std::vector<OutputGroup> CWallet::GroupOutputs(const std::vector<COutput>& outputs, bool single_coin) const {
     std::vector<OutputGroup> groups;
     std::map<CTxDestination, OutputGroup> gmap;

src/wallet/wallet.h

@@ -526,11 +526,8 @@ public:
 
     int64_t GetTxTime() const;
 
-    // Pass this transaction to the node to relay to its peers
+    // Pass this transaction to node for mempool insertion and relay to peers if flag set to true
-    bool RelayWalletTransaction(interfaces::Chain::Lock& locked_chain);
+    bool SubmitMemoryPoolAndRelay(std::string& err_string, bool relay, 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);
 
     // TODO: Remove "NO_THREAD_SAFETY_ANALYSIS" and replace it with the correct
     // annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The annotation