rpc, refactor: Avoid duplicate set lookup in gettxoutproof #19847

1. 
   promag commented at 2:49 pm on August 31, 2020: member
2. promag force-pushed on Aug 31, 2020
3. DrahtBot added the label RPC/REST/ZMQ on Aug 31, 2020
4. 
   instagibbs commented at 12:08 pm on September 3, 2020: member

   ACK https://github.com/bitcoin/bitcoin/pull/19847/commits/9542e0543028572a9380d17041f3d1e93f9d079b
5. 
   MarcoFalke commented at 1:42 pm on September 3, 2020: member

   could also test empty array?
6. 
   promag commented at 9:13 am on September 4, 2020: member

   @MarcoFalke you mean to change from

   0bitcoin-cli -regtest gettxoutproof '[]'
   1error code: -5
   2error message:
   3Transaction not yet in block
   

   to

   0bitcoin-cli -regtest gettxoutproof '[]'
   1error code: -8
   2error message:
   3Parameter 'txids' cannot be empty
   

7. 
   MarcoFalke commented at 8:46 am on September 9, 2020: member

   I’ve taken your test and my test suggestion and added them to #19922 (another pull reworking the test a bit)
8. laanwj referenced this in commit f2d9934381 on Sep 11, 2020
9. sidhujag referenced this in commit 316baa657c on Sep 11, 2020
10. DrahtBot added the label Needs rebase on Sep 11, 2020
11. promag force-pushed on Oct 5, 2020
12. promag renamed this:
    test: gettxoutproof duplicate txid
    rpc, refactor: Avoid duplicate set lookup in gettxoutproof
    on Oct 5, 2020
13. DrahtBot removed the label Needs rebase on Oct 5, 2020
14. theStack approved
15. 
    theStack commented at 3:58 pm on October 11, 2020: member

    Code Review ACK c4d8377fbfa8b7b0f92552b0f66a100626ee178e 🧾
16. 
    MarcoFalke commented at 5:24 pm on October 11, 2020: member

    Not sure if this refactor is worth it. Unless you fix the error string for #19847 (comment) I don’t think this code needs to be touched.
17. 
    promag commented at 0:46 am on October 12, 2020: member

    @MarcoFalke done.
18. promag force-pushed on Oct 12, 2020
19. luke-jr approved
20. 
    luke-jr commented at 3:53 pm on October 20, 2020: member

    utACK
21. rpc, refactor: Avoid duplicate set lookup in gettxoutproof 73dc19a330
22. rpc: Reject empty txids in gettxoutproof 52fc39917f
23. in src/rpc/rawtransaction.cpp:248 in 63ed27a3b3 outdated

    245@@ -246,14 +246,15 @@ static RPCHelpMan gettxoutproof()
    246     std::set<uint256> setTxids;
    247     uint256 oneTxid;
    248     UniValue txids = request.params[0].get_array();
    249+    if (txids.empty()) {
    

    ---

    ---

    

    MarcoFalke commented at 9:21 am on October 21, 2020:

    you can move this to after the for loop and run the check on setTxids, then remove oneTxid and replace it with setTxids.front()

    ---

    

    promag commented at 5:29 pm on December 4, 2020:

    ATM oneTxid is the first txid in the array, with you suggestion that wouldn’t be the case since set::set orders, right?

    ---

    

    MarcoFalke commented at 5:52 pm on December 4, 2020:

    oneTxid can be any txid

    ---

    

    promag commented at 6:04 pm on December 4, 2020:

    Right, all must be in same block.

    ---

    

    promag commented at 6:40 pm on December 4, 2020:

    Done, thanks!
24. promag force-pushed on Dec 4, 2020
25. 
    jonasschnelli commented at 8:16 am on December 7, 2020: contributor

    nice small optimization! code review ACK 52fc39917fc52c2ff279fe434431e18900b347bd
26. jonasschnelli merged this on Dec 7, 2020
27. jonasschnelli closed this on Dec 7, 2020

    ---

28. in src/rpc/rawtransaction.cpp:286 in 73dc19a330 outdated

    282@@ -287,7 +283,7 @@ static RPCHelpMan gettxoutproof()
    283     LOCK(cs_main);
    284 
    285     if (pblockindex == nullptr) {
    286-        const CTransactionRef tx = GetTransaction(/* block_index */ nullptr, /* mempool */ nullptr, oneTxid, Params().GetConsensus(), hashBlock);
    287+        const CTransactionRef tx = GetTransaction(/* block_index */ nullptr, /* mempool */ nullptr, *setTxids.begin(), Params().GetConsensus(), hashBlock);
    

    ---

    ---

    

    MarcoFalke commented at 8:52 am on December 7, 2020:

    in commit 73dc19a330f8cb063af46e6c4246f2e64a04bdc1:

    Just for future reference, this is reading uninitialized memory

    ---

    

    promag commented at 10:19 am on December 7, 2020:

    Care to explain?

    ---

    

    MarcoFalke commented at 10:26 am on December 7, 2020:

    std::set::begin will return the end interator if the set is empty.

    Dereferencing the end pointer is accessing uninitialized memory.

    ---

    

    promag commented at 10:35 am on December 7, 2020:

    Right, would you check !setTxids.empty() again?

    ---

    

    MarcoFalke commented at 10:38 am on December 7, 2020:

    the bug only exists in commit 73dc19a , which can’t be modified because it is already merged

    ---

    

    jonasschnelli commented at 10:41 am on December 7, 2020:

    Thanks for spotting @MarcoFalke. I guess this needs for a follow up PR.

    ---

    

    promag commented at 10:43 am on December 7, 2020:

    The bug is fixed in the 2nd commit 52fc39917fc52c2ff279fe434431e18900b347bd. It should have been the 1st commit.

    ---

    

    jonasschnelli commented at 11:00 am on December 7, 2020:

    Now I got it. An intermediate commit had this uninitialized memory access. No need for action.
29. promag deleted the branch on Dec 7, 2020
30. sidhujag referenced this in commit c8ac34f1d4 on Dec 7, 2020
31. DrahtBot locked this on Feb 15, 2022

Contributors
promag instagibbs MarcoFalke theStack luke-jr jonasschnelli

Labels
RPC/REST/ZMQ