gettxoutproof() should return consistent result #9738

1. 
   jnewbery commented at 4:12 pm on February 10, 2017: member

   We can call gettxoutproof() with a list of transactions. Currently, if the first transaction is unspent (and all other transactions are in the same block), then the call will succeed. If the first transaction has been spent, then the call will fail. The means that the following two calls will return different results:

   • gettxoutproof(unspent_tx1, spent_tx1)
   • gettxoutproof(spent_tx1, unspent_tx1)

   This commit makes behaviour independent of transaction ordering by looping through all transactions provided and trying to find which block they’re in.

   This commit also increases the test coverage and tests more failure cases for gettxoutproof()

   Note to reviewers: builds on #9707 - please review only the second commit in this PR.

2. in src/rpc/rawtransaction.cpp: in 20eaa35519 outdated

   283@@ -284,8 +284,13 @@ UniValue gettxoutproof(const JSONRPCRequest& request)
   284         pblockindex = mapBlockIndex[hashBlock];
   285     } else {
   286         CCoins coins;
   287-        if (pcoinsTip->GetCoins(oneTxid, coins) && coins.nHeight > 0 && coins.nHeight <= chainActive.Height())
   

   ---

   ---

   

   instagibbs commented at 4:20 pm on February 10, 2017:

   seems like this is the last txid, not the first in the list? Little confused on current behavior.

   ---

   

   jnewbery commented at 4:25 pm on February 10, 2017:

   ha. Yes, you’re right. This is the last transaction in the list.

   %s/first/last/g my original comment

3. in src/rpc/rawtransaction.cpp: in 20eaa35519 outdated

   311@@ -307,7 +312,7 @@ UniValue gettxoutproof(const JSONRPCRequest& request)
   312         if (setTxids.count(tx->GetHash()))
   313             ntxFound++;
   314     if (ntxFound != setTxids.size())
   315-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "(Not all) transactions not found in specified block");
   316+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Not all transactions found in specified block");
   

   ---

   ---

   

   instagibbs commented at 4:48 pm on February 10, 2017:

   how about “Not all transactions found in the retrieved block” since this can happen when it is unspecified as well?

   ---

   

   jnewbery commented at 4:51 pm on February 10, 2017:

   I’ve changed it to "Not all transactions found in the specified or retrieved block"
4. jnewbery force-pushed on Feb 10, 2017
5. fanquake added the label RPC/REST/ZMQ on Feb 10, 2017
6. 
   TheBlueMatt commented at 6:07 pm on February 10, 2017: member

   utACK 5dca91d5cf8dfc99343c6e18a223504049a7f68b (assuming the test is fixed - it looks like it was just the string was changed in code but the RPC test’s check for string didnt get updated - yay for tests testing things :+1: )
7. jnewbery force-pushed on Feb 10, 2017
8. 
   jnewbery commented at 6:28 pm on February 10, 2017: member

   ok, should be fixed. #9707 has now been merged so this PR nwo contains only the relevant commit.
9. jnewbery force-pushed on Apr 12, 2017
10. 
    jnewbery commented at 2:09 pm on April 12, 2017: member

    rebased
11. 
    laanwj commented at 10:30 am on June 6, 2017: member

    Needs rebase
12. jnewbery force-pushed on Jun 6, 2017
13. jnewbery force-pushed on Jun 6, 2017
14. 
    jnewbery commented at 4:07 pm on June 6, 2017: member

    rebased
15. in src/rpc/rawtransaction.cpp:225 in ae69c3867b outdated

    223-        if (!coin.IsSpent() && coin.nHeight > 0 && coin.nHeight <= chainActive.Height()) {
    224-            pblockindex = chainActive[coin.nHeight];
    225+        // Loop through txids and try to find which block they're in. Exit loop once a block is found.
    226+        for (const auto& tx : setTxids) {
    227+            const Coin& coin = AccessByTxid(*pcoinsTip, tx);
    228+            if (!coin.IsSpent() && coin.nHeight > 0 && coin.nHeight <= chainActive.Height()) {
    

    ---

    ---

    

    TheBlueMatt commented at 5:56 pm on June 7, 2017:

    I believe the nHeight checks are a mistranslation to per-utxo, so should just be dropped.

    ---

    

    jnewbery commented at 9:39 pm on June 7, 2017:

    Thanks. Fixed
16. gettxoutproof() should return consistent result

    We can call gettxoutproof() with a list of transactions. Currently, if
    the first transaction is unspent (and all other transactions are in the
    same block), then the call will succeed. If the first transaction has
    been spent, then the call will fail. The means that the following two
    calls will return different results:
    
    gettxoutproof(unspent_tx1, spent_tx1)
    gettxoutproof(spent_tx1, unspent_tx1)
    
    This commit makes behaviour independent of transaction ordering by looping
    through all transactions provided and trying to find which block they're in.
    
    This commit also increases the test coverage and tests more failure
    cases for gettxoutproof()

    6294f3283a
17. jnewbery force-pushed on Jun 7, 2017
18. 
    laanwj commented at 1:48 pm on June 14, 2017: member

    utACK 6294f32
19. laanwj merged this on Jun 14, 2017
20. laanwj closed this on Jun 14, 2017

    ---

21. laanwj referenced this in commit c94b89e90d on Jun 14, 2017
22. PastaPastaPasta referenced this in commit e307c53780 on Jul 5, 2019
23. PastaPastaPasta referenced this in commit ee5f7726a5 on Jul 5, 2019
24. PastaPastaPasta referenced this in commit cca9444084 on Jul 6, 2019
25. PastaPastaPasta referenced this in commit 5adc69b312 on Jul 8, 2019
26. PastaPastaPasta referenced this in commit d696fd7c7c on Jul 9, 2019
27. PastaPastaPasta referenced this in commit ab02a43357 on Jul 9, 2019
28. barrystyle referenced this in commit 471eb2322e on Jan 22, 2020
29. DrahtBot locked this on Sep 8, 2021

Contributors
jnewbery instagibbs TheBlueMatt laanwj

Labels
RPC/REST/ZMQ