test: Remove confusing assert_debug_log in wallet

maflcko commented at 7:03 am on March 19, 2026: member

The wallet_reindex.py test has many issues:

It uses a assert_debug_log to ensure the reindex happened via initload thread exit. However, no other test uses this pattern, because restart_node already achieves the same internally (via getmempoolinfo.loaded).
The assert_debug_log may intermittently fail, because the initload thread exit log may happen at any time after the inner thread function (lambda) is fully done.
The test uses an node.syncwithvalidationinterfacequeue() without any explanation. No other test uses this pattern, because all wallet RPCs, in particular the next one (gettransaction) call it internally (via BlockUntilSyncedToCurrentChain).

Fix all issues by removing all confusing, useless, and brittle calls.

Example failure: https://github.com/bitcoin/bitcoin/actions/runs/22671604602/job/65716917459?pr=34419#step:11:22339

 0...
 1 node0 2026-03-04T13:55:23.784715Z (mocktime: 2026-03-04T22:15:17Z) [scheduler] [validationinterface.cpp:185] [operator()] [validation] UpdatedBlockTip: new block hash=24b5cc4c1352bc8841ecbe67a43827acd1adc609bd26b2691e80e89b97eff135 fork block hash=24a4dc8be9c157ac31913397d8bb1653900e12b55539c234039559fdeb6dd2fb (in IBD=true) 
 2 node0 2026-03-04T13:55:23.784851Z (mocktime: 2026-03-04T22:15:17Z) [initload] [util/thread.cpp:22] [TraceThread] initload thread exit 
 3 test  2026-03-04T13:55:23.813824Z TestFramework (ERROR): Unexpected exception: 
 4                                   Traceback (most recent call last):
 5                                     File "/home/admin/actions-runner/_work/_temp/test/functional/test_framework/test_framework.py", line 142, in main
 6                                       self.run_test()
 7                                     File "/home/admin/actions-runner/_work/_temp/build/test/functional/wallet_reindex.py", line 90, in run_test
 8                                       self.birthtime_test(node, miner_wallet)
 9                                     File "/home/admin/actions-runner/_work/_temp/build/test/functional/wallet_reindex.py", line 64, in birthtime_test
10                                       with node.assert_debug_log(expected_msgs=["initload thread exit"]):
11                                     File "/usr/lib/python3.12/contextlib.py", line 144, in __exit__
12                                       next(self.gen)
13                                     File "/home/admin/actions-runner/_work/_temp/test/functional/test_framework/test_node.py", line 607, in assert_debug_log
14                                       self._raise_assertion_error(f'Expected message(s) {remaining_expected!s} '
15                                     File "/home/admin/actions-runner/_work/_temp/test/functional/test_framework/test_node.py", line 241, in _raise_assertion_error
16                                       raise AssertionError(self._node_msg(msg))
17                                   AssertionError: [node 0] Expected message(s) ['initload thread exit'] not found in log:

Diff to reproduce failure:

 0diff --git a/src/util/thread.cpp b/src/util/thread.cpp
 1index 0fde73c4e2..de4c05e752 100644
 2--- a/src/util/thread.cpp
 3+++ b/src/util/thread.cpp
 4@@ -8,2 +8,3 @@
 5 #include <util/log.h>
 6+#include <util/time.h>
 7 #include <util/threadnames.h>
 8@@ -21,2 +22,3 @@ void util::TraceThread(std::string_view thread_name, std::function<void()> threa
 9         thread_func();
10+    UninterruptibleSleep(999ms);
11         LogInfo("%s thread exit", thread_name);
12diff --git a/test/functional/wallet_reindex.py b/test/functional/wallet_reindex.py
13index 71ab69e01b..649df4301a 100755
14--- a/test/functional/wallet_reindex.py
15+++ b/test/functional/wallet_reindex.py
16@@ -62,2 +62,3 @@ class WalletReindexTest(BitcoinTestFramework):
17 
18+        import time; time.sleep(1) # Wait for previous initload thread to exit fully
19         # Reindex and wait for it to finish

Before on master: Test fails After on this pull: Test passes

test: Remove confusing assert_debug_log in wallet_reindex.py fa30951af5

DrahtBot added the label Tests on Mar 19, 2026

DrahtBot commented at 7:04 am on March 19, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34879 (test, wallet: overhaul wallet_reindex test to wallet_birthtime by rkrux)

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.

DrahtBot added the label CI failed on Mar 19, 2026

DrahtBot removed the label CI failed on Mar 19, 2026

maflcko commented at 7:45 am on March 20, 2026: member

cc @furszy As the author of the code you may be qualified to nack/ack this?

in test/functional/wallet_reindex.py:64 in fa30951af5

59@@ -60,10 +60,8 @@ def birthtime_test(self, node, miner_wallet):
60         assert_equal(wallet_watch_only.gettransaction(tx_id)['confirmations'], 50)
61         assert_equal(wallet_watch_only.getbalances()['mine']['trusted'], 2)
62 
63-        # Reindex and wait for it to finish
64-        with node.assert_debug_log(expected_msgs=["initload thread exit"]):
65-            self.restart_node(0, extra_args=['-reindex=1', f'-mocktime={self.node_time}'])
66-        node.syncwithvalidationinterfacequeue()
67+        self.log.info("Reindex ...")  # restart_node waits for it to finish
68+        self.restart_node(0, extra_args=[ f'-mocktime={self.node_time}'])

rkrux commented at 9:56 am on March 20, 2026:

This suggestion is not exactly tied to the motivation of this PR but it is being mentioned because it’s related to the test.

rescanblockchain already updates the birthtime of the wallet and the restart is not necessary. The following diff passes:

 0         # Rescan the wallet to detect the missing transaction
 1         wallet_watch_only.rescanblockchain()
 2-        assert_equal(wallet_watch_only.gettransaction(tx_id)['confirmations'], 50)
 3-        assert_equal(wallet_watch_only.getbalances()['mine']['trusted'], 2)
 4-
 5-        self.log.info("Reindex ...")  # restart_node waits for it to finish
 6-        self.restart_node(0, extra_args=[ f'-mocktime={self.node_time}'])
 7-
 8-        # Verify the transaction is still 'confirmed' after reindex
 9-        wallet_watch_only = node.get_wallet_rpc('watch_only')
10+        # Verify the transaction is 'confirmed'
11         tx_info = wallet_watch_only.gettransaction(tx_id)
12         assert_equal(tx_info['confirmations'], 50)
13+        assert_equal(wallet_watch_only.getbalances()['mine']['trusted'], 2)
14 
15-        # Depending on the wallet type, the birth time changes.
16-        # For descriptors, verify the wallet updated the birth time to the transaction time
17+        # Verify the wallet updated the birth time to the transaction time
18         assert_equal(tx_info['time'], wallet_watch_only.getwalletinfo()['birthtime'])

As the user may have imported a descriptor with a timestamp newer than the actual birth time of the first key (by setting ’timestamp=now’), the wallet needs to update the birth time when it detects a transaction older than the oldest descriptor timestamp.

Based on the motivation of the original PR (#28920), rescanblockchain already achieves this. Maybe there was some legacy wallet anngle for which the original test (https://github.com/bitcoin/bitcoin/pull/28920/changes/83c66444d0604f0a9ec3bc3f89d4f1a810b7cda0) restarted the node originally but it doesn’t seem required anymore.

This suggestion puts into question the existence of this test file as well.

maflcko commented at 10:29 am on March 20, 2026:

Good point. I haven’t looked deeply here myself. Would you mind creating an alternative pull request to remove all the code?

This pull request here should be mechanically correct, because it is a minimal diff to avoid intermittent test failures. So I think it may be easier to review (and backport, if needed).

However, a larger restructure or removal of the test may be more involved to review. No strong opinion. Anything should be fine here, as long as the intermittent failure is correctly fixed.

rkrux commented at 11:09 am on March 20, 2026:

Yeah, I will raise a separate pull. I’ve noticed some other changes here as well that could be done.

rkrux commented at 12:57 pm on March 20, 2026:

I have raised it here #34879.

rkrux commented at 9:58 am on March 20, 2026: contributor

Concept ACK fa30951 for the overall improvement in the test.

rkrux approved

rkrux commented at 10:52 am on March 23, 2026: contributor

ACK fa30951af5b174a1ee5e1c23d84b115c542a9570 if this PR needs to be backported.

These changes are smaller, to-the-point, and easier to review compared to the test revamp PR I have raised separately.

fanquake merged this on Mar 24, 2026

fanquake closed this on Mar 24, 2026

fanquake referenced this in commit e3d571691a on Mar 24, 2026

fanquake commented at 4:08 am on March 24, 2026: member

Backported to 31.x in #34800.

maflcko deleted the branch on Mar 24, 2026

test: Remove confusing assert_debug_log in wallet_reindex.py #34857

Reviews

Conflicts