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

💬 BenWestgate commented on pull request "doc: update multisig tutorial to use multipath descriptors":
(https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2331256410)
Master returns xpub with change index so I replace() inline to maintain the same derivation depth.
This matches the method in our Tutorial and is less code. Finally, if the future wallet ever outputs a multipath descriptor, we'll get an xpub in this format and not have to add `/<0;1>/*`.

💬 Raimo33 commented on pull request "Fix benchmark CSV output":
(https://github.com/bitcoin/bitcoin/pull/33340#issuecomment-3267809025)
Code Review ACK 790b440197bde322432a5bab161f1869b667e681

💬 BenWestgate commented on pull request "doc: update multisig tutorial to use multipath descriptors":
(https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2331277450)
I don't understand this nit. I tried:
```bash
for ((n=1;n<=3;n++))
do
./build/bin/bitcoin rpc -signet createwallet "participant_${n}"
done
```
and got: `bash: ./build/bin/bitcoin: No such file or directory`

💬 achow101 commented on pull request "clang-tidy: Disable `UndefinedBinaryOperatorResult` check in `src/ipc`":
(https://github.com/bitcoin/bitcoin/pull/33312#issuecomment-3267858690)
ACK 589b65f06c3376df4cde3fac5c1d39a2d3254920

✅ achow101 closed an issue: "ci: tidy job is producing output"
(https://github.com/bitcoin/bitcoin/issues/33256)