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

The getblock RPC signature is different from bitcoin core. #1334

Closed
lzl124631x opened this issue Oct 22, 2018 · 11 comments
Closed

The getblock RPC signature is different from bitcoin core. #1334

lzl124631x opened this issue Oct 22, 2018 · 11 comments

Comments

@lzl124631x
Copy link

In bitcoin core, it accepts a number for verbosity

2. verbosity              (numeric, optional, default=1) 0 for hex encoded data, 1 for a json object, and 2 for json object with transaction data

But we are using two booleans. Is it possible to use the same signature as bitcoin core?

@lzl124631x
Copy link
Author

hi there?

@lzl124631x
Copy link
Author

I can help create a PR if we want to make the signature consistent. Currently btcd cannot call the getblock on bitcoin core side.

@l0k18
Copy link
Contributor

l0k18 commented Nov 14, 2018

The code serving RPC requests is not that complicated, especially the parsers. It shouldn't be that difficult to make getblock 000xxxxxxxxxxxxxxxxxxxx true same as getblock 0xxxxxxxxxxxxxxxx 1. In bitcoin core configs, 0 is false and I think, anything else is true, or maybe 1 is true and anything else is false. It surely would not be complicated to parse both syntaxes, which yes would make it a little smoother integrating to other apps.

@lzl124631x
Copy link
Author

@l0k1verloren Thanks for your reply! I know it's not complicated so I want to get a confirmation on whether the project owners want this change.

bitcoind is using verbosity=0,1,2... while btcd is using Verbose=false/true, VerboseTx=false/true (see here and here)

So bitcoind's verbosity and btcd's flags are in these relationships:

verbosity=0 -> Verbose=false, VerboseTx=false
verbosity=1 -> Verbose=true, VerboseTx=false
verbosity=2 -> Verbose=true, VerboseTx=true

I like bitcoind's approach better, since it's merging multiple flags as verbosity levels and more extensible.

Golang is by design strong typed. I'm not sure it's advisable to support both int and boolean as parameter. I'd prefer to stick to one of them.

@l0k18
Copy link
Contributor

l0k18 commented Nov 14, 2018

There is no type rules telling you whether you parse a string to be a number or text.

Implementing both syntaxes in one is not that hard when there is a fixed set of possible options appearing. We have therefore 0-2 possible options. Nothing means defaults. A single parameter is either a number or assigns one of those values. Two parameters you know they both must be named values. I urge you to try implementing both, it would be one less thing to complicate swapping out bitcoind for btcd for applications.

@lzl124631x
Copy link
Author

lzl124631x commented Nov 14, 2018

@l0k1verloren
In this thread, I never though from the difficulty perspective.
May I ask what's the benefit you think of keeping the Verbose=false/true, VerboseTx=false/true syntax? Just for backward compatibility? Think from the customer's perspective, "come on, why do you guys provide two ways to specify verbosity level. Don't make me think."

Besides, I'm more thinking from a compatibility perspective. I don't want to surprise the user when he noticed that both verbosity and Verbose+VerboseTx work for btcd but only verbosity works for bitcoind. (I was surprised btcd's interface is different from bitcoind. My expectation was that they are compatible)

@l0k18
Copy link
Contributor

l0k18 commented Nov 14, 2018

Perhaps the syntax should also be altered by the rpcquirks flag. In fact now I think about it, why isn't it already? Being able to turn-key slot btcd in place of bitcoin core is very nice, and yes, I agree, what was the point of creating a new syntax at all?

@xlk3099
Copy link

xlk3099 commented Nov 21, 2018

A pull request has been opened long time back, #1112

@lzl124631x
Copy link
Author

@xlk3099 Thanks. I'm not alone.

@Rjected
Copy link
Collaborator

Rjected commented May 25, 2020

Should be fixed by #1529 and #1560

@jcvernaleo
Copy link
Member

Closing since handled as stated above.

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

No branches or pull requests

5 participants