👍 ryanofsky approved a pull request: "ci, iwyu: Treat warnings as errors for specific directories"
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-3197403345)
Code review ACK d03d861cb6d81f90f3b4add2db3dbdc0bf95a0ef. Just fixing up comment and changing variable name since last review. Thanks for the updates!

💬 ryanofsky commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2330883192)
re: https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2330818442

In commit "ci: Do not patch `leveldb` to workaround UB in "tidy" CI job" (b7248ee2462a26418f0ef1e0a0f04950269d37aa)

Thanks, with new comment I think I understand why this is needed now. This is fine as is, but in case you do want to update it, I think a change like following would make the link between `git diff` and the UB detector clearer:

```diff
--- a/ci/test/03_test_script.sh
+++ b/ci/test/03_test_script.sh
...

💬 l0rinc commented on pull request "Fix benchmark CSV output":
(https://github.com/bitcoin/bitcoin/pull/33340#discussion_r2330890677)
We should likely update the arm version as well: https://github.com/bitcoin/bitcoin/pull/33340/files#diff-687f22b3470aa3123586a32b91f6aaead5bd364298e6b91954cd9a2ef3f00763L683

💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2330899302)
Thanks! The comments have been reworked per your suggestion.

💬 purpleKarrot commented on pull request "draft: CMake Goals and Guidelines":
(https://github.com/bitcoin/bitcoin/pull/33317#issuecomment-3267270664)
Thanks for the review, @ryanofsky.

I wanted to write down the goals first, and then create issues/PRs to make changes towards the goals bottom up: that means starting from libmultiprocess, minisketch, secp256k1. bitcoin-core will serve as an example how those libraries are consumed as subprojects.

In the next step, bitcoin-core should follow the same goals and guidelines, because external clients may want to consume bitcoinkernel as a subproject, or because bitcoin-gui is extracted as anot
...

🤔 enirox001 reviewed a pull request: "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet"
(https://github.com/bitcoin/bitcoin/pull/33268#pullrequestreview-3197482104)
Tested ACK 113a422. Ran the full functional test suite including `wallet_anchor.py`; all tests passed. Fix for 0 value anchor detection and sendall size errors looks good. LGTM.

💬 hebasto commented on pull request "Fix benchmark CSV output":
(https://github.com/bitcoin/bitcoin/pull/33340#discussion_r2330912591)
Thanks! Fixed.

📝 HowHsu opened a pull request: "help: enrich help text for `-loadblock`"
(https://github.com/bitcoin/bitcoin/pull/33343)
`-loadblock` doesn't support XOR-ed files, mention it in its help text to avoid troubles for users.

👍 l0rinc approved a pull request: "Fix benchmark CSV output"
(https://github.com/bitcoin/bitcoin/pull/33340#pullrequestreview-3197485791)
Code review ACK 790b440197bde322432a5bab161f1869b667e681

💬 m3dwards commented on pull request "ci: reduce runner sizes on various jobs":
(https://github.com/bitcoin/bitcoin/pull/33319#issuecomment-3267291957)
ACK 5eeb2facbbbbf68a2c30ef9e6747e39c85d7b116

💬 HowHsu commented on issue "use -loadblock to load blk*****.dat files, but the blocks in it are not recognized":
(https://github.com/bitcoin/bitcoin/issues/33280#issuecomment-3267297305)
> I looked at the code, and it appears that `loadblock` currently only works with non-xor'ed block files: [AutoFile file{fsbridge::fopen(path, "rb")};](https://github.com/bitcoin/bitcoin/blob/3eea9fd39532eeda648e44de365fc4c9112f6fc6/src/node/blockstorage.cpp#L1253C9-L1254C1) doesn't use an obfuscation key.
>
> I don't know if that was discussed when block obfuscation was added (FYI [@maflcko](https://github.com/maflcko)). `loadblock` is probably not a feature that is used very much these days,
...

💬 m3dwards commented on pull request "ci: disable cirrus cache in 32bit arm job":
(https://github.com/bitcoin/bitcoin/pull/33302#discussion_r2330965907)
This works but I do feel a little icky about using a truthy string value of 'false'. I realise this gets passed to bash which doesn't have types but it is used in gha expression which does have concept of truthy. It took me a second to understand how `false || true` resulted in false and then I clocked it was a string.

Do you think it's too invasive to perhaps change the input from a boolean "use-cirrus" to a string "cache-provider" with values of "cirrus" and "gha"?

🤔 janb84 reviewed a pull request: "ci: Checkout latest merged pulls"
(https://github.com/bitcoin/bitcoin/pull/33303#pullrequestreview-3197606159)
re ACK fa8f081af31cd99155c7545643e7b10beb26714d

changes since last ACK:
- Restore of comment in code

🤔 janb84 reviewed a pull request: "Fix benchmark CSV output"
(https://github.com/bitcoin/bitcoin/pull/33340#pullrequestreview-3197683091)
code review ACK 790b440197bde322432a5bab161f1869b667e681

PR changes the comma's from the static text in the `SHA256AutoDetect` return output to semicolons so that it will not interfere with Comma-Separated Values (CSV) output.

💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3267485313)
> Q1: Could `LocationsIndex` live on the Electrum Server (bindex-rs) side, with bitcoind instead having a REST endpoint for reading a span of bytes from a given block?

Great idea, thanks!
I would try this approach in a separate `bindex` branch, to evaluate its performance.

> Q2: Why is it better to implement a slow fallback in blockstorage than to fail over to requiring TxIndex? Is it a question of not wanting to store both blockhash+N and txid in case users switch around their configurat
...

🤔 fjahr reviewed a pull request: "net: check for empty header before calling FillBlock"
(https://github.com/bitcoin/bitcoin/pull/33296#pullrequestreview-3197621693)
Concept ACK

It's great that there is now documentation for this but it's still not ideal that some magic state has this special meaning that is very implicit. I would have liked it more if it was explicit but I haven't spend enough time on this part of the code base what would be ideal and I don't want this to block v30 more than necessary. Maybe a simple bool flag `failed_reconstruction` on `PartiallyDownloadedBlock` would work? Feel free to ignore.

💬 fjahr commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2331007821)
Hm, curious if this should maybe be a different (unique) message so we can clearly distinguish which case between this one and the one below was hit?

💬 fjahr commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2331085528)
Log strings don't require a newline at the end anymore since #30929

👍 hodlinator approved a pull request: "net: Add interrupt to pcp retry loop"
(https://github.com/bitcoin/bitcoin/pull/33338#pullrequestreview-3196452837)
utACK 188de70c86414b8b2ad5143f5c607b67686526ea

Change just passes down `interrupt` for checking in inner loops.

---

Didn't figure out a way of reproducing the behavior PR-author describes in https://github.com/bitcoin/bitcoin/pull/33338#issuecomment-3265618371.
* Wasn't able to bring to (zombie) life a local IPv6 gateway.
* Only have one ethernet interface on my local machine. Seems the author may have a handful.
* Reproduced something similar anyway through hacking this in:
```di
...

💬 hodlinator commented on pull request "net: Add interrupt to pcp retry loop":
(https://github.com/bitcoin/bitcoin/pull/33338#discussion_r2330188929)
nit: Would prefer keeping the lambda as the last parameter and also avoiding to mix `&`-placement within the same argument list.
```suggestion
CThreadInterrupt &interrupt,
std::function<bool(std::span<const uint8_t>)> check_packet)
```