test: improve txospender index tests code

furszy commented at 11:55 pm on February 22, 2026: member

Was reviewing #34637 and, while reading the new txospender index test code for the first time, found it could use some cleanups. Finding stuff in there is harder than it should be due to the amount of dup code.

The first commit cleans up rpc_gettxspendingprevout.py by introducing helper functions to avoid repeating the same dicts everywhere, using for-loops instead of duplicating the same checks for each node, and renaming variables to better reflect what they actually represent.

The second commit reorganizes txospenderindex_initial_sync() moving index initialization after the test setup phase, since the index doesn’t participate in it anyway. It adds a post-sync check to catch cases where Sync() aborted prematurely.

Note: This is just a pre-work for deeper index changes I’m cooking.

DrahtBot added the label Tests on Feb 22, 2026

DrahtBot commented at 11:55 pm on February 22, 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	sedited, w0xlt, achow101

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

LLM Linter (✨ experimental)

Possible typos and grammar issues:

// Now we concluded the setup phase, run index -> // Now that we have concluded the setup phase, run the index [grammatical error: “Now we concluded …” is ungrammatical/awkward; “Now that we have concluded…” or “After concluding…” fixes it and clarifies the imperative that follows]

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

SignatureHash(coinbase_script, spender[i], 0, SIGHASH_ALL, 0, SigVersion::BASE) in src/test/txospenderindex_tests.cpp

2026-02-24 14:58:26

DrahtBot added the label CI failed on Feb 23, 2026

furszy commented at 3:15 am on February 23, 2026: member

All CI failures are unrelated.

in test/functional/rpc_gettxspendingprevout.py:154 in 35c839f55d

168+
169+        # replace tx1 with tx2 triggering a "reorg"
170+        best_block_hash = self.nodes[0].getbestblockhash()
171+        for node in self.nodes:
172+            node.invalidateblock(best_block_hash)
173+            # The tx1 must back to the mempool

sedited commented at 10:03 am on February 23, 2026:

Nit: There is a typo, but I would just remove the comment.

furszy commented at 2:28 pm on February 23, 2026:

Done as suggested.

sedited approved

sedited commented at 10:35 am on February 23, 2026: contributor

Thanks,

ACK 999bd266ed69580322626a8bead09b4f4c79d6e4

DrahtBot removed the label CI failed on Feb 23, 2026

sedited added this to the milestone 31.0 on Feb 23, 2026

test: improve rpc_gettxspendingprevout.py code

Grouped changes to improve the overall readability and maintainability of the test.
A lot more can be done, but this is a good first step.

1) Use for-loops instead of duplicating lines to perform the same checks for each
   node.

2) The {'txid': x, 'vout': y} dict is repeated everywhere in the test, both as
   input to gettxspendingprevout and as part of its result when an output has no
   known spender, making the test tedious to read and maintain.

   This introduces a prevout(txid, vout) query helper and an unspent_out(txid, vout)
   result helper to reduce the repetition. These two helpers are intentionally kept
   separate to make it immediately clear whether a dict is an input to
   gettxspendingprevout or an assertion on its result.

3) The same repetition problem mentioned above applies to other gettxspendingprevout
   possible results:
   Spent outputs returns {'txid': x, 'vout': y, 'spendingtxid': z} and
   Spent outputs when requesting spending tx returns {'txid': x, 'vout': y,
   'spendingtxid': z, 'blockhash': w, 'spendingtx': v}

   To fix it, this introduces:
   - spent_out(txid, vout, spending_tx_id): for outputs with a known spender
   - spent_out_in_block(txid, vout, spending_tx_id, blockhash, spending_tx): for
     outputs spent in a confirmed block, when full tx data is requested

4) Rename overloaded confirmed_utxo variable (used in three different tests) to more
   descriptive names: root_utxo, reorg_replace_utxo, reorg_cancel_utxo to clarify
   their roles in each of the tests.

ac3bea07cd

furszy force-pushed on Feb 23, 2026

furszy commented at 2:31 pm on February 23, 2026: member

Updated per feedback. Thanks @sedited.

sedited approved

sedited commented at 3:21 pm on February 23, 2026: contributor

Re-ACK 3de8832a299210de16c131bd25605b77c8709bef

Changes include de-structing nodes for conciseness and removing a comment.

sedited requested review from fjahr on Feb 23, 2026

sedited requested review from hodlinator on Feb 23, 2026

achow101 commented at 11:07 pm on February 23, 2026: member

ACK 3de8832a299210de16c131bd25605b77c8709bef

w0xlt commented at 4:15 am on February 24, 2026: contributor

ACK 3de8832a299210de16c131bd25605b77c8709bef

in src/test/txospenderindex_tests.cpp:55 in 3de8832a29

65 
66-    // Transaction should not be found in the index before it is started.
67+    // Now we concluded the setup phase, run index
68+    TxoSpenderIndex txospenderindex(interfaces::MakeChain(m_node), 1 << 20, true);
69+    BOOST_REQUIRE(txospenderindex.Init());
70+    BOOST_CHECK(!txospenderindex.BlockUntilSyncedToCurrentChain());

hodlinator commented at 9:42 am on February 24, 2026:

nit: Would bring back variant of old comment to explain check, and maybe add an early check to demonstrate index not being at the tip:

0    // BlockUntilSyncedToCurrentChain should return false before index has been synced.
1    BOOST_CHECK(!txospenderindex.BlockUntilSyncedToCurrentChain());
2    BOOST_CHECK_NE(txospenderindex.GetSummary().best_block_hash, tip_hash);

furszy commented at 2:51 pm on February 24, 2026:

The check doesn’t add much value. The original author likely just copied it from another index test. It was added in the indexes early days to access the synced flag, but we now have GetSummary(), which retrieves it directly.

I kept it just to stay consistent with the other index tests, but I wouldn’t spend much time on it anymore, the method is just bad and misleading. I’m about to push some updates for it, cleaning up most of this stuff soon #34637 (comment) (see my branch there).

furszy commented at 2:58 pm on February 24, 2026:

Done as suggested

in src/test/txospenderindex_tests.cpp:53 in 3de8832a29 outdated

63+    uint256 tip_hash = block.GetHash(); // Ensure block has been processed
64+    BOOST_CHECK_EQUAL(WITH_LOCK(::cs_main, return m_node.chainman->ActiveTip()->GetBlockHash()), tip_hash);
65 
66-    // Transaction should not be found in the index before it is started.
67+    // Now we concluded the setup phase, run index
68+    TxoSpenderIndex txospenderindex(interfaces::MakeChain(m_node), 1 << 20, true);

hodlinator commented at 9:55 am on February 24, 2026:

nit: Could use _MiB literal and name literal arguments:

0    TxoSpenderIndex txospenderindex(m_node.chain, /*n_cache_size=*/1_MiB, /*f_memory=*/true);

furszy commented at 2:53 pm on February 24, 2026:

This was not modified here. If we are going to do this, we should tackle it in all index tests at once.

in src/test/txospenderindex_tests.cpp:20 in 3de8832a29

22-    for (int i = 0; i < 50; i++) {
23-        std::vector<CMutableTransaction> no_txns;
24-        CreateAndProcessBlock(no_txns, this->m_coinbase_txns[i]->vout[0].scriptPubKey);
25-    }
26+    const CScript& coinbase_script = m_coinbase_txns[0]->vout[0].scriptPubKey;
27+    for (int i = 0; i < 50; i++) CreateAndProcessBlock({}, coinbase_script);

hodlinator commented at 9:58 am on February 24, 2026:

nit: Only need to mine 10 to make the first 10 coinbase_txns usable below:

0    for (int i = 0; i < 10; i++) CreateAndProcessBlock({}, coinbase_script);

furszy commented at 2:38 pm on February 24, 2026:

This was not modified but sure.

in src/test/txospenderindex_tests.cpp:70 in 3de8832a29

69@@ -62,15 +70,6 @@ BOOST_FIXTURE_TEST_CASE(txospenderindex_initial_sync, TestChain100Setup)
70         BOOST_CHECK_EQUAL((*tx_spender)->block_hash, block.GetHash());

hodlinator commented at 10:02 am on February 24, 2026:

nit:

0        BOOST_CHECK_EQUAL((*tx_spender)->block_hash, tip_hash);

furszy commented at 2:57 pm on February 24, 2026:

Done as suggested

in src/test/txospenderindex_tests.cpp:49 in 3de8832a29

58     }
59 
60-    CBlock block = CreateAndProcessBlock(spender, this->m_coinbase_txns[0]->vout[0].scriptPubKey);
61+    CBlock block = CreateAndProcessBlock(spender, coinbase_script);
62+    m_node.validation_signals->SyncWithValidationInterfaceQueue();
63+    uint256 tip_hash = block.GetHash(); // Ensure block has been processed

hodlinator commented at 10:03 am on February 24, 2026:

nits: No need to name block if we use tip_hash consistently, could make tip_hash const, comment in better location.

0    const uint256 tip_hash{CreateAndProcessBlock(spender, coinbase_script).GetHash()};
1    // Ensure block has been processed
2    m_node.validation_signals->SyncWithValidationInterfaceQueue();

furszy commented at 2:57 pm on February 24, 2026:

Done as suggested

hodlinator commented at 10:18 am on February 24, 2026: contributor

Reviewed version with tip 3de8832a299210de16c131bd25605b77c8709bef

It would much easier to review if the first commit modifying rpc_gettxspendingprevout.py if it was split in two:

Doing in-place substitution of helper functions and nodeX
Introduce loops, introduce txs variable, move create_tx(), avoiding variable reuse (root_utxo, reorg_replace_utxo, reorg_cancel_utxo).

I’m not knowledgeable enough around the semantics of the validation interface queue to fully review the C++ unit test commit. Good to see the scary comment at the bottom removed though. Creating the index later in the test may circumvent issues, my only worry is it could be a case of testing less in order to make the test more robust. But I guess the aspects around syncing indexes should™️ be covered in other tests.

Nothing blocking, but wanted to at least leave a quick review since it was requested.

test: index, improve txospenderindex_initial_sync() test code

The index is now initialized after the setup phase (chain generation
and txs creation), since it doesn't participate on it at all.
This improves readability and splits setup from what we actually
want to check.

This also adds a check after Sync() to verify the index best block hash
matches the tip, so we know it fully synced before checking the
processed data. This will help catching errors as Sync() could have
aborted prematurely.

As a happy side effect, the SyncWithValidationInterfaceQueue() call at
the end of the test is no longer needed and has been removed.

e8f8b74a46

furszy force-pushed on Feb 24, 2026

furszy commented at 2:59 pm on February 24, 2026: member

nits tackled, thanks. Ready to go.

sedited approved

sedited commented at 3:25 pm on February 24, 2026: contributor

Re-ACK e8f8b74a46aa075bf6c74c104fd572cc89d3b53b

DrahtBot requested review from achow101 on Feb 24, 2026

DrahtBot requested review from w0xlt on Feb 24, 2026

w0xlt commented at 5:56 pm on February 24, 2026: contributor

reACK e8f8b74a46aa075bf6c74c104fd572cc89d3b53b

achow101 commented at 7:00 pm on February 24, 2026: member

ACK e8f8b74a46aa075bf6c74c104fd572cc89d3b53b

achow101 merged this on Feb 24, 2026

achow101 closed this on Feb 24, 2026

furszy deleted the branch on Feb 24, 2026

test: improve txospender index tests code #34653

Reviews

LLM Linter (✨ experimental)