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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hodlinator, sedited, furszy |
| Stale ACK | rkrux |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Possible typos and grammar issues:
<sup>2026-03-12 10:22:00</sup>
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.
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.
ACK fb4ad3931813e82837b84d2407345098bc0301c7
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
nit (typo): s/appear/appears
Thanks, will fix if I have to push again!
Forgot about it at first but fixed it too now.
ACK fb4ad3931813e82837b84d2407345098bc0301c7
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
const CTransaction* spending_tx{mempool.GetConflictTx(prevout)};
Done
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:
if (const auto& spender_opt{spender.value()}) {
UniValue o{make_output(prevout, spender_opt->tx.get(), return_spending_tx)};
o.pushKV("blockhash", spender_opt->block_hash.GetHex());
result.push_back(std::move(o));
} else {
// Return the input outpoint itself, which indicates it is unspent.
result.push_back(make_output(prevout));
}
op_info" is a name with little info.
I agree with this point.
Taken, thanks!
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.
diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
index 69366eed8a..6ae6a071f6 100644
--- a/src/rpc/mempool.cpp
+++ b/src/rpc/mempool.cpp
@@ -970,13 +970,13 @@ static RPCHelpMan gettxspendingprevout()
prevouts_to_process.emplace_back(txid, static_cast<uint32_t>(nOutput));
}
- auto make_output = [](const COutPoint& prevout, const CTransaction* spending_tx = nullptr, const bool add_spending_tx_hex = false) {
+ auto make_output = [return_spending_tx](const COutPoint& prevout, const CTransaction* spending_tx = nullptr) {
UniValue o{UniValue::VOBJ};
o.pushKV("txid", prevout.hash.GetHex());
o.pushKV("vout", static_cast<int>(prevout.n));
if (spending_tx) {
o.pushKV("spendingtxid", spending_tx->GetHash().ToString());
- if (add_spending_tx_hex) {
+ if (return_spending_tx) {
o.pushKV("spendingtx", EncodeHexTx(*spending_tx));
}
}
@@ -1002,7 +1002,7 @@ static RPCHelpMan gettxspendingprevout()
continue;
}
- result.push_back(make_output(prevout, spendingTx, return_spending_tx));
+ result.push_back(make_output(prevout, spendingTx));
it = prevouts_to_process.erase(it);
}
}
@@ -1032,7 +1032,7 @@ static RPCHelpMan gettxspendingprevout()
continue;
}
- UniValue o{make_output(prevout, op_info->tx.get(), return_spending_tx)};
+ UniValue o{make_output(prevout, op_info->tx.get())};
o.pushKV("blockhash", op_info->block_hash.GetHex());
result.push_back(std::move(o));
}
Nice, done.
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.
diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
index 69366eed8a..bf322150eb 100644
--- a/src/rpc/mempool.cpp
+++ b/src/rpc/mempool.cpp
@@ -1008,7 +1008,7 @@ static RPCHelpMan gettxspendingprevout()
}
// Return early if all requests have been handled by the mempool search
- if (prevouts_to_process.empty()) {
+ if (mempool_only || prevouts_to_process.empty()) {
return result;
}
When mempool_only = true, prevouts_to_process will always be empty by the end of the first loop.
Ah yes, thanks! I guess I was looking for the explicit condition here at the cost of redundancy to make it more readable. Fine if it's apparent to everyone else.
Compared to master, we still add a COutPoint -> UniValue conversion step as mentioned in previous PR.
I'm not fully understanding what this means. What are you trying to avoid? The per-outpoint UniValue output creation is unavoidable (the input is const and we need to append the info there). In the previous PR we copied the input into the output, now we construct it manually.
Compared to master, we still add a COutPoint -> UniValue conversion step as mentioned in previous PR.
I'm not fully understanding what this means. What are you trying to avoid? The per-outpoint
UniValueoutput creation is unavoidable. In the previous PR we copied the input into the output, now we construct it manually.
My complaint is that we don't need to add code to manually serialize back to UniValue if we just use the original UniValue. I guess what we get in return for this PR is deduplicating the "spendingtxid" and "spendingtx" serialization. But to me lambdas are more complex than structs, so complexity still increases in this PR.
My ideal patch to master would be something like the below, decreasing the line count by 3 instead of adding 12.
--- a/src/rpc/mempool.cpp
+++ b/src/rpc/mempool.cpp
@@ -981,12 +981,11 @@ static RPCHelpMan gettxspendingprevout()
const CTxMemPool& mempool = EnsureAnyMemPool(request.context);
LOCK(mempool.cs);
for (auto& entry : prevouts) {
- const CTransaction* spendingTx = mempool.GetConflictTx(entry.prevout);
- if (spendingTx != nullptr) {
+ if (const CTransaction* spending_tx{mempool.GetConflictTx(entry.prevout)}) {
UniValue o{entry.input};
- o.pushKV("spendingtxid", spendingTx->GetHash().ToString());
+ o.pushKV("spendingtxid", spending_tx->GetHash().ToString());
if (return_spending_tx) {
- o.pushKV("spendingtx", EncodeHexTx(*spendingTx));
+ o.pushKV("spendingtx", EncodeHexTx(*spending_tx));
}
entry.output = std::move(o);
} else {
@@ -996,19 +995,17 @@ static RPCHelpMan gettxspendingprevout()
}
// if search is not limited to the mempool and no spender was found for an outpoint, search the txospenderindex
// we call g_txospenderindex->BlockUntilSyncedToCurrentChain() only if g_txospenderindex is going to be used
+ if (missing_from_mempool && !mempool_only && g_txospenderindex && g_txospenderindex->BlockUntilSyncedToCurrentChain()) {
+ throw JSONRPCError(RPC_MISC_ERROR, "Mempool lacks a relevant spend, and txospenderindex is unavailable.");
+ }
UniValue result{UniValue::VARR};
- bool txospenderindex_ready{mempool_only || !missing_from_mempool || (g_txospenderindex && g_txospenderindex->BlockUntilSyncedToCurrentChain())};
for (auto& entry : prevouts) {
if (!entry.output.isNull()) {
result.push_back(std::move(entry.output));
continue;
}
UniValue o{entry.input};
- if (mempool_only) {
- // do nothing, caller has selected to only query the mempool
- } else if (!txospenderindex_ready) {
- 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));
- } else {
+ if (!mempool_only) {
// no spending tx in mempool, query txospender index
const auto spender{g_txospenderindex->FindSpender(entry.prevout)};
if (!spender) {
My complaint is that we don't need to add code to manually serialize back to UniValue if we just use the original UniValue. I guess what we get in return for this PR is deduplicating the "spendingtxid" and "spendingtx" serialization. But to me lambdas are more complex than structs, so complexity still increases in this PR.
The lambda isn't really the heart of this PR. It's just a tiny helper. It could just as well receive and copy the input through a simplified Entry struct and append the spending info, like it did before. That would avoid re-serializing the outpoint hash (that's the only real difference). Or we could do the input copying part outside and only append the spending info internally.
Personally, I prefer constructing the output manually so we know we only include exactly what we want and nothing else. That also guards us against future modifications to the input. That said, I wouldn't be against using the outpoint hash string from the input instead of re-serializing it.
Still, more broadly, the motivation for these changes (at least from my side) is that the current code takes a while to grasp. I even ended up pushing #34748 without realizing there was another PR proposing related changes. I suspect others will run into the same issue in the future, so it would be good to avoid forcing people through the same learning curve again. I'll probably forget how this works in a week or two..
I do agree that continuing this discussion in the original PR would have been better and might have avoided some miscommunication. But let's move forward from here.
ACK f1f846421cbab38ec62c27ef27cd3ad22593281c
ACK f1f8464
git range-diff fb4ad39...f1f8464
@hodlinator does furszy's comment address your concerns?
<!--meta-tag:bot-skip-->
@furszy, thanks for taking the time to reply.
The lambda isn't really the heart of this PR. It's just a tiny helper.
The same goes for the struct.
avoid re-serializing the outpoint hash (that's the only real difference).
This extra hash serialization exactly is was what prevents me from ACKing this PR. Sorry for not being explicit earlier.
I prefer constructing the output manually so we know we only include exactly what we want and nothing else. That also guards us against future modifications to the input.
Including exactly what we want is already ensured by:
RPCTypeCheckObj(o,
{
{"txid", UniValueType(UniValue::VSTR)},
{"vout", UniValueType(UniValue::VNUM)},
}, /*fAllowNull=*/false, /*fStrict=*/true);
Still, more broadly, the motivation for these changes (at least from my side) is that the current code takes a while to grasp. I even ended up pushing #34748 without realizing there was another PR proposing related changes.
Your #34748 did not remove the struct or introduce unnecessary serialization like this does PR though. I agree the code can be improved in other ways (my suggested diff #34749 (comment)).
Your #34748 did not remove the struct or introduce unnecessary serialization like this does PR though. I agree the code can be improved in other ways (my suggested diff #34749 (comment)).
Isn't your suggested-diff proving exactly the main point of this PR? The condition is inverted.
+ if (missing_from_mempool && !mempool_only && g_txospenderindex && g_txospenderindex->BlockUntilSyncedToCurrentChain()) {
+ throw JSONRPCError(RPC_MISC_ERROR, "Mempool lacks a relevant spend, and txospenderindex is unavailable.");
+ }
Here missing_from_mempool means "there is more work to do", !mempool_only means "we are not restricted only to mempool search" and g_txospenderindex && g_txospenderindex->BlockUntilSyncedToCurrentChain() means index synced.
So your suggestion throws the exception when there is more work to do, we are not restricted to the mempool search, and the index is sync. Which is the opposite we want.
I think we can agree the code is not easy to read and maintain in the current form and even in your patch-set (which I agree that is better than the current form but not enough in my opinion).
Also, I dropped my PR for this one because I found the base structure proposed by @fjahr better. It separates concerns cleanly, to the point of being very easy to describe, which makes maintenance easier. Essentially, the first part resolves what the mempool can answer, returns early if done, then falls through to the index.
Note: All of this doesn't mean we shouldn't improve the hash re-serialization you are concerned about. We can do that as well. I think it is an orthogonal improvement we can make.
Please don't take the following badly, this is merely a general thought I have had for a while.
It feels like we are sometimes applying the same review bar we have for validation or consensus code to other, less critical parts of the codebase, like indexes. And I don't think that is always the right call. Some areas would be better served by being a bit more relaxed and letting the code move forward to a better place, rather than going back and forth for changes that are unlikely to cause any real harm and can get improved in a follow-up.
I mean, areas like the indexes have not been very popular in recent years, and when it takes so much time and effort to get improvements in, it makes maintenance and writing more robust code really tedious for anyone working there (I'm saying this based on experience #26966). At some point this just drives people away from these areas, and that is sad to see.
Thanks for catching the bug in my suggested condition, should be:
if (missing_from_mempool && !mempool_only && !(g_txospenderindex && g_txospenderindex->BlockUntilSyncedToCurrentChain())) {
throw ...
I found the base structure proposed by @fjahr better. Essentially, the first part resolves what the mempool can answer, returns early if done, then falls through to the index.
But it also makes these trades:
That said, I've tried 2 code cognitive complexity tools and both prefer this PR. I'm surely biased, having contributed to what is on master.
All of this doesn't mean we shouldn't improve the hash re-serialization you are concerned about. We can do that as well. I think it is an orthogonal improvement we can make.
The below tested diff takes the PR and avoids re-serialization, restoring that aspect of what exists on master.
<details><summary>Diff</summary>
--- a/src/rpc/mempool.cpp
+++ b/src/rpc/mempool.cpp
@@ -951,7 +951,11 @@ static RPCHelpMan gettxspendingprevout()
const bool return_spending_tx{options.exists("return_spending_tx") ? options["return_spending_tx"].get_bool() : false};
// Worklist of outpoints to resolve
- std::vector<COutPoint> prevouts_to_process;
+ struct Entry {
+ COutPoint outpoint;
+ const UniValue* raw;
+ };
+ std::vector<Entry> prevouts_to_process;
prevouts_to_process.reserve(output_params.size());
for (unsigned int idx = 0; idx < output_params.size(); idx++) {
const UniValue& o = output_params[idx].get_obj();
@@ -967,13 +971,11 @@ static RPCHelpMan gettxspendingprevout()
if (nOutput < 0) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, vout cannot be negative");
}
- prevouts_to_process.emplace_back(txid, static_cast<uint32_t>(nOutput));
+ prevouts_to_process.emplace_back(COutPoint{txid, static_cast<uint32_t>(nOutput)}, &o);
}
- auto make_output = [return_spending_tx](const COutPoint& prevout, const CTransaction* spending_tx = nullptr) {
- UniValue o{UniValue::VOBJ};
- o.pushKV("txid", prevout.hash.GetHex());
- o.pushKV("vout", static_cast<int>(prevout.n));
+ auto make_output = [return_spending_tx](const Entry& prevout, const CTransaction* spending_tx = nullptr) {
+ UniValue o{*prevout.raw};
if (spending_tx) {
o.pushKV("spendingtxid", spending_tx->GetHash().ToString());
if (return_spending_tx) {
@@ -992,8 +994,7 @@ static RPCHelpMan gettxspendingprevout()
// Make the result if the spending tx appears in the mempool or this is a mempool_only request
for (auto it = prevouts_to_process.begin(); it != prevouts_to_process.end(); ) {
- const COutPoint& prevout{*it};
- const CTransaction* spending_tx{mempool.GetConflictTx(prevout)};
+ const CTransaction* spending_tx{mempool.GetConflictTx(it->outpoint)};
// If the outpoint is not spent in the mempool and this is not a mempool-only
// request, we cannot answer it yet.
@@ -1002,7 +1003,7 @@ static RPCHelpMan gettxspendingprevout()
continue;
}
- result.push_back(make_output(prevout, spending_tx));
+ result.push_back(make_output(*it, spending_tx));
it = prevouts_to_process.erase(it);
}
}
@@ -1019,7 +1020,7 @@ static RPCHelpMan gettxspendingprevout()
}
for (const auto& prevout : prevouts_to_process) {
- const auto spender{g_txospenderindex->FindSpender(prevout)};
+ const auto spender{g_txospenderindex->FindSpender(prevout.outpoint)};
if (!spender) {
throw JSONRPCError(RPC_MISC_ERROR, spender.error());
}
</details>
Edit: Reverted unintentional removal of static_cast.
As @fjahr has pointed out, this PR has been about style preference from the beginning. It's natural that refactoring PRs that are approachable and fairly easy to reason about attract attention. They should ideally have their own small dedicated time which does not bleed into other work. I can imagine it being frustrating when working on more substantial things in this domain. I was going to sit this one out earlier and not comment any more but #34749 (comment) made me do so and it also was the polite thing to do. Also feel some ownership of the code, even if you are right that it is trivial in the grand scheme of things.
That diff seems simple enough to me, ACK.
Side note: one thing to keep in mind is that UniValue o{*prevout.raw}; copies the value, which is not evident just by reading the code. We should probably think about making it explicit everywhere in another PR.
Co-authored-by: Matias Furszyfer <matiasfurszyfer@protonmail.com>
Applied the patch, thanks for hashing it out @hodlinator @furszy
ACK d236415649e21a4848b5685b6ba41da82c8dde73
Thank you for incorporating my diff! Now addresses my main concern of re-serializing hashes.
Reduces cognitive complexity vs master according to https://calcshub.com/cyclomatic-complexity-calculator/ and https://code-timeandspace.vercel.app/.
Re-ACK d236415649e21a4848b5685b6ba41da82c8dde73
I think the latest revision picked up a stray trailing backslash in src/rpc/mempool.cpp after the options initializer:
c++ const UniValue options{request.params[1].isNull() ? UniValue::VOBJ : request.params[1]};\\ RPCTypeCheckObj(options, ...)
I checked the current tip locally and the file really contains that \ at the end of the line. Unless I’m missing something, that escapes the newline and should break compilation here.
I think the latest revision picked up a stray trailing backslash in src/rpc/mempool.cpp after the options initializer:
c++ const UniValue options{request.params[1].isNull() ? UniValue::VOBJ : request.params[1]};\\ RPCTypeCheckObj(options, ...)I checked the current tip locally and the file really contains that \ at the end of the line. Unless I’m missing something, that escapes the newline and should break compilation here. @Bortlesboat You can remove it by rebasing on current master. This is unrelated to the changed lines here in this pull request.