test: Misc fixes for pending validation events races #36447

maflcko commented at 4:01 PM on October 6, 2026: member

TestChain100Setup enqueues 100 validation events from blocks. Unit tests using that setup and their own validation events may run into non-determinism and even intermittent test failures.

Fix all issues by draining the pending validation events in the ctor.

This can be tested with a diff like:

diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp
index 128f14a6d5..2ea3ec8d4c 100644
--- a/src/validationinterface.cpp
+++ b/src/validationinterface.cpp
@@ -14,2 +14,3 @@
 #include <primitives/transaction.h>
+#include <random.h>
 #include <util/check.h>
@@ -156,2 +157,4 @@ void ValidationSignals::SyncWithValidationInterfaceQueue()
 
+static FastRandomContext g_rnd{};
+
 // Use a macro instead of a function for conditional logging to prevent
@@ -168,2 +171,3 @@ void ValidationSignals::SyncWithValidationInterfaceQueue()
             LOG_EVENT("%s", local_log_msg);                                                                      \
+            UninterruptibleSleep(1ms * g_rnd.randrange(105));   \
             local_event();                                                                                       \

and then running the test scan_for_wallet_transactions_attach_chain a few times.

Or master (or the first commit), it should fail with: error: scan_for_wallet_transactions_attach_chain": check wallet->mapWallet.size() == static_cast<size_t>(NEW_BLOCKS + 1) has failed [7 != 6].

(Also included is a commit to move-only the many rescan tests to a separate file)

(Also included is a commit to fix unsafe index shutdowns in tests)

DrahtBot renamed this:
~~test: Avoid pending validationn events from TestChain100Setup~~
test: Avoid pending validationn events from TestChain100Setup
on Oct 6, 2026

DrahtBot added the label Tests on Oct 6, 2026

DrahtBot commented at 4:01 PM on October 6, 2026: contributor

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

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36447
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36447.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36447#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	sedited, willcl-ark
Concept ACK	StephenChi-hi
Stale ACK	ismaelsadeeq

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36444 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36444.svg"> (wallet: Store information about created and imported multipath descriptors by achow101)
#36443 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36443.svg"> (descriptor: Reconstruct a multipath descriptor from its expansion by achow101)
#36442 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36442.svg"> (descriptor: Represent multipath descriptors in a single Descriptor object by achow101)
#36337 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36337.svg"> (fuzz: Cover block filter P2P messages by maflcko)
#35474 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35474.svg"> (node: move index ownership to NodeContext by w0xlt)
#24230 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/24230.svg"> (indexes: Stop using node internal types and locking cs_main, improve sync logic 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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

src/wallet/test/rescan_tests.cpp: the one of the recorded best block → the coinbase of the recorded best block [the original phrasing is unclear]

2026-10-07 10:42:25

maflcko renamed this:
~~test: Avoid pending validationn events from TestChain100Setup~~
test: Avoid pending validation events from TestChain100Setup
on Oct 6, 2026

DrahtBot renamed this:
~~test: Avoid pending validation events from TestChain100Setup~~
test: Avoid pending validation events from TestChain100Setup
on Oct 6, 2026

maflcko force-pushed on Oct 6, 2026

DrahtBot added the label CI failed on Oct 6, 2026

test: [move-only] wallet rescan tests to separate TU

The general wallet_tests.cpp file was more than 1500 lines long, which
makes it hard to navigate. Extract the 500 lines of rescan test.

Can be reviewed via:
--color-moved=dimmed-zebra

fa21ac647d

maflcko force-pushed on Oct 6, 2026

DrahtBot removed the label CI failed on Oct 6, 2026

sedited approved

sedited commented at 6:41 PM on October 6, 2026: contributor

ACK 9999557b77224be0f6d079af53fa90042bfd01b9

willcl-ark approved

willcl-ark commented at 7:47 PM on October 6, 2026: member

ACK 9999557b77224be0f6d079af53fa90042bfd01b9

Nice!

maflcko commented at 8:05 PM on October 6, 2026: member

Hmm, looks like Ralph Review found another race. To repo:

diff --git a/src/wallet/test/rescan_tests.cpp b/src/wallet/test/rescan_tests.cpp
index 4c5cf76ced..d838e1fc70 100644
--- a/src/wallet/test/rescan_tests.cpp
+++ b/src/wallet/test/rescan_tests.cpp
@@ -29,2 +29,3 @@
 #include <string>
+#include <thread>
 #include <utility>
@@ -567,4 +568,12 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions_attach_chain, TestChain100S
     constexpr int NEW_BLOCKS{5};
+    const auto delayed_blocks_start = std::chrono::steady_clock::now();
     for (int i = 0; i < NEW_BLOCKS; ++i) {
+        if (i >= 3) {
+            m_node.validation_signals->CallFunctionInValidationInterfaceQueue(
+                [] { std::this_thread::sleep_for(std::chrono::milliseconds{500}); });
+        }
         CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
+        if (i < 3) {
+            m_node.validation_signals->SyncWithValidationInterfaceQueue();
+        }
     }
@@ -582,4 +591,9 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions_attach_chain, TestChain100S
     wallet = TestLoadWallet(context);
+    BOOST_TEST_MESSAGE("Wallet load returned " << std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::steady_clock::now() - delayed_blocks_start).count() << " ms after starting delayed blocks");
+
+    // Allow some delayed callbacks to run before checking the wallet tip.
+    std::this_thread::sleep_for(std::chrono::milliseconds{500});
     {
         LOCK(wallet->cs_wallet);
+        BOOST_TEST_MESSAGE("Assertions at " << std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::steady_clock::now() - delayed_blocks_start).count() << " ms, wallet height " << wallet->GetLastBlockHeight());
         BOOST_CHECK_EQUAL(wallet->GetLastBlockHeight(), tip_height);

Let me push a fix for that as well ...

maflcko commented at 8:34 PM on October 6, 2026: member

ok, now the CI is failing because there is another UB race :(

Let's fix that here as well, I guess ...

maflcko renamed this:
~~test: Avoid pending validation events from TestChain100Setup~~
test: Misc fixes for pending validation events races
on Oct 6, 2026

DrahtBot added the label CI failed on Oct 6, 2026

DrahtBot commented at 8:55 PM on October 6, 2026: contributor

🚧 At least one of the CI tasks failed. Task TSan: https://github.com/bitcoin/bitcoin/actions/runs/37524113100/job/112476629653 LLM reason (✨ experimental): CI failed because ThreadSanitizer detected a data race on a vptr in BaseIndex::~BaseIndex() during coinstatsindex_tests.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot removed the label CI failed on Oct 6, 2026

in src/test/util/setup_common.cpp:436 in 9999557b77

 430 | @@ -431,6 +431,10 @@ TestChain100Setup::TestChain100Setup(
 431 |      // Generate a 100-block chain:
 432 |      this->mineBlocks(COINBASE_MATURITY);
 433 |  
 434 | +    // Make the fixture deterministic. Tests requiring validation callbacks
 435 | +    // should explicitly create them.
 436 | +    if (m_node.validation_signals) m_node.validation_signals->SyncWithValidationInterfaceQueue();

ismaelsadeeq commented at 5:38 AM on October 7, 2026:

Perhaps it may be better to just drain in mineBlock? That way other callers that will mine a block get the notification emitted for free, without them having to drain themselves?

maflcko commented at 6:37 AM on October 7, 2026:

I was thinking that it could be useful coverage for the unit tests to be async here. But then, I don't know if they have ever found an issue here. Of course, we still need the option to be async, but maybe it is fine to sync by default.

I'll push that ...

in src/wallet/test/rescan_tests.cpp:574 in fa0df16886

 568 | @@ -569,6 +569,10 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions_attach_chain, TestChain100S
 569 |          CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
 570 |      }
 571 |  
 572 | +    // Drain BlockConnected callbacks before loading the wallet, so stale
 573 | +    // notifications will not update its tip during test checks.
 574 | +    m_node.chain->waitForNotifications();

ismaelsadeeq commented at 5:40 AM on October 7, 2026:

Same comment here: can we move the drain to CreateAndProcessBlock?

ismaelsadeeq approved

ismaelsadeeq commented at 5:41 AM on October 7, 2026: member

Code review ACK fa77f27f98211167d9dfeba8933e6866df5fce8b

Minor comments below

DrahtBot requested review from willcl-ark on Oct 7, 2026

DrahtBot requested review from sedited on Oct 7, 2026

maflcko force-pushed on Oct 7, 2026

ismaelsadeeq approved

ismaelsadeeq commented at 8:11 AM on October 7, 2026: member

reACK fa10ab5fdbb8ca3fede08dd7c93c7359a5e25433

in src/test/txindex_tests.cpp:174 in facf6a5ee3 outdated

 170 | @@ -170,8 +171,7 @@ BOOST_FIXTURE_TEST_CASE(txindex_initial_sync, TestChain100Setup)
 171 |          LookupTx(txindex, txn.GetHash());
 172 |      }
 173 |  
 174 | -    // shutdown sequence (c.f. Shutdown() in init.cpp)
 175 | -    txindex.Stop();
 176 | +    StopIndex(txindex, m_node);

sedited commented at 10:04 AM on October 7, 2026:

What about the other cases of txindex.Stop() in this file? I guess most don't produce more blocks, but the last test for example does, so maybe we should also sync there?

maflcko commented at 10:43 AM on October 7, 2026:

ah, I only grepped for the comment. Fixed the others as well now.

DrahtBot requested review from sedited on Oct 7, 2026

test: Drain all validation events before stopping indexes

No test relies on pending validation events, and it would be unsafe
anyway and lead to intermittent tsan error reports.

fa339713db

test: Avoid pending validation events from TestChain100Setup

Drain events in mineBlock and CreateAndProcessBlock by default to avoid
intermittnet test failures in scan_for_wallet_transactions_attach_chain
and other tests.

fa466dea80

maflcko force-pushed on Oct 7, 2026

sedited approved

sedited commented at 11:13 AM on October 7, 2026: contributor

ACK fa466dea808dedfc281cdff842cbefafae574d8a

DrahtBot requested review from ismaelsadeeq on Oct 7, 2026

willcl-ark commented at 11:27 AM on October 7, 2026: member

reACK fa466dea808dedfc281cdff842cbefafae574d8a

StephenChi-hi commented at 12:03 PM on October 7, 2026: none

tACK

I agree, this will sure help to prevent flaky test failures. built and verified changes, all 862 unit test cases in test_bitcoin passed cleanly.

sedited merged this on Oct 7, 2026

sedited closed this on Oct 7, 2026

maflcko deleted the branch on Oct 7, 2026