rpc: Avoid useless mempool query in gettxoutproof

MarcoFalke commented at 7:36 am on July 26, 2020: member

GetTransaction implicitly and unconditionally asks the mempool global for a transaction. This is problematic for several reasons:

gettxoutproof is for on-chain txs only and asking the mempool for on-chain txs is confusing and minimally wasteful
Globals are confusing and make code harder to test with unit tests

Fix both issues by passing in an optional mempool. This also helps with #19556

MarcoFalke added the label Refactoring on Jul 26, 2020

MarcoFalke added the label RPC/REST/ZMQ on Jul 26, 2020

MarcoFalke added the label Validation on Jul 26, 2020

MarcoFalke commented at 7:37 am on July 26, 2020: member

Based on feedback by @jnewbery in #19556 (review)

MarcoFalke force-pushed on Jul 26, 2020

fanquake deleted a comment on Jul 26, 2020

in src/rest.cpp:81 in fa6e74b3a4 outdated

76+ */
77+static NodeContext* GetNodeContext(const util::Ref& context, HTTPRequest* req)
78+{
79+    NodeContext* node = context.Has<NodeContext>() ? &context.Get<NodeContext>() : nullptr;
80+    if (!node) {
81+        RESTERR(req, HTTP_NOT_FOUND, "node context not found");

promag commented at 10:47 am on July 26, 2020:

I think this should be 500 or 422? I don’t agree with “node context not found” as this is not what the client is requesting.

MarcoFalke commented at 10:56 am on July 26, 2020:

This code path can never be executed, unless there is a programming error. I am happy to change the error string if there is a better suggestion, but I don’t think it matters much, because it is dead code.

promag commented at 11:07 am on July 26, 2020:

Right, meaning its 500 internal server error :trollface:

jnewbery commented at 1:40 pm on July 26, 2020:

@promag makes a convincing argument that this should be a 500 internal server error :)

MarcoFalke commented at 2:25 pm on July 26, 2020:

thx, replaced with “500”

promag commented at 10:48 am on July 26, 2020: member

Code review ACK, refactor looks good with the following exception.

in src/rpc/rawtransaction.cpp:249 in fa46847c02 outdated

244@@ -245,10 +245,11 @@ static UniValue gettxoutproof(const JSONRPCRequest& request)
245     for (unsigned int idx = 0; idx < txids.size(); idx++) {
246         const UniValue& txid = txids[idx];
247         uint256 hash(ParseHashV(txid, "txid"));
248-        if (setTxids.count(hash))
249+        if (setTxids.count(hash)) {
250             throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated txid: ")+txid.get_str());

hebasto commented at 11:08 am on July 26, 2020:

fa46847c02d7f2c37c892bc03584f1eda8c21b34 While this is a style fixup commit, could this also be fixed

0            throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated txid: ") + txid.get_str());

?

MarcoFalke commented at 2:25 pm on July 26, 2020:

thx, added whitespace

hebasto approved

hebasto commented at 11:31 am on July 26, 2020: member

ACK fa6e74b3a40d26026a8dae3847f54f88c9c9761e, I have reviewed the code and it looks OK, I agree it can be merged.

in src/validation.h:168 in fa6e74b3a4 outdated

163@@ -164,8 +164,11 @@ bool LoadGenesisBlock(const CChainParams& chainparams);
164 void UnloadBlockIndex();
165 /** Run an instance of the script checking thread */
166 void ThreadScriptCheck(int worker_num);
167-/** Retrieve a transaction (from memory pool, or from disk, if possible) */
168-bool GetTransaction(const uint256& hash, CTransactionRef& tx, const Consensus::Params& params, uint256& hashBlock, const CBlockIndex* const blockIndex = nullptr);
169+/**
170+ * Return transaction in txOut, and if it was found via block_index, its hash is placed in hashBlock.

jnewbery commented at 1:36 pm on July 26, 2020:

Thanks for consolidating the function comment to the header declaration.

Would it be too much to ask to doxygenize this, specifically to show which arguments are required in args (hash, params), which are optional in args (mempool, block_index), and which are out args (txOut, hashBlock). Even better would be to re-order them to required in args, optional in args, out args.

MarcoFalke commented at 2:25 pm on July 26, 2020:

thx, doxygenized :ox:

jnewbery commented at 1:38 pm on July 26, 2020: member

Concept ACK.

rpc: Style fixups in gettxoutproof fa1f7f28cb

MarcoFalke force-pushed on Jul 26, 2020

rpc: Avoid useless mempool query in gettxoutproof fa5979d12f

MarcoFalke force-pushed on Jul 26, 2020

MarcoFalke commented at 4:09 pm on July 26, 2020: member

Addressed all feedback

jnewbery commented at 6:00 pm on July 26, 2020: member

utACK fa5979d12f8c65754e36cdddb9d032ab81fecc3a

Nice tidy-up. Thanks Marco!

DrahtBot commented at 6:32 pm on July 26, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18731 (refactor: Make CCheckQueue RAII-styled by hebasto)
#18710 (Add local thread pool to CCheckQueue by hebasto)

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.

hebasto approved

hebasto commented at 6:56 pm on July 26, 2020: member

re-ACK fa5979d12f8c65754e36cdddb9d032ab81fecc3a

promag commented at 10:17 pm on July 26, 2020: member

Code review ACK fa5979d12f8c65754e36cdddb9d032ab81fecc3a.

fanquake merged this on Jul 28, 2020

fanquake closed this on Jul 28, 2020

MarcoFalke deleted the branch on Jul 28, 2020

sidhujag referenced this in commit 74e94ca8ae on Jul 28, 2020

deadalnix referenced this in commit d23ae6f6ee on Jul 15, 2021

DrahtBot locked this on Feb 15, 2022

rpc: Avoid useless mempool query in gettxoutproof #19589

Conflicts