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

💬 theuni 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_r2098347059)
Yeah, I had the same thought but I guess I came to a different conclusion. I figured if a false return only has meaning in the sync code-path, the true return should be enforced in the async as well. That way if someone ever tries to add a `return false` in the batched case, we'd catch it right away in testing.

💬 hebasto commented on pull request "threading: drop CSemaphore in favor of c++20 std::counting_semaphore":
(https://github.com/bitcoin/bitcoin/pull/32466#issuecomment-2895066428)
Concept ACK.

⚠️ portlandhodl opened an issue: "[RPC] deriveaddresses pk(descriptor) returns p2pkh address"
(https://github.com/bitcoin/bitcoin/issues/32570)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

While trying to extract the public key from a descriptor I might have walked into this bug. My thought was passing a pk(xpub) descriptor would return either an error or a pubkey to push instead it returned the p2pkh address. This is IMO inconsistent and frankly not very useful because the PK is behind the hash.

Note there are both pk and pkh types.

```
[bitcoin@qrsnaps0 ~]$ bitcoin-cli d
...

💬 fanquake commented on pull request "deps: Bump lief to 0.16.4":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2895075154)
I think it'd be worth investigating any issues with `0.16.5`, rather than deferring. Seeing some build output with this branch, which is unexpected:
```bash
[100%] Built target bitcoin-qt
Checking binary security...
have_gnu_relro:#True and have_bindnow: True
have_gnu_relro:#True and have_bindnow: True
have_gnu_relro:#True and have_bindnow: True
have_gnu_relro:#True and have_bindnow: True
have_gnu_relro:#True and have_bindnow: True
have_gnu_relro:#True and have_bindnow: True
have_gnu_r
...

💬 hebasto commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2098378254)
> Shall I just put it back?

Perhaps updating only the `Path` variable will be sufficient?

💬 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_r2098410495)
> That way if someone ever tries to add a `return false` in the batched case, we'd catch it right away in testing.

Catch what though? Catch CheckInputScripts detecting an invalid input and returning false like someone would reasonably expect it to? There aren't warnings or any indication inside CheckInputScripts saying that returning false may crash the program. So I don't think it's hard to imagine someone optimizing CheckInputScripts or refactoring or just adding a new check that returns fa
...