Tidy up BroadcastTransaction()

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 <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] &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);