💬 i-am-yuvi commented on pull request "rpc: collect transaction fees on generateblock":
(https://github.com/bitcoin/bitcoin/pull/31775#issuecomment-2666140255)
I agree with @maflcko!

> > The block reward should include both the block subsidy and the transaction fees from all transactions included in the block.
>
> Is there a use case for this? `generateblock` is a test-only RPC, so there is no need to write extra code to collect the fees. Note that the other `generate*` calls will collect the fees.

✅ maflcko closed a pull request: "rpc: collect transaction fees on generateblock"
(https://github.com/bitcoin/bitcoin/pull/31775)

💬 maflcko commented on pull request "rpc: collect transaction fees on generateblock":
(https://github.com/bitcoin/bitcoin/pull/31775#issuecomment-2666159529)
Closing for now. For future contributions, my recommendation would be to compile the code and run the tests. Also, new tests should be included for new features.

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2666164979)
My Guix build:
```
aarch64
66e46fc7610a8e3a39b6253672b5063ea0f1c3ea54df6807c8e1d36d416e9d8c guix-build-f7d8a44ce42c/output/aarch64-linux-gnu/SHA256SUMS.part
6333dc4e4233970e5ec19ac3e3b99cc30eaedf78a5ec74da57363e0c6d9993fc guix-build-f7d8a44ce42c/output/aarch64-linux-gnu/bitcoin-f7d8a44ce42c-aarch64-linux-gnu-debug.tar.gz
4225ab5948b61dcc631346c08d1a6afd6263afaaecaf9301098f1edfd2933964 guix-build-f7d8a44ce42c/output/aarch64-linux-gnu/bitcoin-f7d8a44ce42c-aarch64-linux-gnu.tar.gz
d10bfa76
...

💬 hodlinator commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r1960058201)
Might be so cheeky as to order these alphabetically? Also CMake/Boost at the top.

💬 hodlinator commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1960065455)
Looks pretty ready to me, PR description and commits are informative. Left an additional suggestion.

💬 hebasto commented on pull request "cmake: Improve robustness and usability":
(https://github.com/bitcoin/bitcoin/pull/31233#issuecomment-2666177812)
Rebased due to the conflict with the merged bitcoin/bitcoin#31892.

💬 mzumsande commented on issue "test: feature_assumeutxo.py Windows timeout":
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2666205108)
#31019 (maxOS) might be the same issue, in which case this would not be windows-specific.

💬 hebasto commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2666220981)
Shouldn't this behaviour be tested for releases:
```diff
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -21,7 +21,7 @@ set(CLIENT_VERSION_MAJOR 28)
set(CLIENT_VERSION_MINOR 99)
set(CLIENT_VERSION_BUILD 0)
set(CLIENT_VERSION_RC 0)
-set(CLIENT_VERSION_IS_RELEASE "false")
+set(CLIENT_VERSION_IS_RELEASE "true")
set(COPYRIGHT_YEAR "2025")

# During the enabling of the CXX and CXXOBJ languages, we modify
```
?

💬 yancyribbens commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2666233579)
@l0rinc I'm not sure why you're thumbs downing my last comment. I understand your position that it's already like this, so lets leave it and I appreciate the review. You don't provide any reason for why this confusing code was added though. Personally I think code maintenance is a good thing, and the goal should be to help make it readable and understandable for humans.

💬 maflcko commented on issue "test: feature_assumeutxo.py Windows timeout":
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2666236912)
> `StopHTTPServer`

Could update the issue title to say "timeout in StopHTTPServer"?

> #31019

They are not exactly identical. 31019 likely never received an RPC, and the node is expected to raise an init error after running into a wallet init error. However, the function of the deadlock is the same:

```
node0 2024-10-02T00:15:45.783904Z [shutoff] [src/httpserver.cpp:522] [StopHTTPServer] [http] Stopping HTTP server
node0 2024-10-02T00:15:45.783911Z [shutoff] [src/httpserver.cpp:524] [Stop
...

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

👍 theStack approved a pull request: "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`"
(https://github.com/bitcoin/bitcoin/pull/31243#pullrequestreview-2624210415)
re-ACK 1bfabb4f4ec07c8fb0dfc2515d413b5196e01348

💬 theStack commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1960103337)
nit: could move this up, right below `IsSingleType`, for consistency with other classes

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