Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

I think it makes sense to do count = 0 as a single entity, and >= 1 as an array, or otherwise make an additional as_array bool param to control the output type.

Otherwise the "type instability" is difficult to deal with for downstream consumers of the API. E.g., consider the code for https://docs.rs/bitcoincore-rpc/latest/src/bitcoincore_rpc/client.rs.html#337-341 .

I think it makes sense to do count = 0 as a single entity, and >= 1 as an array, or otherwise make an additional as_array bool param to control the output type.

Otherwise the "type instability" is difficult to deal with for downstream consumers of the API. E.g., consider the code for https://docs.rs/bitcoincore-rpc/latest/src/bitcoincore_rpc/client.rs.html#337-341 .

Thanks for the reply. Note that there is no regression impact because all preexisting functionality is intact. Ultimately, it is an optional parameter which can be ignored.

To me, count = 0 is a bit counterintuitive. And as mentioned above, the REST call also interprets count = 0 as invalid, assuming we are aiming to emulate an improved version of the REST functionality in RPC.

From the old PR, it would appear that we are tending away from the unformatted (concatenated) output in new implementation. If this is indeed the case, then such an additional parameter does not make seem useful.

@MarcoFalke Do you have any input on this? And are any further updates to the PR needed at this point? All CI checks have passed. Thanks.

What about:

• null -> "flat" reply
• int -> array reply

What about:

• null -> "flat" reply
• int -> array reply

@JeremyRubin's suggestion has been implemented. I think I misunderstood the difference between count = 0 and count = 1 in terms of the "type instability". It makes sense to me now.

As you've denoted, this is now resolved such that:
count = 0 -> "flat" reply
count > 0 -> array reply

Let me know if any other changes are still needed. Thanks.

Yeah that makes sense. Null = flat works too. Idea isn't to concatenate anything, but rather to provide support for legacy behavior and something that matches the REST API. Since 0 is invalid in rest, all valid queries in JSON will match all valid rest queries, and legacy count=0 queries will return flat.

If we were to do a breaking API change, i'd say always return array :)

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

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

cc @jonatack @luke-jr @LarryRuane

We will be reviewing this PR this Wednesday https://bitcoincore.reviews/25261
@natanleung please join us if you can! (But certainly no pressure or obligation!)

@natanleung please remove the second commit, "Merge branch 'bitcoin:master' into rpc_getblockheader_count"

PRs should not include such merge commits. Actually, it looks like this is a rebase, not a merge commit (the merge commit is empty). But you should not rebase onto latest master unless there is a conflict, see rebasing changes. @DrahtBot will usually let you know (although there can be hidden conflicts that DrahtBot doesn't detect).

You don't want to merge master into this commit with a merge commit. Rather you want to rebase your commit on top of master. You could do that with something like (double check this first!):

# reset to your first commit
git reset --hard 053ccf0468e477283e80f78cc095ffb83bff9b95
# rebase onto master if you have remote configured as "upstream"
git fetch --all && git rebase upstream/master

edit: @LarryRuane beat me to it :)

Not sure why I didn't earlier, but...

Concept NACK. Why shouldn't this be accomplished through JSON-RPC batching?

Why shouldn't this be accomplished through JSON-RPC batching?

My initial intuitive reasoning was that this approach would be significantly faster than batching because of the overhead of each request as well as requiring the blockhash for each request. After doing some performance testing, that doesn't seem to be true. In which case, I think I'm leaning towards Concept NACK too.

Best case scenario (on my setup-up): to fetch n blockheaders getblockheader with count parameter was ~2x as fast as fetching it through a batch request. Even though that is a significant speed-up, I don't think it's significant enough to warrant more code complexity, even if it is quite modest as in this PR. Even though there are some UX benefits on cli to not having to query the blockhashes separately etc, I think this should be solved by tooling and not within Core to keep complexity minimal.

On signet, I've timed how long it takes to fetch n blockheaders using 3 different approaches:
loop: (not really discussed here, just included FYI/illustrative purposes) iteratively send getblockheader requests for single blockhash until n headers are fetched, using the blockhash from the result to query the next one. Only for n<=500 because it started failing for more requests due to timeouts etc.
batch: first send a single batch getblockhash request to query all blockhashes we need, then send a single batch getblockheader request for all the hashes
count: iteratively send getblockheader requests with count=2000 (or less), using the blockhash from the last header in the result to query the next array until n headers are fetched

Time required to fetch n blockheaders:
-----
For 50 blockheaders:
loop: 0.0480s
batch: 0.0066s (and 0.0090s to fetch blockhashes)
count: 0.0038s
-----
For 500 blockheaders:
loop: 0.4594s
batch: 0.0170s (and 0.0063s to fetch blockhashes)
count: 0.0102s
-----
For 2000 blockheaders:
batch: 0.0476s (and 0.0105s to fetch blockhashes)
count: 0.0264s
-----
For 20000 blockheaders:
batch: 0.4847s (and 0.0865s to fetch blockhashes)
count: 0.2556s
-----
For 100000 blockheaders:
batch: 2.5618s (and 0.4156s to fetch blockhashes)
count: 1.2770s

import json
import time

import requests


def get_n_blockhashes(n: int, start_height: int = 0):
    data = [{
        "method": "getblockhash",
        "params": [height]
    } for height in range(start_height, start_height + n)]
    hashes = [item["result"] for item in make_request(data)]
    return hashes


def get_n_headers_batch(hashes):
    """Call getblockheader one time with an array of data items, one for each blockhash"""
    data = []
    for blockhash in hashes:
        data.append({"method": "getblockheader", "params": [blockhash]})
    headers = [item["result"] for item in make_request(data)]
    return headers


def get_n_headers_loop(n: int):
    """Call getblockheader n times to get one blockheader each time"""
    headers = []
    blockhash = get_n_blockhashes(1)[0]
    while len(headers) < n:
        data = {"method": "getblockheader", "params": [blockhash]}
        header = make_request(data)["result"]
        blockhash = header["nextblockhash"]
        headers.append(header)

    return headers


def get_n_headers_count(n: int):
    """Call getblockheader with the `count` parameter until n headers are fetched"""
    max_per_request = 2000
    remaining = n
    headers = []
    blockhash = get_n_blockhashes(1)[0]
    while remaining > 0:
        count = min(remaining, max_per_request)
        data = {
            "method": "getblockheader",
            "params": [blockhash, True, count]
        }
        headers += make_request(data)["result"]
        remaining -= count
        blockhash = headers[-1]["nextblockhash"]
    return headers


def make_request(data):
    url = "http://user:pass@127.0.0.1:38332/"
    r = requests.post(url, data=json.dumps(data))
    assert r.status_code == 200
    return r.json()


def time_function(fn, *args, last_hash_check: str, **kwargs) -> float:
    """Return average fn time execution and check that the last obtained blockheader hash matches last_hash_check """
    iters = 10
    start = time.perf_counter()
    for i in range(iters):
        result = fn(*args, **kwargs)
        assert result[-1]["hash"] == last_hash_check
    avg_duration = (time.perf_counter() - start) / iters
    return avg_duration


if __name__ == '__main__':
    for n in [50, 500, 2000, 20000, 100000]:
        print(f"For {n} blockheaders:")

        hash_start = time.perf_counter()
        hashes = get_n_blockhashes(n)
        hash_time = time.perf_counter() - hash_start

        if n <= 500:
            print(f"loop: {time_function(get_n_headers_loop, n, last_hash_check=hashes[-1]):.4f}s")
        print(f"batch: {time_function(get_n_headers_batch, hashes, last_hash_check=hashes[-1]):.4f}s (and {hash_time:.4f}s to fetch blockhashes)")
        print(f"count: {time_function(get_n_headers_count, n, last_hash_check=hashes[-1]):.4f}s")
        print("-----")

I tested your code @stickies-v and, despite my timings coming out significantly slower than yours (curious about that...), found the same trends, so would tend to agree that this can be done using batching:

❯ ./benchmark_headers.py
For 50 blockheaders:
loop: 0.1021s
batch: 0.0109s (and 0.0049s to fetch blockhashes)
count: 0.0060s
-----
For 500 blockheaders:
loop: 1.0044s
batch: 0.0782s (and 0.0215s to fetch blockhashes)
count: 0.0254s
-----
For 2000 blockheaders:
batch: 0.3283s (and 0.0768s to fetch blockhashes)
count: 0.1245s
-----
For 20000 blockheaders:
batch: 3.0241s (and 0.7651s to fetch blockhashes)
count: 1.1144s
-----
For 100000 blockheaders:
batch: 15.4626s (and 3.8391s to fetch blockhashes)
count: 5.6350s
-----

will@ubuntu in bitcoin on  rpc_getblockheader_count [?] via 🐍 v3.6.15 took 4m34s

edit: debug=1 in config was slowing it down quite a bit

According to #23330 (comment) from the previous PR, the original author considered RPC batching.

I took on this PR as an up-for-grabs ticket so I thought this was desired functionality. If the changes look good, can I fix the CI failure, write tests, and close? Thanks.

I took on this PR as an up-for-grabs ticket so I thought this was desired functionality.

First of all, really appreciate you working on this and contributing - and I hope you'll continue to regardless of the outcome of this PR! Unfortunately, in the decentralized development model of Core, we don't really have a well-defined concept of "desired functionality" - we don't have anyone deciding on features/roadmaps/... It all comes down to emergent consensus among developers. I think in this case, the concern is that even though the PR does seem like an improvement in some areas (e.g. performance, UX) it is also a deterioration in other areas (maintainability, unnecessarily complex method parameters). Some reviewers including myself seem to think that this feature is not desired as we think the cons outweigh the pros. If you look at the original issue #23330, you'll see that there was no clear consensus yet on whether or not batching was a feasible alternative, which could be a hint that desirability is unclear.

The comments in #23330 mention lack of atomicity (i.e. reorgs could happen during the batch request), but I'm not sure that's an important concern. It's quite trivial for the client to inspect that the headers are indeed part of the same chain (each header links to the previous header), and since reorgs rarely happen to more than a few blocks this would never be any kind of performance concern. Moreover, the approach in this PR also doesn't have that kind of atomicity (although it could be introduced with more aggressive locking of cs_main.)

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

With seeming lack of agreement around the concept, and no further follow-up from the author, I think this can be closed?

Going to close for now. Looks like the author has also disappeared from GitHub.

Hey, thanks for keeping up with this PR. I am definitely still interested in working on this, but due to the lack of clarity, I don't know how to proceed. The last few posts suggest that this isn't really wanted, so what would be the best way to modify this into something that is?

If there is something that would be delivered out of this, then I would be more than happy to work on implementing that. Let me know what you all think. Thanks!