💬 kevkevinpal commented on pull request "doc: Amend notes on benchmarking":
(https://github.com/bitcoin/bitcoin/pull/31690#issuecomment-2602619041)
ACK [2db6923](https://github.com/bitcoin/bitcoin/pull/31690/commits/2db6923332c7daa20d250cc5aa6bde080a7d0caf)

If it is wanted I can cherry pick the commits from https://github.com/bitcoin/bitcoin/pull/31153 and open up a PR to remove the various extraneous benchmarks

💬 fjahr commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-2602649006)
Code review ACK 2f19e42ddd85598438d0b8b49ef93c312c45c21b

🤔 jonatack reviewed a pull request: "doc: Amend notes on benchmarking"
(https://github.com/bitcoin/bitcoin/pull/31690#pullrequestreview-2562606947)
-0 on this, conceptually prefer ad hoc individual PR review over setting top-down collectivist norms. The latter will occur anyway, and ponderous notes like this will be used selectively and not evenhandedly.

💬 jonatack commented on pull request "doc: Amend notes on benchmarking":
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922530956)
Would prefer to drop this in favor of per-PR review feedback.

💬 maflcko commented on pull request "doc: Amend notes on benchmarking":
(https://github.com/bitcoin/bitcoin/pull/31690#issuecomment-2602655101)
> If it is wanted I can cherry pick the commits from #31153 and open up a PR to remove the various extraneous benchmarks

The goal of this pull request is to add useful context. Removing benchmarks can be done on a case-by-case basis in a separate pull request, if properly motivated. However, it is not the goal of this pull request to serve as the sole reason to remove (or refuse to add) benchmarks.

💬 maflcko commented on pull request "doc: Amend notes on benchmarking":
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922555138)
The goal here is to provide context that a change may be rejected if a performance improvement is the only goal and it can not be observed end-to-end. This has been the case in the past and the goal here is not to change anything about that.

Happy to close this pull, if this is obvious to everyone and the note isn't needed.

👍 0xBEEFCAF3 approved a pull request: "tests: improves tapscript unit tests"
(https://github.com/bitcoin/bitcoin/pull/31640#pullrequestreview-2562641059)
tACK 1bd2af1de6551d67c1c363b4cd04d4973e3650bf
Tested by rebasing the new [bip347 tapscript unit tests](https://github.com/bitcoin/bitcoin/pull/29247) on top of these changes.

💬 0xBEEFCAF3 commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#discussion_r1922550941)
Nit: I would use "taproot output key" here instead of "Tapscript Script Pubkey" per [BIP341](https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki)

🤔 fjahr reviewed a pull request: "rpc: fix mintime field testnet4, expand timewarp attack test"
(https://github.com/bitcoin/bitcoin/pull/31600#pullrequestreview-2562679974)
utACK 4d8dea9cb92efa362f72b0c2a5b89d312b452c0a

The comments in the tests are nice!

💬 fjahr commented on pull request "rpc: fix mintime field testnet4, expand timewarp attack test":
(https://github.com/bitcoin/bitcoin/pull/31600#discussion_r1922574227)
nit: Could have had anther info log here for completeness but completely optional

💬 0xB10C commented on pull request "ci: optionally use local docker build cache":
(https://github.com/bitcoin/bitcoin/pull/31545#issuecomment-2602731122)
> Rather than removing the previous local cache and replacing with the newly-created cache, would it make sense to use dockers build cache pruning mechanism, [`docker builder prune`](https://docs.docker.com/reference/cli/docker/builder/prune/), where we can specify removing either objects older than `x`, or to a certain storage (size) requirement?

I wasn't aware of `docker builder prune` - though I don't think it would help use here: If we keep dangling image layers around, they will remain i
...

💬 EthanHeilman commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#discussion_r1922598956)
Fixed!

💬 hebasto commented on pull request "util: fix compiler warning about deprecated space before _MiB":
(https://github.com/bitcoin/bitcoin/pull/31691#issuecomment-2602779258)
> There should be an rc1 in two weeks, so the CI could be bumped to that to avoid issues in the future: https://llvm.org/docs/HowToReleaseLLVM.html#annual-release-schedule

In the meantime, I've added a [nightly build](https://github.com/hebasto/bitcoin-core-nightly/pull/34) that uses the [LLVM Ubuntu nightly packages](https://apt.llvm.org/).

💬 maflcko commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2602793391)
Personally, I found this a bit easier to review by having the stuff that simply moves the fields from one struct to another in one commit and the remaining changes in another commit. This means the second commit would contain the following diff (and the third commit would contain whatever remains to restore an empty diff to the `HEAD` of this pull):


<details><summary>a diff</summary>

```diff
diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp
index 51c482607a..e657f018
...

👍 maflcko approved a pull request: "kernel: Move block tree db open to block manager"
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2562705180)
left some style-nits (only if you re-touch)

review ACK f3cfdf2b511776211a0399510ced735e556d510d 🥃

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0
...

💬 maflcko commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1922596559)
```suggestion
LogError("%s", e.what());
```

style-nit: Newline not needed. Alternatively, if you want to keep it move-only, I'd suggest to keep the name of `err` as `err`, instead of `e`.

💬 maflcko commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1922589316)
not sure if this was ever needed (maybe `m_blockman` is supposed to be under the chainman mutex?), but this is now a no-op, as the scope ends in the next line.

🚀 fanquake merged a pull request: "ci: Supply `--platform` argument to `docker` commands."
(https://github.com/bitcoin/bitcoin/pull/31657)

👍 fanquake approved a pull request: "ci: Bump centos stream 10"
(https://github.com/bitcoin/bitcoin/pull/31593#pullrequestreview-2562759079)
ACK faaabfaea768deb7767c489d32fd2097fd180872

💬 willcl-ark commented on pull request "ci: optionally use local docker build cache":
(https://github.com/bitcoin/bitcoin/pull/31545#issuecomment-2602807869)
> I wasn't aware of `docker builder prune` - though I don't think it would help use here: If we keep dangling image layers around, they will remain in the `blob` directory, but aren't referenced in the `index.json` file anymore. So even if they are available in theory, `docker build --cache-from` won't use them as they aren't indexed.
>
> See also [#31377 (comment)](https://github.com/bitcoin/bitcoin/pull/31377#issuecomment-2503777192) for an example index.json file.

Reading https://github
...