test: fix p2p_leak

mzumsande commented at 10:22 pm on August 1, 2025: contributor

This fixes two issues with p2p_leak_tx.py:

1.) #33090: As far as I can see, this is just the randomness of NextInvToInbounds/ rand_exp_duration, which has a probability of e^-(60s/5s) = 6.14×10^−6 to result in a period > 60s (our waiting time), so that the test would fail every 160k runs… Doubling the timeout should be sufficient to lower the probability drastically.

2.) The subtest test_notfound_on_unannounced_tx has some (w)txid confusion: we send a MSG_TX-type getdata with a wtxid in it, which necessarily always results in a NOTFOUND. I changed this to use wtxids everywhere - because of the timing, the branch where no notfound is received is still extremely unlikely to reach though. Not sure if this is essential for the test, but we could add a random delay of 2-3s to trigger both paths on a regular basis.

test: increase timeout in p2p_leak_tx.py

With a low but not negligible probability in the order
of 10^-6 the exponential timer NextInvToInBounds can lead
to an interval >60s, making the test fail.

3a32ecd761

test: fix (w)txid confusion in p2p_leak_tx.py

Before, we'd send a MSG_TX with a wtxid in it, which
would always result in a notfound answer

ab43e0bac7

DrahtBot added the label Tests on Aug 1, 2025

DrahtBot commented at 10:22 pm on August 1, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33121.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	jonatack, stratospher, naiyoma, enirox001

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label CI failed on Aug 2, 2025

fanquake commented at 10:37 am on August 2, 2025: member

MSAN failure (https://cirrus-ci.com/task/6435705422872576?logs=ci#L1863) is #33114.

jonatack commented at 3:51 pm on August 4, 2025: member

Concept ACK

DrahtBot removed the label CI failed on Aug 5, 2025

stratospher commented at 11:44 am on August 5, 2025: contributor

Nice detective work! Just 1 question, will ACK after that.

I tried some modifications of the test locally but wasn’t able to enter the “else branch” where “tx was already announced to us”. Were you able to test it?
Could also change MAX_REPEATS to a lower number. I’m getting bad-txns-inputs-missingorspent at run 47 when I remove the break in the loop. So don’t think the test would work for full 100 runs anyways.

Not sure if this is essential for the test, but we could add a random delay of 2-3s to trigger both paths on a regular basis.

Hmm I’m fine either ways. On one hand - point of the test is to make sure that node responds with NOTFOUND for unannounced transactions, on the other hand - silent errors like the one fixed in this PR can slip through undetected.

mzumsande commented at 5:20 pm on August 6, 2025: contributor

I tried some modifications of the test locally but wasn’t able to enter the “else branch” where “tx was already announced to us”. Were you able to test it?

No, but with a time.sleep(3) before inbound_peer.send_and_ping(want_tx) it would go into that branch regularly. because the NextInvToInbounds will often generate a random interval <3s - that’s why I mentioned it above.

In general I was a bit unsure about the main point of the subtest (maybe @maflcko you can help?). Is the goal to have both branches executed regularly, or are we just interested in the notfound branch and the rest is there mostly to avoid intermittent failure? By the way, the failure from 1) could also be easily avoided by setting self.noban_tx_relay = True, but that seemed to be at odds with the general intention of the test.

Could also change MAX_REPEATS to a lower number

True, depending on whether we put a sleep in or not.

maflcko commented at 10:50 am on August 7, 2025: member

are we just interested in the notfound branch and the rest is there mostly to avoid intermittent failure?

Yes.

I think mocktime could be used to remove the loop and just do a one-shot notfound message?

naiyoma commented at 6:48 pm on August 8, 2025: contributor

Concept ACK

Increasing timeout to 120 reduces probability to P(120s) = e^(-120/5) = e^(-24) ≈ 3.78×10^-11 (~1 in 26.5B runs)

naiyoma commented at 7:12 pm on August 8, 2025: contributor

but with a time.sleep(3) before inbound_peer.send_and_ping(want_tx) it would go into that branch regularly.

time.sleep(8) after wtxid = self.miniwallet.send_self_transfer(from_node=self.gen_node)["wtxid"] also works

enirox001 commented at 2:24 pm on August 23, 2025: contributor

Concept ACK ab43e0b on fixing the test flakiness.

I took a shot at making test_notfound_on_unannounced_tx() more deterministic using mocktime. Feel free to use it if helpful:

 0diff --git a/test/functional/p2p_leak_tx.py b/test/functional/p2p_leak_tx.py
 1index 3fd324a029..f50993e932 100755
 2--- a/test/functional/p2p_leak_tx.py
 3+++ b/test/functional/p2p_leak_tx.py
 4@@ -11,6 +11,7 @@ from test_framework.util import (
 5     assert_equal,
 6 )
 7 from test_framework.wallet import MiniWallet
 8+import time
 9 
10 
11 class P2PNode(P2PDataStore):
12@@ -79,27 +80,33 @@ class P2PLeakTxTest(BitcoinTestFramework):
13         self.gen_node.disconnect_p2ps()
14         inbound_peer = self.gen_node.add_p2p_connection(P2PNode())  # An "attacking" inbound peer
15 
16-        MAX_REPEATS = 100
17-        self.log.info("Running test up to {} times.".format(MAX_REPEATS))
18-        for i in range(MAX_REPEATS):
19-            self.log.info('Run repeat {}'.format(i + 1))
20-            wtxid = self.miniwallet.send_self_transfer(from_node=self.gen_node)["wtxid"]
21-
22-            want_tx = msg_getdata()
23-            want_tx.inv.append(CInv(t=MSG_WTX, h=int(wtxid, 16)))
24-            with p2p_lock:
25-                inbound_peer.last_message.pop('notfound', None)
26-            inbound_peer.send_and_ping(want_tx)
27-            if inbound_peer.last_message.get('notfound'):
28-                self.log.debug('tx {} was not yet announced to us.'.format(wtxid))
29-                self.log.debug("node has responded with a notfound message. End test.")
30-                assert_equal(inbound_peer.last_message['notfound'].vec[0].hash, int(wtxid, 16))
31+        MOCK_DELTA = 120
32+        MAX_REPEATS = 10
33+        try:
34+            self.gen_node.setmocktime(int(time.time()) + MOCK_DELTA)
35+            for i in range(MAX_REPEATS):
36+                self.log.info('Run repeat {}'.format(i + 1))
37+                wtxid = self.miniwallet.send_self_transfer(from_node=self.gen_node)["wtxid"]
38+                want_tx = msg_getdata()
39+                want_tx.inv.append(CInv(t=MSG_WTX, h=int(wtxid, 16)))
40                 with p2p_lock:
41-                    inbound_peer.last_message.pop('notfound')
42-                break
43+                    inbound_peer.last_message.pop('notfound', None)
44+                    inbound_peer.last_message.pop('tx', None)
45+                inbound_peer.send_and_ping(want_tx)
46+                inbound_peer.wait_until(lambda: inbound_peer.last_message.get('notfound') or inbound_peer.last_message.get('tx'), timeout=10)
47+                if inbound_peer.last_message.get('notfound'):
48+                    self.log.debug('tx {} was not yet announced to us.'.format(wtxid))
49+                    assert_equal(inbound_peer.last_message['notfound'].vec[0].hash, int(wtxid, 16))
50+                    with p2p_lock:
51+                        inbound_peer.last_message.pop('notfound')
52+                    break
53+                else:
54+                    self.log.debug('tx {} was already announced to us. Try test again.'.format(wtxid))
55+                    assert int(wtxid, 16) in [inv.hash for inv in inbound_peer.last_message['inv'].inv]
56             else:
57-                self.log.debug('tx {} was already announced to us. Try test again.'.format(wtxid))
58-                assert int(wtxid, 16) in [inv.hash for inv in inbound_peer.last_message['inv'].inv]
59+                raise AssertionError("Test failed after {} attempts - transactions kept being announced".format(MAX_REPEATS))
60+        finally:
61+            self.gen_node.setmocktime(0)
62 
63 
64 if __name__ == '__main__':

This uses mocktime to control announcement timing more deterministically while keeping a small retry loop for robustness.

davidgumberg commented at 10:35 pm on August 27, 2025: contributor

Ideally, p2p_leak_tx.py would be rewritten so that it would deterministically exercise both branches, my impression is the that we only really care about the first case, that tx’es that haven’t been inv’ed don’t get leaked, but exercising the second case is a good validation of the assumptions of the test, and would have caught that the existing test was broken.

One solution is to set a mocktime as suggested by @enirox001 above so that no time passes and no inv’s can be sent and the getdata will surely fail, then disable mocktimes and wait until an inv is sent, then send the same getdata again and check that we get the expected tx response.

e.g. https://github.com/davidgumberg/bitcoin/commit/f56c27662ddd3dab5b58cd5d8c8fb91b432c94c5

 0def test_notfound_on_unannounced_tx(self):
 1    self.log.info("Check that we don't leak txs to inbound peers that we haven't yet announced to")
 2    self.gen_node.disconnect_p2ps()
 3    inbound_peer = self.gen_node.add_p2p_connection(P2PNode())  # An "attacking" inbound peer
 4
 5    # Set a mock time so that time does not pass, and gen_node never announces the transaction
 6    self.gen_node.setmocktime(1)
 7    wtxid = int(self.miniwallet.send_self_transfer(from_node=self.gen_node)["wtxid"], 16)
 8
 9    want_tx = msg_getdata()
10    want_tx.inv.append(CInv(t=MSG_WTX, h=wtxid))
11    with p2p_lock:
12        inbound_peer.last_message.pop('notfound', None)
13    inbound_peer.send_and_ping(want_tx)
14    inbound_peer.wait_until(lambda: "notfound" in inbound_peer.last_message)
15    with p2p_lock:
16        assert_equal(inbound_peer.last_message.get("notfound").vec[0].hash, wtxid)
17        inbound_peer.last_message.pop('notfound')
18
19    # Disable mocktime and wait for the announcement.
20    inbound_peer.last_message.pop('inv', None)
21    self.gen_node.setmocktime(0)
22
23    # Checking that the inv contains the wtxid would have a 1 in 10^6 chance
24    # of spuriously failing, and the getdata response will show what we really
25    # care about, so we don't check it.
26    self.wait_until(lambda: "inv" in inbound_peer.last_message)
27
28    # Send the getdata again, this time the node should send us a TX message.
29    inbound_peer.last_message.pop('tx', None)
30    inbound_peer.send_and_ping(want_tx)
31    self.wait_until(lambda: "tx" in inbound_peer.last_message)
32    assert_equal(wtxid, int(inbound_peer.last_message["tx"].tx.wtxid_hex, 16))

mzumsande commented at 10:48 pm on August 27, 2025: contributor

Thanks for the reviews and suggestions - I was busy the last couple of weeks, but I’ll look at them in detail / update the PR next week!

test: fix p2p_leak_tx.py #33121

Code Coverage & Benchmarks

Reviews