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

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2497640819)
Re: https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2451143073

21bc3a3657fea5ed44172963639ee8ca5cd8e46c

> Very subjective, but I think the unrelated code cleanups in this commit make it hard to follow and are very distracting. Would be much happier to see them dropped or moved to a refactoring commit.

Broke out 215be3dfba90a6d5b2b37f0f94bcb621dd3d4a9a from commit. Also broke out eb23f55ae2470bd2376307ca293feb0be972de50 from test-commit.


---

879a8d3a6820a86e6faa8d
...

👍 hebasto approved a pull request: "ci: Split out native fuzz jobs for macOS and windows (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31221#pullrequestreview-2458051442)
ACK b031b7910d62819443813b91705211c466933a53.

💬 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_r1856372906)
nit: Add a year?
```suggestion
# Copyright (c) 2024-present The Bitcoin Core developers
```

💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#discussion_r1856388559)
Took the comment and expanded it a bit.

💬 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_r1856390207)
I'll leave this as is unless another reviewer prefers having the year as well.

💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#issuecomment-2497685083)
I added a comment based on https://github.com/bitcoin/bitcoin/pull/31346#discussion_r1855741271 to clarify under which circumstances `m_tip_block` is unset at this init stage.

I find it quite confusing that `ActivateBestChain` is called by `ImportBlocks` even if no blocks are imported, but haven't dared to refactor it.

💬 maflcko commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#issuecomment-2497727233)
> I find it quite confusing that `ActivateBestChain` is called by `ImportBlocks` even if no blocks are imported, but haven't dared to refactor it.

It is explained in the code comment above the line that does it. It was added as a fix for https://github.com/bitcoin/bitcoin/issues/2239

💬 fanquake commented on pull request "depends, refactor: Avoid hardcoding `host_prefix` in toolchain file":
(https://github.com/bitcoin/bitcoin/pull/31358#issuecomment-2497736026)
Is there any particular motivation (to do this now), given it doesn't work/can't be tested?

🤔 rkrux reviewed a pull request: "include verbose "reject-details" field in testmempoolaccept response"
(https://github.com/bitcoin/bitcoin/pull/28121#pullrequestreview-2458147883)
I want to test this out, do you mind rebasing it so that the `CMake` build changes are in this as well?

```
➜ bitcoin git:(tma-debug) ✗ bitcoinmakeall
CMake Error: The source directory "/Users/rkrux/projects/bitcoin" does not appear to contain CMakeLists.txt.
Specify --help for usage, or press the help button on the CMake GUI.
make: *** [cmake_check_build_system] Error 1
```