New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RPC] Transaction details in getblock #8704

Merged
merged 2 commits into from May 15, 2017

Conversation

Projects
None yet
10 participants
@achow101
Member

achow101 commented Sep 12, 2016

Adds an optional parameter extraVerbose to getblock to have transaction details displayed in a getblock call.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Sep 12, 2016

Contributor

concept NACK, is two RPC requests really so bad?

edit: weak concept NACK

Contributor

dcousens commented Sep 12, 2016

concept NACK, is two RPC requests really so bad?

edit: weak concept NACK

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Sep 12, 2016

Member

@dcousens yes, two RPC calls is really bad, especially when you want the details of all the transactions in a block. Then you end up running getrawtransaction thousands of times. Also, AFAICT, this doesn't require the txindex in order to get the transactions, unlike getrawtransaction.

Member

achow101 commented Sep 12, 2016

@dcousens yes, two RPC calls is really bad, especially when you want the details of all the transactions in a block. Then you end up running getrawtransaction thousands of times. Also, AFAICT, this doesn't require the txindex in order to get the transactions, unlike getrawtransaction.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Sep 12, 2016

Contributor

@dcousens yes, two RPC calls is really bad

Why?
If you're following best practices (RPC is over localhost only) in terms of access, the latency should be irrelevant.

In a batched RPC call, the overhead is literally just 1RTT more.
Not N (aka, thousands), 1.

AFAICT, this doesn't require the txindex in order to get the transactions, unlike getrawtransaction.

Perhaps getrawtransaction should have an optional block parameter?

edit: In fact, adding that option would be significantly more useful than this RTT optimisation IMHO.

Contributor

dcousens commented Sep 12, 2016

@dcousens yes, two RPC calls is really bad

Why?
If you're following best practices (RPC is over localhost only) in terms of access, the latency should be irrelevant.

In a batched RPC call, the overhead is literally just 1RTT more.
Not N (aka, thousands), 1.

AFAICT, this doesn't require the txindex in order to get the transactions, unlike getrawtransaction.

Perhaps getrawtransaction should have an optional block parameter?

edit: In fact, adding that option would be significantly more useful than this RTT optimisation IMHO.

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Sep 12, 2016

Member

In a batched RPC call, the overhead is literally just 1RTT more.
Not N (aka, thousands), 1.

How?

edit: In fact, adding that option would be significantly more useful than this RTT optimisation IMHO.

Well the functionality to this was already in the code. The BlockToJson method took a parameter for txDetails which was by default false. This just lets you set that to true.

Member

achow101 commented Sep 12, 2016

In a batched RPC call, the overhead is literally just 1RTT more.
Not N (aka, thousands), 1.

How?

edit: In fact, adding that option would be significantly more useful than this RTT optimisation IMHO.

Well the functionality to this was already in the code. The BlockToJson method took a parameter for txDetails which was by default false. This just lets you set that to true.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Sep 12, 2016

Contributor

http://www.jsonrpc.org/specification#batch

What language are you using? Your RPC library should be able to handle this quite easily.

Contributor

dcousens commented Sep 12, 2016

http://www.jsonrpc.org/specification#batch

What language are you using? Your RPC library should be able to handle this quite easily.

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Sep 12, 2016

Member

Huh. Didn't know that. I've been using bash with either curl or bitcoin-cli and python.

Member

achow101 commented Sep 12, 2016

Huh. Didn't know that. I've been using bash with either curl or bitcoin-cli and python.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Sep 13, 2016

Member

Saving roundtrip time is not a valid reason to add a RPC API (see discussion in #8457).

Also, AFAICT, this doesn't require the txindex in order to get the transactions, unlike getrawtransaction.

This is the only important fact here: you cannot get this information any other way. getrawtransaction won't get you transactions in blocks, at least without tx index.

The only way to get this information right now is to getblock raw then parse the block locally.

So concept ACK because of that.

Member

laanwj commented Sep 13, 2016

Saving roundtrip time is not a valid reason to add a RPC API (see discussion in #8457).

Also, AFAICT, this doesn't require the txindex in order to get the transactions, unlike getrawtransaction.

This is the only important fact here: you cannot get this information any other way. getrawtransaction won't get you transactions in blocks, at least without tx index.

The only way to get this information right now is to getblock raw then parse the block locally.

So concept ACK because of that.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Sep 13, 2016

Contributor

@laanwj would it maybe be better to add functionality via getrawtransaction w/ a block id? (perhaps in addition to this)

Contributor

dcousens commented Sep 13, 2016

@laanwj would it maybe be better to add functionality via getrawtransaction w/ a block id? (perhaps in addition to this)

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Sep 14, 2016

Member

would it maybe be better to add functionality via getrawtransaction w/ a block id? (perhaps in addition to this)

Well yes a "gather transaction [X,...] from prespecified block Y" RPC call could be useful in some rare cases. But on the other hand it'll still have to read the entire block from disk and deserialize it. It's terribly inefficient already.
So if it decoded the entire block why not report details for the entire block as done here?

Member

laanwj commented Sep 14, 2016

would it maybe be better to add functionality via getrawtransaction w/ a block id? (perhaps in addition to this)

Well yes a "gather transaction [X,...] from prespecified block Y" RPC call could be useful in some rare cases. But on the other hand it'll still have to read the entire block from disk and deserialize it. It's terribly inefficient already.
So if it decoded the entire block why not report details for the entire block as done here?

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Sep 14, 2016

Contributor

@laanwj indeed! Forgot about the resulting deserialization.

Contributor

dcousens commented Sep 14, 2016

@laanwj indeed! Forgot about the resulting deserialization.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Sep 22, 2016

Member

Concept ACK for all the reasons @laanwj already discussed.

Member

luke-jr commented Sep 22, 2016

Concept ACK for all the reasons @laanwj already discussed.

Show outdated Hide outdated src/rpc/blockchain.cpp
throw runtime_error(
"getblock \"hash\" ( verbose )\n"
"getblock \"hash\" ( verbose ) ( extraVerbose )\n"

This comment has been minimized.

@luke-jr

luke-jr Oct 18, 2016

Member

Double boolean here seems ugly. Maybe allow verbose to be boolean or a number 0-2 (with 2 being extraVerbose)?

@luke-jr

luke-jr Oct 18, 2016

Member

Double boolean here seems ugly. Maybe allow verbose to be boolean or a number 0-2 (with 2 being extraVerbose)?

This comment has been minimized.

@laanwj

laanwj Oct 18, 2016

Member

It is ugly, but overloading on type in JSON which is essentially dynamically typed is also ugly.

Named parameters as implemented in #8811 would make this more bearable.

@laanwj

laanwj Oct 18, 2016

Member

It is ugly, but overloading on type in JSON which is essentially dynamically typed is also ugly.

Named parameters as implemented in #8811 would make this more bearable.

This comment has been minimized.

@laanwj

laanwj Oct 18, 2016

Member

I do agree that for a new interface, a scale for verbosity would have made more sense instead of a boolean.
Maybe that's a better choice I'm just not sure. Changing the meaning of existing arguments is always annoying and means extra testing for backwards compatibility.

@laanwj

laanwj Oct 18, 2016

Member

I do agree that for a new interface, a scale for verbosity would have made more sense instead of a boolean.
Maybe that's a better choice I'm just not sure. Changing the meaning of existing arguments is always annoying and means extra testing for backwards compatibility.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 26, 2016

Member

I'm starting to realize that @luke-jr's idea to accept 0-2 isn't such a bad idea after all. Just rename the argument 'verbosityLevel'. It is easier to use and understand from a user perspective than two booleans (we regularly get confused there in the tests ourselves). Also it'd remove having to deal with the redundant and nonsensical combo verbose=false extraVerbose=true.

It should still accept false and true too. For consistency we should do the same in getrawtransaction. This method currently accepts a verbose argument that can be 0 or 1 but not false or true. The mapping false:0,true:1 should be added there (not in this pull though).

Also: needs rebase.

Member

laanwj commented Oct 26, 2016

I'm starting to realize that @luke-jr's idea to accept 0-2 isn't such a bad idea after all. Just rename the argument 'verbosityLevel'. It is easier to use and understand from a user perspective than two booleans (we regularly get confused there in the tests ourselves). Also it'd remove having to deal with the redundant and nonsensical combo verbose=false extraVerbose=true.

It should still accept false and true too. For consistency we should do the same in getrawtransaction. This method currently accepts a verbose argument that can be 0 or 1 but not false or true. The mapping false:0,true:1 should be added there (not in this pull though).

Also: needs rebase.

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Oct 26, 2016

Member

How should it document in the help message that the old behavior is still accepted?

Member

achow101 commented Oct 26, 2016

How should it document in the help message that the old behavior is still accepted?

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Oct 26, 2016

Member

Rebased and made verbose an int from 0-2

Member

achow101 commented Oct 26, 2016

Rebased and made verbose an int from 0-2

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Nov 24, 2016

Member

@achow101 The old behaviour should be considered deprecated, and therefore not documented.

Member

luke-jr commented Nov 24, 2016

@achow101 The old behaviour should be considered deprecated, and therefore not documented.

Show outdated Hide outdated src/rpc/blockchain.cpp
"\nArguments:\n"
"1. \"hash\" (string, required) The block hash\n"
"2. verbose (boolean, optional, default=true) true for a json object, false for the hex encoded data\n"
"\nResult (for verbose = true):\n"
"2. verbose (boolean, optional, default=1) 0 for hex encoded data, 1 for a json object, and 2 for json object with transaction data\n"

This comment has been minimized.

@luke-jr

luke-jr Nov 24, 2016

Member

Should be renamed to verbosity.

@luke-jr

luke-jr Nov 24, 2016

Member

Should be renamed to verbosity.

@achow101

This comment has been minimized.

Show comment
Hide comment
Member

achow101 commented Nov 24, 2016

@luke-jr done

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 21, 2016

Use a verbosity instead of two verbose parameters
Instead of having verbose and extraVerbose, verbose is just changed to an int. This can have values from 0-2 for each level of verbosity.

Github-Pull: #8704
Rebased-From: 82a491f
@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Jan 10, 2017

Member

merge please?

Member

achow101 commented Jan 10, 2017

merge please?

@luke-jr

luke-jr approved these changes Feb 3, 2017

utACK

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Feb 3, 2017

Member

(Needs rebase)

Member

luke-jr commented Feb 3, 2017

(Needs rebase)

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Feb 3, 2017

Member

rebased

Member

achow101 commented Feb 3, 2017

rebased

@luke-jr

luke-jr approved these changes Feb 3, 2017

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Feb 15, 2017

Contributor

Is this compatible with callers of getblock which send true instead of 0/1 ?

Contributor

pstratem commented Feb 15, 2017

Is this compatible with callers of getblock which send true instead of 0/1 ?

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Feb 15, 2017

Member

@pstratem Yes. The old style of calling is maintained for compatibility.

Member

achow101 commented Feb 15, 2017

@pstratem Yes. The old style of calling is maintained for compatibility.

Show outdated Hide outdated src/rpc/blockchain.cpp
" \"version\" : n, (numeric) The block version\n"
" \"versionHex\" : \"00000000\", (string) The block version formatted in hexadecimal\n"
" \"merkleroot\" : \"xxxx\", (string) The merkle root\n"
" \"tx\" : [ (array of Objects) The transactions\n"

This comment has been minimized.

@jnewbery

jnewbery Feb 15, 2017

Member

I'd suggest not including this in the help output, and just having something like:

           "  \"tx\" : [               (array of Objects) The transactions. Format is the same as for getrawtransaction call\n"
           "         ,...\n"
           "  ],\n"

If the output format of TxToJSON() ever changes, I'm sure that changing this help text will be forgotten!

@jnewbery

jnewbery Feb 15, 2017

Member

I'd suggest not including this in the help output, and just having something like:

           "  \"tx\" : [               (array of Objects) The transactions. Format is the same as for getrawtransaction call\n"
           "         ,...\n"
           "  ],\n"

If the output format of TxToJSON() ever changes, I'm sure that changing this help text will be forgotten!

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 9, 2017

Member

Needs rebase.

Member

sipa commented Apr 9, 2017

Needs rebase.

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Apr 10, 2017

Member

rebased. also added @jnewbery's suggestion with the help text.

Member

achow101 commented Apr 10, 2017

rebased. also added @jnewbery's suggestion with the help text.

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Apr 21, 2017

@@ -716,8 +719,29 @@ UniValue getblock(const JSONRPCRequest& request)
" \"previousblockhash\" : \"hash\", (string) The hash of the previous block\n"
" \"nextblockhash\" : \"hash\" (string) The hash of the next block\n"
"}\n"
"\nResult (for verbose=false):\n"
"\"data\" (string) A string that is serialized, hex-encoded data for block 'hash'.\n"
"\nResult (for verbosity = 2):\n"

This comment has been minimized.

@paveljanik

paveljanik May 12, 2017

Contributor

What about mentioning only the differences to verbosity=1 instead?

@paveljanik

paveljanik May 12, 2017

Contributor

What about mentioning only the differences to verbosity=1 instead?

This comment has been minimized.

@achow101

achow101 May 12, 2017

Member

something like this?

{
..., Same as for verbosity=1
"tx" : [           (array of Objects) The transactions in the format of the getrawtransaction RPC.
         , ...
],
... Same as for verbosity=1
}
@achow101

achow101 May 12, 2017

Member

something like this?

{
..., Same as for verbosity=1
"tx" : [           (array of Objects) The transactions in the format of the getrawtransaction RPC.
         , ...
],
... Same as for verbosity=1
}

This comment has been minimized.

@paveljanik

paveljanik May 12, 2017

Contributor

Yup, or maybe even better:

Additional output (for verbosity=1):...
@paveljanik

paveljanik May 12, 2017

Contributor

Yup, or maybe even better:

Additional output (for verbosity=1):...
Use a verbosity instead of two verbose parameters
Verbose is changed to an int. This can have values from 0-2 for each level of verbosity.
Verbosity level 2 has transaction details displayed in the results.
@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 May 12, 2017

Member

@paveljanik I made the change.

Member

achow101 commented May 12, 2017

@paveljanik I made the change.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 15, 2017

Member

Tested ACK e3c9f2d

Member

laanwj commented May 15, 2017

Tested ACK e3c9f2d

@laanwj laanwj merged commit e3c9f2d into bitcoin:master May 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request May 15, 2017

Merge #8704: [RPC] Transaction details in getblock
e3c9f2d Use a verbosity instead of two verbose parameters (Andrew Chow)
c99ab3c RPC: Allow multiple names for parameters (Luke Dashjr)

Tree-SHA512: 686b38f6b0106563738d51f55666fe6d49a5b121b30d4480c2bfb640a59ede8e6f7f3c05c3c5d80a5288e127991e191d19d1d4f9ace566fd39edeb27b31857ff

@achow101 achow101 deleted the achow101:getblock-extraverbose branch May 17, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 15, 2017

wirwolf added a commit to wirwolf/minexcoin that referenced this pull request Sep 28, 2017

Merge #9025: getrawtransaction should take a bool for verbose
240189b add testcases for getrawtransaction (John Newbery)
ce2bb23 getrawtransaction should take a bool for verbose (jnewbery)

(cherry picked from commit 4d8558a2871ec70b9d09028c49fa39e4911435f5)
RPC: Allow multiple names for parameters
Use a verbosity instead of two verbose parameters

Verbose is changed to an int. This can have values from 0-2 for each level of verbosity.
Verbosity level 2 has transaction details displayed in the results.

(cherry picked from commit e3c9f2ddb1e49f719cc1f3f7cc2a98626bee4364)

bitcoin/bitcoin#8704

zkbot added a commit to zcash/zcash that referenced this pull request May 1, 2018

Auto merge of #3179 - bitcartel:backport_transaction_details_in_getbl…
…ock_v2bitcartel, r=str4d

Add improvements to getblock RPC output

Includes and supercedes #3095. Includes code cherry-picked from bitcoin/bitcoin#8704.

zkbot added a commit to zcash/zcash that referenced this pull request May 1, 2018

Auto merge of #3179 - bitcartel:backport_transaction_details_in_getbl…
…ock_v2bitcartel, r=str4d

Add improvements to getblock RPC output

Includes and supercedes #3095. Includes code cherry-picked from bitcoin/bitcoin#8704.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment