Merge bitcoin#18807: [doc / test / mempool] unbroadcast follow-ups

9e1cb1a [trivial/doc] Fix comment type (Amiti Uttarwar)
8f30260 [doc] Update unbroadcast description in RPC results (Amiti Uttarwar)
750456d [trivial] Remove misleading 'const' (Amiti Uttarwar)
fa32e67 [test] Manage node connections better in mempool persist test (Amiti Uttarwar)
1f94bb0 [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar)
9c8a55d [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar)
ba54983 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar)
00d44a5 [test] P2P connection behavior should meet expectations (Amiti Uttarwar)
bd093ca [test] updates to unbroadcast test (Amiti Uttarwar)
dab298d [docs] add release notes (Amiti Uttarwar)

Pull request description:

  This PR is a follow up to bitcoin#18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in `LoadMempool` when you upgrade software versions.

  bitcoin#18895 is another follow up to that addresses other functionality updates.

  Background context:
  The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a `GETDATA` is received or the transaction is removed from the mempool.

ACKs for top commit:
  MarcoFalke:
    ACK 9e1cb1a eye
  gzhao408:
    ACK [`9e1cb1a`](9e1cb1a)

Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6

src/net_processing.cpp

@@ -792,7 +792,8 @@ void PeerLogicValidation::ReattemptInitialBroadcast(CScheduler& scheduler) const
         }
     }
 
-    // schedule next run for 10-15 minutes in the future
+    // Schedule next run for 10-15 minutes in the future.
+    // We add randomness on every cycle to avoid the possibility of P2P fingerprinting.
     const std::chrono::milliseconds delta = std::chrono::minutes{10} + GetRandMillis(std::chrono::minutes{5});
     scheduler.scheduleFromNow([&] { ReattemptInitialBroadcast(scheduler); }, delta);
 }

src/rpc/blockchain.cpp

@@ -399,7 +399,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)"},
+    RPCResult{RPCResult::Type::BOOL, "unbroadcast", "Whether this transaction is currently unbroadcast (initial broadcast not yet acknowledged by any peers)"},
 };}
 
 static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPoolEntry& e) EXCLUSIVE_LOCKS_REQUIRED(pool.cs)

src/txmempool.h

@@ -704,7 +704,7 @@ class CTxMemPool
     /** Adds a transaction to the unbroadcast set */
     void AddUnbroadcastTx(const uint256& txid) {
         LOCK(cs);
-        /** Sanity Check: the transaction should also be in the mempool */
+        // Sanity Check: the transaction should also be in the mempool
         if (exists(txid)) {
             m_unbroadcast_txids.insert(txid);
         }
@@ -714,12 +714,12 @@ class CTxMemPool
     void RemoveUnbroadcastTx(const uint256& txid, const bool unchecked = false);
 
     /** Returns transactions in unbroadcast set */
-    const std::set<uint256> GetUnbroadcastTxs() const {
+    std::set<uint256> GetUnbroadcastTxs() const {
         LOCK(cs);
         return m_unbroadcast_txids;
     }
 
-    // Returns if a txid is in the unbroadcast set
+    /** Returns whether a txid is in the unbroadcast set */
     bool IsUnbroadcastTx(const uint256& txid) const {
         LOCK(cs);
         return (m_unbroadcast_txids.count(txid) != 0);

src/validation.cpp

@@ -5032,12 +5032,18 @@ bool LoadMempool(CTxMemPool& pool)
             pool.PrioritiseTransaction(i.first, i.second);
         }
 
-        std::set<uint256> unbroadcast_txids;
-        file >> unbroadcast_txids;
-        unbroadcast = unbroadcast_txids.size();
+        // TODO: remove this try except in v0.22
+        try {
+          std::set<uint256> unbroadcast_txids;
+          file >> unbroadcast_txids;
+          unbroadcast = unbroadcast_txids.size();
 
-        for (const auto& txid : unbroadcast_txids) {
+          for (const auto& txid : unbroadcast_txids) {
             pool.AddUnbroadcastTx(txid);
+          }
+        } catch (const std::exception&) {
+          // mempool.dat files created prior to v0.21 will not have an
+          // unbroadcast set. No need to log a failure if parsing fails here.
         }
 
     } catch (const std::exception& e) {

test/functional/mempool_persist.py

@@ -84,7 +84,9 @@ def run_test(self):
         assert_greater_than_or_equal(tx_creation_time_higher, tx_creation_time)
 
         # disconnect nodes & make a txn that remains in the unbroadcast set.
-        disconnect_nodes(self.nodes[0], 2)
+        disconnect_nodes(self.nodes[0], 1)
+        assert(len(self.nodes[0].getpeerinfo()) == 0)
+        assert(len(self.nodes[0].p2ps) == 0)
         self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), Decimal("12"))
         connect_nodes(self.nodes[0], 2)
 
@@ -157,8 +159,10 @@ def test_persist_unbroadcast(self):
         # clear out mempool
         node0.generate(1)
 
-        # disconnect nodes to make a txn that remains in the unbroadcast set.
-        disconnect_nodes(node0, 1)
+        # ensure node0 doesn't have any connections
+        # make a transaction that will remain in the unbroadcast set
+        assert(len(node0.getpeerinfo()) == 0)
+        assert(len(node0.p2ps) == 0)
         node0.sendtoaddress(self.nodes[1].getnewaddress(), Decimal("12"))
 
         # shutdown, then startup with wallet disabled

test/functional/mempool_unbroadcast.py

@@ -16,6 +16,7 @@
     disconnect_nodes,
 )
 
+MAX_INITIAL_BROADCAST_DELAY = 15 * 60 # 15 minutes in seconds
 
 class MempoolUnbroadcastTest(BitcoinTestFramework):
     def set_test_params(self):
@@ -72,7 +73,7 @@ def test_broadcast(self):
         connect_nodes(node, 1)
 
         # fast forward into the future & ensure that the second node has the txns
-        node.mockscheduler(15 * 60)  # 15 min in seconds
+        node.mockscheduler(MAX_INITIAL_BROADCAST_DELAY)
         self.sync_mempools(timeout=30)
         mempool = self.nodes[1].getrawmempool()
         assert rpc_tx_hsh in mempool
@@ -86,15 +87,16 @@ def test_broadcast(self):
         self.log.info("Add another connection & ensure transactions aren't broadcast again")
 
         conn = node.add_p2p_connection(P2PTxInvStore())
-        node.mockscheduler(15 * 60)
-        time.sleep(5)
+        node.mockscheduler(MAX_INITIAL_BROADCAST_DELAY)
+        time.sleep(2) # allow sufficient time for possibility of broadcast
         assert_equal(len(conn.get_invs()), 0)
 
+        disconnect_nodes(node, 1)
+        node.disconnect_p2ps()
+
     def test_txn_removal(self):
         self.log.info("Test that transactions removed from mempool are removed from unbroadcast set")
         node = self.nodes[0]
-        disconnect_nodes(node, 1)
-        node.disconnect_p2ps
 
         # since the node doesn't have any connections, it will not receive
         # any GETDATAs & thus the transaction will remain in the unbroadcast set.

test/functional/test_framework/mininode.py

@@ -651,6 +651,8 @@ def on_inv(self, message):
                 # save txid
                 self.tx_invs_received[i.hash] += 1
 
+        super().on_inv(message)
+
     def get_invs(self):
         with mininode_lock:
             return list(self.tx_invs_received.keys())

test/functional/wallet_resendwallettransactions.py

@@ -49,16 +49,21 @@ def run_test(self):
         block.solve()
         node.submitblock(ToHex(block))
 
-        # Transaction should not be rebroadcast
         node.syncwithvalidationinterfacequeue()
-        node.p2ps[1].sync_with_ping()
-        assert_equal(node.p2ps[1].tx_invs_received[txid], 0)
+        now = int(time.time())
+
+        # Transaction should not be rebroadcast within first 12 hours
+        # Leave 2 mins for buffer
+        twelve_hrs = 12 * 60 * 60
+        two_min = 2 * 60
+        node.setmocktime(now + twelve_hrs - two_min)
+        time.sleep(2) # ensure enough time has passed for rebroadcast attempt to occur
+        assert_equal(txid in node.p2ps[1].get_invs(), False)
 
         self.log.info("Bump time & check that transaction is rebroadcast")
         # Transaction should be rebroadcast approximately 24 hours in the future,
         # but can range from 12-36. So bump 36 hours to be sure.
-        rebroadcast_time = int(time.time()) + 36 * 60 * 60
-        node.setmocktime(rebroadcast_time)
+        node.setmocktime(now + 36 * 60 * 60)
         wait_until(lambda: node.p2ps[1].tx_invs_received[txid] >= 1, lock=mininode_lock)