💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1986101911)
This comment seems partly outdated now because, as I see, there is no more copying happening.

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1986101919)
Overall the update count (in this file atleast) of the members of the class `FlatSigningProvider& out` has decreased now because there are fewer `PubkeyProvider`s and more `DescrImpl`s , which IMO is desirable.

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1986101904)
Took me some time to gather what's going on here.

For reviewing and future reference as well, it would be easier if this commit 070e746c2f72db2cc24b1c267b91fafbcc586736 is split into 2:
Usage of just `info` object and the removal of `parent_info`, `final_info_out_tmp` objects with the changes related to it can be one commit. Rest of the commit changes can be part of the other.

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