💬 hebasto commented on pull request "build: set build type and per-build-type flags as early as possible":
(https://github.com/bitcoin/bitcoin/pull/31711#discussion_r1939413700)
Is the default build configuration really important for detecting the compiler and its capabilities?

🤔 hebasto reviewed a pull request: "build: set build type and per-build-type flags as early as possible"
(https://github.com/bitcoin/bitcoin/pull/31711#pullrequestreview-2589973088)
Tested 240ff782ad5a8974aff35e78356520917926e525 on Ubuntu 24.10 with CMake 3.30.3.

The `CMAKE_BUILD_TYPE` cache variable lost their help string:
- on the master @ d7f56cc5d9e12ad31dd1ce8b34c3ff4ec5c1b70c:
```
$ cmake -B build -G "Ninja" -LH 2>&1 | grep -B 1 CMAKE_BUILD_TYPE:
// Choose the type of build, options are: None Debug Release RelWithDebInfo MinSizeRel ...
CMAKE_BUILD_TYPE:STRING=RelWithDebInfo
```
- with this PR:
```
$ cmake -B build -G "Ninja" -LH 2>&1 | grep -B 1 CMAKE_BUI
...

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1939408935)
In 91d97a198fb57d47870ce094454c3776bc737ba0 "net: split CConnman::ConnectNode()": looking at the body of `ConnectAndMakeNodeId` the code paths with and without proxy are vastly different. They also have separate call sites.

So instead of using an `std::variant`, it seems more clear to just have both `ConnectAndMakeNodeId` and `ProxyConnectAndMakeNodeId`.

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1939396960)
In 91d97a198fb57d47870ce094454c3776bc737ba0 "net: split CConnman::ConnectNode()": why this new `assert`? If you need it all, maybe make it an `Assume`?

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939436862)
But what about this comment:

https://github.com/bitcoin/bitcoin/blob/1172bc4157eefe80d1aaf0b56459857ec651e535/src/node/kernel_notifications.h#L61-L62

related:

https://github.com/bitcoin/bitcoin/blob/1172bc4157eefe80d1aaf0b56459857ec651e535/src/init.cpp#L1817-L1827

📝 kevkevinpal opened a pull request: "test: added additional coverage to waitforblock and waitforblockheight rpc's"
(https://github.com/bitcoin/bitcoin/pull/31784)
Similar to https://github.com/bitcoin/bitcoin/pull/31746

This adds test coverage to the `waitforblock` and `waitforblockheight` rpc's by adding a test to assert we get an rpc error if we include a negative timeout

💬 laanwj commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1939445814)
```
# collection of types that map to MIME and file types
```

💬 laanwj commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1939481222)
```python
'api-ms-win-core-synch-l1-2-0.dll', # Synchronization Primitives API
'api-ms-win-core-winrt-l1-1-0.dll', # Windows Runtime API
'api-ms-win-core-winrt-string-l1-1-0.dll', # WinRT String API
'AUTHZ.dll', # Windows Authorization Framework
'comdlg32.dll', # Common Dialog Box Library
'd3d11.dll', # Direct3D 11 API
'd3d12.dll', # Direct3D 12 API
'd3d9.dll', # Direct3D 9 API
'dwmapi.dll', # desktop window manager
'DWrite.dll', # DirectX Typography Services
'dxgi.dll', # DirectX Gr
...

💬 hebasto commented on pull request "RFC: Riscv bare metal CI job":
(https://github.com/bitcoin/bitcoin/pull/31425#discussion_r1939486828)
I imagine that expanding this build scenario might enable building `boost` and `libevent` in depends, no?

If so, the Boost check should have another option to be disabled.

Additionally, mind considering this comment: https://github.com/bitcoin/bitcoin/blob/1172bc4157eefe80d1aaf0b56459857ec651e535/cmake/module/AddBoostIfNeeded.cmake#L7-L8

which, in turn, was based on this legacy code:https://github.com/bitcoin/bitcoin/blob/32efe850438ef22e2de39e562af557872a402c31/configure.ac#L1247-L1251
...

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939503331)
@vasild it's possible to start the node and quickly shut it down. If that happens then `m_tip_block` remains `null`.

If you call `createNewBlock()` during that time I'm not sure what happens. I think it's best to add an extra guard there. The Template Provider calls `waitTipChanged` before calling `createNewBlock()`, but not every interface user might do that.

With that in place you can't get here.

💬 brunoerg commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#issuecomment-2631212572)
Concept ACK

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1939513224)
Right. From https://formulae.brew.sh/formula/qt:
> Also known as: **qt6**, **qt@6**

📝 Sjors opened a pull request: "Have createNewBlock() ensure m_tip_block is set"
(https://github.com/bitcoin/bitcoin/pull/31785)
This allows the proposed `waitNext()` method in https://github.com/bitcoin/bitcoin/pull/31283 to safely assume `TipBlock()` isn't `null`, not even during a scenario of early shutdown.

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939522785)
Opened #31785 for this check, though I could include it here.

✅ fanquake closed an issue: "multiprocess: build failure on Alpine with depends & `DEBUG=1`"
(https://github.com/bitcoin/bitcoin/issues/31455)

💬 l0rinc commented on pull request "Have createNewBlock() ensure m_tip_block is set":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1939533024)
If it's not too difficult, can we reproduce this with a test?

💬 Sjors commented on pull request "Have createNewBlock() ensure m_tip_block is set":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2631258942)
The Stratum v2 Template Provider that I implemented first calls `mining.waitTipChanged(uint256::ZERO)` before calling `mining.createNewBlock()`. This ensures that now matter we order the node & IPC startup sequence, it won't prematurely try to make a block.

See https://github.com/Sjors/bitcoin/pull/49 (`src/sv2/template_provider.cpp` in `Sv2TemplateProvider::ThreadSv2Handler()`).

But other implementers may not realise this, so having `createNewBlock()` return nothing when called too early
...

💬 Sjors commented on pull request "Have createNewBlock() ensure m_tip_block is set":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1939540523)
This depends on the node initialization and shutdown sequence. I'm not sure how to reproduce that in a test. And we might change that sequence in a way that this never happens in the first place.

💬 maflcko commented on pull request "Have createNewBlock() ensure m_tip_block is set":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1939572655)
Would be good to update the docs, because `CreateNewBlock` never returns null and always returns a block.

👍 danielabrozzoni approved a pull request: "test: deduplicates p2p_tx_download constants"
(https://github.com/bitcoin/bitcoin/pull/31758#pullrequestreview-2590273881)
tACK 2b94e1abbfdef058546221d63f53b0b58eea22ea

The code looks good to me, I think renaming `MAX_GETDATA_IN_FLIGHT` to `MAX_PEER_TX_REQUEST_IN_FLIGHT` makes it easier to refer back to txdownloadman