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

651f1d816f [test] wait for inital broadcast before comparing mempool entries (gzhao408)
9d3f7eb986 [mempool] sanity check that all unbroadcast txns are in mempool (gzhao408)
a7ebe48b94 [rpc] add unbroadcast info to mempool entries and getmempoolinfo (gzhao408)
d160069604 [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 651f1d816f
  amitiuttarwar:
    ACK 651f1d816f 🎉
  MarcoFalke:
    Review ACK 651f1d816f

Tree-SHA512: d5327e95ef39d44152b48df5c610502ae11c168f43dbbfb2885340c93d1ba9426eb3a5794573f5fc843502109cb3ffb63efa3f2db4f8f112efcde8f76d9a8845

test/functional/mempool_packages.py

@@ -7,6 +7,7 @@
 from decimal import Decimal
 
 from test_framework.messages import COIN
+from test_framework.mininode import P2PTxInvStore
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
@@ -58,6 +59,7 @@ class MempoolPackagesTest(BitcoinTestFramework):
 
     def run_test(self):
         # Mine some blocks and have them mature.
+        self.nodes[0].add_p2p_connection(P2PTxInvStore()) # keep track of invs
         self.nodes[0].generate(101)
         utxo = self.nodes[0].listunspent(10)
         txid = utxo[0]['txid']
@@ -72,6 +74,10 @@ class MempoolPackagesTest(BitcoinTestFramework):
             value = sent_value
             chain.append(txid)
 
+        # Wait until mempool transactions have passed initial broadcast (sent inv and received getdata)
+        # Otherwise, getrawmempool may be inconsistent with getmempoolentry if unbroadcast changes in between
+        self.nodes[0].p2p.wait_for_broadcast(chain)
+
         # Check mempool has MAX_ANCESTORS transactions in it, and descendant and ancestor
         # count and fees should look correct
         mempool = self.nodes[0].getrawmempool(True)
@@ -212,6 +218,10 @@ class MempoolPackagesTest(BitcoinTestFramework):
         for tx in chain[:MAX_ANCESTORS_CUSTOM]:
             assert tx in mempool1
         # TODO: more detailed check of node1's mempool (fees etc.)
+        # check transaction unbroadcast info (should be false if in both mempools)
+        mempool = self.nodes[0].getrawmempool(True)
+        for tx in mempool:
+            assert_equal(mempool[tx]['unbroadcast'], False)
 
         # TODO: test ancestor size limits
 

test/functional/mempool_unbroadcast.py

@@ -53,6 +53,13 @@ class MempoolUnbroadcastTest(BitcoinTestFramework):
         txFS = node.signrawtransactionwithwallet(txF["hex"])
         rpc_tx_hsh = node.sendrawtransaction(txFS["hex"])
 
+        # check transactions are in unbroadcast using rpc
+        mempoolinfo = self.nodes[0].getmempoolinfo()
+        assert_equal(mempoolinfo['unbroadcastcount'], 2)
+        mempool = self.nodes[0].getrawmempool(True)
+        for tx in mempool:
+            assert_equal(mempool[tx]['unbroadcast'], True)
+
         # check that second node doesn't have these two txns
         mempool = self.nodes[1].getrawmempool()
         assert rpc_tx_hsh not in mempool
@@ -71,6 +78,11 @@ class MempoolUnbroadcastTest(BitcoinTestFramework):
         assert rpc_tx_hsh in mempool
         assert wallet_tx_hsh in mempool
 
+        # check that transactions are no longer in first node's unbroadcast set
+        mempool = self.nodes[0].getrawmempool(True)
+        for tx in mempool:
+            assert_equal(mempool[tx]['unbroadcast'], False)
+
         self.log.info("Add another connection & ensure transactions aren't broadcast again")
 
         conn = node.add_p2p_connection(P2PTxInvStore())

test/functional/test_framework/mininode.py

@@ -645,6 +645,7 @@ class P2PTxInvStore(P2PInterface):
         self.tx_invs_received = defaultdict(int)
 
     def on_inv(self, message):
+        super().on_inv(message) # Send getdata in response.
         # Store how many times invs have been received for each tx.
         for i in message.inv:
             if i.type == MSG_TX:
@@ -654,3 +655,12 @@ class P2PTxInvStore(P2PInterface):
     def get_invs(self):
         with mininode_lock:
             return list(self.tx_invs_received.keys())
+
+    def wait_for_broadcast(self, txns, timeout=60):
+        """Waits for the txns (list of txids) to complete initial broadcast.
+        The mempool should mark unbroadcast=False for these transactions.
+        """
+        # Wait until invs have been received (and getdatas sent) for each txid.
+        self.wait_until(lambda: set(self.get_invs()) == set([int(tx, 16) for tx in txns]), timeout)
+        # Flush messages and wait for the getdatas to be processed
+        self.sync_with_ping()