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

fb62f128bbfd8c6cd72ea8e23331a4bae23883ab Tidy up BroadcastTransaction() (John Newbery)
b8eecf8e79dad92ff07b851b1b29c2a66546bbc1 Remove unused submitToMemoryPool and relayTransactions Chain interfaces (Antoine Riard)
8753f5652b4710e66b50ce87788bf6f33619b75a Remove duplicate checks in SubmitMemoryPoolAndRelay (Antoine Riard)
611291c198eb2be9bf1aea1bf9b2187b18bdb3aa Introduce CWalletTx::SubmitMemoryPoolAndRelay (Antoine Riard)
8c8aa19b4b4fa56cd359092ef099bcfc7b26c334 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 https://github.com/bitcoin/bitcoin/pull/14978#issuecomment-459373984 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 fb62f128bbfd8c6cd72ea8e23331a4bae23883ab
  Sjors:
    re-ACK fb62f128bbfd8c6cd72ea8e23331a4bae23883ab

Tree-SHA512: a7ee48b0545f537fa65cac8ed4cb24e777ab90b877d4eefb87971fa93c6a59bd555b62ad8940c6ffb40592a0bd50787d27587af99f20b56af72b415b6394251f

7 files changed, 87 insertions, 97 deletions

diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
index 22e4aaedaf..1ad4308f29 100644
--- a/src/interfaces/chain.cpp
+++ b/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 @@ 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
     {
-        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
     {
diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
index e675defd47..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 @@ 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
@@ -164,8 +155,10 @@ public:
     //! 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;
diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp
index 5cd567a5c4..8e56496358 100644
--- a/src/node/transaction.cpp
+++ b/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;
 }
diff --git a/src/node/transaction.h b/src/node/transaction.h
index 51033f94e5..cf64fc28d9 100644
--- a/src/node/transaction.h
+++ b/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
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index d28071bcd6..966c159f0f 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/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)
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index ef5f0a61e1..b3269083ec 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<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)) {
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 8b0ae8bb85..3a45c1ccc5 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -579,11 +579,8 @@ public:
 
     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