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

maflcko · 2020-11-10T14:39:16Z

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.

DrahtBot · 2020-11-10T15:43:22Z

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

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.

practicalswift · 2020-11-10T15:53:03Z

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 :)

promag

Concept ACK.

How about for now:

try_dispatch_with_sync
    try
        dispatch
    except
        sync_all
        dispatch
        log with print_stack

promag · 2020-11-14T10:44:53Z

test/functional/test_framework/test_node.py

@@ -296,9 +296,27 @@ def wait_for_cookie_credentials(self):
            time.sleep(1.0 / poll_per_s)
        self._raise_assertion_error("Unable to retrieve cookie credentials after {}s".format(self.rpc_timeout))

-    def generate(self, nblocks, maxtries=1000000):
+    def generateblock(self, *args, sync_fun=True, **kwargs):


faf4ef6

An alternative to "overload" these generate* is to add a generic _rpc_dispatch_and_sync (__getattr__ -> _rpc_dispatch_and_sync -> _rpc_dispatch).

It would be cool to keep the last generate* extract_stack that when a related error occurs we could print it.

That would make calling code tremendously verbose, no?

Maybe I am misunderstanding. Do you have a working diff that I can steal?

test/functional/test_framework/test_framework.py

amitiuttarwar · 2020-11-15T18:34:04Z

Concept ACK!

very nice, thank you

laanwj · 2020-11-20T03:55:07Z

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

jnewbery · 2021-07-23T14:00:39Z

Concept ACK

jnewbery · 2021-07-26T12:40:28Z

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.

maflcko · 2021-07-26T13:01:06Z

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.

jnewbery · 2021-07-26T15:39:15Z

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.

maflcko · 2021-07-28T09:32:51Z

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

-BEGIN VERIFY SCRIPT- perl -0777 -pi -e 's/(generate[^\n]*\)[^\n]*)(\n|\s)+self.sync_.*\n/\1\n/g' $(git grep -l generate ./test) -END VERIFY SCRIPT-

DrahtBot · 2021-08-18T21:00:44Z

🐙 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".}

maflcko mentioned this pull request Nov 10, 2020

Tracking CI false positive rates #19684

Closed

DrahtBot added the Tests label Nov 10, 2020

This was referenced Nov 10, 2020

wallet: introduce fee_rate sat/vB param/option #20305

Merged

Disable and fix tests for when BDB is not compiled #20267

Merged

maflcko force-pushed the 2011-noSync branch 2 times, most recently from faf0319 to fae7684 Compare November 10, 2020 18:06

maflcko force-pushed the 2011-noSync branch from fae7684 to fa1c978 Compare November 12, 2020 10:16

promag reviewed Nov 14, 2020

View reviewed changes

DrahtBot mentioned this pull request Nov 15, 2020

wallet: introduce setfeerate (an improved settxfee, in sat/vB) #20391

Closed

DrahtBot added the Needs rebase label Nov 16, 2020

maflcko force-pushed the 2011-noSync branch from fa1c978 to fa10d23 Compare November 16, 2020 11:30

DrahtBot added Needs rebase and removed Needs rebase labels Nov 16, 2020

maflcko force-pushed the 2011-noSync branch from fa10d23 to faeb229 Compare November 17, 2020 19:33

DrahtBot removed the Needs rebase label Nov 17, 2020

DrahtBot mentioned this pull request Nov 17, 2020

wallet: Do not treat default constructed types as None-type #20410

Merged

DrahtBot added the Needs rebase label Nov 25, 2020

maflcko force-pushed the 2011-noSync branch from faeb229 to faeef7a Compare November 25, 2020 09:20

DrahtBot removed the Needs rebase label Nov 25, 2020

This was referenced Nov 26, 2020

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Draft

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

maflcko force-pushed the 2011-noSync branch from fa244b1 to fa97430 Compare July 7, 2021 13:23

This was referenced Jul 10, 2021

test: wallet_listtransactions improvements (speedup, cleanup, logging) #22423

Merged

test, refactor: add GetTransaction() coverage, improve rpc_rawtransaction #22437

Merged

maflcko force-pushed the 2011-noSync branch 2 times, most recently from fa7ef91 to faf6073 Compare July 21, 2021 09:26

DrahtBot added the Needs rebase label Jul 22, 2021

mzumsande mentioned this pull request Jul 22, 2021

CI timeout in test/functional/p2p_feefilter.py #22483

Closed

maflcko force-pushed the 2011-noSync branch from faf6073 to fa5fb76 Compare July 23, 2021 07:38

DrahtBot removed the Needs rebase label Jul 23, 2021

maflcko mentioned this pull request Jul 28, 2021

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

Closed

DrahtBot added the Needs rebase label Jul 28, 2021

MarcoFalke added 4 commits July 28, 2021 17:58

test: Pass default_sync_fun to TestNode constructor

fa891eb

test: Create TestNode._rpc_dispatch method

fac2235

test: Implicitly sync after generate*, unless opted out

faa9359

scripted-diff: Remove redundant sync_all

fa77478

-BEGIN VERIFY SCRIPT- perl -0777 -pi -e 's/(generate[^\n]*\)[^\n]*)(\n|\s)+self.sync_.*\n/\1\n/g' $(git grep -l generate ./test) -END VERIFY SCRIPT-

maflcko force-pushed the 2011-noSync branch from fa5fb76 to fa77478 Compare July 28, 2021 16:01

DrahtBot removed the Needs rebase label Jul 28, 2021

DrahtBot mentioned this pull request Aug 18, 2021

test: consolidate to f-strings (part 1) #22229

Merged

DrahtBot added the Needs rebase label Aug 18, 2021

maflcko closed this Aug 19, 2021

maflcko deleted the 2011-noSync branch August 19, 2021 13:37

maflcko mentioned this pull request Nov 8, 2021

ci: Increase --timeout-factor in the native Windows CI task #23463

Closed

maflcko mentioned this pull request Jan 11, 2022

test: MiniWallet: support default from_node for sending/creating txs #24025

Closed

bitcoin locked as resolved and limited conversation to collaborators Aug 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

maflcko commented Nov 10, 2020

DrahtBot commented Nov 10, 2020 •

edited

Loading

practicalswift commented Nov 10, 2020 •

edited

Loading

promag left a comment

promag Nov 14, 2020

maflcko Nov 15, 2020

maflcko Jun 23, 2021

amitiuttarwar commented Nov 15, 2020

laanwj commented Nov 20, 2020 •

edited

Loading

jnewbery commented Jul 23, 2021

jnewbery commented Jul 26, 2021

maflcko commented Jul 26, 2021

jnewbery commented Jul 26, 2021

maflcko commented Jul 28, 2021

DrahtBot commented Aug 18, 2021

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

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

Conversation

maflcko commented Nov 10, 2020

DrahtBot commented Nov 10, 2020 • edited Loading

Conflicts

practicalswift commented Nov 10, 2020 • edited Loading

promag left a comment

Choose a reason for hiding this comment

promag Nov 14, 2020

Choose a reason for hiding this comment

maflcko Nov 15, 2020

Choose a reason for hiding this comment

maflcko Jun 23, 2021

Choose a reason for hiding this comment

amitiuttarwar commented Nov 15, 2020

laanwj commented Nov 20, 2020 • edited Loading

jnewbery commented Jul 23, 2021

jnewbery commented Jul 26, 2021

maflcko commented Jul 26, 2021

jnewbery commented Jul 26, 2021

maflcko commented Jul 28, 2021

DrahtBot commented Aug 18, 2021

DrahtBot commented Nov 10, 2020 •

edited

Loading

practicalswift commented Nov 10, 2020 •

edited

Loading

laanwj commented Nov 20, 2020 •

edited

Loading