wallet, rpc, gui: Allow users to manually rebroadcast a wallet transaction #36481

    //! Return whether transaction can be broadcast.

Done

    //! Rebroadcast a transaction.

Done

What is the & in Re&broadcast? It is also in A&bandon.

It specifies the previous character as a shortcut key. With this menu open, the user can do ctrl + e to trigger this action rather than needing to move the mouse to it and clicking.

    if (!transactionView || !transactionView->selectionModel())
        return;

also, the return should be either on the same line as if or there should be { and }.

Done

    // Rebroadcast the wallet transaction over the walletModel

Done

If the wallet broadcast is disabled (-walletbroadcast=0), then the "is not eligible to be rebroadcast" will be misleading because it implies that there is something specific about this transaction that makes it not eligible.

Consider extending TransactionCanBeBroadcast() to return the reason why the transaction can not be broadcast:

<details> <summary>[patch] TransactionCanBeBroadcast() with a reason</summary>

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 29460e3ba2..dcd571b684 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1832,51 +1832,64 @@ util::Expected<void, WalletError> CWallet::RebroadcastSingleTransaction(const Tx
 {
     LOCK(cs_wallet);
     auto it = mapWallet.find(txid);
     if (it == mapWallet.end()) {
         return util::Unexpected{WalletError{WalletErrorCode::InvalidParameter, strprintf(_("%s not in wallet"), txid.ToString())}};
     }
-    if (!TransactionCanBeBroadcast(it->second)) {
-        return util::Unexpected{WalletError{WalletErrorCode::InvalidParameter, strprintf(_("%s is not eligible to be rebroadcast"), txid.ToString())}};
-    }
     std::string err;
+    if (!TransactionCanBeBroadcast(it->second, err)) {
+        return util::Unexpected{WalletError{WalletErrorCode::InvalidParameter, strprintf(_("can not rebroadcast %s: %s"), txid.ToString(), err)}};
+    }
     if (!SubmitTxMemoryPoolAndRelay(it->second, err, node::TxBroadcast::MEMPOOL_AND_BROADCAST_TO_ALL)) {
         return util::Unexpected{WalletError{WalletErrorCode::MiscError, Untranslated(err)}};
     }
     return {};
 }
 
-bool CWallet::TransactionCanBeBroadcast(const Txid& txid) const
+bool CWallet::TransactionCanBeBroadcast(const Txid& txid, std::string& reason) const
 {
     LOCK(cs_wallet);
     const CWalletTx* wtx = GetWalletTx(txid);
-    return wtx && TransactionCanBeBroadcast(*wtx);
+    return wtx && TransactionCanBeBroadcast(*wtx, reason);
 }
 
-bool CWallet::TransactionCanBeBroadcast(const CWalletTx& wtx) const
+bool CWallet::TransactionCanBeBroadcast(const CWalletTx& wtx, std::string& reason) const
 {
     AssertLockHeld(cs_wallet);
     // Can't relay if wallet is not broadcasting
-    if (!GetBroadcastTransactions()) return false;
+    if (!GetBroadcastTransactions()) {
+        reason = "the wallet is configured to not broadcast transactions";
+        return false;
+    }
     // Don't relay abandoned transactions
-    if (wtx.isAbandoned()) return false;
+    if (wtx.isAbandoned()) {
+        reason = "the transaction is abandoned";
+        return false;
+    }
     // Don't try to submit coinbase transactions. These would fail anyway but would
     // cause log spam.
-    if (wtx.IsCoinBase()) return false;
+    if (wtx.IsCoinBase()) {
+        reason = "the transaction is a coinbase transaction";
+        return false;
+    }
     // Don't try to submit conflicted or confirmed transactions.
-    if (GetTxDepthInMainChain(wtx) != 0) return false;
+    if (GetTxDepthInMainChain(wtx) != 0) {
+        reason = "the transaction is already confirmed or is conflicted";
+        return false;
+    }
     return true;
 }
 
 bool CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx,
                                          std::string& err_string,
                                          node::TxBroadcast broadcast_method) const
 {
     AssertLockHeld(cs_wallet);
 
-    if (!TransactionCanBeBroadcast(wtx)) return false;
+    std::string reason;
+    if (!TransactionCanBeBroadcast(wtx, reason)) return false;
 
     const char* what{""};
     switch (broadcast_method) {
     case node::TxBroadcast::MEMPOOL_AND_BROADCAST_TO_ALL:
         what = "to mempool and for broadcast to peers";
         break;
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 8d7c72f4d7..0b64d62571 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -443,13 +443,13 @@ private:
     // Update last block processed in memory only
     void SetLastBlockProcessedInMem(int block_height, uint256 block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     //! Update mempool conflicts for TRUC sibling transactions
     void UpdateTrucSiblingConflicts(const CWalletTx& parent_wtx, const Txid& child_txid, bool add_conflict) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
-    bool TransactionCanBeBroadcast(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    bool TransactionCanBeBroadcast(const CWalletTx& wtx, std::string& reason) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
 public:
     /**
      * Main wallet lock.
      * This lock protects all the fields added by CWallet.
      */
@@ -635,13 +635,13 @@ public:
     void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) override;
     /** Set the next time this wallet should resend transactions to 12-36 hours from now, ~1 day on average. */
     void SetNextResend() { m_next_resend = GetDefaultNextResend(); }
     /** Return true if all conditions for periodically resending transactions are met. */
     bool ShouldResend() const;
     void ResubmitWalletTransactions(node::TxBroadcast broadcast_method, bool force);
-    bool TransactionCanBeBroadcast(const Txid& txid) const;
+    bool TransactionCanBeBroadcast(const Txid& txid, std::string& reason) const;
     util::Expected<void, WalletError> RebroadcastSingleTransaction(const Txid& txid);
 
     OutputType TransactionChangeType(const std::optional<OutputType>& change_type, const std::vector<CRecipient>& vecSend) const;
 
     /** Fetch the inputs and sign with SIGHASH_ALL. */
     bool SignTransaction(CMutableTransaction& tx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp
index 78139f635d..1759680b02 100644
--- a/src/wallet/interfaces.cpp
+++ b/src/wallet/interfaces.cpp
@@ -283,13 +283,17 @@ public:
     bool transactionCanBeAbandoned(const Txid& txid) override { return m_wallet->TransactionCanBeAbandoned(txid); }
     bool abandonTransaction(const Txid& txid) override
     {
         LOCK(m_wallet->cs_wallet);
         return m_wallet->AbandonTransaction(txid);
     }
-    bool transactionCanBeBroadcast(const Txid& txid) override { return m_wallet->TransactionCanBeBroadcast(txid); }
+    bool transactionCanBeBroadcast(const Txid& txid) override
+    {
+        std::string reason;
+        return m_wallet->TransactionCanBeBroadcast(txid, reason);
+    }
     util::Expected<void, WalletError> rebroadcastTransaction(const Txid& txid) override
     {
         LOCK(m_wallet->cs_wallet);
         return m_wallet->RebroadcastSingleTransaction(txid);
     }
     bool transactionCanBeBumped(const Txid& txid) override

</details>

Added a commit for this

Should we not throw here? UniValue::VNULL is for "If rebroadcast is successfully initiated".

-            if (!wallet) return UniValue::VNULL;
+            if (!wallet) {
+                throw JSONRPCError(RPC_WALLET_ERROR, "no wallet");
+            }

GetWalletForJSONRPCRequest never returns nullptr, this should really be a Assume return a reference. But this is the pattern that we currently use for all of the wallet RPCs.

I think it would be useful to add the sentence Use the listtransactions RPC to see the wallet transactions here.

Done

In commit "gui: Add rebroadcast tx menu action" (96c0e432770d5396294034618d35260510211bff)

Could this (and RebroadcastSingleTransaction) return util::Result<void> instead of util::Expected<void, wallet::WalletError>? That would make the interface say what it returns: an error message, with no codes callers are expected to handle.

WalletError can hold any of six codes, but this method only returns two, InvalidParameter and MiscError, and callers can't tell that from the declaration. None of them handles the codes differently either: the GUI shows the message, and the RPC only uses the code to pick an RPC error number. The WalletErrorCode documentation asks for a specific code only "when callers need to handle the condition differently", and GenericError otherwise (https://github.com/bitcoin/bitcoin/blob/0e38ad26af922abdc0fc36c9379e17efd2a72b80/src/wallet/types.h#L63-L65).

util::Result is meant for this case, functions reporting error strings to end users, and displayAddress() above already returns util::Result<void>. TransactionCanBeBroadcast could return it too, instead of util::Expected<void, bilingual_str>. The RPC could then throw RPC_WALLET_ERROR for failures. It would also stop "Wallet broadcast is disabled" from being reported as an invalid parameter. If the RPC should report an unknown txid the way gettransaction does (RPC_INVALID_ADDRESS_OR_KEY), it could check mapWallet itself before calling, without a wallet-wide code. Since rebroadcastwallettx is new, no client depends on its error codes yet.

Using util::Expected<void, bilingual_str> instead of util::Result<void> would be fine too. My main concern is the signature saying the function can return error codes that it never returns and no caller handles.

My understanding is that for the QML GUI, they generally want error codes as some errors may be handled differently. While the GUI as I have implemented it currently does not use these codes, that does not preclude the future GUI work from using them either, and I'd prefer to avoid needing another interface change at that time as well. cc @pseudoramdom

Even for the new RPC, I think it makes sense to get the codes so that we can give reasonable RPC error codes. I think it is incorrect to return RPC_WALLET_ERROR for things that are clearly the user providing a transaction that cannot be broadcast, hence using InvalidParameter so that it can return RPC_INVALID_PARAMETER.

Furthermore, I agree that it is strange that "Wallet broadcast is disabled" is InvalidParameter, but I don't think the solution is to drop the error codes from the wallet, but rather that TransactionCanBeBroadcast should use util::Expected<void, WalletError> as well, which I've done.

in eba7a572f8832fa355f943f005708e4f20a1b5eb wallet, rpc: Add rebroadcastwallettx

Why do we need two separate functions? This first one only adds one check, and because of the second one being private, they are never called independently outside the class.

Can't we add the if wtx check inside the private function and make it take a txid? A caller knowing the CWalletTx can always know a txid.

<details> <summary>suggested diff</summary>

$ git diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index cf0c45a1b4..c420be2f5f 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1835,7 +1835,7 @@ util::Expected<void, WalletError> CWallet::RebroadcastSingleTransaction(const Tx
     if (it == mapWallet.end()) {
         return util::Unexpected{WalletError{WalletErrorCode::InvalidParameter, strprintf(_("%s not in wallet"), txid.ToString())}};
     }
-    if (!TransactionCanBeBroadcast(it->second)) {
+    if (!TransactionCanBeBroadcast(it->first)) {
         return util::Unexpected{WalletError{WalletErrorCode::InvalidParameter, strprintf(_("%s is not eligible to be rebroadcast"), txid.ToString())}};
     }
     std::string err;
@@ -1849,21 +1849,17 @@ bool CWallet::TransactionCanBeBroadcast(const Txid& txid) const
 {
     LOCK(cs_wallet);
     const CWalletTx* wtx = GetWalletTx(txid);
-    return wtx && TransactionCanBeBroadcast(*wtx);
-}
-
-bool CWallet::TransactionCanBeBroadcast(const CWalletTx& wtx) const
-{
-    AssertLockHeld(cs_wallet);
+    // Transaction is not in the wallet
+    if (!wtx) return false;
     // Can't relay if wallet is not broadcasting
     if (!GetBroadcastTransactions()) return false;
     // Don't relay abandoned transactions
-    if (wtx.isAbandoned()) return false;
+    if (wtx->isAbandoned()) return false;
     // Don't try to submit coinbase transactions. These would fail anyway but would
     // cause log spam.
-    if (wtx.IsCoinBase()) return false;
+    if (wtx->IsCoinBase()) return false;
     // Don't try to submit conflicted or confirmed transactions.
-    if (GetTxDepthInMainChain(wtx) != 0) return false;
+    if (GetTxDepthInMainChain(*wtx) != 0) return false;
     return true;
 }
 
@@ -1873,7 +1869,7 @@ bool CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx,
 {
     AssertLockHeld(cs_wallet);
 
-    if (!TransactionCanBeBroadcast(wtx)) return false;
+    if (!TransactionCanBeBroadcast(wtx.GetHash())) return false;
 
     const char* what{""};
     switch (broadcast_method) {
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 0633e928e1..c64e53e0f5 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -446,8 +446,6 @@ private:
     //! Update mempool conflicts for TRUC sibling transactions
     void UpdateTrucSiblingConflicts(const CWalletTx& parent_wtx, const Txid& child_txid, bool add_conflict) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
-    bool TransactionCanBeBroadcast(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
-
 public:
     /**
      * Main wallet lock.

</details>

To avoid needing to do another mapWallet lookup for callers that already have a CWalletTx.

in eba7a572f8832fa355f943f005708e4f20a1b5eb wallet, rpc: Add rebroadcastwallettx

Why not use wtx.isBlockConflicted()?

Also random and unrelated question. Why IsUnconfirmed() checks if there is any mempool conflict? That existance of a mempool conflict doesn't change the unconfirmed state...

Why not use wtx.isBlockConflicted()?

This is moved code.

Also random and unrelated question. Why IsUnconfirmed() checks if there is any mempool conflict? That existance of a mempool conflict doesn't change the unconfirmed state...

Same reason it checks isAbandoned() and isConflicted - unconfirmed in this context means that this transaction is unconfirmed, and we want/expect it to become confirmed. If the transaction is abandoned or has a conflict already in the mempool, we either do not want it to become confirmed, or it cannot become confirmed.

in eba7a572f8832fa355f943f005708e4f20a1b5eb wallet, rpc: Add rebroadcastwallettx

small nit, feel free to ignore

This check is duplicated, it is already done inside TransactionCanBeBroadcast. Not criticla and i guess you need the find function to call SubmitTxMemoryPoolAndRelay.

I tried to refactor it so TransactionCanBeBroadcast returns a CWalletTx instead of a Void in the next commit, but I think is not worth the work.

Mentioning in case you come with a better idea :)

in eba7a572f8832fa355f943f005708e4f20a1b5eb wallet, rpc: Add rebroadcastwallettx

nit: missing empty space or line jump after "receiver in the transaction."

$ ./build/bin/bitcoin-cli -regtest rebroadcastwallettx
error code: -1
error message:
rebroadcastwallettx "txid"

Rebroadcast the specified wallet transaction.
Note that rebroadcasting a transaction may reveal that your node is involved as either a sender or receiver in the transaction.Use listrawtransactions to see the wallet's transactions

DOe

in 592e7465b475fa6a6e5af846e694b84990505878 wallet: Return specific error messages from TransactionCanBeBroadcast

nit: there are two ; here? I am surprised this does compile with one or two.

<details> <summary>fun fact</summary>

Am I the only one who did not know that you can put as many semicolons as you want and still being correct? This does compile:

$ git diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 5de55db7fa..c104bbe409 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1860,7 +1860,7 @@ util::Expected<void, WalletError> CWallet::TransactionCanBeBroadcast(const CWall
 {
     AssertLockHeld(cs_wallet);
     // Can't relay if wallet is not broadcasting
-    if (!GetBroadcastTransactions()) return util::Unexpected{WalletError{WalletErrorCode::MiscError, _("Wallet broadcast is disabled")}};;
+    if (!GetBroadcastTransactions()) return util::Unexpected{WalletError{WalletErrorCode::MiscError, _("Wallet broadcast is disabled")}};;;;;;;;;;;;;;;;;;;
     // Don't relay abandoned transactions
     if (wtx.isAbandoned()) return util::Unexpected{WalletError{WalletErrorCode::InvalidParameter, strprintf(_("%s is not eligible to be rebroadcast, tx is abandoned"), wtx.GetHash().ToString())}};
     // Don't try to submit coinbase transactions. These would fail anyway but would
@@ -1869,7 +1869,7 @@ util::Expected<void, WalletError> CWallet::TransactionCanBeBroadcast(const CWall
     // Don't try to submit conflicted or confirmed transactions.
     if (GetTxDepthInMainChain(wtx) > 0) return util::Unexpected{WalletError{WalletErrorCode::InvalidParameter, strprintf(_("%s is not eligible to be rebroadcast, tx is confirmed"), wtx.GetHash().ToString())}};
     if (GetTxDepthInMainChain(wtx) < 0) return util::Unexpected{WalletError{WalletErrorCode::InvalidParameter, strprintf(_("%s is not eligible to be rebroadcast, tx is conflicted"), wtx.GetHash().ToString())}};
-    return {};
+    return {};;;;;;;;;;;;;;;
 }

</details>

Fixed