test: Implicitly sync after generate* to preempt races and intermittent test failures #20362

The most frequent failure in functional tests are intermittent races. Fixing such bugs is cumbersome because it involves:

• Noticing the failure
• Fetching and reading the log to determine the test case that failed
• Adding a self.sync_all() where it was forgotten
• Spamming out a pr and waiting for review, which is already sparse

Also, writing a linter to catch those is not possible, nor is review effective in finding these bugs prior to merge.

Fix all future intermittent races caused by a missing sync_block call by calling sync_all implicitly after each generate*, unless opted out. This ensures that the code is race-free (with regards to blocks) when the tests pass once, instead of our current approach where the code can never be guaranteed to be race-free.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #22567 (test: Implicitly sync after generate* to preempt races and intermittent test failures by MarcoFalke)
• #22437 (test, refactor: add GetTransaction() coverage, improve rpc_rawtransaction by jonatack)
• #22364 (wallet: Make a tr() descriptor by default by achow101)
• #22229 (test: consolidate to f-strings (part 1) by fanquake)
• #21283 (Implement BIP 370 PSBTv2 by achow101)
• #10102 ([experimental] 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.

Concept ACK: thanks for doing this - intermittent test failures are the worst!

Bike shed nit: fn or func might be more unambiguous (but less fun) abbreviations than fun :)

Concept ACK.

How about for now:

try_dispatch_with_sync
    try
        dispatch
    except
        sync_all
        dispatch
        log with print_stack

Concept ACK!

very nice, thank you

Concept ACK, rooting out a cause of intermittent issues (due to test flakiness) is great.

Nice! utACK faeef7a61307b99654a445dffe05b4aed561639d

Rebased. Should be trivial to re-ACK

Code review ACK, needs rebase.

Rebased. Should be trivial to re-ACK, as most conflicts were in the scripted diff.

Concept ACK

What do you think about adding a generate() method to TestFramework that takes the index of the node that you want to generate on? It seems strange to pass a TestFramework function to the constructor of the TestNode.

That would require changing all calls to generate* and also disable TestNode::generate* to avoid races creeping in by accident. Happy to do that refactor if reviewers think it is worth it. I am mostly interested in the functional changes here.

Happy to do that refactor if reviewers think it is worth it.

I think it's worth it, to avoid adding an implicit dependency from TestNode onto TestFramework.

Done in #22567. (Different pr, because the discussion here wasn't substantial, mostly GitHubs own spam, and I wanted to keep this version in case someone decides against #22567).

<!--cf906140f33d8803c4a75a2196329ecb-->

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}