[RPC] verifytxoutproof returns object including blockhash #11120

pull instagibbs wants to merge 2 commits into bitcoin:master from instagibbs:verifytxoutproof-blockhash changing 3 files +50 −19
  1. instagibbs commented at 3:50 PM on August 23, 2017: member

    Allows direct use of the proof to get block header related info without additional parsing, and more directly mirrors gettxoutproof arguments.

  2. instagibbs renamed this:
    verifytxoutproof returns object including blockhash
    [RPC] verifytxoutproof returns object including blockhash
    on Aug 23, 2017
  3. laanwj added the label RPC/REST/ZMQ on Aug 23, 2017
  4. in src/rpc/rawtransaction.cpp:265 in d77cded0b1 outdated 
     264 | +            ", throwing an RPC error if the block is not in our best chain\n"
 265 |              "\nArguments:\n"
 266 |              "1. \"proof\"    (string, required) The hex-encoded proof generated by gettxoutproof\n"
 267 | -            "\nResult:\n"
 268 | -            "[\"txid\"]      (array, strings) The txid(s) which the proof commits to, or empty array if the proof is invalid\n"
 269 | +            "\nResult: (or empty if proof is invalid)\n"
    kallewoof commented at 8:21 AM on August 24, 2017:

    Remove or maybe? I.e. Result: (empty if proof is invalid)\n

  5. kallewoof commented at 8:22 AM on August 24, 2017: member

    utACK d77cded0b1d026f030ee424b89a01cfd6b7b9000

  6. instagibbs force-pushed on Aug 24, 2017
  7. instagibbs commented at 1:34 PM on August 24, 2017: member

    updated with nit fix

  8. in src/rpc/rawtransaction.cpp:262 in e7f9824f49 outdated 
     257 | @@ -258,19 +258,23 @@ UniValue verifytxoutproof(const JSONRPCRequest& request)
 258 |      if (request.fHelp || request.params.size() != 1)
 259 |          throw std::runtime_error(
 260 |              "verifytxoutproof \"proof\"\n"
 261 | -            "\nVerifies that a proof points to a transaction in a block, returning the transaction it commits to\n"
 262 | -            "and throwing an RPC error if the block is not in our best chain\n"
 263 | +            "\nVerifies that a proof points to a transaction in a block, returning the blockhash and transactions it commits to\n"
 264 | +            ", throwing an RPC error if the block is not in our best chain\n"
    promag commented at 1:50 PM on August 25, 2017:

    Nit, , in the above line?

    kallewoof commented at 2:04 PM on August 25, 2017:

    I was gonna mention that but chose not to. Agree though, I think that would look better.

  9. in src/rpc/rawtransaction.cpp:261 in e7f9824f49 outdated 
     257 | @@ -258,19 +258,23 @@ UniValue verifytxoutproof(const JSONRPCRequest& request)
 258 |      if (request.fHelp || request.params.size() != 1)
 259 |          throw std::runtime_error(
 260 |              "verifytxoutproof \"proof\"\n"
 261 | -            "\nVerifies that a proof points to a transaction in a block, returning the transaction it commits to\n"
 262 | -            "and throwing an RPC error if the block is not in our best chain\n"
 263 | +            "\nVerifies that a proof points to a transaction in a block, returning the blockhash and transactions it commits to\n"
    promag commented at 1:51 PM on August 25, 2017:

    ... the block hash and ...

  10. in src/rpc/rawtransaction.cpp:289 in e7f9824f49 outdated 
     286 | @@ -283,7 +287,10 @@ UniValue verifytxoutproof(const JSONRPCRequest& request)
 287 |          throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found in chain");
 288 |  
 289 |      for (const uint256& hash : vMatch)
    promag commented at 1:56 PM on August 25, 2017:

    Nit, add curly braces.

  11. promag commented at 1:57 PM on August 25, 2017: member

    Concept ACK.

  12. instagibbs force-pushed on Aug 25, 2017
  13. instagibbs commented at 2:07 PM on August 25, 2017: member

    fixed @promag nits, curly-bracketed the surrounding statements since I'm touching it already

  14. in src/rpc/rawtransaction.cpp:295 in 57d35fff53 outdated 
     300 | +    }
 301 |  
 302 | -    for (const uint256& hash : vMatch)
 303 | -        res.push_back(hash.GetHex());
 304 | +    res.push_back(Pair("txids", txids));
 305 | +    res.push_back(Pair("blockhash", merkleBlock.header.GetHash().GetHex()));
    promag commented at 2:26 PM on August 25, 2017:

    Push blockhash first.

    kallewoof commented at 10:47 PM on August 25, 2017:

    I'm curious about the reason for this request. Alphabetical ordering?

    promag commented at 11:35 PM on August 25, 2017:

    Also matches the documentation above.

  15. instagibbs force-pushed on Aug 25, 2017
  16. instagibbs force-pushed on Mar 6, 2018
  17. instagibbs commented at 6:04 PM on March 6, 2018: member

    rebased, re-added legacy behavior under deprecation

  18. instagibbs commented at 6:08 PM on March 6, 2018: member

    ping @kallewoof

  19. fanquake added the label Needs release notes on Mar 6, 2018
  20. in src/rpc/rawtransaction.cpp:297 in a3716960de outdated 
     293 | @@ -294,6 +294,8 @@ UniValue verifytxoutproof(const JSONRPCRequest& request)
 294 |              "    \"blockhash\"   (string) The blockhash the included proof commits to\n"
 295 |              "    [\"txid\"]      (array, strings) The txid(s) which the proof commits to\n"
 296 |              "}\n"
 297 | +            "\nResult: (DEPRECATED. To see this result in v0.17 instead, please start bitcoind with -deprecatedrpc=verifytxoutproof. Empty if proof is invalid)\n"
    kallewoof commented at 6:16 PM on March 6, 2018:

    I'm not sure about the in v0.17 instead wording -- what about when bitcoin hits v0.18?

    promag commented at 6:19 PM on March 6, 2018:

    I thought the same, but there are other occurrences in the code. I would remove the version though.

    instagibbs commented at 6:25 PM on March 6, 2018:

    I'm a bit confused, if this is merged now, it definitely would be the way to see the old text for 0.17. Now, perhaps the note should also say when it will be removed?

    instagibbs commented at 6:27 PM on March 6, 2018:

    Pushed a new wording, hopefully clearer and more pertinent to the user.

  21. kallewoof commented at 6:17 PM on March 6, 2018: member

    re-utACK a3716960de5555572408efdab4eba75484c8c1e3

  22. promag commented at 6:19 PM on March 6, 2018: member

    @instagibbs See https://github.com/bitcoin/bitcoin/pull/11872/files#diff-ef76fd6674f07db88c3422fdbf0bcf9fR65 for release note example.

  23. instagibbs force-pushed on Mar 6, 2018
  24. instagibbs commented at 6:36 PM on March 6, 2018: member

    fixed up deprecation message, added release notes

  25. promag commented at 6:37 PM on March 6, 2018: member

    Could add a test for the empty response {}, at least I don't see it?

  26. in doc/release-notes.md:65 in 423c9d6e08 outdated 
      61 | @@ -62,6 +62,7 @@ RPC changes
  62 |  ### Low-level changes
  63 |  
  64 |  - The `fundrawtransaction` rpc will reject the previously deprecated `reserveChangeKey` option.
  65 | +- The `verifytxoutproof` rpc will output an object instead of an array unless `-deprecatedrpc=verifytxoutproof` is set
    promag commented at 6:39 PM on March 6, 2018:

    Nit, missing period, sorry.

  27. instagibbs force-pushed on Mar 6, 2018
  28. instagibbs commented at 6:46 PM on March 6, 2018: member

    fixed up release note nit. I think the only way to return {} is to have a non-matching hash header... too lazy to construct one in paste it in.

  29. promag commented at 7:16 PM on March 6, 2018: member

    @instagibbs here is one

    verifytxoutproof 0000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
{
}

    Maybe it should throw an error in this case?

  30. kallewoof approved
  31. kallewoof commented at 4:35 AM on March 7, 2018: member

    utACK - looks good

  32. instagibbs commented at 5:53 PM on March 7, 2018: member

    @promag yeah I'll catch the deprecated case of [] and throw an error for new behavior

  33. instagibbs force-pushed on Mar 7, 2018
  34. instagibbs force-pushed on Mar 7, 2018
  35. in src/rpc/rawtransaction.cpp:314 in ce6ae10e5c outdated 
     315 | -        return res;
 316 | +    if (merkleBlock.txn.ExtractMatches(vMatch, vIndex) != merkleBlock.header.hashMerkleRoot) {
 317 | +        if (IsDeprecatedRPCEnabled("verifytxoutproof")) {
 318 | +            return txids;
 319 | +        } else {
 320 | +            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No valid transaction commitments are found in the proof");
    promag commented at 6:17 PM on March 7, 2018:

    s/RPC_INVALID_ADDRESS_OR_KEY/RPC_INVALID_PARAMETER?

    Add a test for the error, like:

    assert_raises_rpc_error(-8, 'No valid transaction commitments are found in the proof', self.nodes[2].gettxoutproof, 
'0000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000')
  36. instagibbs force-pushed on Mar 7, 2018
  37. instagibbs commented at 6:21 PM on March 7, 2018: member

    done

  38. MarcoFalke commented at 3:21 PM on March 8, 2018: member

    Needs rebase to fix travis (sorry)

  39. instagibbs force-pushed on Mar 8, 2018
  40. instagibbs commented at 3:24 PM on March 8, 2018: member

    rebased

  41. kallewoof approved
  42. kallewoof commented at 7:05 PM on March 8, 2018: member

    utACK ec1695eedc545545e5273e19f1e1b80aaebecf28

  43. promag commented at 11:18 PM on March 11, 2018: member

    utACK ec1695e.

  44. MarcoFalke commented at 6:31 PM on March 13, 2018: member

    Needs rebase (after merge of LookupBlockIndex)

  45. instagibbs force-pushed on Mar 13, 2018
  46. instagibbs commented at 6:39 PM on March 13, 2018: member

    rebased

  47. kallewoof commented at 11:21 PM on March 13, 2018: member

    @instagibbs Travis is timing out. I tried restarting the two jobs that timed out but it still fails. Not sure what's up.

  48. promag commented at 11:24 PM on March 13, 2018: member

    master should be fixed after #12681.

  49. kallewoof commented at 11:29 PM on March 13, 2018: member

    @promag I think that's unrelated (and I don't think @instagibbs merged the commit that triggered the error that was fixed).

  50. in src/rpc/rawtransaction.cpp:314 in d43e00a902 outdated 
     315 | -        return res;
 316 | +    if (merkleBlock.txn.ExtractMatches(vMatch, vIndex) != merkleBlock.header.hashMerkleRoot) {
 317 | +        if (IsDeprecatedRPCEnabled("verifytxoutproof")) {
 318 | +            return txids;
 319 | +        } else {
 320 | +            throw JSONRPCError(RPC_INVALID_PARAMETER, "No valid transaction commitments are found in the proof");
    sipa commented at 12:25 AM on March 14, 2018:

    Is the RPC user at fault when this happens? If not, I don't think this should be an RPC error.

    instagibbs commented at 1:51 PM on March 14, 2018:

    We already error out when the block is not found in the chain, which is just as much the users' fault.

    I'll leave this up to others to chime in.

    sipa commented at 12:50 AM on March 15, 2018:

    Ok, fair. Let's keep it this way then.

  51. kallewoof commented at 12:49 AM on March 15, 2018: member

    re-utACK d43e00a

  52. instagibbs force-pushed on Mar 23, 2018
  53. instagibbs commented at 12:53 PM on March 23, 2018: member

    rebased due to release notes conflict

  54. instagibbs force-pushed on Mar 26, 2018
  55. instagibbs commented at 3:16 PM on March 26, 2018: member

    rebased again...

  56. verifytxoutproof returns object including blockhash 453c28751b
  57. add release notes for verifytxoutproof api break 674b877fad
  58. instagibbs force-pushed on Apr 16, 2018
  59. instagibbs commented at 4:59 PM on April 16, 2018: member

    rebased again for yet another release notes conflict

  60. MarcoFalke added the label Needs rebase on Jun 6, 2018
  61. in src/rpc/rawtransaction.cpp:295 in 674b877fad 
     293 |              "1. \"proof\"    (string, required) The hex-encoded proof generated by gettxoutproof\n"
 294 |              "\nResult:\n"
 295 | -            "[\"txid\"]      (array, strings) The txid(s) which the proof commits to, or empty array if the proof is invalid\n"
 296 | +            "{\n"
 297 | +            "    \"blockhash\"   (string) The blockhash the included proof commits to\n"
 298 | +            "    [\"txid\"]      (array, strings) The txid(s) which the proof commits to\n"
    promag commented at 12:26 AM on June 14, 2018:

    Should mention response key: \"txids\".

  62. instagibbs closed this on Jul 10, 2018
  63. fanquake removed the label Needs release note on Mar 20, 2019
  64. laanwj removed the label Needs rebase on Oct 24, 2019
  65. DrahtBot locked this on Dec 16, 2021
Contributors
instagibbskallewoofpromagMarcoFalkesipa
Labels
RPC/REST/ZMQ
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-03 21:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me