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

👍 ryanofsky approved a pull request: "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods"
(https://github.com/bitcoin/bitcoin/pull/31785#pullrequestreview-2624284851)
Code review ACK 6806651d2f64d51094178cbfb23ef2f4a956aa4d

As mentioned in a comment below, I don't really like how this PR is changing startup behavior of the RPC methods without documenting it, and when it seem like it expands the scope of this PR and makes it harder to understand. Would prefer doing that in #30635 so this PR could be more focused. But current approach does seem ok too.





, and

💬 ryanofsky commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1960188607)
re: https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1955115023

> I made a variant of this that's a bit more in line with #30635; during startup we'll wait by using the 0 hash.

This seems ok to me, and I initially considered suggesting that, but it seemed like it expanded the scope of the PR too much and was not related to its original goals.

I think if the PR will do this, it should also include release notes that say these RPCs will now wait on startup instead of returning er
...

💬 ryanofsky commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1960264859)
In commit "rpc: clarify longpoll behavior" (6806651d2f64d51094178cbfb23ef2f4a956aa4d)

s/is not be/is not/