💬 theStack commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1960100489)
nit: missing paren
```suggestion
/** Whether this descriptor only produces single key scripts (i.e. pk(), pkh(), wpkh(), sh() and wsh() nested of those, and combo())
```

💬 fanquake commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2666248929)
`CLIENT_VERSION_IS_RELEASE` only influences the version suffix information. https://github.com/bitcoin/bitcoin/blob/43e287b3ff5f0d223b0911b6ef90054ce5eb69d2/src/clientversion.cpp#L40-L44

So even if that was set we'd stil be producing a tarball from a tag `v99.99`, which contained the version information `28.99`.

💬 l0rinc commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2666252807)
@yancyribbens, please read the room and stop spamming.
This change was very opinionated, changed one set of tradeoffs with some other ones (without any professional explanation) in a part of the code that isn't read often enough for the review investment to be worth it. The changes and the reviews have opportunity costs that have to be weighed. The author of the PR should do 10x the work to simplify it to reviewers as much as possible, while `This refactor is a no-brainer` is condescending and
...

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

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

The tarball for 28.1 contains the version information 28.1.

💬 brunoerg commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1960120161)
Ok.

💬 fanquake commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2666262921)
i.e
```bash
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
# ./src/bitcoind --version
Bitcoin Core version v28.1.0
Copyright (C) 2009-2024 The Bitcoin Core developers
```

💬 NicolaLS commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r1960123485)
14c85a9b3753b40e8cf9f2808530fa922c8a3473

👋 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?