🤔 jonatack reviewed a pull request: "docs: remove repetitive words"
(https://github.com/bitcoin/bitcoin/pull/32022#pullrequestreview-2669776693)
NACK. This patch is trying to touch a subtree repo that would need to be changed upstream and not here, as the failing CI is confirming.

✅ fanquake closed a pull request: "docs: remove repetitive words"
(https://github.com/bitcoin/bitcoin/pull/32022)

📝 saikiran57 opened a pull request: "removed duplicate call to GetDescriptorScriptPubKeyMan "
(https://github.com/bitcoin/bitcoin/pull/32023)
https://github.com/bitcoin/bitcoin/issues/32013

🤔 rkrux reviewed a pull request: "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`"
(https://github.com/bitcoin/bitcoin/pull/31243#pullrequestreview-2669096676)
Few comments.

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1986049686)
This new checks here makes the flow more robust and avoids relying on implicit assumptions of what's going on inside the `Expand` function.

> Instead of relying on implicit assumptions about whether pubkeys show up
after expanding a descriptor, just explicitly allow only single
key descriptors to import keys into a legacy wallet's keypool.

The commit message 6a31eb5dca1f8eb5fd0507852b49c7452031269e, however, was difficult for me to parse. The second clause, which I understand (and made m
...

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1986101902)
This comment is vaguely correct now, technically ok but doesn't gel well with the new object name, causes confusion. IMO best to update it.

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