💬 laanwj commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#discussion_r2098190718)
Done, thanks

🚀 fanquake merged a pull request: "Remove legacy `Parse(U)Int*`"
(https://github.com/bitcoin/bitcoin/pull/32520)

💬 fanquake commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2894739331)
```bash
181155a38912fab7e25d751d1f0281d7127ca8c309e2835bf0b109f30d425f91 guix-build-db895c0e3842/output/aarch64-linux-gnu/SHA256SUMS.part
1b89a9dc265956572226b7a1558f8cb24c6f65f490c82382cbfba16500c91d9c guix-build-db895c0e3842/output/aarch64-linux-gnu/bitcoin-db895c0e3842-aarch64-linux-gnu-debug.tar.gz
7180be8208cfad61ba6fda74d10f942769a8e3c7cbd6a141b3d53e9905d1a76f guix-build-db895c0e3842/output/aarch64-linux-gnu/bitcoin-db895c0e3842-aarch64-linux-gnu.tar.gz
459deb9325f13dc780a138e52e10f
...

💬 furszy commented on issue "rpc method removeprunedfunds should take an array of txids":
(https://github.com/bitcoin/bitcoin/issues/29466#issuecomment-2894743434)
> Can't you just make a batch RPC request?

Cannot batch wallet db writes by batching RPC requests.

💬 fanquake commented on pull request "depends: use "mkdir -p" when installing xproto":
(https://github.com/bitcoin/bitcoin/pull/32568#discussion_r2098215129)
Added a comment.

👍 ryanofsky approved a pull request: "wallet: Use `util::Error` throughout `AddWalletDescriptor` instead of returning `nullptr` for some errors"
(https://github.com/bitcoin/bitcoin/pull/32475#pullrequestreview-2854592072)
Code review ACK 785e1407b0a39fef81a7b25554aab88d4cecd66b

🤔 pablomartin4btc reviewed a pull request: "wallet: Use `util::Error` throughout `AddWalletDescriptor` instead of returning `nullptr` for some errors"
(https://github.com/bitcoin/bitcoin/pull/32475#pullrequestreview-2854602428)
Concept ACK

👍 ryanofsky approved a pull request: "Add checkBlock() to Mining interface"
(https://github.com/bitcoin/bitcoin/pull/31981#pullrequestreview-2854605689)
Code review ACK 975991571de3f11399ca01753e41753e0008b5a0. Since last review just dropped some RPC changes which were not strictly necessary and added test comments

💬 pablomartin4btc commented on pull request "wallet: Use `util::Error` throughout `AddWalletDescriptor` instead of returning `nullptr` for some errors":
(https://github.com/bitcoin/bitcoin/pull/32475#discussion_r2098230420)
I guess `reference_wrapper` then is still needed in CWallet::AddWalletDescriptor?

🤔 hebasto reviewed a pull request: "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage"
(https://github.com/bitcoin/bitcoin/pull/32467#pullrequestreview-2854708043)
Approach ACK 833dc7b9e3672b9a9033d02802ac60a48d4a8b3c, I have reviewed the code and it looks OK. Still testing.

> It also adds an `IN_PLACE_LOCK` macro for `UniqueLock` initialization which may be helpful elsewhere.

Update macro name in the PR description?

💬 fanquake commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#discussion_r2098299332)
I think you can just drop these `#ifdef`s around our own headers. Same for in `RunCommandParseJSON` below.

👍 hebasto approved a pull request: "depends: use "mkdir -p" when installing xproto"
(https://github.com/bitcoin/bitcoin/pull/32568#pullrequestreview-2854723778)
ACK df9ebbf659d5d1282289f36d7f9ee7103aa33a17, I have reviewed the code and it looks OK.

💬 fanquake commented on pull request "cmake: Remove `ENABLE_{SSE41,AVX2,X86_SHANI,ARM_SHANI}` from `bitcoin-build-config.h`":
(https://github.com/bitcoin/bitcoin/pull/32551#issuecomment-2894938226)
Concept ACK

💬 hebasto commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2894938428)
My Guix build:
```
aarch64
181155a38912fab7e25d751d1f0281d7127ca8c309e2835bf0b109f30d425f91 guix-build-db895c0e3842/output/aarch64-linux-gnu/SHA256SUMS.part
1b89a9dc265956572226b7a1558f8cb24c6f65f490c82382cbfba16500c91d9c guix-build-db895c0e3842/output/aarch64-linux-gnu/bitcoin-db895c0e3842-aarch64-linux-gnu-debug.tar.gz
7180be8208cfad61ba6fda74d10f942769a8e3c7cbd6a141b3d53e9905d1a76f guix-build-db895c0e3842/output/aarch64-linux-gnu/bitcoin-db895c0e3842-aarch64-linux-gnu.tar.gz
459deb93
...

💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2098307589)
The only tool we need is msbuild.exe which is what my original version did? Shall I just put it back?

👍 ryanofsky approved a pull request: "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage"
(https://github.com/bitcoin/bitcoin/pull/32467#pullrequestreview-2854727001)
Code review ACK 833dc7b9e3672b9a9033d02802ac60a48d4a8b3c. Main changes since last review were adding lock annotations and a new commit to clarify CheckInputScripts.

I definitely think the new commit 833dc7b9e3672b9a9033d02802ac60a48d4a8b3c is good and makes code less confusing. I left one suggestion about it, but it is not important.

💬 ryanofsky commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2098305199)
In commit "validation: clean up and clarify CheckInputScripts logic" (833dc7b9e3672b9a9033d02802ac60a48d4a8b3c)

Not important, but I think it is not ideal for CheckInputScripts return value to be handled inconsistently and sometimes abort. It makes it more difficult for CheckInputScripts to change in the future (to add checks or shift between delayed and upfront checking) if one caller is making stronger assumptions than the other callers about how it works and what it may return. Would sugge
...

💬 hebasto commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2098321034)
> The only tool we need is msbuild.exe...

We also use now `mt.exe`: https://github.com/bitcoin/bitcoin/blob/dc403f0c13e1ceee3d4651b4056ff2bad47d0315/.github/workflows/ci.yml#L246-L253

💬 fanquake commented on pull request "random: Use modern Windows randomness functions":
(https://github.com/bitcoin/bitcoin/pull/32400#issuecomment-2894970975)
> This change only partially resolves https://github.com/bitcoin/bitcoin/issues/32391

What is left unsolved after this PR?

💬 fanquake commented on pull request "doc: add alpine build instructions":
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2098329990)
Don't think you need `m4` here?

💬 ryanofsky commented on pull request "wallet: Use `util::Error` throughout `AddWalletDescriptor` instead of returning `nullptr` for some errors":
(https://github.com/bitcoin/bitcoin/pull/32475#discussion_r2098345031)
re: https://github.com/bitcoin/bitcoin/pull/32475#discussion_r2098230420

> I guess `reference_wrapper` then is still needed in CWallet::AddWalletDescriptor?

Yes it's still probably better to use reference_wrapper there instead of using a pointer type which could be null. I think more ideally util::Result would support reference types, but it doesn't currently. (When I wrote previous comment I though #25665 would allow util::Result to support reference types, but that's not actually true. I
...