💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1987014769)
```diff
-const SigningProvider& arg
+const SigningProvider&
```

`arg` is not used in this function, though not caused by this PR.

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1986101951)
IMO `GetPrivKey` commit 72694b8d1a9863771cf1095037caeafb3f1a5635 changes deserves a mention in the PR description, currently only the pubkey changes are mentioned.

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1987021149)
```diff
-// All subproviders must be inserting a valid origin already
+// `m_provider` must be inserting a valid origin already
```

Using the word subproviders here strips this function off its individuality and instead brings in context from its caller. At any time, there would just be one subprovider that is referenced by the `OriginPubkeyProvider` class member `m_provider`.

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1987033523)
+1 for removing the usage of `entries.back().first, entries.back().second` as the output parameters to the `GetPubKey()` - this was not intuitive to read for me.

💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#discussion_r1987131185)
I removed this translation on Transifex.

Additionally, I promoted the following translation checks from warning to errors:
- "New lines at the beginning of a source string are preserved in its translations."
- "New lines at the end of a source string are preserved in its translations."

💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#discussion_r1987163464)
Removed this one and other cases (bitcoin addresses) from this translation.

💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#discussion_r1987171128)
Fixed on Transifex.

👍 hodlinator approved a pull request: "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets"
(https://github.com/bitcoin/bitcoin/pull/32019#pullrequestreview-2670874014)
re-ACK 0ecd2e0748bdbfc12af1fc100862a1a6f046c2c9

Good that nsis and zip could be made optional unless deploying.

Only re-tested Windows-side, but Mac side diff looks equivalent.

Without nsis:
```shell
/mnt/c/Users/hodlinator/bitcoin$ cmake --build build --target deploy
Error: NSIS not found
Built target deploy
```

I needed to re-run `cmake -B build --toolchain depends/x86_64-w64-mingw32/toolchain.cmake` for the build to pick up that makensis was available. It would be good if we
...

💬 hebasto commented on pull request "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets":
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2710603528)
> re-ACK [0ecd2e0](https://github.com/bitcoin/bitcoin/commit/0ecd2e0748bdbfc12af1fc100862a1a6f046c2c9)
>
> Good that nsis and zip could be made optional unless deploying.
>
> Only re-tested Windows-side, but Mac side diff looks equivalent.
>
> Without nsis:
>
> ```shell
> /mnt/c/Users/hodlinator/bitcoin$ cmake --build build --target deploy
> Error: NSIS not found
> Built target deploy
> ```
>
> I needed to re-run `cmake -B build --toolchain depends/x86_64-w64-mingw32/toolchain
...

💬 hodlinator commented on pull request "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets":
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2710629594)
> > or indicate in the error message that one has to regenerate the build config.
>
> Agree. Mind suggesting a wording for the error message?

Felt I was going out on a limb using the term "build config", was thinking something like:
```
Error: NSIS not found. Install it (make it available to find_program()) and regenerate the build config.
```

💬 furszy commented on pull request "removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1987312628)
This introduces a cyclic dependency. Please do not add RPC dependencies to the wallet. These are two separate components, and the wallet operates at a lower level.

💬 furszy commented on pull request "removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1987318572)
No need to log the error message. It will be retrieved to the user via the RPC response.

💬 hodlinator commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1987418788)
Looked around for `wait_for_`-functions, but a bunch of those are used to check for `last_message` in `P2PInterface`. Staring a bit at `sync_txindex` I realized the wait condition could be simplified, decided to do that to pre-empt other reviewers. Excuse me for invalidating your ACK.

📝 Chand-ra opened a pull request: "test: get rid of redundant TODO tag in fuzz tests"
(https://github.com/bitcoin/bitcoin/pull/32024)
`list.size()` is determined at runtime, so using `static_assert` on it as suggested by the TODO comment is not feasible and produces the following error when done:

`error: static assertion expression is not an integral constant expression`

💬 hebasto commented on pull request "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets":
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2710893296)
Addressed the feedback from @hodlinator. Thank you!

👍 laanwj approved a pull request: "wallet: Replace "non-0" with "non-zero" in translatable error message"
(https://github.com/bitcoin/bitcoin/pull/31987#pullrequestreview-2671328596)
Code review ACK 18e83534ace7aa2d26bc7dfa521b1d591b66edfa
Aside from the issue raised, this makes the message more readable.

💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#discussion_r1987507317)
Removed on Transifex.

💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#discussion_r1987507697)
Fixed on Transifex.

💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#discussion_r1987508337)
Removed on Transifex.

💬 Sjors commented on pull request "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets":
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2710949958)
Tested on macOS 15.3.1 that `cmake --build build --target deploy` still works. It's impossible (?) to test the missing condition, because /usr/bin/zip comes with the OS.