Merge bitcoin#23300: test: Implicitly sync after generate*, unless opted out

facc352 test: Implicitly sync after generate*, unless opted out (MarcoFalke)

Pull request description:

  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.

  There are some scripted-diff cleanups (see bitcoin#22567), but they will be submitted in a follow-up to reduce the conflicts in this pull.

ACKs for top commit:
  lsilva01:
    tACK facc352 on Ubuntu 20.04
  brunoerg:
    tACK facc352 on MacOS 11.6

Tree-SHA512: 046a40a066b4a3bd28a3077bd654fa8887442dd1f0ec6fd11671865809ef02376f126eb667a1320ebd67b6e372c78c00dbf8bd25d86ed86f1d9a25363103ed97

Author: MarcoFalke

test/functional/example_test.py

@@ -141,8 +141,7 @@ def run_test(self):
         peer_messaging = self.nodes[0].add_p2p_connection(BaseNode())
 
         # Generating a block on one of the nodes will get us out of IBD
-        blocks = [int(self.generate(self.nodes[0], nblocks=1)[0], 16)]
-        self.sync_all(self.nodes[0:2])
+        blocks = [int(self.generate(self.nodes[0], sync_fun=lambda: self.sync_all(self.nodes[0:2]), nblocks=1)[0], 16)]
 
         # Notice above how we called an RPC by calling a method with the same
         # name on the node object. Notice also how we used a keyword argument

test/functional/feature_abortnode.py

@@ -26,18 +26,18 @@ def setup_network(self):
         # We'll connect the nodes later
 
     def run_test(self):
-        self.generate(self.nodes[0], 3)
+        self.generate(self.nodes[0], 3, sync_fun=self.no_op)
         datadir = get_datadir_path(self.options.tmpdir, 0)
 
         # Deleting the undo file will result in reorg failure
         os.unlink(os.path.join(datadir, self.chain, 'blocks', 'rev00000.dat'))
 
         # Connecting to a node with a more work chain will trigger a reorg
         # attempt.
-        self.generate(self.nodes[1], 3)
+        self.generate(self.nodes[1], 3, sync_fun=self.no_op)
         with self.nodes[0].assert_debug_log(["Failed to disconnect block"]):
             self.connect_nodes(0, 1)
-            self.generate(self.nodes[1], 1)
+            self.generate(self.nodes[1], 1, sync_fun=self.no_op)
 
             # Check that node0 aborted
             self.log.info("Waiting for crash")

test/functional/feature_bip68_sequence.py

@@ -275,7 +275,7 @@ def test_nonzero_locks(orig_tx, node, relayfee, use_height_lock):
         cur_time = int(time.time())
         for _ in range(10):
             self.nodes[0].setmocktime(cur_time + 600)
-            self.generate(self.nodes[0], 1)
+            self.generate(self.nodes[0], 1, sync_fun=self.no_op)
             cur_time += 600
 
         assert tx2.hash in self.nodes[0].getrawmempool()
@@ -351,7 +351,7 @@ def test_nonzero_locks(orig_tx, node, relayfee, use_height_lock):
         # Reset the chain and get rid of the mocktimed-blocks
         self.nodes[0].setmocktime(0)
         self.nodes[0].invalidateblock(self.nodes[0].getblockhash(cur_height+1))
-        self.generate(self.nodes[0], 10)
+        self.generate(self.nodes[0], 10, sync_fun=self.no_op)
 
     # Make sure that BIP68 isn't being used to validate blocks prior to
     # activation height.  If more blocks are mined prior to this test
@@ -405,9 +405,9 @@ def activateCSV(self):
         min_activation_height = 432
         height = self.nodes[0].getblockcount()
         assert_greater_than(min_activation_height - height, 2)
-        self.generate(self.nodes[0], min_activation_height - height - 2)
+        self.generate(self.nodes[0], min_activation_height - height - 2, sync_fun=self.no_op)
         assert not softfork_active(self.nodes[0], 'csv')
-        self.generate(self.nodes[0], 1)
+        self.generate(self.nodes[0], 1, sync_fun=self.no_op)
         assert softfork_active(self.nodes[0], 'csv')
         self.sync_blocks()
 

test/functional/feature_coinstatsindex.py

@@ -228,7 +228,7 @@ def _test_coin_stats_index(self):
         res9 = index_node.gettxoutsetinfo('muhash')
         assert_equal(res8, res9)
 
-        self.generate(index_node, 1)
+        self.generate(index_node, 1, sync_fun=self.no_op)
         res10 = index_node.gettxoutsetinfo('muhash')
         assert(res8['txouts'] < res10['txouts'])
 
@@ -254,7 +254,7 @@ def _test_reorg_index(self):
         assert_equal(index_node.gettxoutsetinfo('muhash')['height'], 110)
 
         # Add two new blocks
-        block = self.generate(index_node, 2)[1]
+        block = self.generate(index_node, 2, sync_fun=self.no_op)[1]
         res = index_node.gettxoutsetinfo(hash_type='muhash', hash_or_height=None, use_index=False)
 
         # Test that the result of the reorged block is not returned for its old block height

test/functional/feature_dbcrash.py

@@ -217,7 +217,7 @@ def run_test(self):
 
         # Start by creating a lot of utxos on node3
         initial_height = self.nodes[3].getblockcount()
-        utxo_list = create_confirmed_utxos(self, self.nodes[3].getnetworkinfo()['relayfee'], self.nodes[3], 5000)
+        utxo_list = create_confirmed_utxos(self, self.nodes[3].getnetworkinfo()['relayfee'], self.nodes[3], 5000, sync_fun=self.no_op)
         self.log.info(f"Prepped {len(utxo_list)} utxo entries")
 
         # Sync these blocks with the other nodes
@@ -258,6 +258,7 @@ def run_test(self):
                     nblocks=min(10, current_height + 1 - self.nodes[3].getblockcount()),
                     # new address to avoid mining a block that has just been invalidated
                     address=self.nodes[3].getnewaddress(),
+                    sync_fun=self.no_op,
                 ))
             self.log.debug(f"Syncing {len(block_hashes)} new blocks...")
             self.sync_node3blocks(block_hashes)

test/functional/feature_fee_estimation.py

@@ -237,7 +237,7 @@ def initial_split(self, node):
 
         # Mine
         while len(node.getrawmempool()) > 0:
-            self.generate(node, 1)
+            self.generate(node, 1, sync_fun=self.no_op)
 
         # Repeatedly split those 2 outputs, doubling twice for each rep
         # Use txouts to monitor the available utxo, since these won't be tracked in wallet
@@ -247,12 +247,12 @@ def initial_split(self, node):
             while len(self.txouts) > 0:
                 split_inputs(node, self.txouts, self.txouts2)
             while len(node.getrawmempool()) > 0:
-                self.generate(node, 1)
+                self.generate(node, 1, sync_fun=self.no_op)
             # Double txouts2 to txouts
             while len(self.txouts2) > 0:
                 split_inputs(node, self.txouts2, self.txouts)
             while len(node.getrawmempool()) > 0:
-                self.generate(node, 1)
+                self.generate(node, 1, sync_fun=self.no_op)
             reps += 1
 
     def sanity_check_estimates_range(self):

test/functional/feature_loadblock.py

@@ -29,7 +29,7 @@ def set_test_params(self):
 
     def run_test(self):
         self.nodes[1].setnetworkactive(state=False)
-        self.generate(self.nodes[0], COINBASE_MATURITY)
+        self.generate(self.nodes[0], COINBASE_MATURITY, sync_fun=self.no_op)
 
         # Parsing the url of our node to get settings for config file
         data_dir = self.nodes[0].datadir

test/functional/feature_minchainwork.py

@@ -51,8 +51,7 @@ def run_test(self):
 
         num_blocks_to_generate = int((self.node_min_work[1] - starting_chain_work) / REGTEST_WORK_PER_BLOCK)
         self.log.info(f"Generating {num_blocks_to_generate} blocks on node0")
-        hashes = self.generatetoaddress(self.nodes[0], num_blocks_to_generate,
-                                                 self.nodes[0].get_deterministic_priv_key().address)
+        hashes = self.generate(self.nodes[0], num_blocks_to_generate, sync_fun=self.no_op)
 
         self.log.info(f"Node0 current chain work: {self.nodes[0].getblockheader(hashes[-1])['chainwork']}")
 
@@ -73,7 +72,7 @@ def run_test(self):
         assert_equal(self.nodes[2].getblockcount(), starting_blockcount)
 
         self.log.info("Generating one more block")
-        self.generatetoaddress(self.nodes[0], 1, self.nodes[0].get_deterministic_priv_key().address)
+        self.generate(self.nodes[0], 1)
 
         self.log.info("Verifying nodes are all synced")
 

test/functional/feature_notifications.py

@@ -149,7 +149,7 @@ def run_test(self):
             # about newly confirmed bump2 and newly conflicted tx2.
             self.disconnect_nodes(0, 1)
             bump2 = self.nodes[0].bumpfee(tx2)["txid"]
-            blockhash2 = self.generatetoaddress(self.nodes[0], 1, ADDRESS_BCRT1_UNSPENDABLE)[0]
+            blockhash2 = self.generatetoaddress(self.nodes[0], 1, ADDRESS_BCRT1_UNSPENDABLE, sync_fun=self.no_op)[0]
             blockheight2 = self.nodes[0].getblockcount()
             assert_equal(self.nodes[0].gettransaction(bump2)["confirmations"], 1)
             assert_equal(tx2 in self.nodes[1].getrawmempool(), True)

test/functional/feature_pruning.py

@@ -118,9 +118,8 @@ def setup_nodes(self):
 
     def create_big_chain(self):
         # Start by creating some coinbases we can spend later
-        self.generate(self.nodes[1], 200)
-        self.sync_blocks(self.nodes[0:2])
-        self.generate(self.nodes[0], 150)
+        self.generate(self.nodes[1], 200, sync_fun=lambda: self.sync_blocks(self.nodes[0:2]))
+        self.generate(self.nodes[0], 150, sync_fun=self.no_op)
 
         # Then mine enough full blocks to create more than 550MiB of data
         mine_large_blocks(self.nodes[0], 645)
@@ -211,7 +210,7 @@ def reorg_test(self):
         self.disconnect_nodes(1, 2)
 
         self.log.info("Generating new longer chain of 300 more blocks")
-        self.generate(self.nodes[1], 300)
+        self.generate(self.nodes[1], 300, sync_fun=self.no_op)
 
         self.log.info("Reconnect nodes")
         self.connect_nodes(0, 1)
@@ -263,7 +262,7 @@ def reorg_back(self):
             self.nodes[0].invalidateblock(curchainhash)
             assert_equal(self.nodes[0].getblockcount(), self.mainchainheight)
             assert_equal(self.nodes[0].getbestblockhash(), self.mainchainhash2)
-            goalbesthash = self.generate(self.nodes[0], blocks_to_mine)[-1]
+            goalbesthash = self.generate(self.nodes[0], blocks_to_mine, sync_fun=self.no_op)[-1]
             goalbestheight = first_reorg_height + 1
 
         self.log.info("Verify node 2 reorged back to the main chain, some blocks of which it had to redownload")
@@ -306,7 +305,7 @@ def has_block(index):
         assert_equal(block1_details["nTx"], len(block1_details["tx"]))
 
         # mine 6 blocks so we are at height 1001 (i.e., above PruneAfterHeight)
-        self.generate(node, 6)
+        self.generate(node, 6, sync_fun=self.no_op)
         assert_equal(node.getblockchaininfo()["blocks"], 1001)
 
         # Pruned block should still know the number of transactions
@@ -337,7 +336,7 @@ def has_block(index):
         assert has_block(2), "blk00002.dat is still there, should be pruned by now"
 
         # advance the tip so blk00002.dat and blk00003.dat can be pruned (the last 288 blocks should now be in blk00004.dat)
-        self.generate(node, 288)
+        self.generate(node, 288, sync_fun=self.no_op)
         prune(1000)
         assert not has_block(2), "blk00002.dat is still there, should be pruned by now"
         assert not has_block(3), "blk00003.dat is still there, should be pruned by now"