Skip to content
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

[Chore] Backport GetBlockVerbosity Fixes & Add getbalances RPC client command #389

Merged
merged 15 commits into from
Jul 22, 2020
Merged

[Chore] Backport GetBlockVerbosity Fixes & Add getbalances RPC client command #389

merged 15 commits into from
Jul 22, 2020

Conversation

JettScythe
Copy link
Contributor

@zquestz
Copy link
Contributor

zquestz commented Jul 20, 2020

It is important to note we had a previous PR that had this functionality but it never landed in btcd. This is the latest and greatest code and includes a bunch of small fixes. =)

@zquestz zquestz requested review from cpacia and zquestz July 20, 2020 00:23
JettScythe and others added 2 commits July 19, 2020 22:50
updated docs for getblock-verbosity fixes
@JettScythe
Copy link
Contributor Author

Added commit for

  • Add getbalances RPC client command

btcsuite/btcd#1595

@JettScythe JettScythe requested a review from zquestz July 20, 2020 04:31
@zquestz
Copy link
Contributor

zquestz commented Jul 20, 2020

Added commit for

  • Add getbalances RPC client command

btcsuite/btcd#1595

I don't think that ABC or BCHN have actually picked up the getbalances call...

@JettScythe JettScythe changed the title [Chore] Backport GetBlockVerbosity Fixes [Chore] Backport GetBlockVerbosity Fixes & Add getbalances RPC client command Jul 20, 2020
@zquestz
Copy link
Contributor

zquestz commented Jul 20, 2020

Actually, I stand corrected, BCHN doesn't have it, but ABC does as of 0.21.9!

@JettScythe
Copy link
Contributor Author

ABC enabled getbalances as of 0.21.9 :)

@cculianu
Copy link

cculianu commented Jul 20, 2020

Guys please keep the getblock call (and any other calls) consistent with BCHN, ABC, BU and core. All 3 expect bool for the verbosity flag.

Changing the verbosity flag to int and throwing an error if it’s bool introduces a needless bchd specific quirk and/or a special case code path in codebases (such as mine) that try to support multiple bitcoin node implementations... for bchd now I have to add more code and spend time dealing with quirks of new bchd vs old bchd vs whatever .. this is a problem for all existing systems.

It also risks introducing a possible regression for all the code out there sending bools.

Please make verbosity remain bool to avoid this.

@cculianu
Copy link

Also I want to point out that for getblock — ABC accepts both int and bool there.

@zquestz
Copy link
Contributor

zquestz commented Jul 20, 2020

Actually ABC/BCHN can accept an int or a bool, and if you use a bool it can't actually use the highest verbosity level. It accepts 0, 1, 2 as ints, or a bool.

We can make sure a bool still works before this PR lands.

Technically any consumer still sending a bool is sending the deprecated format, and can't even take full advantage of the different output modes.

@zquestz
Copy link
Contributor

zquestz commented Jul 20, 2020

Here are the current docs for ABC and BCHN.

root@bitcoin-555854b677-xsnvk:/# bitcoin-cli -rpcuser=$BITCOIN_RPC_USER -rpcpassword=$BITCOIN_RPC_PASSWORD help getblock
getblock "blockhash" ( verbosity )

If verbosity is 0 or false, returns a string that is serialized, hex-encoded data for block 'hash'.
If verbosity is 1 or true, returns an Object with information about block <hash>.
If verbosity is 2, returns an Object with information about block <hash> and information about each transaction.

@zquestz
Copy link
Contributor

zquestz commented Jul 20, 2020

@JettScythe want to try and make sure we can also support a boolean value? That would make sure we were completely compatible before this lands.

@cculianu
Copy link

cculianu commented Jul 20, 2020

@zquestz thanks for clarifying everything. Yeah well the docs make no claim that boolean is deprecated (EDIT: Oh I see -- you consider that deprecated; got it).

There is the matter of existing code that is using bchd (that is now sending bool because int is rejected) that will insta-break if you take only int now.. and then they have the problem of having to probe bchd version to figure out which bchd they are talking to. (I'm talking about myself here and my own pain :D ).

Thanks man I don't see the harm in supporting both. I'm super glad you are doing that. 👍

Technically any consumer still sending a bool is sending the deprecated format, and can't even take full advantage of the different output modes.

Hmm. As Obi Wan Kenobi famously said: "That is true... from a certain point of view"

@zquestz
Copy link
Contributor

zquestz commented Jul 21, 2020

@cculianu the latest commits should make sure boolean values continue to work as expected in the API.

  • bchctl allows for getblock <hash> <int/bool/string>. Where string is a valid boolean value like "false", "true".
  • The API server will accept raw requests with (0,1,2,true,false) and should be fully backward compatible.

Please test and make sure things work as you expect. =)

Example working bchctl commands.

bchctl getblock "00000000000000000003c9b8bcf48b943f9b9174143419665f7b9d35146c42d6"
bchctl getblock "00000000000000000003c9b8bcf48b943f9b9174143419665f7b9d35146c42d6" 0
bchctl getblock "00000000000000000003c9b8bcf48b943f9b9174143419665f7b9d35146c42d6" 1
bchctl getblock "00000000000000000003c9b8bcf48b943f9b9174143419665f7b9d35146c42d6" 2
bchctl getblock "00000000000000000003c9b8bcf48b943f9b9174143419665f7b9d35146c42d6" true
bchctl getblock "00000000000000000003c9b8bcf48b943f9b9174143419665f7b9d35146c42d6" false
bchctl getblock "00000000000000000003c9b8bcf48b943f9b9174143419665f7b9d35146c42d6" "true"
bchctl getblock "00000000000000000003c9b8bcf48b943f9b9174143419665f7b9d35146c42d6" "false"

Invalid bchctl commands.

bchctl getblock "00000000000000000003c9b8bcf48b943f9b9174143419665f7b9d35146c42d6" 3
bchctl getblock "00000000000000000003c9b8bcf48b943f9b9174143419665f7b9d35146c42d6" "bad"

Working curl commands.

curl --header "Content-Type: application/json" https://127.0.0.1:8334/ -d '{ "jsonrpc": "2.0", "id":   1, "method": "getblock", "params": ["0000000000000000024f31fa48016e8462fbd338e64e2a61930f2dd253ee3c1e"] }' -k
curl --header "Content-Type: application/json" https://127.0.0.1:8334/ -d '{ "jsonrpc": "2.0", "id":   1, "method": "getblock", "params": ["0000000000000000024f31fa48016e8462fbd338e64e2a61930f2dd253ee3c1e", 0] }' -k
curl --header "Content-Type: application/json" https://127.0.0.1:8334/ -d '{ "jsonrpc": "2.0", "id":   1, "method": "getblock", "params": ["0000000000000000024f31fa48016e8462fbd338e64e2a61930f2dd253ee3c1e", 1] }' -k
curl --header "Content-Type: application/json" https://127.0.0.1:8334/ -d '{ "jsonrpc": "2.0", "id":   1, "method": "getblock", "params": ["0000000000000000024f31fa48016e8462fbd338e64e2a61930f2dd253ee3c1e", 2] }' -k
curl --header "Content-Type: application/json" https://127.0.0.1:8334/ -d '{ "jsonrpc": "2.0", "id":   1, "method": "getblock", "params": ["0000000000000000024f31fa48016e8462fbd338e64e2a61930f2dd253ee3c1e", true] }' -k
curl --header "Content-Type: application/json" https://127.0.0.1:8334/ -d '{ "jsonrpc": "2.0", "id":   1, "method": "getblock", "params": ["0000000000000000024f31fa48016e8462fbd338e64e2a61930f2dd253ee3c1e", false] }' -k

@zquestz zquestz merged commit 7244caf into gcash:master Jul 22, 2020
@JettScythe JettScythe deleted the getblock-verbosity branch July 22, 2020 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants