rpc: fetch multiple headers in getblockheader() #25261

natanleung commented at 2:02 PM on June 1, 2022: none

Motivation: getblockheader() has the count parameter to fetch more than one header in a single GET via REST.

This patch extends this functionality to bitcoin-cli and properly formats output arrays based on the verbose parameter. This is a follow-up to an up-for-grabs issue #23330.

Implementation: I took into account the review comments and previous implementation. The CLI method is used as follows:

bitcoin-cli getblockheader <block_hash> <verbose=true> <count=0>

count defaults to 0.

If count is excluded or count = 0, the preexisting functionality is used to return the single block header of block <hash> in either hex or JSON format based on verbose.

If 0 < count <= 2000, an array of block headers starting with block <hash> is returned in either hex or JSON format based on verbose.

The previous implementation traversed the ActiveChain upwards (towards the genesis block), but again, this differs from the REST functionality that traverses the ActiveChain downwards (away from the genesis block), which I replicated instead. If count surpasses the number of blocks from the desired <HASH> to the active tip block, then the final block header in the array is simply that of the newest block on the ActiveChain.

No regression impact.

~~The previous implementation defaulted to 0, but in the REST syntax, this is actually an invalid count value. I believe that it makes sense to return a single block header by default.~~

If count < 0 or count > 2000, return an INVALID_PARAMETER error.
If count is not an integer, report a generic JSON parsing error.

Changes:

The argument has been added to rpc/client.cpp.
The getblockheader() help message has been updated accordingly.
The getblockheader() functionality has been expanded, as described above.

in src/rpc/blockchain.cpp:558 in ac3083bc32 outdated

 552 | @@ -539,10 +553,16 @@ static RPCHelpMan getblockheader()
 553 |      if (!request.params[1].isNull())
 554 |          fVerbose = request.params[1].get_bool();
 555 |  
 556 | +    int count = 1;
 557 | +    if (!request.params[2].isNull())
 558 | +        count = request.params[2].get_int();

maflcko commented at 2:14 PM on June 1, 2022:

    const auto count{request.params[2].isNull() ? 1 : request.params[2].getInt<int>()};

can be written in a single line

in src/rpc/blockchain.cpp:560 in ac3083bc32 outdated

 552 | @@ -539,10 +553,16 @@ static RPCHelpMan getblockheader()
 553 |      if (!request.params[1].isNull())
 554 |          fVerbose = request.params[1].get_bool();
 555 |  
 556 | +    int count = 1;
 557 | +    if (!request.params[2].isNull())
 558 | +        count = request.params[2].get_int();
 559 | +    if ((count <= 0) || (count > 2000))
 560 | +        throw JSONRPCError(RPC_INVALID_PARAMETER, "Header count is invalid or out of acceptable range (1-2000): " + std::to_string(count));

maflcko commented at 2:16 PM on June 1, 2022:

        throw JSONRPCError(RPC_INVALID_PARAMETER, "Header count is invalid or out of acceptable range (1-2000): " + ::ToString(count));

DrahtBot added the label RPC/REST/ZMQ on Jun 1, 2022

natanleung requested review from maflcko on Jun 1, 2022

maflcko commented at 3:57 PM on June 1, 2022: member

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

JeremyRubin commented at 6:02 PM on June 1, 2022: contributor

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 .

natanleung commented at 6:54 PM on June 1, 2022: none

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.

maflcko commented at 6:57 PM on June 1, 2022: member

What about:

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

natanleung commented at 7:39 PM on June 1, 2022: none

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.

JeremyRubin commented at 8:34 PM on June 1, 2022: contributor

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 :)

in src/rpc/blockchain.cpp:573 in a2a73a2360 outdated

 569 | @@ -552,15 +570,40 @@ static RPCHelpMan getblockheader()
 570 |          throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
 571 |      }
 572 |  
 573 | -    if (!fVerbose)
 574 | +    if (count == 0)

luke-jr commented at 1:41 AM on June 18, 2022:

I don't think it's a good idea to duplicate the logic for each "mode".

Better to build a 1-item array, and return only headers[0] as a special case.

in src/rpc/blockchain.cpp:509 in a2a73a2360 outdated

 502 | @@ -503,9 +503,10 @@ static RPCHelpMan getblockheader()
 503 |                  {
 504 |                      {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash"},
 505 |                      {"verbose", RPCArg::Type::BOOL, RPCArg::Default{true}, "true for a json object, false for the hex-encoded data"},
 506 | +                    {"count", RPCArg::Type::NUM, RPCArg::Default{0}, "Number of headers to get (maximum: 2000)"},
 507 |                  },
 508 |                  {
 509 | -                    RPCResult{"for verbose = true",
 510 | +                    RPCResult{"for verbose = true, count = 0",

luke-jr commented at 1:41 AM on June 18, 2022:

Prefer restricting special case to count=null

luke-jr changes_requested

natanleung requested review from luke-jr on Jun 19, 2022

DrahtBot commented at 1:35 AM on June 21, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	luke-jr, stickies-v
Concept ACK	LarryRuane

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.

achow101 commented at 7:26 PM on October 12, 2022: member

cc @jonatack @luke-jr @LarryRuane

LarryRuane commented at 1:23 PM on November 14, 2022: contributor

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!)

in src/rpc/blockchain.cpp:596 in 053ccf0468 outdated

 569 | @@ -552,15 +570,23 @@ static RPCHelpMan getblockheader()
 570 |          throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
 571 |      }
 572 |  
 573 | -    if (!fVerbose)
 574 | +    UniValue headers{UniValue::VARR};
 575 | +    do
 576 |      {

LarryRuane commented at 2:04 PM on November 16, 2022:

    do {

in src/rpc/blockchain.cpp:577 in 053ccf0468 outdated

 577 | -        CDataStream ssBlock(SER_NETWORK, PROTOCOL_VERSION);
 578 | -        ssBlock << pblockindex->GetBlockHeader();
 579 | -        std::string strHex = HexStr(ssBlock);
 580 | -        return strHex;
 581 | +        if (!fVerbose)
 582 | +        {

LarryRuane commented at 2:04 PM on November 16, 2022:

        if (!fVerbose) {

in src/rpc/blockchain.cpp:587 in 053ccf0468 outdated

 587 | +        else
 588 | +            headers.push_back(blockheaderToJSON(tip, pblockindex));
 589 | +
 590 | +        pblockindex = chainman.ActiveChain().Next(pblockindex);
 591 |      }
 592 | +    while ((headers.size() < (long unsigned int)count) && (pblockindex != nullptr));

LarryRuane commented at 2:07 PM on November 16, 2022:

    } while ((headers.size() < static_cast<size_t>(count) && (pblockindex != nullptr));

in src/rpc/blockchain.cpp:583 in 053ccf0468 outdated

 583 | +            CDataStream ssBlock(SER_NETWORK, PROTOCOL_VERSION);
 584 | +            ssBlock << pblockindex->GetBlockHeader();
 585 | +            headers.push_back(HexStr(ssBlock));
 586 | +        }
 587 | +        else
 588 | +            headers.push_back(blockheaderToJSON(tip, pblockindex));

LarryRuane commented at 2:11 PM on November 16, 2022:

        } else {
            headers.push_back(blockheaderToJSON(tip, pblockindex));
        }

in src/rpc/blockchain.cpp:585 in 053ccf0468 outdated

 585 | +            headers.push_back(HexStr(ssBlock));
 586 | +        }
 587 | +        else
 588 | +            headers.push_back(blockheaderToJSON(tip, pblockindex));
 589 | +
 590 | +        pblockindex = chainman.ActiveChain().Next(pblockindex);

LarryRuane commented at 2:27 PM on November 16, 2022:

I'm pretty sure calling Next() requires the thread to hold cs_main. But I don't think you want to grab and release the lock on each iteration of this loop; it's probably better to take the lock close to where you currently do, and hold it throughout this loop.

But it's not ideal to hold cs_main during the conversions to JSON or (for non-verbose) encodings to hex. So you may want to consider creating a vector of CBlockindex pointers, have each iteration of this loop push_back() to this vector, drop cs_main, then run a separate loop to format the results. (This is okay because once you have a CBlockIndex pointer, both the pointer and the object it points to remain valid without having to hold cs_main.)

willcl-ark commented at 4:14 PM on November 16, 2022:

It appears to be what caused CI to fail with -Wthread-safety-analysis enabled.

natanleung commented at 11:02 PM on November 20, 2022:

I have implemented @LarryRuane's suggestion to store the CBlockindex pointers and then iterate through them. However, the CI failure is still occurring. From what I recall, this wasn't happening before. Any insight into why this is now the case?

stickies-v commented at 2:36 PM on November 21, 2022:

Have you looked at the CI logs? You can view them by clicking "details" next to the CI failure message here on GitHub. The error message is quite clear in this case:

rpc/blockchain.cpp:591:32: error: calling function 'ActiveChain' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
        pblockindex = chainman.ActiveChain().Next(pblockindex);

You can't call ActiveChain() without holding the cs_main mutex, as is done e.g. a few blocks of code above this line.

LarryRuane commented at 2:38 PM on November 16, 2022: contributor

Concept and approach ACK, my suggestions are mostly formatting, except the comment about locking needs careful consideration.

This doc helps with formatting. But we don't accept everything it suggests, especially in RPC code where we use some non-standard formatting conventions.

This PR could also benefit from some tests. (But I understand the current lack of tests may be intentional because you want to get at least concept or approach ACKs before going to the trouble of writing tests.)

LarryRuane commented at 4:40 PM on November 16, 2022: contributor

@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).

willcl-ark commented at 4:40 PM on November 16, 2022: contributor

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 :)

in src/rpc/blockchain.cpp:518 in b29c8cc29f outdated

 514 | @@ -515,9 +515,10 @@ static RPCHelpMan getblockheader()
 515 |                  {
 516 |                      {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash"},
 517 |                      {"verbose", RPCArg::Type::BOOL, RPCArg::Default{true}, "true for a json object, false for the hex-encoded data"},
 518 | +                    {"count", RPCArg::Type::AMOUNT, RPCArg::Default{"null"}, "Number of headers to get (maximum: 2000)"},

stickies-v commented at 4:44 PM on November 16, 2022:

Could put 2000 in a variable and re-use it in all 3 locations for clarity and to prevent it from going out of sync

stickies-v commented at 5:50 PM on November 16, 2022:

I don't think this needs to be an AMOUNT, NUM would be preferred I think? We don't want floats or strings here, just integers.

                    {"count", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "Number of headers to get (maximum: 2000)"},

LarryRuane commented at 6:37 PM on November 16, 2022:

Yes, but I'd prefer no limit. We don't have to be compatible with REST to that extent. RPC clients aren't DoS threats. They can DoS the node anyway by running many of these as quickly as possible, and even in parallel.

in src/rpc/blockchain.cpp:569 in b29c8cc29f outdated

 564 | @@ -551,10 +565,14 @@ static RPCHelpMan getblockheader()
 565 |      if (!request.params[1].isNull())
 566 |          fVerbose = request.params[1].get_bool();
 567 |  
 568 | +    const auto count{request.params[2].isNull() ? 0 : request.params[2].getInt<int>()};
 569 | +    if ((count < 0) || (count > 2000))

stickies-v commented at 4:59 PM on November 16, 2022:

I think currently passing count=0 means we return a single (non-array) header. That's not how it's documented, and also imo unintuitive and undesired. I think we can be firm in rejecting count=0, and only allowing count=null for the single non-array header result.

    if ((count < 1) || (count > 2000))

willcl-ark commented at 5:13 PM on November 16, 2022:

I think it was thought here that returning a single non-array header was best to maintain backwards compatibility

LarryRuane commented at 6:27 PM on November 16, 2022:

I think it was thought here that returning a single non-array header was best to maintain backwards compatibility

You still can by specifying count=null (or taking the default, which is the same).

in src/rpc/blockchain.cpp:568 in b29c8cc29f outdated

 564 | @@ -551,10 +565,14 @@ static RPCHelpMan getblockheader()
 565 |      if (!request.params[1].isNull())
 566 |          fVerbose = request.params[1].get_bool();
 567 |  
 568 | +    const auto count{request.params[2].isNull() ? 0 : request.params[2].getInt<int>()};

stickies-v commented at 4:59 PM on November 16, 2022:

I would store request.params[2].isNull() into a bool return_array and avoid relying on a magic count==0 value. That makes the code more readable imo.

natanleung commented at 10:59 PM on November 20, 2022:

This change has been implemented as follows:

bool returnArray = !request.params[2].isNull();

The negation operator ! is used because if count != null, then an array is returned.

stickies-v commented at 5:02 PM on November 16, 2022: contributor

~Concept ACK~

Agreed with @LarryRuane that functional tests need updating to cover this.

Edit: leaning towards Concept NACK now

luke-jr commented at 6:35 PM on November 16, 2022: member

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

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

stickies-v commented at 2:53 PM on November 17, 2022: contributor

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

<details> <summary>performance benchmarking results</summary>

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

</details>

<details> <summary>performance testing code (`-signet -rpcuser user -rpcpassword pass`)</summary>

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("-----")

</details

willcl-ark commented at 4:29 PM on November 17, 2022: contributor

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

rpc: fetch multiple headers in getblockheader()

getblockheader() has the count parameter to fetch more than one header in a single GET via REST.

This patch extends this functionality to the CLI and properly formats output arrays based on the verbose parameter.

73f513507f

natanleung commented at 11:09 PM on November 20, 2022: none

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.

natanleung removed review request from luke-jr on Nov 21, 2022

natanleung removed review request from maflcko on Nov 21, 2022

natanleung requested review from stickies-v on Nov 21, 2022

natanleung removed review request from stickies-v on Nov 21, 2022

natanleung requested review from luke-jr on Nov 21, 2022

natanleung removed review request from luke-jr on Nov 21, 2022

natanleung requested review from LarryRuane on Nov 21, 2022

natanleung removed review request from LarryRuane on Nov 21, 2022

natanleung requested review from willcl-ark on Nov 21, 2022

natanleung removed review request from willcl-ark on Nov 21, 2022

natanleung requested review from stickies-v on Nov 21, 2022

natanleung removed review request from stickies-v on Nov 21, 2022

natanleung requested review from maflcko on Nov 21, 2022

natanleung removed review request from maflcko on Nov 21, 2022

natanleung requested review from luke-jr on Nov 21, 2022

stickies-v commented at 2:32 PM on November 21, 2022: contributor

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.)

DrahtBot added the label Needs rebase on Jan 26, 2023

DrahtBot commented at 11:34 AM on January 26, 2023: contributor

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

stickies-v commented at 11:56 AM on January 26, 2023: contributor

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

fanquake commented at 12:25 PM on January 26, 2023: member

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

fanquake closed this on Jan 26, 2023

natanleung commented at 3:26 PM on January 26, 2023: none

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!

bitcoin locked this on Jan 26, 2024