This refactoring commit was originally part of #34635 but the the change was some-what controversial: #34635 (review) and the discussion was blocking the rest of the uncontroversial changes from moving forward.
It looks like #34748 is trying to achieve something similar now, we’ll have to see how the different approached converge but at least there seems to be some more conceptual interest than appeared on #34635 originally.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Possible typos and grammar issues:
2026-03-06 21:51:49
As discussed in #34748, I took another pass at this. Have combined the best of both PRs: your structural changes with my extended docs, plus added a bit of extra magic on top. I think the end result is quite nice. Please see branch (commit b69ec5517c9d6c697c3aff688dddd7f9f7321eb0).
If you like it, can directly replace the commit here.
Co-authored-by: Matias Furszyfer <matiasfurszyfer@protonmail.com>
If you like it, can directly replace the commit here.
Thanks, I have taken it with only minor edits, primarily fixing a remaining cast that @maflcko already complained about in the earlier PR.
Many of the changes like one vec with erase vs two vecs and letting the lambda do more were approaches that I also considered and while I went the other way I was almost indifferent between them, so I really didn’t have anything to complain. I hope other reviewers see the value in this as well.
1002- }
1003- entry.output = std::move(o);
1004- } else {
1005- missing_from_mempool = true;
1006+
1007+ // Make the result if spending tx appear in the mempool or this is a mempool_only request
s/appear/appears
1005- missing_from_mempool = true;
1006+
1007+ // Make the result if spending tx appear in the mempool or this is a mempool_only request
1008+ for (auto it = prevouts_to_process.begin(); it != prevouts_to_process.end(); ) {
1009+ const COutPoint& prevout{*it};
1010+ const CTransaction* spendingTx{mempool.GetConflictTx(prevout)};
nit: snake_case
0 const CTransaction* spending_tx{mempool.GetConflictTx(prevout)};
1072+ continue;
1073 }
1074+
1075+ UniValue o{make_output(prevout, op_info->tx.get(), return_spending_tx)};
1076+ o.pushKV("blockhash", op_info->block_hash.GetHex());
1077 result.push_back(std::move(o));
nits:
0 if (const auto& spender_opt{spender.value()}) {
1 UniValue o{make_output(prevout, spender_opt->tx.get(), return_spending_tx)};
2 o.pushKV("blockhash", spender_opt->block_hash.GetHex());
3 result.push_back(std::move(o));
4 } else {
5 // Return the input outpoint itself, which indicates it is unspent.
6 result.push_back(make_output(prevout));
7 }
Concept ~0.
An attempt of peeling off PRs until the change gets in, based on subjective style preference.
COutPoint -> UniValue conversion step as mentioned in previous PR.Agree with removing bool txospenderindex_ready.
(I tried a variant setting result = output_params and then mutating to see if that yeilded something interesting, but UniValue only has const operator[]-accessors).
An attempt of peeling off PRs until the change gets in, based on subjective style preference.
This sounds like I am trying to make this change appear more that it is and sneak it in with brute force and I don’t think that is fair. I have been calling it a style preference myself in the PR description from the first time I opened the original PR and I only peeled it so the uncontroversial part of the PR could go through quicker and the discussion on refactor could be more focussed without blocking it. The code also was changed again significantly with the input from @furszy which would have triggered re-reviews and extended discussions further. The original PR couldn’t have received a Concept NACK from a reviewer if they had only disagreed with the refactor but not the other changes. Now this PR is easy to NACK if that’s how people feel so the pull can be closed quick and cleanly one way or another and I certainly wouldn’t include the commit anywhere a third time.
EDIT: Will address the nits if I have to push again.
Concept ACK fb4ad3931813e82837b84d2407345098bc0301c7, I’m reviewing atm.
Couple things that I like seeing updated here:
I didn’t see a rush to get #34635 merged without resolving the removal of the txospenderindex_ready bool.
Guess it’s rare for me to disagree with pure refactoring PRs as a whole, usually other reviewers get them rejected first, but we can’t all agree on everything and I don’t disagree enough to N-A-C-K this one.
949@@ -950,14 +950,9 @@ static RPCHelpMan gettxspendingprevout()
950 const bool mempool_only{options.exists("mempool_only") ? options["mempool_only"].get_bool() : !g_txospenderindex};
951 const bool return_spending_tx{options.exists("return_spending_tx") ? options["return_spending_tx"].get_bool() : false};
This variable is unchanged after this line, can capture it in make_output and avoid passing the third variable there.
0diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
1index 69366eed8a..6ae6a071f6 100644
2--- a/src/rpc/mempool.cpp
3+++ b/src/rpc/mempool.cpp
4@@ -970,13 +970,13 @@ static RPCHelpMan gettxspendingprevout()
5 prevouts_to_process.emplace_back(txid, static_cast<uint32_t>(nOutput));
6 }
7
8- auto make_output = [](const COutPoint& prevout, const CTransaction* spending_tx = nullptr, const bool add_spending_tx_hex = false) {
9+ auto make_output = [return_spending_tx](const COutPoint& prevout, const CTransaction* spending_tx = nullptr) {
10 UniValue o{UniValue::VOBJ};
11 o.pushKV("txid", prevout.hash.GetHex());
12 o.pushKV("vout", static_cast<int>(prevout.n));
13 if (spending_tx) {
14 o.pushKV("spendingtxid", spending_tx->GetHash().ToString());
15- if (add_spending_tx_hex) {
16+ if (return_spending_tx) {
17 o.pushKV("spendingtx", EncodeHexTx(*spending_tx));
18 }
19 }
20@@ -1002,7 +1002,7 @@ static RPCHelpMan gettxspendingprevout()
21 continue;
22 }
23
24- result.push_back(make_output(prevout, spendingTx, return_spending_tx));
25+ result.push_back(make_output(prevout, spendingTx));
26 it = prevouts_to_process.erase(it);
27 }
28 }
29@@ -1032,7 +1032,7 @@ static RPCHelpMan gettxspendingprevout()
30 continue;
31 }
32
33- UniValue o{make_output(prevout, op_info->tx.get(), return_spending_tx)};
34+ UniValue o{make_output(prevout, op_info->tx.get())};
35 o.pushKV("blockhash", op_info->block_hash.GetHex());
36 result.push_back(std::move(o));
37 }
1030- continue;
1031+
1032+ // Return early if all requests have been handled by the mempool search
1033+ if (prevouts_to_process.empty()) {
1034+ return result;
1035+ }
I like the search separation between in the mempool and in the index. I’m wondering why don’t we return here when the request passes mempool_only so that the index search is not done later.
0diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
1index 69366eed8a..bf322150eb 100644
2--- a/src/rpc/mempool.cpp
3+++ b/src/rpc/mempool.cpp
4@@ -1008,7 +1008,7 @@ static RPCHelpMan gettxspendingprevout()
5 }
6
7 // Return early if all requests have been handled by the mempool search
8- if (prevouts_to_process.empty()) {
9+ if (mempool_only || prevouts_to_process.empty()) {
10 return result;
11 }
12
mempool_only = true, prevouts_to_process will always be empty by the end of the first loop.