👋 NicolaLS's pull request is ready for review: "doc: Improve `dependencies.md`"
(https://github.com/bitcoin/bitcoin/pull/31895)

💬 brunoerg commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1960124970)
I meant due to `DecodeExt{Pub}Key` which calls `DecodeBase58Check`.

💬 brunoerg commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2666271032)
> > is to incorporate the following change.
>
> Improving the descriptor unit test utilities can be done in a followup. It would be nice to get the user-facing fix in first.

Yes, any other minor stuff I will leave for a follow-up.

👍 ryanofsky approved a pull request: "mining: drop unused -nFees and sigops from CBlockTemplate"
(https://github.com/bitcoin/bitcoin/pull/31897#pullrequestreview-2624259635)
Code review ACK d1cc874d1f87a654aa2da50dbcf850b84517e938

This does seem like a good simplification, but maybe instead of changing from non-zero dummy values to zero dummy values, it could make sense to drop dummy values entirely like:

<details><summary>diff</summary>
<p>

```diff
--- a/src/interfaces/mining.h
+++ b/src/interfaces/mining.h
@@ -37,11 +37,9 @@ public:
// Block contains a dummy coinbase transaction that should not be used.
virtual CBlock getBlock() = 0;

-
...

💬 fanquake commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2666322941)
> Shouldn't this behaviour be tested for releases:
> CLIENT_VERSION_IS_RELEASE only influences the version suffix information.

Looking at the code further, if `BUILD_GIT_TAG` is defined, which should be the case when the top commit is tagged according to https://github.com/bitcoin/bitcoin/blob/43e287b3ff5f0d223b0911b6ef90054ce5eb69d2/src/clientversion.cpp#L28-L40

then `CLIENT_VERSION_IS_RELEASE` shouldn't be used in creating the version string. So it shouldn't matter if that is set or not.

💬 ismaelsadeeq commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2666337742)
Concept ACK
Will ack after https://github.com/bitcoin/bitcoin/pull/31897#pullrequestreview-2624259635

💬 Sjors commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2666356729)
> maybe instead of changing from non-zero dummy values to zero dummy values, it could make sense to drop dummy values entirely

I think it's more intuitive for `block.vtx` and these two vectors to have the same length.

> Also if clients of IPC interface just need total fees and sigops

Agreed, but at this point I haven't seen a need for it yet.

💬 hebasto commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2666357003)
> > Shouldn't this behaviour be tested for releases:
> > CLIENT_VERSION_IS_RELEASE only influences the version suffix information.
>
> Looking at the code further, if `BUILD_GIT_TAG` is defined, which should be the case when the top commit is tagged according to

I don't think `BUILD_GIT_TAG` is defined:
```
wget https://bitcoincore.org/bin/bitcoin-core-28.1/bitcoin-28.1.tar.gz
tar xf bitcoin-28.1.tar.gz
cd bitcoin-28.1
./autogen.sh
./configure
make
$ cat src/obj/build.h
// No build information
...

📝 hebasto opened a pull request: "cmake: Exclude generated sources from translation"
(https://github.com/bitcoin/bitcoin/pull/31899)
This PR fixes an error encountered when building the `translate` target:
```
$ gmake -j $(nproc) -C depends MULTIPROCESS=1
$ cmake -G "Unix Makefiles" --preset dev-mode --toolchain depends/x86_64-pc-linux-gnu/toolchain.cmake -DWITH_USDT=OFF
$ cmake --build build_dev_mode -t translate
gmake[3]: *** No rule to make target 'src/test/ipc_test.capnp.c++', needed by 'src/qt/CMakeFiles/translate'. Stop.
gmake[2]: *** [CMakeFiles/Makefile2:1646: src/qt/CMakeFiles/translate.dir/all] Error 2
gmake[
...

💬 hebasto commented on pull request "cmake: Exclude generated sources from translation":
(https://github.com/bitcoin/bitcoin/pull/31899#issuecomment-2666364882)
> This PR fixes an error encountered when building the `translate` target...

The issue was reported offline by @pablomartin4btc.

💬 theuni commented on pull request "cmake: Add optional sources to `minisketch` library directly":
(https://github.com/bitcoin/bitcoin/pull/31880#issuecomment-2666422318)
What's the advantage here over simply making `minisketch_clmul` static?

💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1960241439)
There is an example pair (Cli+rpc) under `Load legacy wallet.dat file directly under the wallets/ directory`. Should we delete that as well?

💬 hebasto commented on pull request "cmake: Add optional sources to `minisketch` library directly":
(https://github.com/bitcoin/bitcoin/pull/31880#issuecomment-2666487748)
> What's the advantage here over simply making `minisketch_clmul` static?

To use the same approach across all similar cases in the project.

💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1960261844)
No, watch-only descriptor wallets do not change the GUI as watch-only legacy wallets did.

💬 theuni commented on pull request "cmake: Add optional sources to `minisketch` library directly":
(https://github.com/bitcoin/bitcoin/pull/31880#issuecomment-2666500965)
> > What's the advantage here over simply making `minisketch_clmul` static?
>
> To use the same approach across all similar cases in the project.

Yeah, ok, agreed.

💬 hebasto commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2666502305)
> What was the autotools behavior, to compare?

I've reproduced all steps from https://github.com/bitcoin/bitcoin/issues/31898#issue-2860887337 for the pre-CMake [commit](https://github.com/bitcoin/bitcoin/commit/80f00cafdeef0600fa1a5e906686720786c2336c):
```
$ tar xf bitcoin-99.99.tar.gz
$ cd bitcoin-99.99/
$ ./autogen.sh
$ ./configure
$ make -j $(nproc)
$ ./src/bitcoind --version
Bitcoin Core version v28.99.0-g80f00cafdeef0600fa1a5e906686720786c2336c
Copyright (C) 2009-2024 The Bitcoin Core
...

👍 theuni approved a pull request: "cmake: Add optional sources to `minisketch` library directly"
(https://github.com/bitcoin/bitcoin/pull/31880#pullrequestreview-2624493666)
utACK 9919e92022ba61d2dc1b13b1116b56035be459a6

One less autotools-ism and CMake hack.

💬 fanquake commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2666507401)
So then the bug was just ported/these code paths untested during the transition?

💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1960266934)
`importaddress` calls `importdescriptors` in the test framework. These checks are that `importaddress` (the legacy wallet RPC) doesn't rescan and will require a separate rescan. But that will not work once the tests is descriptor wallets only.

Will expand the commit message.

💬 hebasto commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2666510689)
> So then the bug was just ported/these code paths untested during the transition?

The mirroring of behaviour was tested.