💬 hodlinator commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1960014260)
Makes more sense to use `class`es for these as they use `virtual` functions. Not worth changing just to be able to remove `public` IMO.

⚠️ fanquake opened an issue: "cmake: (release) version handling is broken"
(https://github.com/bitcoin/bitcoin/issues/31898)
Create a test tag: `v99.99`
Checkout `v99.99` and perform a guix build:`./contrib/guix/guix-build`.
Take the dist-archive tarball: `/bitcoin/guix-build-99.99/output/dist-archive/bitcoin-99.99.tar.gz` and perform a build:
```bash
tar xf bitcoin-99.99.tar.gz
cd bitcoin-99.99
cmake -B build
cmake --build build
# ./build/src/bitcoind --version
Bitcoin Core daemon version v28.99.0-g43e287b3ff5f0d223b0911b6ef90054ce5eb69d2
```

👍 darosior approved a pull request: "descriptor: check whitespace in keys within fragments"
(https://github.com/bitcoin/bitcoin/pull/31603#pullrequestreview-2623967849)
ACK 35d5adbcd58e04991bb4651d2dd97af6e186d1f9

💬 darosior commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1959975429)
*(comment about the commit, not this line)*

nit: in commit e91a9cf9d2ca50ee31ed36a975d2e01faa666c92 you state:
> Due to Base58, pubkeys with whitespace at the beginning
end/or at the end are successfully parsed.

But pubkeys are never base58 encoded.

💬 darosior commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1960036391)
Interestingly this line is correct only because we check `IsHex` before `ParseHex` below, as otherwise we would need to check for any space character across the string since `ParseHex` would skip those even if they happen in the middle.

Checking for a space character at any position would also improve the error message for instance when parsing `pk(03 a34b...)`. But no need to retouch.

💬 darosior commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2666139402)
> 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.

💬 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