🤔 vasild reviewed a pull request: "Add waitNext() to BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/31283#pullrequestreview-2585936766)
Approach ACK 6bf77e5a37

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937133582)
I think the loop should be `while (now < deadline)` (instead of `<=`) and this should not exist. But then I do not understand what this means: "because then there's no wait to test its internals, e.g. the 20 minute testnet exception". Can you elaborate?

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1936835457)
nit: strictly speaking it does not return `nullptr` but an empty `std::unique_ptr` (which owns `nullptr`).

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937123826)
When `options.timeout` is `MillisecondsDouble::max()` it looks odd to add to it. This seems to work because of the underlying `double` type which ends up being `inf` and adding to `inf` results in `inf`. But if that is changed to an integer type this will break (overflow). Maybe at least comment on `BlockWaitOptions::timeout` that it must be of `double` type because of this, or to have `BlockWaitOptions::timeout` be of type `std::optional` with an empty optional meaning "no timeout". Then `deadl
...

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937332052)
nit:
```suggestion
if (now > tip_time + 20min) {
```

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937311760)
Looking at

https://github.com/bitcoin/bitcoin/blob/8fa10edcd1706a1f0dc9d8c3adbc8efa3c7755bf/src/node/miner.cpp#L157

isn't this equivalent to just `current_fees = m_block_template->vTxFees[0];`?

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937136963)
Consider using `std::optional` with an empty optional meaning "no value" instead of the magic values `MillisecondsDouble::max()` and `MAX_MONEY`.

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937078288)
What is the reasoning to abort the entire program if `TipBlock()` returns an empty optional? Looks a bit too aggressive to me. `TipBlock()`'s doc says "Might be unset during an early shutdown". Is it not better to return an empty unique_ptr from `waitNext()` if this happens?

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2627377624)
Tidy is happy!

But now macOS 14 native trips over building those examples (`ld: library 'stdc++fs' not found`).

https://github.com/bitcoin/bitcoin/actions/runs/13073304594/job/36479449027?pr=30975#step:7:2141

💬 hebasto commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2627403382)
> > The "clang-tidy" CI job shouldn't process sources in the `src/ipc/libmultiprocess/example/` [directory](https://github.com/hebasto/bitcoin/actions/runs/13072023868/job/36476232260)
>
> Maybe not, but I didn't see an easy way to exclude them, so I thought I would just fix the clang-tidy errors. I fixed the missing inputs problem by just adding necessary targets back to all:
>
> ```diff
> --- a/src/ipc/CMakeLists.txt
> +++ b/src/ipc/CMakeLists.txt
> @@ -9,6 +9,10 @@ if (NOT WITH_LIBMU
...

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937354583)
Just stumbled on this and posted a [comment to the PR elsewhere](https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937133582)... Having this only to make unit tests easier seems not enough justification to me.

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937357533)
Lets continue the discussion here https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1925533767 which is a previous comment about the same thing.

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937363516)
If the loop uses `while (now < deadline)` then it will never generate a template, so all the `BOOST_REQUIRE(block_template)` checks will fail.

So it has to use `<=`, but without the `break` here the test will freeze, because time doesn't move.

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937372244)
That might work actually, with an extra minus. But it seems brittle, because the line `pblocktemplate->vTxFees[0] = -nFees;` doesn't do anything afaik and might one day get deleted, causes this code to thing fees are 0.

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937378509)
> unit tests can't easily move mock time during a blocking call

Consider the following pattern to do that. I think it would be better to have that in the tests instead of this `if (now == deadline)` in the main (production) code.

```cpp
std::atomic_int now = 0;

void waitNext(int timeout)
{
const int deadline = now + timeout;
while (now < deadline) {
std::cout << "waitNext() now=" << now << "\n";
std::this_thread::sleep_for(500ms);
}
}

int main(int,
...

🤔 Sjors reviewed a pull request: "Split CConnman"
(https://github.com/bitcoin/bitcoin/pull/30988#pullrequestreview-2586038876)
Reviewing in reverse order.

f7dd4373b06daf34033fa84fa99408a3938e4ad4 is a beast, but I'm not sure if it can be split further. There's some mixing of code modernisation with the move which make the diff a bit trickier to follow.

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1936898358)
f7dd4373b06daf34033fa84fa99408a3938e4ad4: would it make sense to call `MarkAsDisconnectAndCloseConnection` in the connman fuzzer?

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1937107332)
f7dd4373b06daf34033fa84fa99408a3938e4ad4: why plural and not `NodeSocket`?

Does the I2P session `sess` also contain a socket? Or does it use `s`?

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1937394233)
f7dd4373b06daf34033fa84fa99408a3938e4ad4: maybe add a comment here that both `-1` and `0` still warrant (require?) reaching `EventIOLoopCompletedForNode`, so we can't just have individual `if ... continue` guards for them.

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1937302384)
f7dd4373b06daf34033fa84fa99408a3938e4ad4: it would be useful to document these two values.