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

2 files changed, 11 insertions, 19 deletions

diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
index 22e4aaeda..1ad4308f2 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 e675defd4..1d6ed0552 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;