test: add option to skip large re-org test in feature_block #33003

pull brunoerg wants to merge 1 commits into bitcoin:master from brunoerg:2025-07-test-featureblock-skiplargereorg changing 1 files +49 −44
  1. brunoerg commented at 1:57 pm on July 17, 2025: contributor

    Fixes #32877

    This PR adds a config flag --skipreorg which is used to skip the large re-org test. According to corecheck, feature_block is our slowest functional test and primarily because of this large re-org test. However, this test might not be useful for the mutation analysis of some files and could be skipped to save a huge amount of time.

    0time ./build/test/functional/feature_block.py --skipreorg
1./build/test/functional/feature_block.py --skipreorg  11.38s user 0.33s system 37% cpu 31.422 total
2time ./build/test/functional/feature_block.py
3./build/test/functional/feature_block.py  25.87s user 3.53s system 56% cpu 52.317 total
  2. brunoerg marked this as ready for review on Jul 17, 2025
  3. in test/functional/feature_block.py:1335 in c8db1bb6ed outdated 
    1368         self.send_blocks([b_v1], success=False, force_send=True, reject_reason='bad-version(0x00000001)', reconnect=True)
1369 
1370-        self.move_tip(chain1_tip + 2)
1371+        if not self.options.skip_reorg:
1372+            self.move_tip(chain1_tip + 2)
1373+        else:
    maflcko commented at 2:35 pm on July 17, 2025:
    i wonder if the small test cases can be run before the reorg, to avoid having to branch on the move_tip
    brunoerg commented at 7:20 pm on July 17, 2025:
    Yes, it’s possible. I just addressed it.
  4. DrahtBot commented at 2:35 pm on July 17, 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/33003.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko, enirox001, theStack, glozow

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
    • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
    • #10102 (Multiprocess bitcoin by ryanofsky)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  5. maflcko added the label Tests on Jul 17, 2025
  6. test: add option to skip large re-org test in feature_block 8810642b57
  7. brunoerg force-pushed on Jul 17, 2025
  8. brunoerg commented at 7:20 pm on July 17, 2025: contributor
    Force-pushed addressing #33003 (review)
  9. maflcko commented at 8:41 am on July 18, 2025: member

    review ACK 8810642b571e1d8482375e962a1129b691d5d226 🥁

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 8810642b571e1d8482375e962a1129b691d5d226 🥁
3DpyLlo3oKv/19B8tyRDZvdShApH95O1ZOgFaYz2g8sYDmOkLXTmdio0voYe7Sj7BJ1uLuGQwSnEQdU/9TxjwBQ==
  10. fanquake commented at 9:57 am on July 26, 2025: member
    Looking at corecheck, there are currently 3 other functional tests that run slower than feature_block.py (p2p_segwit.py, p2p_opportunistic_1p1c.py & mining_getblocktemplate_longpoll.py). p2p_opportunistic_1p1c.py should be better after #33048.
  11. brunoerg commented at 9:22 pm on July 26, 2025: contributor

    Looking at corecheck, there are currently 3 other functional tests that run slower than feature_block.py (p2p_segwit.py, p2p_opportunistic_1p1c.py & mining_getblocktemplate_longpoll.py). p2p_opportunistic_1p1c.py should be better after #33048.

    Sure, I’m taking a look at these other ones as well. I’m addressing feature_block for now because it’s used a lot on the mutation analysis.

  12. enirox001 commented at 2:22 pm on July 28, 2025: contributor
    tACK 8810642 – Ran tests with/without –skipreorg; saw ~40 % speedup; no regressions.
  13. luke-jr commented at 4:56 am on July 29, 2025: member
    Seems like it would be better to split it into two, so the quicker part can be run by test_runner every time (without running it twice when the long reorg test is also desired)
  14. naiyoma commented at 9:08 pm on August 23, 2025: contributor
    Tested, and --skipreorg successfully reduces test runtime from 60s to 35s. I’m not very sure about this approach — splitting was also suggested here #16613 (comment) I think it’s a better long-term solution, but could be done in a follow-up.
  15. brunoerg commented at 5:41 pm on October 15, 2025: contributor

    Tested, and --skipreorg successfully reduces test runtime from 60s to 35s.

    I’m not very sure about this approach — splitting was also suggested here #16613 (comment) I think it’s a better long-term solution, but could be done in a follow-up.

    I agree on that for the long-term, I could address on a follow-up and keep this as a simple solution for now.

  16. maflcko commented at 6:26 pm on October 15, 2025: member
    I am not sure about splitting this test. I think there is a benefit to run a reorg on a “dirty” state, so if the reorg test is run, we’d want to fully run it. If the reorg should be skipped, there is the option added in this pull.
  17. brunoerg commented at 7:04 pm on October 15, 2025: contributor

    I am not sure about splitting this test. I think there is a benefit to run a reorg on a “dirty” state, so if the reorg test is run, we’d want to fully run it. If the reorg should be skipped, there is the option added in this pull.

    Make sense. Given that running a reorg on a “dirty” state is useful, this flag is the best option. Otherwise we would have 2 tests doing similar things since we would have to reproduce a dirty state for the reorg anyway.

  18. in test/functional/feature_block.py:1299 in 8810642b57 
    1338         self.send_blocks([b_cb34], success=False, reject_reason='bad-cb-height', reconnect=True)
1339 
1340+        # Don't use v2transport for the large reorg, which is too slow with the unoptimized python ChaCha20 implementation
1341+        if self.options.v2transport:
1342+            self.nodes[0].disconnect_p2ps()
1343+            self.helper_peer = self.nodes[0].add_p2p_connection(P2PDataStore(), supports_v2_p2p=False)
    theStack commented at 4:35 pm on November 4, 2025:
    nit: this part could be moved inside the if-block below as well, as there is no need to disable v2transport if the re-org is skipped (though it currently doesn’t matter much, as re-orging is the last part of the functional tests anyway)
  19. theStack approved
  20. theStack commented at 4:41 pm on November 4, 2025: contributor
    Concept and code-review ACK 8810642b571e1d8482375e962a1129b691d5d226
  21. glozow commented at 2:53 pm on November 12, 2025: member
    lgtm ACK 8810642b571e1d8482375e962a1129b691d5d226
  22. glozow merged this on Nov 12, 2025
  23. glozow closed this on Nov 12, 2025


brunoerg maflcko DrahtBot fanquake enirox001 luke-jr naiyoma theStack glozow

Labels
Tests

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-12-08 21:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me