✅ hebasto closed a pull request: "depends: Update minimum required CMake version"
(https://github.com/bitcoin/bitcoin/pull/31300)

💬 hebasto commented on pull request "depends: Update minimum required CMake version":
(https://github.com/bitcoin/bitcoin/pull/31300#issuecomment-2495445996)
This change may introduce new behaviour due to new CMake policies across the span of CMake versions up to the updated one. This would require additional review effort, which does not seem justified for this repository.

Closing.

💬 laanwj commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1855176189)
so something like
```python
def satToBtc(sat_value: int) -> Decimal:
return Decimal(sat_value) / COIN

def btcToSat(sat_value: Decimal) -> int:
return int(sat_value * COIN)
```

💬 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_r1855185601)
Might be separated with commas for consistency with the other `job-name`?

💬 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_r1855185948)
Why are these lines needed?

🤔 hebasto reviewed a pull request: "ci: Split out native fuzz jobs for macOS and windows (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31221#pullrequestreview-2456407424)
Approach ACK c3354ea6dc34a3cadc60346c363478af22a5c0c2.

Please add [`strategy.fail-fast: false`](https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idstrategyfail-fast) everywhere.

💬 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_r1855194609)
This allows skipping the build of unneeded vcpkg packages:
```suggestion
generate-options: '-DVCPKG_MANIFEST_NO_DEFAULT_FEATURES=ON -DVCPKG_MANIFEST_FEATURES="sqlite" -DBUILD_GUI=OFF -DBUILD_FOR_FUZZING=ON -DWERROR=ON'
```

💬 brunoerg commented on pull request "Update addresstype.cpp":
(https://github.com/bitcoin/bitcoin/pull/31354#issuecomment-2495487280)
NACK

💬 furszy commented on pull request "wallet: remove BDB dependency from wallet migration benchmark":
(https://github.com/bitcoin/bitcoin/pull/31241#discussion_r1855213452)
> In [f7dbb30](https://github.com/bitcoin/bitcoin/commit/f7dbb300d7a4d18c9d85f80e5fdd7e5bcd21c6f0): Do we really need `TestLoadWallet` here? Couldn't we simply create the wallet directly (e.g. `std::unique_ptr<CWallet> wallet_ptr{std::make_unique<CWallet>(test_setup->m_node.chain.get()), "", CreateMockableWalletDatabase())};`)?

Not entirely, needed to also set the best locator manually. But done too. Thanks.

📝 hebasto opened a pull request: "cmake: Improve build script correctness"
(https://github.com/bitcoin/bitcoin/pull/31357)
When no build targets are specified, it is reasonable to expect the configuration step to succeed and produce a build system that does not build any targets.

This PR updates the code to ensure this behaviour:
```
$ cmake -B build -G "Ninja" -DBUILD_DAEMON=OFF -DBUILD_CLI=OFF -DBUILD_TX=OFF -DBUILD_UTIL=OFF -DENABLE_WALLET=OFF -DBUILD_TESTS=OFF
$ cmake --build build
ninja: no work to do.
```

💬 hebasto commented on pull request "cmake: Improve build script correctness":
(https://github.com/bitcoin/bitcoin/pull/31357#issuecomment-2495517355)
> When no build targets are specified, it is reasonable to expect the configuration step to succeed...

The issue was reported offline by @TheCharlatan.

👍 TheCharlatan approved a pull request: "cmake: Improve build script correctness"
(https://github.com/bitcoin/bitcoin/pull/31357#pullrequestreview-2456426859)
ACK ab5c63edccea7957e2c7b3b9d8b04655a7f82f22

💬 furszy commented on pull request "wallet: remove BDB dependency from wallet migration benchmark":
(https://github.com/bitcoin/bitcoin/pull/31241#issuecomment-2495531747)
> While this does remove the BDB requirement, it doesn't remove the reliance on legacy wallet specific code that will be removed. Would it be possible to change this to create a wallet with a LegacyDataSPKM without utilizing any of the legacy wallet functions?

Yes. Pushed the watch-only part. The spendable ones part is not that trivial, will finish it in the coming days.

📝 hebasto opened a pull request: "depends, refactor: Avoid hardcoding `host_prefix` in toolchain file"
(https://github.com/bitcoin/bitcoin/pull/31358)
This PR allows the entire `depends/<host_prefix>` directory to be relocatable.

Only `libevent` package configuration files are non-relocatable for the version `2.1.12-stable` we use now. However, this issue has been fixed upstream in https://github.com/libevent/libevent/commit/1f1593ff27bdf51c3e1c45b92cfb0ac931960298 and friends.

👍 tdb3 approved a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2456458800)
code review and test re ACK 1ca5f7b998c14c44b6afd5e362fc4c007d8eebd3

Repeated test in https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2450158177

💬 furszy commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r1855217304)
In order to continue testing this without manually writing the bdb file, we need to allow the 'mock' db format here:
```c++
const std::string& db_format{m_database->Format()};
Assert(db_format == "bdb_ro" || db_format == "mock");
```

💬 furszy commented on pull request "wallet: remove BDB dependency from wallet migration benchmark":
(https://github.com/bitcoin/bitcoin/pull/31241#issuecomment-2495680227)
Done, updated. The benchmark is no longer depending on wallet functions to fill-up the legacy wallet.
Tested that it works fine with #28710 (only need to tackle https://github.com/bitcoin/bitcoin/pull/28710#discussion_r1855217304).

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1855301316)
Tests are running in a scratch directory - if part of the test fails, we don't worry about leaving some files behind.

👋 andremralves's pull request is ready for review: "Add and use `satToBtc` and `btcToSat` util functions"
(https://github.com/bitcoin/bitcoin/pull/31356)

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1855390360)
Can we remove the cleanup in that case?