Merge #15713: refactor: Replace chain relayTransactions/submitMemoryPool by higher method

fb62f12 Tidy up BroadcastTransaction() (John Newbery)
b8eecf8 Remove unused submitToMemoryPool and relayTransactions Chain interfaces (Antoine Riard)
8753f56 Remove duplicate checks in SubmitMemoryPoolAndRelay (Antoine Riard)
611291c Introduce CWalletTx::SubmitMemoryPoolAndRelay (Antoine Riard)
8c8aa19 Add BroadcastTransaction utility usage in Chain interface (Antoine Riard)

Pull request description:

  Remove CWalletTx::AcceptToMemoryPool

  Replace CWalletTx::RelayWalletTransaction by SubmitMemoryPoolAndRelay

  Add a relay flag to broadcastTransaction because wasn't sure of ReacceptWalletTransactions semantic.

  Obviously, working on implementing bitcoin/bitcoin#14978 (comment) to add the new higher-method in Node interface, will add a commit, just need more thought to do it cleanly

ACKs for top commit:
  MarcoFalke:
    re-ACK fb62f12
  Sjors:
    re-ACK fb62f12

Tree-SHA512: a7ee48b0545f537fa65cac8ed4cb24e777ab90b877d4eefb87971fa93c6a59bd555b62ad8940c6ffb40592a0bd50787d27587af99f20b56af72b415b6394251f

Author: MarcoFalke

src/interfaces/chain.cpp

@@ -11,6 +11,7 @@
 #include <net.h>
 #include <net_processing.h>
 #include <node/coin.h>
+#include <node/transaction.h>
 #include <policy/fees.h>
 #include <policy/policy.h>
 #include <policy/rbf.h>
@@ -150,12 +151,6 @@ class LockImpl : public Chain::Lock, public UniqueLock<CCriticalSection>
         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;
 };
@@ -291,9 +286,13 @@ class ChainImpl : public Chain
         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
     {
-        RelayTransaction(txid, *g_connman);
+        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
     {

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,8 +155,10 @@ 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.
+    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

@@ -14,26 +14,30 @@
 
 #include <future>
 
-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<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 = *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,
-                                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,33 +48,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
-            // 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;
-    } 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) {
+        // Wait until Validation Interface clients have been notified of the
+        // transaction entering the mempool.
+        promise.get_future().wait();
     }
 
-    RelayTransaction(hashTx, *g_connman);
+    if (relay) {
+        RelayTransaction(hashTx, *g_connman);
+    }
 
     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);
+NODISCARD TransactionError BroadcastTransaction(CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback);
 
 #endif // BITCOIN_NODE_TRANSACTION_H

src/rpc/rawtransaction.cpp

@@ -814,14 +814,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)

src/wallet/wallet.cpp

@@ -2134,8 +2134,7 @@ void CWallet::ReacceptWalletTransactions(interfaces::Chain::Lock& locked_chain)
     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);
@@ -2150,32 +2149,32 @@ void CWallet::ReacceptWalletTransactions(interfaces::Chain::Lock& locked_chain)
     // Try to add wallet transactions to memory pool
     for (const std::pair<const int64_t, CWalletTx*>& item : mapSorted) {
         CWalletTx& wtx = *(item.second);
-        CValidationState state;
-        wtx.AcceptToMemoryPool(locked_chain, state);
+        std::string unused_err_string;
+        wtx.SubmitMemoryPoolAndRelay(unused_err_string, false);
     }
 }
 
-bool CWalletTx::RelayWalletTransaction(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;
-    // 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<uint256> CWalletTx::GetConflicts() const
@@ -2366,7 +2365,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 +2377,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)) ++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 +3322,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)) {
+                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 +4660,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)) {

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);
 
     // TODO: Remove "NO_THREAD_SAFETY_ANALYSIS" and replace it with the correct
     // annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The annotation