💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#discussion_r1856151977)
It's clearly doing something though, because the following breaks several functional tests:

```patch
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1792,13 +1792,14 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
});

// Wait for genesis block to be processed
{
WAIT_LOCK(kernel_notifications.m_tip_block_mutex, lock);
- kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip
...

💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#discussion_r1856178140)
With some manual testing I indeed get all three conditions from your comment to hit that assert. I'm just confused why.

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2497346388)
@laanwj, Right! And `ps ax` in the VM looks suspiciously scarce: https://github.com/bitcoin/bitcoin/issues/31339#issuecomment-2491003878 showing just `bash` and `03_test_script.sh`.

Another source of false positive could be if somebody from the outside initiates communication to the VM to which it responds. E.g. an outsider tries to connect to the VM to which it responds with an outbound packet e.g. TCP RST. At least that should be obvious from the error log, showing the incoming packet first
...

👍 rkrux approved a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2457786528)
re-ACK 1ca5f7b998c14c44b6afd5e362fc4c007d8eebd3

Successful make and tests. I reviewed the range diff.

```
git range-diff 878b6c8...1ca5f7b
```

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1856221103)
The cleanup is still there so that other tests following it in the Python script are not impacted.

💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#discussion_r1856227125)
Outside the context of a snapshot, we only call `LoadChainTip()` if `if (!is_coinsview_empty(chainstate)) `:

https://github.com/bitcoin/bitcoin/blob/2638fdb4f934be96b7c798dbac38ea5ab8a6374a/src/node/chainstate.cpp#L147-L150

That explains why these three scenarios are different.

Still looking into where it _is_ set.

💬 martinus commented on pull request "util/blockstorage: Add `TRACE_RAII`, slightly faster -reindex-chainstate with CBufferedFile":
(https://github.com/bitcoin/bitcoin/pull/28226#issuecomment-2497416409)
@l0rinc I'm not working on it any more, feel free to take over!

✅ fanquake closed a pull request: "util/blockstorage: Add `TRACE_RAII`, slightly faster -reindex-chainstate with CBufferedFile"
(https://github.com/bitcoin/bitcoin/pull/28226)

🤔 rkrux reviewed a pull request: "Add and use `satToBtc` and `btcToSat` util functions"
(https://github.com/bitcoin/bitcoin/pull/31356#pullrequestreview-2457837013)
Thanks for picking this up! Left few suggestions.
The PR can be split into 2 commits for each util function that will make it easier to process piece by piece.

💬 rkrux commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1856260835)
```diff
- def branch(prevout, initial_value, max_txs, tree_width=5, fee=0.00001 * COIN, _total_txs=None):
+ def branch(prevout, initial_value, max_txs, tree_width=5, fee=btcToSat(0.00001), _total_txs=None):
```

Seems to work.

💬 rkrux commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1856236488)
I see that `COIN` is defined in this file but not sure if this is the right file to put these functions in, maybe `util.py`?

💬 rkrux commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1856293868)
Maybe accepting a string with internal string to decimal checking and conversion would also make it more flexible? The explicit `Decimal` conversion could be removed here then.

💬 rkrux commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1856247723)
Maybe this utility function can become more useful if it accepts float too, thereby getting rid of explicit conversion while calling?

💬 rkrux commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1856297065)
Doesn't `satToBtc` accept an `int`?

💬 rkrux commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1856237945)
Sorry for using camelCase in the issue description, snake_case is preferred in functional tests.

💬 rkrux commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1856276355)
While `10 * COIN` seems self-explanatory, I find the util function usage in the following diff more readable and easier on the eyes. Instead of having to process 3 things in one go, namely int conversion, sum() with an argument that's being calculated, multiplication with COIN, I have to process only one function call with an argument.

https://github.com/bitcoin/bitcoin/pull/31356/files#diff-9e79d56c581ca71e62a898ee0d2afda253081118ebcd1744ba9b3d16f0958a80L192

```diff
- input_amount = int
...

💬 rkrux commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1856287079)
> Possibly there is a case to have feerate conversion helpers, or a class to hold feerates.

Agree, a fee rate conversion helper here would make reading this far easier.

💬 rkrux commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1856288113)
This conversion was not intuitive to me in the first glance: https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599591670

💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#discussion_r1856318118)
In the case of fresh data dir or reindex, it's `ActivateBestChain()` that calls `blockTip()` once it sets the genesis block to the tip.

This indeed happens in the `initload` threads, which seems to keep doing this throughout the reindex process. Once all previously stored blocks are done, it exits and the `msghand` thread takes over.

💬 dergoegge commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#discussion_r1856322551)
They're probably not, removed

💬 dergoegge commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#discussion_r1856326249)
`-DBUILD_GUI=OFF` is implied by `-DBUILD_FOR_FUZZING=ON`.

What does `-DVCPKG_MANIFEST_NO_DEFAULT_FEATURES=ON -DVCPKG_MANIFEST_FEATURES="sqlite"` do? we're using the same vcpkg cache for both jobs would that still work with this change?