highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-03-05 20:49:19 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#34646: Fix two issues in p2p_private_broadcast.py

Browse Source 
c462e54f9d test: don't always assert NUM_PRIVATE_BROADCAST_PER_TX broadcasts (Vasil Dimov)
3710566305 test: move abortprivatebroadcast test at the end (Vasil Dimov)

Pull request description:

  _test: move abortprivatebroadcast test at the end_

  The piece of `p2p_private_broadcast.py` which tests the correctness of
  `abortprivatebroadcast` issues a new `sendrawtransaction` call. That
  call schedules up to 3 new connections: peer=13, peer=14 and possibly
  peer=15 before it gets aborted.

  These up to 3 in-the-process-of-opening private broadcast connections
  have `CNode::m_connected` set early - when the `CNode` object is
  created. Later in the test the mock time is advanced by 20 minutes and
  those "old" connections pick a transaction for rebroadcast but that
  triggers `PRIVATE_BROADCAST_MAX_CONNECTION_LIFETIME` immediately:

  ```
  2026-02-21T13:28:14.209766Z [privbcast] [net.cpp:4006] [CNode] [net] Added connection peer=20
  2026-02-21T13:28:14.309792Z (mocktime: 2026-02-21T13:48:14Z) [msghand] [net.cpp:4074] [PushMessage] [net] sending inv (37 bytes) peer=20
  2026-02-21T13:28:14.309801Z (mocktime: 2026-02-21T13:48:14Z) [msghand] [net_processing.cpp:5745] [SendMessages] [privatebroadcast] Disconnecting: did not complete the transaction send within 180 seconds, peer=20
  ```

  This prematurely stops the private broadcast connection and results in
  a failure like:

  ```
  AssertionError: ... not({} == {'ping': 1, 'tx': 1})
  ```

  ---

  _test: don't always assert NUM_PRIVATE_BROADCAST_PER_TX broadcasts_

  In `p2p_private_broadcast.py` in the function `check_broadcasts()` we
  should assert that the broadcast was done to `broadcasts_to_expect`
  peers, not to `NUM_PRIVATE_BROADCAST_PER_TX`. This is because in the
  "Basic" test we check the first broadcast manually because it is done to
  `nodes[1]` and then check the other two by
  `check_broadcasts(..., NUM_PRIVATE_BROADCAST_PER_TX - 1, ...)`.
  The first broadcast might not have fully concluded by the time we call
  `check_broadcasts()` to check the remaining 2.

  Demanding always `NUM_PRIVATE_BROADCAST_PER_TX` can lead to:

  ```
  Traceback (most recent call last):
    File "/home/vd/gh/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 142, in main
      self.run_test()
      ~~~~~~~~~~~~~^^
    File "/tmp/build/clang22/test/functional/p2p_private_broadcast.py", line 347, in run_test
      self.check_broadcasts("Basic", txs[0], NUM_PRIVATE_BROADCAST_PER_TX - 1, NUM_INITIAL_CONNECTIONS + 1)
      ~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/tmp/build/clang22/test/functional/p2p_private_broadcast.py", line 313, in check_broadcasts
      assert_greater_than_or_equal(sum(1 for p in peers if "received" in p), NUM_PRIVATE_BROADCAST_PER_TX)
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/home/vd/gh/bitcoin/bitcoin/test/functional/test_framework/util.py", line 94, in assert_greater_than_or_equal
      raise AssertionError("%s < %s" % (str(thing1), str(thing2)))
  AssertionError: 2 < 3
  ```

ACKs for top commit:
  l0rinc:
    ACK c462e54f9d
  achow101:
    ACK c462e54f9d
  andrewtoth:
    ACK c462e54f9d

Tree-SHA512: 0de8d0eae079eeedc3bfad39df8129a8fa0d7734bdc03b4fb3e520a2f13a187d68118ffc210556af125d634f0ff51a1b081b34a023ac68a1c6a0caf541cecb91
This commit is contained in:
Ava Chow
2026-02-23 13:45:25 -08:00
parent 9581a0a5b1 c462e54f9d
commit ab8a7af742
1 changed files with 20 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files

40
test/functional/p2p_private_broadcast.py
View File

@@ -310,7 +310,7 @@ class P2PPrivateBroadcast(BitcoinTestFramework):
peers = pending[0]["peers"]
assert len(peers) >= NUM_PRIVATE_BROADCAST_PER_TX
assert all("address" in p and "sent" in p for p in peers)
assert_greater_than_or_equal(sum(1 for p in peers if "received" in p), NUM_PRIVATE_BROADCAST_PER_TX)
assert_greater_than_or_equal(sum(1 for p in peers if "received" in p), broadcasts_to_expect)
def run_test(self):
tx_originator = self.nodes[0]
@@ -382,25 +382,6 @@ class P2PPrivateBroadcast(BitcoinTestFramework):
pending = [t for t in pbinfo["transactions"] if t["txid"] == txs[0]["txid"] and t["wtxid"] == txs[0]["wtxid"]]
assert_equal(len(pending), 0)
self.log.info("Checking abortprivatebroadcast removes a pending private-broadcast transaction")
tx_abort = wallet.create_self_transfer()
tx_originator.sendrawtransaction(hexstring=tx_abort["hex"], maxfeerate=0.1)
assert any(t["wtxid"] == tx_abort["wtxid"] for t in tx_originator.getprivatebroadcastinfo()["transactions"])
abort_res = tx_originator.abortprivatebroadcast(tx_abort["txid"])
assert_equal(len(abort_res["removed_transactions"]), 1)
assert_equal(abort_res["removed_transactions"][0]["txid"], tx_abort["txid"])
assert_equal(abort_res["removed_transactions"][0]["wtxid"], tx_abort["wtxid"])
assert_equal(abort_res["removed_transactions"][0]["hex"].lower(), tx_abort["hex"].lower())
assert all(t["wtxid"] != tx_abort["wtxid"] for t in tx_originator.getprivatebroadcastinfo()["transactions"])
self.log.info("Checking abortprivatebroadcast fails for non-existent transaction")
assert_raises_rpc_error(
-5,
"Transaction not in private broadcast queue",
tx_originator.abortprivatebroadcast,
"0" * 64,
)
self.log.info("Sending a transaction that is already in the mempool")
skip_destinations = len(self.destinations)
tx_originator.sendrawtransaction(hexstring=txs[0]["hex"], maxfeerate=0)
@@ -453,6 +434,25 @@ class P2PPrivateBroadcast(BitcoinTestFramework):
tx_originator.sendrawtransaction(hexstring=sibling2.serialize_with_witness().hex(), maxfeerate=0.1)
self.log.info(" - sent sibling2: ok")
self.log.info("Checking abortprivatebroadcast removes a pending private-broadcast transaction")
tx_abort = wallet.create_self_transfer()
tx_originator.sendrawtransaction(hexstring=tx_abort["hex"], maxfeerate=0.1)
assert any(t["wtxid"] == tx_abort["wtxid"] for t in tx_originator.getprivatebroadcastinfo()["transactions"])
abort_res = tx_originator.abortprivatebroadcast(tx_abort["txid"])
assert_equal(len(abort_res["removed_transactions"]), 1)
assert_equal(abort_res["removed_transactions"][0]["txid"], tx_abort["txid"])
assert_equal(abort_res["removed_transactions"][0]["wtxid"], tx_abort["wtxid"])
assert_equal(abort_res["removed_transactions"][0]["hex"].lower(), tx_abort["hex"].lower())
assert all(t["wtxid"] != tx_abort["wtxid"] for t in tx_originator.getprivatebroadcastinfo()["transactions"])
self.log.info("Checking abortprivatebroadcast fails for non-existent transaction")
assert_raises_rpc_error(
-5,
"Transaction not in private broadcast queue",
tx_originator.abortprivatebroadcast,
"0" * 64,
)
# Stop the SOCKS5 proxy server to avoid it being upset by the bitcoin
# node disconnecting in the middle of the SOCKS5 handshake when we
# restart below.