💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3267157378)
The feedback from @ryanofsky has been addressed.

💬 ryanofsky commented on pull request "draft: CMake Goals and Guidelines":
(https://github.com/bitcoin/bitcoin/pull/33317#issuecomment-3267190797)
Concept ACK. Seems nice to document goals of cmake build. To avoid confusion, I hope the document will be expanded to say which of the guidelines are currently being met, which are more aspirational, and what needs to change to meet the goals. i think it would also be helpful to link to PRs/issues where these topics have been discussed. Other ideas (not sure they are good but just in case):

- Maybe add this to the doc/design/ folder instead of the doc/ folder, especially if you think if will
...

💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2330865174)
An upstream issue: https://github.com/include-what-you-use/include-what-you-use/issues/1809.

💬 l0rinc commented on pull request "chore: compare against keys.size() in writeObject comma check":
(https://github.com/bitcoin/bitcoin/pull/33337#issuecomment-3267229143)
NACK, the existing value makes more sense to me, it's not solving any actual problem

👍 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?