rpc, index: txospenderindex improve formatting, docs and test coverage

fjahr commented at 12:58 pm on February 20, 2026: contributor

This addresses my own comments in the last review of #24539: #24539#pullrequestreview-3829110465

The first commit fixes three small formatting errors.

The second commit adds some missing coverage in feature_init and refactors the code a bit as well so these misses don’t happen so easily in the future.

The third commit is by furzy:

TxoSpenderIndex::FindSpender returns an Expected<optional> but the two levels of the return type were undocumented, making it unclear what a returned nullopt means. So added doc clarifying each return case.

DrahtBot commented at 12:58 pm on February 20, 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	furszy, sedited, rkrux

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

fanquake commented at 1:07 pm on February 20, 2026: member

https://github.com/bitcoin/bitcoin/actions/runs/22225024019/job/64289906351?pr=34635#step:8:650:

0./test/txospenderindex_tests.cpp(13): Entering test suite "txospenderindex_tests"
1./test/txospenderindex_tests.cpp(15): Entering test case "txospenderindex_initial_sync"
2./test/txospenderindex_tests.cpp(60): fatal error: in "txospenderindex_tests/txospenderindex_initial_sync": critical check tx_spender->has_value() has failed
3./test/txospenderindex_tests.cpp(15): Leaving test case "txospenderindex_initial_sync"; testing time: 255711us
4./test/txospenderindex_tests.cpp(13): Leaving test suite "txospenderindex_tests"; testing time: 255742us

in src/rpc/mempool.cpp:956 in dd9bc08f0b

955-                const UniValue& input;
956-                UniValue output;
957+            auto make_output = [](const COutPoint& prevout) {
958+                UniValue o{UniValue::VOBJ};
959+                o.pushKV("txid", prevout.hash.GetHex());
960+                o.pushKV("vout", (int)prevout.n);

maflcko commented at 1:32 pm on February 20, 2026:

Please no new c-style casts in rpc code, when they are not needed and confusing.

fjahr commented at 1:33 pm on March 5, 2026:

This part of the change is dropped here now.

DrahtBot added the label CI failed on Feb 20, 2026

fanquake commented at 2:39 pm on February 20, 2026: member

https://github.com/bitcoin/bitcoin/actions/runs/22225024019/job/64289906351?pr=34635#step:8:650:

Put this failure into #34637, because it’s also happening on master (initially assumed it was only here).

in src/rpc/mempool.cpp:1 in dd9bc08f0b outdated

hodlinator commented at 2:40 pm on February 20, 2026:

I like the first commit, but not so sure about the second one:

Removes struct but adds lambda
Converts UniValue -> COutpoint -> UniValue, whereas master only does UniValue -> COutpoint
Adds 3 extra lines, complicating the mempool loop a lot, which simplifies the final loop, but not to much/any gain?
Edit: Adds and an additional vector and early return
Edit: [~] May change order of inputs vs result (but elements are identified by txid and vout)
Edit: Reduces word count by ~29!
Edit: Removes txospenderindex_ready. ✔️

fjahr commented at 11:06 pm on February 20, 2026:

Sure, as I said if nobody is excited about the refactoring I will drop it, but let me respond with my reasoning for the changes:

Removes struct but adds lambda

The struct only seemed to have the purpose to transport the data between the two loops, this isn’t necessary with the new structure so it seemed to make more sense to remove it even though that didn’t save any LOCs. I find the lambda is simpler to reason about because it doesn’t carry any state.

Converts UniValue -> COutpoint -> UniValue, whereas master only does UniValue -> COutpoint

I would rather describe master as “UniValue → COutPoint + UniValue (wrapped in struct) → UniValue (unwrapped)” :)

It just seems more straight forward to me to handle the simpler std::vector<COutPoint>. The COutPoint already contains all the information needed to reconstruct the output object (txid + vout) so I just wouldn’t make a struct when it isn’t needed and I rely on existing, well-known types without too much hassle.

Adds 3 extra lines, complicating the mempool loop a lot, which simplifies the final loop, but not to much/any gain?

I still think that the new code is easier to reason about and if you want to quantify it I ran both RPC bodies through a word counter, the one I used says old code: 251, new code: 222, e.g. -10%. So while there are more lines, there is just less going on per line and the mental load to parse the code is still lower overall IMO.

But as I have said in the PR description already it isn’t a game changer, more of a style preference. I think there is a tangible benefit from this style that stuff like unnecessary checks/duplicate logic like this: #24539 (review) can be easier to spot and avoided. That’s why I prefer it :)

If nobody else shows up here over the weekend who is excited about the proposed changes then I will drop them (or even earlier if someone says they decidedly prefer that I drop them).

hodlinator commented at 7:38 pm on February 23, 2026:

On master we never convert COutpoint back to UniValue, it seems like more work than copying input UniValue -> output UniValue.

Added to my previous post:

On top of the lambda your change adds an extra index_prevouts-vector and adds an early return
Yours may change the order between the input list and output list while master does not.
The word count has something going for it. I wonder what the guiding principle behind that is.
Removal of txospenderindex_ready.

I agree that it’s clearer to get rid of txospenderindex_ready and to move the exception out of the loop. Here is a version on top of the first commit that does this while still being able to point out a missing prevout. It also makes the optional wrapped in Expected stick out a bit more at the end. What do you think?

 0--- a/src/rpc/mempool.cpp
 1+++ b/src/rpc/mempool.cpp
 2@@ -976,7 +976,7 @@ static RPCHelpMan gettxspendingprevout()
 3             }
 4 
 5             // search the mempool first
 6-            bool missing_from_mempool{false};
 7+            const COutPoint* missing_from_mempool{nullptr};
 8             {
 9                 const CTxMemPool& mempool = EnsureAnyMemPool(request.context);
10                 LOCK(mempool.cs);
11@@ -990,14 +990,18 @@ static RPCHelpMan gettxspendingprevout()
12                         }
13                         entry.output = std::move(o);
14                     } else {
15-                        missing_from_mempool = true;
16+                        missing_from_mempool = &entry.prevout;
17                     }
18                 }
19             }
20+
21             // if search is not limited to the mempool and no spender was found for an outpoint, search the txospenderindex
22             // we call g_txospenderindex->BlockUntilSyncedToCurrentChain() only if g_txospenderindex is going to be used
23+            if (missing_from_mempool && !mempool_only && (!g_txospenderindex || !g_txospenderindex->BlockUntilSyncedToCurrentChain())) {
24+                throw JSONRPCError(RPC_MISC_ERROR, strprintf("No spending tx for the outpoint %s:%d in mempool, and txospenderindex is unavailable.", missing_from_mempool->hash.GetHex(), missing_from_mempool->n));
25+            }
26+
27             UniValue result{UniValue::VARR};
28-            bool txospenderindex_ready{mempool_only || !missing_from_mempool || (g_txospenderindex && g_txospenderindex->BlockUntilSyncedToCurrentChain())};
29             for (auto& entry : prevouts) {
30                 if (!entry.output.isNull()) {
31                     result.push_back(std::move(entry.output));
32@@ -1006,15 +1010,13 @@ static RPCHelpMan gettxspendingprevout()
33                 UniValue o{entry.input};
34                 if (mempool_only) {
35                     // do nothing, caller has selected to only query the mempool
36-                } else if (!txospenderindex_ready) {
37-                    throw JSONRPCError(RPC_MISC_ERROR, strprintf("No spending tx for the outpoint %s:%d in mempool, and txospenderindex is unavailable.", entry.prevout.hash.GetHex(), entry.prevout.n));
38                 } else {
39                     // no spending tx in mempool, query txospender index
40                     const auto spender{g_txospenderindex->FindSpender(entry.prevout)};
41                     if (!spender) {
42                         throw JSONRPCError(RPC_MISC_ERROR, spender.error());
43                     }
44-                    if (spender.value()) {
45+                    if (spender.value().has_value()) {
46                         o.pushKV("spendingtxid", spender.value()->tx->GetHash().GetHex());
47                         o.pushKV("blockhash", spender.value()->block_hash.GetHex());
48                         if (return_spending_tx) {

fjahr commented at 12:57 pm on March 5, 2026:

I think it’s not that much of an improvement mostly because I don’t like to report a specific “offender” when there could be multiples actually, I would call this a very mild version of shadowing information. In this case there may be multiple outpoints for which the spender is missing but we report one as the issue. Maybe it would be best to return a list the more I think about it. This shouldn’t waste any resources since we have iterated over everything already at this point. And even if instead we want to just break as early as possible and report the first missing outpoint we could move the line with the throw directly into the loop where missing_from_mempool is assigned in your suggestion and get rid of missing_from_mempool entirely that way. It would make the remove more code, let the RPC return faster and should result in the same behavior afaict. (I guess I really would like to get rid of these bools ;) )

But what actually icks me even a bit more here is that in this particular case the user most likely has either tried to use the index by changing the default from mempool only or the index isn’t ready yet. So since this is the more likely source of issue we should point them at fixing the index rather then pointing at a particular outpoint which seems to be a distraction from the real issue.

I actually hadn’t thought about this particular part in this depth yet and I am not dealing with this in an ideal way in my refactored version too. So I will just pull the refactoring in it’s own PR now and we can continue the discussion there if you are not tired of it yet ;)

hodlinator commented at 2:32 pm on March 5, 2026:

Experimented with listing all offenders.

 0--- a/src/rpc/mempool.cpp
 1+++ b/src/rpc/mempool.cpp
 2@@ -976,7 +976,7 @@ static RPCHelpMan gettxspendingprevout()
 3             }
 4 
 5             // search the mempool first
 6-            bool missing_from_mempool{false};
 7+            UniValue missing_from_mempool{UniValue::VARR};
 8             {
 9                 const CTxMemPool& mempool = EnsureAnyMemPool(request.context);
10                 LOCK(mempool.cs);
11@@ -990,14 +990,17 @@ static RPCHelpMan gettxspendingprevout()
12                         }
13                         entry.output = std::move(o);
14                     } else {
15-                        missing_from_mempool = true;
16+                        missing_from_mempool.push_back(entry.input);
17                     }
18                 }
19             }
20             // if search is not limited to the mempool and no spender was found for an outpoint, search the txospenderindex
21             // we call g_txospenderindex->BlockUntilSyncedToCurrentChain() only if g_txospenderindex is going to be used
22+            if (!missing_from_mempool.empty() && !mempool_only && (!g_txospenderindex || !g_txospenderindex->BlockUntilSyncedToCurrentChain())) {
23+                throw JSONRPCError(RPC_MISC_ERROR, strprintf("No spending tx for the outpoints %s in mempool, and txospenderindex is unavailable.", missing_from_mempool.write()));
24+            }
25+
26             UniValue result{UniValue::VARR};
27-            bool txospenderindex_ready{!missing_from_mempool || (g_txospenderindex && g_txospenderindex->BlockUntilSyncedToCurrentChain())};
28             for (auto& entry : prevouts) {
29                 if (!entry.output.isNull()) {
30                     result.push_back(std::move(entry.output));
31@@ -1006,8 +1009,6 @@ static RPCHelpMan gettxspendingprevout()
32                 UniValue o{entry.input};
33                 if (mempool_only) {
34                     // do nothing, caller has selected to only query the mempool
35-                } else if (!txospenderindex_ready) {
36-                    throw JSONRPCError(RPC_MISC_ERROR, strprintf("No spending tx for the outpoint %s:%d in mempool, and txospenderindex is unavailable.", entry.prevout.hash.GetHex(), entry.prevout.n));
37                 } else {
38                     // no spending tx in mempool, query txospender index
39                     const auto spender{g_txospenderindex->FindSpender(entry.prevout)};

0₿ ./build/bin/bitcoin-cli -regtest gettxspendingprevout "[{\"txid\":\"1111111111111111111111111111111111111111111111111111111111111111\", \"vout\":0}, {\"txid\":\"2222222222222222222222222222222222222222222222222222222222222222\", \"vout\":0}]" {\"mempool_only\":false}
1error code: -1
2error message:
3No spending tx for the outpoints [{"txid":"1111111111111111111111111111111111111111111111111111111111111111","vout":0},{"txid":"2222222222222222222222222222222222222222222222222222222222222222","vout":0}] in mempool, and txospenderindex is unavailable.

But yeah, the main issue as you say is that they are trying a mempool_only=false query without also properly enabling the TxoSpenderIndex, or that the index is just not ready yet.

sedited commented at 7:58 pm on February 23, 2026: contributor

I briefly glanced through the changes landed in #29770 and I think we should add the index in feature_init.py:

0diff --git a/test/functional/feature_init.py b/test/functional/feature_init.py
1index 733f518852..dda0980d3f 100755
2--- a/test/functional/feature_init.py
3+++ b/test/functional/feature_init.py
4@@ -316 +316 @@ class InitTest(BitcoinTestFramework):
5-            ["-txindex=1", "-blockfilterindex=1", "-coinstatsindex=1"],
6+            ["-txindex=1", "-blockfilterindex=1", "-coinstatsindex=1", "-txospenderindex=1"],

There might be other such cases from that PR that I may have overlooked.

DrahtBot removed the label CI failed on Mar 3, 2026

index, rpc, test: Misc formatting fixes a1074d852a

fjahr force-pushed on Mar 5, 2026

DrahtBot added the label CI failed on Mar 5, 2026

fjahr commented at 12:56 pm on March 5, 2026: contributor

Sorry for the long wait, I had to be afk for a few days for unexpected reasons.

Addressed feedback and rebased to be able to accomodate the #29770 changes to the init test. As I kind of expected the extend of the refactor is kind of controversial so I will just pull it into it’s own PR where there may or may not be enough appetite to push some version of it through.

There might be other such cases from that PR that I may have overlooked.

Good find, yeah, not related to the changes in #29770 but there were was some additional coverage missing from feature_init which I have added now in the last commit.

in src/rpc/mempool.cpp:1000 in 6cecc09b56

 996@@ -997,7 +997,7 @@ static RPCHelpMan gettxspendingprevout()
 997             // if search is not limited to the mempool and no spender was found for an outpoint, search the txospenderindex
 998             // we call g_txospenderindex->BlockUntilSyncedToCurrentChain() only if g_txospenderindex is going to be used
 999             UniValue result{UniValue::VARR};
1000-            bool txospenderindex_ready{mempool_only || !missing_from_mempool || (g_txospenderindex && g_txospenderindex->BlockUntilSyncedToCurrentChain())};

hodlinator commented at 1:58 pm on March 5, 2026:

The user may have turned on the TxoSpenderIndex, but then calls this RPC, passing mempool_only=true. In that case this check on master helps short-circuit the expression when an outpoint is not found in the mempool, so we avoid calling BlockUntilSyncedToCurrentChain(). Removing this short-circuiting is a step backwards IMO.

fjahr commented at 11:50 pm on March 5, 2026:

I have dropped the commit, these kinds of changes can be discussed further in #34748 and/or #34749

DrahtBot removed the label CI failed on Mar 5, 2026

ekzyis commented at 4:00 pm on March 5, 2026: none

nit: should this patch be part of the first commit?

 0diff --git a/src/init.cpp b/src/init.cpp
 1index adc1dacc75..bed53f98df 100644
 2--- a/src/init.cpp
 3+++ b/src/init.cpp
 4@@ -999,10 +999,12 @@ bool AppInitParameterInteraction(const ArgsManager& args)
 5     }
 6
 7     if (args.GetIntArg("-prune", 0)) {
 8-        if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX))
 9+        if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
10             return InitError(_("Prune mode is incompatible with -txindex."));
11-        if (args.GetBoolArg("-txospenderindex", DEFAULT_TXOSPENDERINDEX))
12+        }
13+        if (args.GetBoolArg("-txospenderindex", DEFAULT_TXOSPENDERINDEX)) {
14             return InitError(_("Prune mode is incompatible with -txospenderindex."));
15+        }
16         if (args.GetBoolArg("-reindex-chainstate", false)) {
17             return InitError(_("Prune mode is incompatible with -reindex-chainstate. Use full -reindex instead."));
18         }

https://github.com/bitcoin/bitcoin/blob/fc39a4f5686fce42fd846583e3f6ba6d39b64ea1/doc/developer-notes.md?plain=1#L25-L28

hodlinator commented at 4:13 pm on March 5, 2026: contributor

Reviewed 6cecc09b56b481bf4838c75c7e2025ce0c40e85d

test: Add missing txospenderindex coverage in feature_init

Also refactors the list of all index args into a constant that can be reused across tests.

Co-authored-by: sedited <seb.kung@gmail.com>

f8b9595aaa

fjahr force-pushed on Mar 5, 2026

fjahr renamed this:
~~rpc, index: txospenderindex follow-ups~~
rpc, index: txospenderindex formatting and test coverage follow-ups
on Mar 5, 2026

fjahr commented at 11:56 pm on March 5, 2026: contributor

Dropped the second commit so this PR now only focusses on the formatting fixes and the feature_init improvements.

nit: should this patch be part of the first commit?

Maybe if I have to retouch but these style issues are a bit less severe than what the other things the first commit fixes. So I would usually only do this if these lines are touched anyway.

index: document TxoSpenderIndex::FindSpender

Hard to know what a returned std::Expected(std::nullopt) mean
if it is not documented anywhere.

15c4889497

DrahtBot added the label CI failed on Mar 6, 2026

fjahr commented at 0:07 am on March 6, 2026: contributor

Also included the first commit from #34748 with documentation improvements by @furszy . This should get the less controversial follow-ups out of the way and we can focus on the refactoring of the RPC elsewhere.

fjahr renamed this:
~~rpc, index: txospenderindex formatting and test coverage follow-ups~~
rpc, index: txospenderindex improve formatting, docs and test coverage
on Mar 6, 2026

DrahtBot removed the label CI failed on Mar 6, 2026

furszy commented at 2:17 am on March 6, 2026: member

ACK 15c4889497b96037e41019a8f43090af841b36ec

sedited approved

sedited commented at 7:25 am on March 6, 2026: contributor

ACK 15c4889497b96037e41019a8f43090af841b36ec

rkrux approved

rkrux commented at 9:30 am on March 6, 2026: contributor

crACK 15c4889497b96037e41019a8f43090af841b36ec

fanquake merged this on Mar 6, 2026

fanquake closed this on Mar 6, 2026

rpc, index: txospenderindex improve formatting, docs and test coverage #34635

Reviews