Merge bitcoin/bitcoin#33567: node: change a tx-relay on/off flag to enum

07a9264 node: change a tx-relay on/off flag to enum (Vasil Dimov)

Pull request description:

  Previously the `bool relay` argument to `BroadcastTransaction()` designated:

  ```
  relay=true: add to the mempool and broadcast to all peers
  relay=false: add to the mempool
  ```

  Change this to an `enum`, so it is more readable and easier to extend with a 3rd option. Consider these example call sites:

  ```cpp
  Paint(true);
  // Or
  Paint(/*is_red=*/true);
  ```

  vs

  ```cpp
  Paint(RED);
  ```

  The idea for putting `TxBroadcastMethod` into `node/types.h` by Ryan.

  ---

  This is part of [#29415 Broadcast own transactions only via short-lived Tor or I2P connections](bitcoin/bitcoin#29415). Putting it in its own PR to reduce the size of #29415 and because it does not logically depend on the other commits from there.

ACKs for top commit:
  optout21:
    ACK 07a9264
  kevkevinpal:
    ACK [07a9264](bitcoin/bitcoin@07a9264)
  laanwj:
    Concept and code review ACK 07a9264. Agree with the general reasoning and the change in #29415 is a valid motivation to change this interface.
  glozow:
    utACK 07a9264

Tree-SHA512: ec8f6fa56a6d2422a0fbd5941ff2792685e8d8e7b9dd50bba9f3e21ed9b4a4a26c89b0d7e4895d48f30b7a635f2eddd894af26b5266410952cbdaf5c40b42966

Author: glozow

src/interfaces/chain.h

@@ -7,6 +7,7 @@
 
 #include <blockfilter.h>
 #include <common/settings.h>
+#include <node/types.h>
 #include <primitives/transaction.h>
 #include <util/result.h>
 
@@ -206,13 +207,19 @@ class Chain
     //! Check if transaction has descendants in mempool.
     virtual bool hasDescendantsInMempool(const Txid& 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.
+    //! Process a local transaction, optionally adding it to the mempool and
+    //! optionally broadcasting it to the network.
+    //! @param[in] tx Transaction to process.
+    //! @param[in] max_tx_fee Don't add the transaction to the mempool or
+    //! broadcast it if its fee is higher than this.
+    //! @param[in] broadcast_method Whether to add the transaction to the
+    //! mempool and how/whether to broadcast it.
+    //! @param[out] err_string Set if an error occurs.
+    //! @return False if the transaction could not be added due to the fee or for another reason.
     virtual bool broadcastTransaction(const CTransactionRef& tx,
-        const CAmount& max_tx_fee,
-        bool relay,
-        std::string& err_string) = 0;
+                                      const CAmount& max_tx_fee,
+                                      node::TxBroadcast broadcast_method,
+                                      std::string& err_string) = 0;
 
     //! Calculate mempool ancestor and descendant counts for the given transaction.
     virtual void getTransactionAncestry(const Txid& txid, size_t& ancestors, size_t& descendants, size_t* ancestorsize = nullptr, CAmount* ancestorfees = nullptr) = 0;

src/node/interfaces.cpp

@@ -363,7 +363,12 @@ class NodeImpl : public Node
     }
     TransactionError broadcastTransaction(CTransactionRef tx, CAmount max_tx_fee, std::string& err_string) override
     {
-        return BroadcastTransaction(*m_context, std::move(tx), err_string, max_tx_fee, /*relay=*/ true, /*wait_callback=*/ false);
+        return BroadcastTransaction(*m_context,
+                                    std::move(tx),
+                                    err_string,
+                                    max_tx_fee,
+                                    TxBroadcast::MEMPOOL_AND_BROADCAST_TO_ALL,
+                                    /*wait_callback=*/false);
     }
     WalletLoader& walletLoader() override
     {
@@ -672,10 +677,10 @@ class ChainImpl : public Chain
     }
     bool broadcastTransaction(const CTransactionRef& tx,
         const CAmount& max_tx_fee,
-        bool relay,
+        TxBroadcast broadcast_method,
         std::string& err_string) override
     {
-        const TransactionError err = BroadcastTransaction(m_node, tx, err_string, max_tx_fee, relay, /*wait_callback=*/false);
+        const TransactionError err = BroadcastTransaction(m_node, tx, err_string, max_tx_fee, broadcast_method, /*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.

src/node/transaction.cpp

@@ -31,7 +31,12 @@ static TransactionError HandleATMPError(const TxValidationState& state, std::str
     }
 }
 
-TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback)
+TransactionError BroadcastTransaction(NodeContext& node,
+                                      const CTransactionRef tx,
+                                      std::string& err_string,
+                                      const CAmount& max_tx_fee,
+                                      TxBroadcast broadcast_method,
+                                      bool wait_callback)
 {
     // BroadcastTransaction can be called by RPC or by the wallet.
     // chainman, mempool and peerman are initialized before the RPC server and wallet are started
@@ -62,7 +67,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
             // There's already a transaction in the mempool with this txid. Don't
             // try to submit this transaction to the mempool (since it'll be
             // rejected as a TX_CONFLICT), but do attempt to reannounce the mempool
-            // transaction if relay=true.
+            // transaction if broadcast_method is not TxBroadcast::MEMPOOL_NO_BROADCAST.
             //
             // The mempool transaction may have the same or different witness (and
             // wtxid) as this transaction. Use the mempool's wtxid for reannouncement.
@@ -79,18 +84,26 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
                     return TransactionError::MAX_FEE_EXCEEDED;
                 }
             }
-            // Try to submit the transaction to the mempool.
-            const MempoolAcceptResult result = node.chainman->ProcessTransaction(tx, /*test_accept=*/ false);
-            if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) {
-                return HandleATMPError(result.m_state, err_string);
-            }
 
-            // Transaction was accepted to the mempool.
+            switch (broadcast_method) {
+            case TxBroadcast::MEMPOOL_NO_BROADCAST:
+            case TxBroadcast::MEMPOOL_AND_BROADCAST_TO_ALL:
+                // Try to submit the transaction to the mempool.
+                {
+                    const MempoolAcceptResult result =
+                        node.chainman->ProcessTransaction(tx, /*test_accept=*/false);
+                    if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) {
+                        return HandleATMPError(result.m_state, err_string);
+                    }
+                }
+                // Transaction was accepted to the mempool.
 
-            if (relay) {
-                // the mempool tracks locally submitted transactions to make a
-                // best-effort of initial broadcast
-                node.mempool->AddUnbroadcastTx(txid);
+                if (broadcast_method == TxBroadcast::MEMPOOL_AND_BROADCAST_TO_ALL) {
+                    // the mempool tracks locally submitted transactions to make a
+                    // best-effort of initial broadcast
+                    node.mempool->AddUnbroadcastTx(txid);
+                }
+                break;
             }
 
             if (wait_callback && node.validation_signals) {
@@ -116,8 +129,12 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
         promise.get_future().wait();
     }
 
-    if (relay) {
+    switch (broadcast_method) {
+    case TxBroadcast::MEMPOOL_NO_BROADCAST:
+        break;
+    case TxBroadcast::MEMPOOL_AND_BROADCAST_TO_ALL:
         node.peerman->RelayTransaction(txid, wtxid);
+        break;
     }
 
     return TransactionError::OK;

src/node/transaction.h

@@ -6,6 +6,7 @@
 #define BITCOIN_NODE_TRANSACTION_H
 
 #include <common/messages.h>
+#include <node/types.h>
 #include <policy/feerate.h>
 #include <primitives/transaction.h>
 
@@ -45,11 +46,16 @@ static const CAmount DEFAULT_MAX_BURN_AMOUNT{0};
  * @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]  broadcast_method whether to add the transaction to the mempool and how to broadcast it
  * @param[in]  wait_callback wait until callbacks have been processed to avoid stale result due to a sequentially RPC.
  * return error
  */
-[[nodiscard]] TransactionError BroadcastTransaction(NodeContext& node, CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback);
+[[nodiscard]] TransactionError BroadcastTransaction(NodeContext& node,
+                                                    CTransactionRef tx,
+                                                    std::string& err_string,
+                                                    const CAmount& max_tx_fee,
+                                                    TxBroadcast broadcast_method,
+                                                    bool wait_callback);
 
 /**
  * Return transaction with a given hash.

src/node/types.h

@@ -15,6 +15,7 @@
 
 #include <consensus/amount.h>
 #include <cstddef>
+#include <cstdint>
 #include <policy/policy.h>
 #include <script/script.h>
 #include <uint256.h>
@@ -97,6 +98,18 @@ struct BlockCheckOptions {
      */
     bool check_pow{true};
 };
+
+/**
+ * How to broadcast a local transaction.
+ * Used to influence `BroadcastTransaction()` and its callers.
+ */
+enum class TxBroadcast : uint8_t {
+    /// Add the transaction to the mempool and broadcast to all peers for which tx relay is enabled.
+    MEMPOOL_AND_BROADCAST_TO_ALL,
+    /// Add the transaction to the mempool, but don't broadcast to anybody.
+    MEMPOOL_NO_BROADCAST,
+};
+
 } // namespace node
 
 #endif // BITCOIN_NODE_TYPES_H

src/rpc/mempool.cpp

@@ -98,7 +98,12 @@ static RPCHelpMan sendrawtransaction()
             std::string err_string;
             AssertLockNotHeld(cs_main);
             NodeContext& node = EnsureAnyNodeContext(request.context);
-            const TransactionError err = BroadcastTransaction(node, tx, err_string, max_raw_tx_fee, /*relay=*/true, /*wait_callback=*/true);
+            const TransactionError err = BroadcastTransaction(node,
+                                                              tx,
+                                                              err_string,
+                                                              max_raw_tx_fee,
+                                                              node::TxBroadcast::MEMPOOL_AND_BROADCAST_TO_ALL,
+                                                              /*wait_callback=*/true);
             if (TransactionError::OK != err) {
                 throw JSONRPCTransactionError(err, err_string);
             }
@@ -1068,7 +1073,12 @@ static RPCHelpMan submitpackage()
 
                 // We do not expect an error here; we are only broadcasting things already/still in mempool
                 std::string err_string;
-                const auto err = BroadcastTransaction(node, tx, err_string, /*max_tx_fee=*/0, /*relay=*/true, /*wait_callback=*/true);
+                const auto err = BroadcastTransaction(node,
+                                                      tx,
+                                                      err_string,
+                                                      /*max_tx_fee=*/0,
+                                                      node::TxBroadcast::MEMPOOL_AND_BROADCAST_TO_ALL,
+                                                      /*wait_callback=*/true);
                 if (err != TransactionError::OK) {
                     throw JSONRPCTransactionError(err,
                         strprintf("transaction broadcast failed: %s (%d transactions were broadcast successfully)",

src/wallet/rpc/backup.cpp

@@ -9,6 +9,7 @@
 #include <interfaces/chain.h>
 #include <key_io.h>
 #include <merkleblock.h>
+#include <node/types.h>
 #include <rpc/util.h>
 #include <script/descriptor.h>
 #include <script/script.h>
@@ -402,7 +403,7 @@ RPCHelpMan importdescriptors()
     // Rescan the blockchain using the lowest timestamp
     if (rescan) {
         int64_t scanned_time = pwallet->RescanFromTime(lowest_timestamp, reserver, /*update=*/true);
-        pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true);
+        pwallet->ResubmitWalletTransactions(node::TxBroadcast::MEMPOOL_NO_BROADCAST, /*force=*/true);
 
         if (pwallet->IsAbortingRescan()) {
             throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted by user.");

src/wallet/test/wallet_tests.cpp

@@ -13,6 +13,7 @@
 #include <interfaces/chain.h>
 #include <key_io.h>
 #include <node/blockstorage.h>
+#include <node/types.h>
 #include <policy/policy.h>
 #include <rpc/server.h>
 #include <script/solver.h>
@@ -618,7 +619,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
     auto block_tx = TestSimpleSpend(*m_coinbase_txns[0], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
     m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
     auto mempool_tx = TestSimpleSpend(*m_coinbase_txns[1], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
-    BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error));
+    BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, node::TxBroadcast::MEMPOOL_NO_BROADCAST, error));
 
 
     // Reload wallet and make sure new transactions are detected despite events
@@ -660,7 +661,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
             block_tx = TestSimpleSpend(*m_coinbase_txns[2], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
             m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
             mempool_tx = TestSimpleSpend(*m_coinbase_txns[3], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
-            BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error));
+            BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, node::TxBroadcast::MEMPOOL_NO_BROADCAST, error));
             m_node.validation_signals->SyncWithValidationInterfaceQueue();
         });
     wallet = TestLoadWallet(context);

src/wallet/wallet.cpp

@@ -1959,7 +1959,9 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
     return result;
 }
 
-bool CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, std::string& err_string, bool relay) const
+bool CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx,
+                                         std::string& err_string,
+                                         node::TxBroadcast broadcast_method) const
 {
     AssertLockHeld(cs_wallet);
 
@@ -1973,8 +1975,16 @@ bool CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, std::string& err_string
     // Don't try to submit conflicted or confirmed transactions.
     if (GetTxDepthInMainChain(wtx) != 0) return false;
 
-    // Submit transaction to mempool for relay
-    WalletLogPrintf("Submitting wtx %s to mempool for relay\n", wtx.GetHash().ToString());
+    const char* what{""};
+    switch (broadcast_method) {
+    case node::TxBroadcast::MEMPOOL_AND_BROADCAST_TO_ALL:
+        what = "to mempool and for broadcast to peers";
+        break;
+    case node::TxBroadcast::MEMPOOL_NO_BROADCAST:
+        what = "to mempool without broadcast";
+        break;
+    }
+    WalletLogPrintf("Submitting wtx %s %s\n", wtx.GetHash().ToString(), what);
     // We must set TxStateInMempool here. Even though it will also be set later 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
@@ -1984,7 +1994,7 @@ bool CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, std::string& err_string
     // If broadcast fails for any reason, trying to set wtx.m_state here would be incorrect.
     // If transaction was previously in the mempool, it should be updated when
     // TransactionRemovedFromMempool fires.
-    bool ret = chain().broadcastTransaction(wtx.tx, m_default_max_tx_fee, relay, err_string);
+    bool ret = chain().broadcastTransaction(wtx.tx, m_default_max_tx_fee, broadcast_method, err_string);
     if (ret) wtx.m_state = TxStateInMempool{};
     return ret;
 }
@@ -2039,10 +2049,11 @@ NodeClock::time_point CWallet::GetDefaultNextResend() { return FastRandomContext
 //
 // The `force` option results in all unconfirmed transactions being submitted to
 // the mempool. This does not necessarily result in those transactions being relayed,
-// that depends on the `relay` option. Periodic rebroadcast uses the pattern
-// relay=true force=false, while loading into the mempool
-// (on start, or after import) uses relay=false force=true.
-void CWallet::ResubmitWalletTransactions(bool relay, bool force)
+// that depends on the `broadcast_method` option. Periodic rebroadcast uses the pattern
+// broadcast_method=TxBroadcast::MEMPOOL_AND_BROADCAST_TO_ALL force=false, while loading into
+// the mempool (on start, or after import) uses
+// broadcast_method=TxBroadcast::MEMPOOL_NO_BROADCAST force=true.
+void CWallet::ResubmitWalletTransactions(node::TxBroadcast broadcast_method, bool force)
 {
     // Don't attempt to resubmit if the wallet is configured to not broadcast,
     // even if forcing.
@@ -2068,7 +2079,7 @@ void CWallet::ResubmitWalletTransactions(bool relay, bool force)
         // Now try submitting the transactions to the memory pool and (optionally) relay them.
         for (auto wtx : to_submit) {
             std::string unused_err_string;
-            if (SubmitTxMemoryPoolAndRelay(*wtx, unused_err_string, relay)) ++submitted_tx_count;
+            if (SubmitTxMemoryPoolAndRelay(*wtx, unused_err_string, broadcast_method)) ++submitted_tx_count;
         }
     } // cs_wallet
 
@@ -2083,7 +2094,7 @@ void MaybeResendWalletTxs(WalletContext& context)
 {
     for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
         if (!pwallet->ShouldResend()) continue;
-        pwallet->ResubmitWalletTransactions(/*relay=*/true, /*force=*/false);
+        pwallet->ResubmitWalletTransactions(node::TxBroadcast::MEMPOOL_AND_BROADCAST_TO_ALL, /*force=*/false);
         pwallet->SetNextResend();
     }
 }
@@ -2284,7 +2295,7 @@ void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve
     }
 
     std::string err_string;
-    if (!SubmitTxMemoryPoolAndRelay(*wtx, err_string, true)) {
+    if (!SubmitTxMemoryPoolAndRelay(*wtx, err_string, node::TxBroadcast::MEMPOOL_AND_BROADCAST_TO_ALL)) {
         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.
     }
@@ -3233,7 +3244,7 @@ void CWallet::postInitProcess()
 {
     // Add wallet transactions that aren't already in a block to mempool
     // Do this here as mempool requires genesis block to be loaded
-    ResubmitWalletTransactions(/*relay=*/false, /*force=*/true);
+    ResubmitWalletTransactions(node::TxBroadcast::MEMPOOL_NO_BROADCAST, /*force=*/true);
 
     // Update wallet transactions with current mempool transactions.
     WITH_LOCK(cs_wallet, chain().requestMempoolTransactions(*this));