Merge bitcoin/bitcoin#22261: [p2p/mempool] Two small fixes to node broadcast logic

5a77abd4e657458852875a07692898982f4b1db5 [style] Clean up BroadcastTransaction() (John Newbery)
7282d4c0363ab5152baa34af626cb49afbfddc32 [test] Allow rebroadcast for same-txid-different-wtxid transactions (glozow)
cd48372b67d961fe661990a2c6d3cc3d91478924 [mempool] Allow rebroadcast for same-txid-different-wtxid transactions (John Newbery)
847b6ed48d7bacec9024618922e9b339d2d97676 [test] Test transactions are not re-added to unbroadcast set (Duncan Dean)
2837a9f1eaa2c6bf402d1d9891d9aa84c4a56033 [mempool] Only add a transaction to the unbroadcast set when it's added to the mempool (John Newbery)

Pull request description:

  1. Only add a transaction to the unbroadcast set when it's added to the mempool

      Currently, if BroadcastTransaction() is called to rebroadcast a
      transaction (e.g. by ResendWalletTransactions()), then we add the
      transaction to the unbroadcast set. That transaction has already been
      broadcast in the past, so peers are unlikely to request it again,
      meaning RemoveUnbroadcastTx() won't be called and it won't be removed
      from m_unbroadcast_txids.

      Net processing will therefore continue to attempt rebroadcast for the
      transaction every 10-15 minutes. This will most likely continue until
      the node connects to a new peer which hasn't yet seen the transaction
      (or perhaps indefinitely).

      Fix by only adding the transaction to the broadcast set when it's added to the mempool.

  2. Allow rebroadcast for same-txid-different-wtxid transactions

      There is some slightly unexpected behaviour when:

      - there is already transaction in the mempool (the "mempool tx")
      - BroadcastTransaction() is called for a transaction with the same txid
        as the mempool transaction but a different witness (the "new tx")

      Prior to this commit, if BroadcastTransaction() is called with
      relay=true, then it'll call RelayTransaction() using the txid/wtxid of
      the new tx, not the txid/wtxid of the mempool tx. For wtxid relay peers,
      in SendMessages(), the wtxid of the new tx will be taken from
      setInventoryTxToSend, but will then be filtered out from the vector of
      wtxids to announce, since m_mempool.info() won't find the transaction
      (the mempool contains the mempool tx, which has a different wtxid from
      the new tx).

      Fix this by calling RelayTransaction() with the wtxid of the mempool
      transaction in this case.

  The third commit is a comment/whitespace only change to tidy up the BroadcastTransaction() function.

ACKs for top commit:
  duncandean:
    reACK 5a77abd
  naumenkogs:
    ACK 5a77abd4e657458852875a07692898982f4b1db5
  theStack:
    re-ACK 5a77abd4e657458852875a07692898982f4b1db5
  lsilva01:
    re-ACK https://github.com/bitcoin/bitcoin/pull/22261/commits/5a77abd4e657458852875a07692898982f4b1db5

Tree-SHA512: d1a46d32a9f975220e5b432ff6633fac9be01ea41925b4958395b8d641680500dc44476b12d18852e5b674d2d87e4d0160b4483e45d3d149176bdff9f4dc8516

2 files changed, 22 insertions, 1 deletions

diff --git a/test/functional/mempool_accept_wtxid.py b/test/functional/mempool_accept_wtxid.py
index dd1f8997ad..63ecc8ee2a 100755
--- a/test/functional/mempool_accept_wtxid.py
+++ b/test/functional/mempool_accept_wtxid.py
@@ -16,6 +16,7 @@ from test_framework.messages import (
     CTxOut,
     sha256,
 )
+from test_framework.p2p import P2PTxInvStore
 from test_framework.script import (
     CScript,
     OP_0,
@@ -62,6 +63,8 @@ class MempoolWtxidTest(BitcoinTestFramework):
         parent_txid = node.sendrawtransaction(hexstring=raw_parent, maxfeerate=0)
         node.generate(1)
 
+        peer_wtxid_relay = node.add_p2p_connection(P2PTxInvStore())
+
         # Create a new transaction with witness solving first branch
         child_witness_script = CScript([OP_TRUE])
         child_witness_program = sha256(child_witness_script)
@@ -87,10 +90,13 @@ class MempoolWtxidTest(BitcoinTestFramework):
         assert_equal(child_one_txid, child_two_txid)
         assert child_one_wtxid != child_two_wtxid
 
-        self.log.info("Submit one child to the mempool")
+        self.log.info("Submit child_one to the mempool")
         txid_submitted = node.sendrawtransaction(child_one.serialize().hex())
         assert_equal(node.getrawmempool(True)[txid_submitted]['wtxid'], child_one_wtxid)
 
+        peer_wtxid_relay.wait_for_broadcast([child_one_wtxid])
+        assert_equal(node.getmempoolinfo()["unbroadcastcount"], 0)
+
         # testmempoolaccept reports the "already in mempool" error
         assert_equal(node.testmempoolaccept([child_one.serialize().hex()]), [{
             "txid": child_one_txid,
@@ -108,9 +114,18 @@ class MempoolWtxidTest(BitcoinTestFramework):
 
         # sendrawtransaction will not throw but quits early when the exact same transaction is already in mempool
         node.sendrawtransaction(child_one.serialize().hex())
+
+        self.log.info("Connect another peer that hasn't seen child_one before")
+        peer_wtxid_relay_2 = node.add_p2p_connection(P2PTxInvStore())
+
+        self.log.info("Submit child_two to the mempool")
         # sendrawtransaction will not throw but quits early when a transaction with the same non-witness data is already in mempool
         node.sendrawtransaction(child_two.serialize().hex())
 
+        # The node should rebroadcast the transaction using the wtxid of the correct transaction
+        # (child_one, which is in its mempool).
+        peer_wtxid_relay_2.wait_for_broadcast([child_one_wtxid])
+        assert_equal(node.getmempoolinfo()["unbroadcastcount"], 0)
 
 if __name__ == '__main__':
     MempoolWtxidTest().main()
diff --git a/test/functional/mempool_unbroadcast.py b/test/functional/mempool_unbroadcast.py
index b475b65e68..7d9e6c306d 100755
--- a/test/functional/mempool_unbroadcast.py
+++ b/test/functional/mempool_unbroadcast.py
@@ -92,6 +92,12 @@ class MempoolUnbroadcastTest(BitcoinTestFramework):
         self.disconnect_nodes(0, 1)
         node.disconnect_p2ps()
 
+        self.log.info("Rebroadcast transaction and ensure it is not added to unbroadcast set when already in mempool")
+        rpc_tx_hsh = node.sendrawtransaction(txFS["hex"])
+        mempool = node.getrawmempool(True)
+        assert rpc_tx_hsh in mempool
+        assert not mempool[rpc_tx_hsh]['unbroadcast']
+
     def test_txn_removal(self):
         self.log.info("Test that transactions removed from mempool are removed from unbroadcast set")
         node = self.nodes[0]