WIP: [RPC] allow fetching headers by pages #23330

pull JeremyRubin wants to merge 1 commits into bitcoin:master from JeremyRubin:header-fetch changing 1 files +63 −10

JeremyRubin commented at 10:37 PM on October 20, 2021: contributor

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.
fanquake added the label RPC/REST/ZMQ on Oct 20, 2021
[RPC] allow fetching headers by pages b13b8ca3be
JeremyRubin force-pushed on Oct 21, 2021
JeremyRubin commented at 3:27 PM on October 21, 2021: contributor

I had a great question come in on twitter: https://twitter.com/lightclients/status/1450955679116587016

Q: Why not just encourage RPC Batching? A: Because this RPC takes parameters of a Hash (and not height), we can't call the i+1st RPC until we know the result of the i-th.

in src/rpc/blockchain.cpp:911 in b13b8ca3be

 902 | @@ -876,6 +903,14 @@ static RPCHelpMan getblockheader()
 903 |      if (!request.params[1].isNull())
 904 |          fVerbose = request.params[1].get_bool();
 905 |  
 906 | +    int count = 0;
 907 | +    if (!request.params[2].isNull())
 908 | +        count = request.params[2].get_int();
 909 | +    if (count < 0)
 910 | +        throw JSONRPCError(RPC_INVALID_PARAMETER, "Count must be >= 0");
 911 | +    if (count > 2000)

brunoerg commented at 1:21 PM on October 22, 2021:

maybe else if here?

MarcoFalke commented at 3:15 PM on October 22, 2021:

The early-return statements are already mutually exclusive, so no need for else. Though {} would be good for new code.

brunoerg commented at 3:26 PM on October 22, 2021:

Yea, sorry, didn't notice the throw

JeremyRubin commented at 4:57 PM on October 22, 2021:

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.

luke-jr commented at 4:41 PM on October 29, 2021: member

Batch getblockhash and then batch this?

in src/rpc/blockchain.cpp:912 in b13b8ca3be

 907 | +    if (!request.params[2].isNull())
 908 | +        count = request.params[2].get_int();
 909 | +    if (count < 0)
 910 | +        throw JSONRPCError(RPC_INVALID_PARAMETER, "Count must be >= 0");
 911 | +    if (count > 2000)
 912 | +        throw JSONRPCError(RPC_INVALID_PARAMETER, "Count must be <= 2000");

luke-jr commented at 4:42 PM on October 29, 2021:

Maybe it should just stop at 2000?

JeremyRubin commented at 4:50 PM on October 29, 2021:

goal was to match the Rest API

in src/rpc/blockchain.cpp:943 in b13b8ca3be

 944 | +        if (!fVerbose) {
 945 | +            CDataStream ssBlocks(SER_NETWORK, PROTOCOL_VERSION);
 946 | +            for (int i = 0; i < count && pblockindex != nullptr; ++i, pblockindex = pblockindex->pprev) {
 947 | +                ssBlocks << pblockindex->GetBlockHeader();
 948 | +            }
 949 | +            std::string strHex = HexStr(ssBlocks);

luke-jr commented at 4:44 PM on October 29, 2021:

IMO it would make more sense to return an Array of Strings, each with a single block header serialised.

JeremyRubin commented at 4:49 PM on October 29, 2021:

goal was to have the same output format as the rest API.

JeremyRubin commented at 4:55 PM on October 29, 2021: contributor

Batch getblockhash and then batch this?

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).
DrahtBot commented at 2:59 AM on November 19, 2021: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #23703 (scripted-diff: Use named args in RPC docs by MarcoFalke)
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.
DrahtBot added the label Needs rebase on Dec 9, 2021
DrahtBot commented at 10:45 AM on December 9, 2021: member



🐙 This pull request conflicts with the target branch and needs rebase.

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
DrahtBot commented at 12:15 PM on March 21, 2022: member
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
- Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
- Is it no longer relevant? ➡️ Please close.
- Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.
MarcoFalke added the label Up for grabs on May 12, 2022
MarcoFalke commented at 1:13 PM on May 12, 2022: member

Marked "up for grabs"
natanleung commented at 2:06 PM on June 1, 2022: none

As this PR is marked as "up for grabs", I have opened a new PR #25261 that reimplements this functionality.
fanquake removed the label Up for grabs on Jun 1, 2022
fanquake removed the label Needs rebase on Jun 1, 2022
fanquake closed this on Jun 1, 2022
DrahtBot locked this on Jun 1, 2023

Contributors

Labels

Linked (view graph)

#25261 rpc: fetch multiple headers in getblockheader()

WIP: [RPC] allow fetching headers by pages #23330

Conflicts