Merge #18895: p2p: unbroadcast followups: rpcs, nLastResend, mempool sanity check

651f1d816f054cb9c637f8a99c9360bba381ef58 [test] wait for inital broadcast before comparing mempool entries (gzhao408)
9d3f7eb9860254eb787ebe2734fd6a26bcf365c1 [mempool] sanity check that all unbroadcast txns are in mempool (gzhao408)
a7ebe48b94c5a9195c8eabd193204c499cb4bfdb [rpc] add unbroadcast info to mempool entries and getmempoolinfo (gzhao408)
d16006960443c2efe37c896e46edae9dca86c57d [wallet] remove nLastResend logic (gzhao408)

Pull request description:

  Followup to #18038 by amitiuttarwar which introduces the unbroadcast set: "a mechanism for the mempool to track locally submitted transactions" and decreases the frequency of rebroadcast from 10-15 minutes to 12-36 hours.

  This PR addresses some of the outstanding TODOs building on top of it:
  - remove `nLastResend` logic, which is used to ensure rebroadcast doesn't happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416826914))
  - expose unbroadcast info via RPCs, for more informative queries and testing (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416837980))
  - add sanity check to verify unbroadcast transactions still exist in mempool before rebroadcasting (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416861609))

ACKs for top commit:
  naumenkogs:
    Code review ACK 651f1d816f054cb9c637f8a99c9360bba381ef58
  amitiuttarwar:
    ACK 651f1d816f054cb9c637f8a99c9360bba381ef58 🎉
  MarcoFalke:
    Review ACK 651f1d816f054cb9c637f8a99c9360bba381ef58

Tree-SHA512: d5327e95ef39d44152b48df5c610502ae11c168f43dbbfb2885340c93d1ba9426eb3a5794573f5fc843502109cb3ffb63efa3f2db4f8f112efcde8f76d9a8845

5 files changed, 20 insertions, 8 deletions

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 9d9e31d3ed..ac5300990f 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -817,7 +817,12 @@ void PeerLogicValidation::ReattemptInitialBroadcast(CScheduler& scheduler) const
     std::set<uint256> unbroadcast_txids = m_mempool.GetUnbroadcastTxs();
 
     for (const uint256& txid : unbroadcast_txids) {
-        RelayTransaction(txid, *connman);
+        // Sanity check: all unbroadcast txns should exist in the mempool
+        if (m_mempool.exists(txid)) {
+            RelayTransaction(txid, *connman);
+        } else {
+            m_mempool.RemoveUnbroadcastTx(txid, true);
+        }
     }
 
     // schedule next run for 10-15 minutes in the future
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 3c5fd565e3..ac567e16ce 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -408,6 +408,7 @@ static std::vector<RPCResult> MempoolEntryDescription() { return {
     RPCResult{RPCResult::Type::ARR, "spentby", "unconfirmed transactions spending outputs from this transaction",
         {RPCResult{RPCResult::Type::STR_HEX, "transactionid", "child transaction id"}}},
     RPCResult{RPCResult::Type::BOOL, "bip125-replaceable", "Whether this transaction could be replaced due to BIP125 (replace-by-fee)"},
+    RPCResult{RPCResult::Type::BOOL, "unbroadcast", "Whether this transaction is currently unbroadcast (initial broadcast not yet confirmed)"},
 };}
 
 static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPoolEntry& e) EXCLUSIVE_LOCKS_REQUIRED(pool.cs)
@@ -469,6 +470,7 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool
     }
 
     info.pushKV("bip125-replaceable", rbfStatus);
+    info.pushKV("unbroadcast", pool.IsUnbroadcastTx(tx.GetHash()));
 }
 
 UniValue MempoolToJSON(const CTxMemPool& pool, bool verbose)
@@ -1398,7 +1400,7 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool)
     ret.pushKV("maxmempool", (int64_t) maxmempool);
     ret.pushKV("mempoolminfee", ValueFromAmount(std::max(pool.GetMinFee(maxmempool), ::minRelayTxFee).GetFeePerK()));
     ret.pushKV("minrelaytxfee", ValueFromAmount(::minRelayTxFee.GetFeePerK()));
-
+    ret.pushKV("unbroadcastcount", uint64_t{pool.GetUnbroadcastTxs().size()});
     return ret;
 }
 
@@ -1417,6 +1419,7 @@ static UniValue getmempoolinfo(const JSONRPCRequest& request)
                         {RPCResult::Type::NUM, "maxmempool", "Maximum memory usage for the mempool"},
                         {RPCResult::Type::STR_AMOUNT, "mempoolminfee", "Minimum fee rate in " + CURRENCY_UNIT + "/kB for tx to be accepted. Is the maximum of minrelaytxfee and minimum mempool fee"},
                         {RPCResult::Type::STR_AMOUNT, "minrelaytxfee", "Current minimum relay fee for transactions"},
+                        {RPCResult::Type::NUM, "unbroadcastcount", "Current number of transactions that haven't passed initial broadcast yet"}
                     }},
                 RPCExamples{
                     HelpExampleCli("getmempoolinfo", "")
diff --git a/src/txmempool.h b/src/txmempool.h
index 4bee78b8d6..4568eb928d 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -704,7 +704,10 @@ public:
     /** Adds a transaction to the unbroadcast set */
     void AddUnbroadcastTx(const uint256& txid) {
         LOCK(cs);
-        m_unbroadcast_txids.insert(txid);
+        /** Sanity Check: the transaction should also be in the mempool */
+        if (exists(txid)) {
+            m_unbroadcast_txids.insert(txid);
+        }
     }
 
     /** Removes a transaction from the unbroadcast set */
@@ -716,6 +719,12 @@ public:
         return m_unbroadcast_txids;
     }
 
+    // Returns if a txid is in the unbroadcast set
+    bool IsUnbroadcastTx(const uint256& txid) const {
+        LOCK(cs);
+        return (m_unbroadcast_txids.count(txid) != 0);
+    }
+
 private:
     /** UpdateForDescendants is used by UpdateTransactionsFromBlock to update
      *  the descendants for a single transaction that has been added to the
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 2b45c6a536..e1e8263ee1 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1982,10 +1982,6 @@ void CWallet::ResendWalletTransactions()
     nNextResend = GetTime() + (12 * 60 * 60) + GetRand(24 * 60 * 60);
     if (fFirst) return;
 
-    // Only do it if there's been a new block since last time
-    if (m_best_block_time < nLastResend) return;
-    nLastResend = GetTime();
-
     int submitted_tx_count = 0;
 
     { // cs_wallet scope
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index a29fa22207..9d312e8ee5 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -641,7 +641,6 @@ private:
     int nWalletMaxVersion GUARDED_BY(cs_wallet) = FEATURE_BASE;
 
     int64_t nNextResend = 0;
-    int64_t nLastResend = 0;
     bool fBroadcastTransactions = false;
     // Local time that the tip block was received. Used to schedule wallet rebroadcasts.
     std::atomic<int64_t> m_best_block_time {0};