[RPC] verifytxoutproof returns object including blockhash

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.

instagibbs renamed this:
~~verifytxoutproof returns object including blockhash~~
[RPC] verifytxoutproof returns object including blockhash
on Aug 23, 2017

laanwj added the label RPC/REST/ZMQ on Aug 23, 2017

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

kallewoof commented at 8:22 am on August 24, 2017: member

utACK d77cded0b1d026f030ee424b89a01cfd6b7b9000

instagibbs force-pushed on Aug 24, 2017

instagibbs commented at 1:34 pm on August 24, 2017: member

updated with nit fix

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.

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 ...

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.

promag commented at 1:57 pm on August 25, 2017: member

Concept ACK.

instagibbs force-pushed on Aug 25, 2017

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

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.

instagibbs force-pushed on Aug 25, 2017

instagibbs force-pushed on Mar 6, 2018

instagibbs commented at 6:04 pm on March 6, 2018: member

rebased, re-added legacy behavior under deprecation

instagibbs commented at 6:08 pm on March 6, 2018: member

ping @kallewoof

fanquake added the label Needs release notes on Mar 6, 2018

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.

kallewoof commented at 6:17 pm on March 6, 2018: member

re-utACK a3716960de5555572408efdab4eba75484c8c1e3

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.

instagibbs force-pushed on Mar 6, 2018

instagibbs commented at 6:36 pm on March 6, 2018: member

fixed up deprecation message, added release notes

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?

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.

instagibbs force-pushed on Mar 6, 2018

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.

promag commented at 7:16 pm on March 6, 2018: member

@instagibbs here is one

0verifytxoutproof 0000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
1{
2}

Maybe it should throw an error in this case?

kallewoof approved

kallewoof commented at 4:35 am on March 7, 2018: member

utACK - looks good

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

instagibbs force-pushed on Mar 7, 2018

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:

0assert_raises_rpc_error(-8, 'No valid transaction commitments are found in the proof', self.nodes[2].gettxoutproof, 
1'0000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000')

instagibbs force-pushed on Mar 7, 2018

instagibbs commented at 6:21 pm on March 7, 2018: member

done

MarcoFalke commented at 3:21 pm on March 8, 2018: member

Needs rebase to fix travis (sorry)

instagibbs force-pushed on Mar 8, 2018

instagibbs commented at 3:24 pm on March 8, 2018: member

rebased

kallewoof approved

kallewoof commented at 7:05 pm on March 8, 2018: member

utACK ec1695eedc545545e5273e19f1e1b80aaebecf28

promag commented at 11:18 pm on March 11, 2018: member

utACK ec1695e.

MarcoFalke commented at 6:31 pm on March 13, 2018: member

Needs rebase (after merge of LookupBlockIndex)

instagibbs force-pushed on Mar 13, 2018

instagibbs commented at 6:39 pm on March 13, 2018: member

rebased

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.

promag commented at 11:24 pm on March 13, 2018: member

master should be fixed after #12681.

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).

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 0: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 0:50 am on March 15, 2018:

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

kallewoof commented at 0:49 am on March 15, 2018: member

re-utACK d43e00a

instagibbs force-pushed on Mar 23, 2018

instagibbs commented at 12:53 pm on March 23, 2018: member

rebased due to release notes conflict

instagibbs force-pushed on Mar 26, 2018

instagibbs commented at 3:16 pm on March 26, 2018: member

rebased again…

verifytxoutproof returns object including blockhash 453c28751b

add release notes for verifytxoutproof api break 674b877fad

instagibbs force-pushed on Apr 16, 2018

instagibbs commented at 4:59 pm on April 16, 2018: member

rebased again for yet another release notes conflict

MarcoFalke added the label Needs rebase on Jun 6, 2018

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 0:26 am on June 14, 2018:

Should mention response key: \"txids\".

instagibbs closed this on Jul 10, 2018

fanquake removed the label Needs release note on Mar 20, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

DrahtBot locked this on Dec 16, 2021

[RPC] verifytxoutproof returns object including blockhash #11120