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

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937405785)
IIRC I used magic values back when these were passed in as arguments, but in a struct using an `std::optional` is not much friction. I'll look into that.

💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937410103)
> I think it would be better to have that in the tests instead of this `if (now == deadline)` in the main (production) code.

Do we actually think that behavior is bad to have in production code? It seems like it's perfectly reasonable for production code to return either at the deadline or immediately after, and the choice is basically arbitrary for normal code but important for tests, so we are choosing a behavior that makes it more convenient to write tests and documenting that fact. This d
...

⚠️ fanquake opened an issue: "cmake: incorrectly reporting MSVC as using ccache"
(https://github.com/bitcoin/bitcoin/issues/31771)
Mentioned by @stickies-v. The CMake configure currently outputs that MSVC builds are using ccache, i.e (https://github.com/bitcoin/bitcoin/actions/runs/13063954360/job/36452895056#step:8:2374):
```bash
Treat compiler warnings as errors ..... ON
Use ccache for compiling .............. ON
```

However that's a bug, as MSVC is currently excluded entirely from using ccache:
https://github.com/bitcoin/bitcoin/blob/8fa10edcd1706a1f0dc9d8c3adbc8efa3c7755bf/cmake/ccache.cmake#L4-L6

💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937423612)
> IIRC I used magic values back when these were passed in as arguments, but in a struct using an `std::optional` is not much friction. I'll look into that.

IMO, MAX_MONEY is a magic value that would be good to replace with nullopt, but MillisecondsDouble::max() is not a magic value and the code would be simpler if it were kept.

A magic value (imo) is a value that is handled specially and adds a special case, so replacing `CAmount fee_threshold` with std::optional` is good because the code
...