[RPC] verifytxoutproof returns object including blockhash #11120

instagibbs · 2017-08-23T15:50:58Z

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

kallewoof

utACK d77cded0b1d026f030ee424b89a01cfd6b7b9000

kallewoof · 2017-08-24T08:21:04Z

src/rpc/rawtransaction.cpp

            "\nArguments:\n"
            "1. \"proof\"    (string, required) The hex-encoded proof generated by gettxoutproof\n"
-            "\nResult:\n"
-            "[\"txid\"]      (array, strings) The txid(s) which the proof commits to, or empty array if the proof is invalid\n"
+            "\nResult: (or empty if proof is invalid)\n"


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

instagibbs · 2017-08-24T13:34:45Z

updated with nit fix

promag

Concept ACK.

promag · 2017-08-25T13:50:30Z

src/rpc/rawtransaction.cpp

-            "\nVerifies that a proof points to a transaction in a block, returning the transaction it commits to\n"
-            "and throwing an RPC error if the block is not in our best chain\n"
+            "\nVerifies that a proof points to a transaction in a block, returning the blockhash and transactions it commits to\n"
+            ", throwing an RPC error if the block is not in our best chain\n"


Nit, , in the above line?

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

promag · 2017-08-25T13:51:04Z

src/rpc/rawtransaction.cpp

@@ -258,19 +258,23 @@ UniValue verifytxoutproof(const JSONRPCRequest& request)
    if (request.fHelp || request.params.size() != 1)
        throw std::runtime_error(
            "verifytxoutproof \"proof\"\n"
-            "\nVerifies that a proof points to a transaction in a block, returning the transaction it commits to\n"
-            "and throwing an RPC error if the block is not in our best chain\n"
+            "\nVerifies that a proof points to a transaction in a block, returning the blockhash and transactions it commits to\n"


... the block hash and ...

promag · 2017-08-25T13:56:53Z

src/rpc/rawtransaction.cpp

@@ -283,7 +287,10 @@ UniValue verifytxoutproof(const JSONRPCRequest& request)
        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found in chain");

    for (const uint256& hash : vMatch)


Nit, add curly braces.

instagibbs · 2017-08-25T14:07:45Z

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

promag · 2017-08-25T14:26:36Z

src/rpc/rawtransaction.cpp

-    for (const uint256& hash : vMatch)
-        res.push_back(hash.GetHex());
+    res.push_back(Pair("txids", txids));
+    res.push_back(Pair("blockhash", merkleBlock.header.GetHash().GetHex()));


Push blockhash first.

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

Also matches the documentation above.

instagibbs · 2018-03-06T18:04:09Z

rebased, re-added legacy behavior under deprecation

instagibbs · 2018-03-06T18:08:51Z

ping @kallewoof

kallewoof

re-utACK a3716960de5555572408efdab4eba75484c8c1e3

kallewoof · 2018-03-06T18:16:45Z

src/rpc/rawtransaction.cpp

@@ -294,6 +294,8 @@ UniValue verifytxoutproof(const JSONRPCRequest& request)
            "    \"blockhash\"   (string) The blockhash the included proof commits to\n"
            "    [\"txid\"]      (array, strings) The txid(s) which the proof commits to\n"
            "}\n"
+            "\nResult: (DEPRECATED. To see this result in v0.17 instead, please start bitcoind with -deprecatedrpc=verifytxoutproof. Empty if proof is invalid)\n"


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

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

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?

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

promag · 2018-03-06T18:19:50Z

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

instagibbs · 2018-03-06T18:36:31Z

fixed up deprecation message, added release notes

promag

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

promag · 2018-03-06T18:39:20Z

doc/release-notes.md

@@ -62,6 +62,7 @@ RPC changes
 ### Low-level changes

 - The `fundrawtransaction` rpc will reject the previously deprecated `reserveChangeKey` option.
+- The `verifytxoutproof` rpc will output an object instead of an array unless `-deprecatedrpc=verifytxoutproof` is set


Nit, missing period, sorry.

instagibbs · 2018-03-06T18:46:50Z

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 · 2018-03-06T19:16:04Z

@instagibbs here is one

verifytxoutproof 0000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
{
}

Maybe it should throw an error in this case?

kallewoof

utACK - looks good

instagibbs · 2018-03-07T17:53:36Z

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

promag · 2018-03-07T18:17:52Z

src/rpc/rawtransaction.cpp

+        if (IsDeprecatedRPCEnabled("verifytxoutproof")) {
+            return txids;
+        } else {
+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No valid transaction commitments are found in the proof");


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

instagibbs · 2018-03-07T18:21:54Z

done

maflcko · 2018-03-08T15:21:04Z

Needs rebase to fix travis (sorry)

instagibbs · 2018-03-08T15:24:33Z

rebased

kallewoof

utACK ec1695eedc545545e5273e19f1e1b80aaebecf28

promag · 2018-03-11T23:18:52Z

utACK ec1695e.

maflcko · 2018-03-13T18:31:50Z

Needs rebase (after merge of LookupBlockIndex)

instagibbs · 2018-03-13T18:39:35Z

rebased

kallewoof · 2018-03-13T23:21:57Z

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

promag · 2018-03-13T23:24:32Z

master should be fixed after #12681.

kallewoof · 2018-03-13T23:29:20Z

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

sipa · 2018-03-14T00:25:37Z

src/rpc/rawtransaction.cpp

+        if (IsDeprecatedRPCEnabled("verifytxoutproof")) {
+            return txids;
+        } else {
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "No valid transaction commitments are found in the proof");


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

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.

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

kallewoof · 2018-03-15T00:49:23Z

re-utACK d43e00a

instagibbs · 2018-03-23T12:53:50Z

rebased due to release notes conflict

instagibbs · 2018-03-26T15:16:29Z

rebased again...

instagibbs · 2018-04-16T16:59:10Z

rebased again for yet another release notes conflict

promag · 2018-06-14T00:26:08Z

src/rpc/rawtransaction.cpp

-            "[\"txid\"]      (array, strings) The txid(s) which the proof commits to, or empty array if the proof is invalid\n"
+            "{\n"
+            "    \"blockhash\"   (string) The blockhash the included proof commits to\n"
+            "    [\"txid\"]      (array, strings) The txid(s) which the proof commits to\n"


Should mention response key: \"txids\".

instagibbs changed the title ~~verifytxoutproof returns object including blockhash~~ [RPC] verifytxoutproof returns object including blockhash Aug 23, 2017

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

kallewoof reviewed Aug 24, 2017

View reviewed changes

instagibbs force-pushed the verifytxoutproof-blockhash branch from d77cded to e7f9824 Compare August 24, 2017 13:34

promag reviewed Aug 25, 2017

View reviewed changes

instagibbs force-pushed the verifytxoutproof-blockhash branch from e7f9824 to 57d35ff Compare August 25, 2017 14:07

promag reviewed Aug 25, 2017

View reviewed changes

instagibbs force-pushed the verifytxoutproof-blockhash branch from 57d35ff to 9efd4fe Compare August 25, 2017 14:28

ajtowns mentioned this pull request Jan 16, 2018

[tests] Rename functional tests #11774

Merged

instagibbs force-pushed the verifytxoutproof-blockhash branch from 9efd4fe to a371696 Compare March 6, 2018 18:03

fanquake added the Needs release note label Mar 6, 2018

kallewoof reviewed Mar 6, 2018

View reviewed changes

instagibbs force-pushed the verifytxoutproof-blockhash branch from a371696 to c1c3631 Compare March 6, 2018 18:27

promag reviewed Mar 6, 2018

View reviewed changes

instagibbs force-pushed the verifytxoutproof-blockhash branch from 423c9d6 to 954ec5b Compare March 6, 2018 18:45

kallewoof approved these changes Mar 7, 2018

View reviewed changes

instagibbs force-pushed the verifytxoutproof-blockhash branch 2 times, most recently from 0d9330b to ce6ae10 Compare March 7, 2018 18:00

promag reviewed Mar 7, 2018

View reviewed changes

instagibbs force-pushed the verifytxoutproof-blockhash branch from ce6ae10 to 7045e31 Compare March 7, 2018 18:21

instagibbs force-pushed the verifytxoutproof-blockhash branch from 7045e31 to ec1695e Compare March 8, 2018 15:24

kallewoof approved these changes Mar 8, 2018

View reviewed changes

instagibbs force-pushed the verifytxoutproof-blockhash branch from ec1695e to d43e00a Compare March 13, 2018 18:37

sipa reviewed Mar 14, 2018

View reviewed changes

instagibbs force-pushed the verifytxoutproof-blockhash branch from d43e00a to b761342 Compare March 23, 2018 12:53

instagibbs force-pushed the verifytxoutproof-blockhash branch from b761342 to 5f63fa9 Compare March 26, 2018 15:15

instagibbs added 2 commits April 16, 2018 12:58

verifytxoutproof returns object including blockhash

453c287

add release notes for verifytxoutproof api break

674b877

instagibbs force-pushed the verifytxoutproof-blockhash branch from 5f63fa9 to 674b877 Compare April 16, 2018 16:58

maflcko added the Needs rebase label Jun 6, 2018

instagibbs mentioned this pull request Jun 13, 2018

rpc: have verifytxoutproof check the number of txns in proof structure #13452

Merged

promag reviewed Jun 14, 2018

View reviewed changes

instagibbs closed this Jul 10, 2018

fanquake removed the Needs release note label Mar 20, 2019

laanwj removed the Needs rebase label Oct 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

		@@ -283,7 +287,10 @@ UniValue verifytxoutproof(const JSONRPCRequest& request)
		throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found in chain");

		for (const uint256& hash : vMatch)

[RPC] verifytxoutproof returns object including blockhash #11120

[RPC] verifytxoutproof returns object including blockhash #11120

Uh oh!

Conversation

instagibbs commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Aug 24, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Aug 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Mar 6, 2018

Uh oh!

instagibbs commented Mar 6, 2018

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Mar 6, 2018

Uh oh!

instagibbs commented Mar 6, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Mar 6, 2018

Uh oh!

promag commented Mar 6, 2018

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Mar 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Mar 7, 2018

Uh oh!

maflcko commented Mar 8, 2018

Uh oh!

instagibbs commented Mar 8, 2018

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Mar 11, 2018

instagibbs commented Aug 23, 2017 •

edited

Loading

promag Aug 25, 2017 •

edited

Loading

kallewoof commented Mar 15, 2018 •

edited

Loading