💬 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)
```

💬 fjahr commented on pull request "net: Add interrupt to pcp retry loop":
(https://github.com/bitcoin/bitcoin/pull/33338#discussion_r2331175271)
`CThreadInterrupt& interrupt` is our preferred style though (see `PointerAlignment: Left` in `src/.clang-format`), so maybe we rather want to fix the other parameter to keep it consistent.

💬 achow101 commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3267672803)
ACK c76797481155754329ec6a6f58e8402569043944

🤔 mzumsande reviewed a pull request: "index: Fix coinstats overflow"
(https://github.com/bitcoin/bitcoin/pull/30469#pullrequestreview-3197889608)
Tested / Code Review ACK c76797481155754329ec6a6f58e8402569043944

Please adjust the PR description to the current approach ("switching most of the values tracked in the index from historical totals to values per block" is oudated).

I synced both current branch and master on signet with `undefined` sanitizer, and verified that muhash / blockstats at selected blocks are identical, and that the overflow doesn't happen anymore at height 112516.

💬 l0rinc 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-3267757875)
> A simple workaround for the use case here should be to use -blocksxor=0 in all datadirs.

But that would only work for a pristine IBD, right? I have opened https://github.com/bitcoin/bitcoin/pull/33324 to be able to change the obfuscation of existing blocks - this should allow you to remove an existing obfuscation after block download.