-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Refactor GetBlockVerboseTx to reflect correct getblock RPC call parameters. #1529
Conversation
Additional data from the RPC documentation:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're still referring to c.Verbose
here, a type which you deleted
See the CI output for details
@jakesyl thanks for the heads up, I'll fix that in another commit. |
@jcvernaleo (as per #1530)
|
@jakesyl @jcvernaleo Please review this when you get the chance, and sorry about all the tiny commits -- it's been a long night. Edit: Did a force push to my branch with a squash + rebase to upstream. |
… bitcoin json RPC verbosity format for getblock, which uses 0, 1, or 2 as parameters rather than a boolean true or false.
…getblock "hash" verbosity=1, and a second type for getblock "hash" verbosity=2. This is necessary due to how getblock returns a block's transaction data based on the provided verbosity parameter. If verbosity=1, then getblock.Tx is an array of a block's transaction ids (txids) as strings. If verbosity=2, then getblock.Tx is an array of raw transaction data.
…rboseResult, and FutureGetBlockVerboseTxResult. Due to differences in how getblock returns data based on the provided verbosity parameter, it's necessary to have two separate return types based on verbosity. This necessitates a separate unmarshalling function (represented throughout rpcclient/chain.go as Result.Receive()) to ensure that data is correctly unmarshalled and returned to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Could you squash it to one commit and then I'll merge it.
Does #1232 still block this? I don't think so but I might have missed it. |
Nope, read the test output wrong |
For future PRs, could we try to stick to squash and rebase first so we can stick with a linear history? |
@jcvernaleo my bad - I hit "Squash and Merge" and no clue what happened, haven't merged any non-squashed since |
Currently, rpcclient.GetBlockVerboseTx(...) does not return raw transaction data. This is due to how GetBlockCmd is implemented in btcjson; in upstream/master,
getblock
sends two boolean parameters (verbose
andverbosetx
), which throws an error.According to the bitcoin JSON RPC documentation,
getblock
accepts two parameters:blockhash
(as a string) andverbosity
(as an integer).verbosity
can be any of0
,1
, or2
, with0
being the default.This pull request aims to correctly implement
verbosity
as a parameter togetblock
by refactoringGetBlockCmd
(inbtcjson/chainsvrcmds.go
) to have two fields:Hash
andVerbosity
, withVerbosity
being an optional int pointer.In order to implement correct behavior, three primary changes are necessary: the above refactor, an additional type specific to
getblock verbosity=2
, and a newFuture
type to enable proper (and error-free) unmarshalling ofverbosity=2
data.The new type (
GetBlockVerboseTxResult
) is almost entirely the same asGetBlockVerboseResult
, but with the minor modification of changing theTx
field from a[]string
to a[]TxRawResult
so as to enable proper unmarshalling.The new
Future
type (FutureGetBlockVerboseTxResult
) returns aGetBlockVerboseTxResult
rather than aGetBlockVerboseResult
so that data can be properly returned to the user without error.