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

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

Yes. But it is evaluated _after_ an attempt to look for the Qt packages. In this case, `-DBUILD_GUI=OFF` simply negates `-DBUILD_GUI=ON` from the "vs2022-static" preset.

> What does `-DVCPKG_MANIFEST_NO_DEFAULT_FEATURES=ON -DVCPKG_MANIFEST_FEATURES="sqlite"` do?

The optional vcpkg packages are organised in "features" in the [`vcpkg.json`](https://github.com/bitcoin/bitcoin/blob/master/vcpkg.json) manifest file.

`-DVCPKG_MA
...

💬 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_r1856353296)
Done.

💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#discussion_r1856353967)
Specifically it's `ImportBlocks()` that does this, even if no blocks are passed to it.

> which seems to keep doing this throughout the reindex process.

In fact it does it _after_ reindex is finished. `ActivateBestChain` connects blocks one by one until it has nothing left to do so that makes sense.