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.

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 <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>
@@ -295,6 +296,14 @@ public:
     {
         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 @@ public:
     //! 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 <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);
@@ -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)