💬 maflcko commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2111303331)
> Reading the undo data directly from storage (i.e. without re-serialization) results in ~3.2x requests per second, and even a bit less data sent over the network:

Hmm. I am not sure if this is representative of the end-to-end performance. Simply fetching the raw (compressed) data is missing the decompression overhead. The end product can probably not do much with the compressed data by itself, so the benchmark should include the decompression as well.

💬 fanquake commented on pull request "guix: accomodate migration to codeberg":
(https://github.com/bitcoin/bitcoin/pull/32439#issuecomment-2915516389)
Backported to `29.x` in #32589.

💬 stringintech commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2915572044)
> An alternative would be to not give these peers unlimited time but clear their block requests (so that these blocks can be requested from other peers) so that they wouldn't be disconnected but also wouldn't be able to stall us indefinitely.

This makes sense, as the PR tries to achieve a similar goal with initial headers sync timeouts in commit 64b956f.

👍 hodlinator approved a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2874254446)
re-ACK 63854e8a81fdef3ca99ebd339db72563d053b9d0

While the code is made less straightforward due to optimizations, I think it's still worth doing it to minimize overhead from obfuscation.

Changes since first ACK (https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2852896828):
* Changed commit messages to include benchmark results from more recent GCC and newer vanilla instead of Apple-flavoured Clang.
* Modified unit test to add verification of obfuscation starting at non-ali
...

📝 strmfos opened a pull request: "Replace dead gnome link notificator.cpp"
(https://github.com/bitcoin/bitcoin/pull/32629)
Hey team—noticed a dead link, replaced it with a working URL
https://developer.gnome.org/notification-spec/ - old link
https://developer.gnome.org/documentation/tutorials/notifications.html - new link

💬 furszy commented on pull request "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/31423#issuecomment-2915854169)
Rebased, fixed a typo and added an extra commit deduplicating the wallet re-loading code.

> There are some typos reported, which look real: [#31423 (comment)](https://github.com/bitcoin/bitcoin/pull/31423#issuecomment-2520009159).

done thanks. It would be neat to have the bot outputting the typo commit hash too (just a small idea, maybe someone else wants to implement it).

💬 furszy commented on pull request "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/31423#discussion_r2111558553)
if your implementation works, we could replace the current sqlite `Files()` code for your code (using `sqlite3_filename_database` and `sqlite3_filename_journal` instead of the hardcoded path).

🤔 ismaelsadeeq reviewed a pull request: "random: Use modern Windows randomness functions"
(https://github.com/bitcoin/bitcoin/pull/32400#pullrequestreview-2874584557)
Post Merge Concept and Code review ACK 6b4bcc16234575108bb691c15c3532198d9bf98a

I found this issue orthogonally, and discovered that the fix has already being merged which is nice.

I verified that the implementation here matches the API https://learn.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptgenrandom

💬 hebasto commented on pull request "Replace dead gnome link notificator.cpp":
(https://github.com/bitcoin/bitcoin/pull/32629#discussion_r2111680585)
I don't see how the new link helps with understanding the `Notificator::notifyDBus()` function code Perhaps it's best to just remove the outdated comment?

🤔 maflcko reviewed a pull request: "ci, iwyu: Treat warnings as errors for specific targets"
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-2874741428)
left some comments

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific targets":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2111687366)
Ok, fair enough. It may be fine, and should be trivial to revert, if it doesn't work as expected.

However, I worry a bit about the output not being copy-paste-able for developers to apply to their source code.

Not sure if this is a valid concern, but an alternative approach would be to just call `git diff` below on the `TARGETS_WITH_FORCED_IWYU` folders (not targets). Then exit, when the diff is not empty?

This should also address the issue of having to run twice.

The only downside I
...

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific targets":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2111679951)
nit: no need to disable shellcheck here via `bash -c`?

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific targets":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2111680762)
nit: Could inline the `TARGETS_WITH_FORCED_IWYU` to avoid the `bash -c` disable of shellcheck?

🤔 rkrux reviewed a pull request: "wallet, rpc: Return normalized descriptor in parent_descs"
(https://github.com/bitcoin/bitcoin/pull/32594#pullrequestreview-2874402429)
ACK fdabfc7b01ec2cff5f2356e1b13b5537d30dbaf8

Thanks for incorporating the previously suggested small changes.

💬 rkrux commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2111461649)
```python
or c == "'"
```

I don't think checking for apostrophe is really required specifically in this case because as I checked in the code that only the hardened derive types are used when the SPKMs are setup & that's what this test is using.
https://github.com/bitcoin/bitcoin/blob/88b22acc3d6faeb2c0860695783a2a082d4c8098/src/wallet/walletutil.cpp#L58-L75

And the `ToNormalizedString` doesn't use the compact string version as well.
https://github.com/bitcoin/bitcoin/blob/88b22acc3d6
...

💬 rkrux commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2111471325)
I wanted to see how the case would be handled by `CHECK_NONFATAL` when for some buggy reason the call here fails. I simulated a scenario and found that the error is propagated to the RPC result - good.

```bash
➜ bitcoin git:(getaddrinfo-normalized-parent) ✗ bitcoinclireg listunspent
error code: -1
error message:
Internal bug detected: false
wallet/rpc/util.cpp:115 (PushParentDescriptors)
Bitcoin Core v29.99.0-a759a22d59e8-dirty
Please report this issue here: https://github.com/bitcoin
...

💬 fanquake commented on pull request "deps: Bump lief to 0.16.5":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2916237433)
> Thanks for the suggested fix @hebasto! Bumping the guix time machine to

I don't think we need to bump anything here (putting aside the fact that the suggested change breaks the riscv & windows builds, and will make many other changes to the release env). We can just apply a one line patch to LIEF. i.e:
```diff
Partially revert f23ced2f4ffc170d0a6f40ff4a1bee575e3447cf

Restore compat with python-scikit-build-core 0.10.x
Can be dropped when using python-scikit-build-core 0.11.x

--- a
...

💬 hebasto commented on pull request "deps: Bump lief to 0.16.5":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2916430210)
> ```diff
> Restore compat with python-scikit-build-core 0.10.x
> Can be dropped when using python-scikit-build-core 0.11.x
> ```

It seems should be:
```
Restore compat with python-scikit-build-core 0.9.x
Can be dropped when using python-scikit-build-core 0.10.x
```

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2916452571)
Rebased 65fe5d03e7a2d0d00d7d37bd426fd6532fff3c06 -> 1417e0b3b1b03dd014a3459c10a5ae7ab0c3687f ([kernelApi_38](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_38) -> [kernelApi_39](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_39), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_38..kernelApi_39))

* Fixed conflict with #32528

💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#issuecomment-2916512903)
ACK https://github.com/bitcoin/bitcoin/pull/31553/commits/e1f501cab224608004e3a1eee7a48eec3cea25fc

I appreciate the additional coverage added; I know it's not particularly blackbox but should catch the most basic regressions going forward.