Skip to content

RPC: Add feature to getblock by using the index #16317

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

RPC: Add feature to getblock by using the index #16317

wants to merge 1 commit into from

Conversation

cvengler
Copy link
Contributor

Hello, this PR adds a feature to getblock which allows to get the blockdata directly without the need for using getblockhash first.

First it checks if you have provided an input smaller than 64 characters.
After that it checks if the input is a valid number or if it contains letters.
If it is a valid number it will request the blockhash for this index.

The feature to use the hash directly works of course as well.

If the user provides an input which is smaller than 64 characters and is not a valid number it will return an error.

The changes were tested by myself and they work.

The reason for this improvement is that it is too exaggerated for me to first get the hash of an index and then get the block data with two separate commands.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 30, 2019 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16240 (JSONRPCRequest-aware RPCHelpMan by kallewoof)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@promag
Copy link
Contributor

promag commented Jun 30, 2019 •
edited
Loading

This was attempted before and rejected. Last attempted #14858.

@laanwj
Copy link
Member

laanwj commented Jul 4, 2019 •
edited
Loading

Besides the conceptual discussion, I think this particular implementation has a few issues:

  • Interpreting a string as a number breaks API consistency; sure, from bitcoin-cli you won't be able to see the difference, but from other languages using RPC it's unexpected to have to handle a number differently from other places
  • Overloading based on how the value looks just feels wrong to me

If we really want this (I'm doubtful about this, but it seems to come up often) then my suggestion would be to define a new RPC call getblockbyheight instead of the usually brittle ways to re-define arguments.

@cvengler
Copy link
Contributor Author

cvengler commented Jul 4, 2019

Good point, haven't had this in mind when I was doing it.
To your second point, I think it would become problematic only if the block height becomes 64-digits long. But I'm also convinced now that a separate RPC call is probably a better solution.

@cvengler cvengler closed this Jul 4, 2019
@promag
Copy link
Contributor

promag commented Jul 4, 2019

What it the problem of 2 calls?

@cvengler
Copy link
Contributor Author

cvengler commented Jul 4, 2019

Actually I don't see a problem with two calls as it is the best solution.
But I would prefer to do this in a new pull request rather than by this.

@cvengler cvengler mentioned this pull request Jul 5, 2019
Closed
@ajtowns ajtowns mentioned this pull request Jul 23, 2019
Closed
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
RPC/REST/ZMQ
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cvengler @DrahtBot @promag @laanwj @fanquake