test: fix p2p_leak_tx.py #33121

pull mzumsande wants to merge 2 commits into bitcoin:master from mzumsande:202508_fix_p2p_leak_tx.py changing 1 files +8 −9
  1. 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.

  2. 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
  3. 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
  4. DrahtBot added the label Tests on Aug 1, 2025
  5. 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

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

  6. DrahtBot added the label CI failed on Aug 2, 2025
  7. fanquake commented at 10:37 am on August 2, 2025: member
  8. jonatack commented at 3:51 pm on August 4, 2025: member
    Concept ACK
  9. DrahtBot removed the label CI failed on Aug 5, 2025
  10. stratospher commented at 11:44 am on August 5, 2025: contributor

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

    1. 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?
    2. 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.

  11. 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.

  12. 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?


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-08-08 18:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me