WIP: [RPC] allow fetching headers by pages#23330
WIP: [RPC] allow fetching headers by pages#23330JeremyRubin wants to merge 1 commit intobitcoin:masterfrom
Conversation
JeremyRubin
force-pushed
the
header-fetch
branch
from
2fddbd3 to
b13b8ca
Compare
|
I had a great question come in on twitter: https://twitter.com/lightclients/status/1450955679116587016 Q: Why not just encourage RPC Batching? |
| count = request.params[2].get_int(); | ||
| if (count < 0) | ||
| throw JSONRPCError(RPC_INVALID_PARAMETER, "Count must be >= 0"); | ||
| if (count > 2000) |
There was a problem hiding this comment.
The early-return statements are already mutually exclusive, so no need for else. Though {} would be good for new code.
There was a problem hiding this comment.
Yea, sorry, didn't notice the throw
There was a problem hiding this comment.
ah -- I didn't realize the preference was to have it on a single line as per:
If an if only has a single-statement then-clause, it can appear on the same line as the if, without braces. In every other case, braces are required, and the then and else clauses must appear correctly indented on a new line.
|
Batch |
| if (count < 0) | ||
| throw JSONRPCError(RPC_INVALID_PARAMETER, "Count must be >= 0"); | ||
| if (count > 2000) | ||
| throw JSONRPCError(RPC_INVALID_PARAMETER, "Count must be <= 2000"); |
There was a problem hiding this comment.
Maybe it should just stop at 2000?
There was a problem hiding this comment.
goal was to match the Rest API
| ssBlocks << pblockindex->GetBlockHeader(); | ||
| } | ||
| std::string strHex = HexStr(ssBlocks); |
There was a problem hiding this comment.
IMO it would make more sense to return an Array of Strings, each with a single block header serialised.
There was a problem hiding this comment.
goal was to have the same output format as the rest API.
you actually need to getblockheader to get the height, then getblockhash for the N you need, and then call getblockheader for the N. And this still has a concurrency bug since the getblockhash do not execute atomically afaiu (a reorg could happen in between). |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
Marked "up for grabs" |
|
As this PR is marked as "up for grabs", I have opened a new PR #25261 that reimplements this functionality. |
7 participants
One feature in the REST API not in the JSON RPC (afaict) is that you can fetch a page of headers (up to 2k) at a time via rest but only single headers via RPC. This patch adds a count argument to the rpc to allow for similar functionality without having to make N RPC calls (more efficient).
The API i chose is if the count = 0, then a single header will be returned (matching the previous behavior). If count > 0, then the returned result will either be a concatenated hex string array or a json object array. Perhaps a bikesheddable semantic, but I think it's reasonable to do it this way given that consumers of the API probably never actually want to return 0 headers, but may want the return type to be consistently an array whether 1 or 2 or 1000 headers are requested.