💬 ryanofsky commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1854796297)
re: https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1850701093

> Maybe add value-less negation?

Thanks! Added. The test is currently very minimal, and I hope to do more with it, especially after this PR when I would like to add support for runtime settings validation.

💬 ryanofsky commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1854806210)
reL https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1850981914

> In [f991093](https://github.com/bitcoin/bitcoin/commit/f9910935c880c782c2b1d35acd38e1e18e6b4ad8): Better to document type? This style of using `auto` as function-arg-type when not necessary feels like the opposite of C++ concepts, potentially leading to downstream compile errors. But interested to hear reasoning behind it.

Good catch, this was just done early in a prototype and I never changed it. Updated to use exp
...

💬 ryanofsky commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1854804622)
re: https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1850906340

> WIP?

Unclear right now. Adding this wasn't as necessary as I initially thought it might be to remove most of the GetArg* calls in the codebase. But I am looking into more ways to expand the PR and maybe drop the GetArg methods methods entirely. Depending on how much work this it could be a follow-up and in any case doesn't need to block anything.

💬 ryanofsky commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1854806918)
reL https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1850983219

> Document types and remove unused `T`?

Thanks! Added types. I do want to keep T around because my changes in #22978 branch adding runtime setting validation need to know the setting type (also so `T` and `options` are available consistently as template parameters).

💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1854914157)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1854660293

> But since we don't have that here, I would prefer having the test in the same work unit as the fix, to provide the necessary context for the current fix.
Is this still controversial?

I think it's a perfectly reasonable suggestion. To paraphrase, you are saying that when code changes are made in a commit without test changes (which is how the PR is currently structured, though not what I was suggesting) then it is h
...

👋 jonatack's pull request is ready for review: "rpc, cli: add getbalances#total, and use it for -getinfo"
(https://github.com/bitcoin/bitcoin/pull/31353)

📝 karurung opened a pull request: "Update addresstype.cpp"
(https://github.com/bitcoin/bitcoin/pull/31354)
I removed `else` block. Also, I fixed 'return false' in the PUBKEY case. It helps the logic and improves readability.

📝 andremralves opened a pull request: "Add and use `satToBtc` and `btcToSat` util functions"
(https://github.com/bitcoin/bitcoin/pull/31356)
Introduce utility functions `satToBtc` and `btcToSat` to simplify and standardize conversions between satoshis and bitcoins in functional tests

#closes #31345

## Number of updates
```bash
➜ bitcoin git:(btc-to-sat) ✗ git grep -n "satToBtc(" -- '*.py' | wc -l
22
➜ bitcoin git:(btc-to-sat) ✗ git grep -n "btcToSat(" -- '*.py' | wc -l
152
```

<details>
<summary>Satoshis to BTC conversion instances</summary>

```bash
test/functional/feature_fee_estimation.py:130: fee_rate=
...

💬 l0rinc commented on pull request "Update addresstype.cpp":
(https://github.com/bitcoin/bitcoin/pull/31354#issuecomment-2495418167)
NACK, no high-level motivation given, code is worse than before (unformatted)

💬 laanwj commented on pull request "doc: Fix dead links to mailing list archives":
(https://github.com/bitcoin/bitcoin/pull/31240#issuecomment-2495442748)
That's unexpected! i hope they'll stick with those redirects.

At the least, there's no urgency to this, this can be postponed until it becomes a problem again.

💬 laanwj commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#issuecomment-2495443894)
Concept -0, it seems like a huge change to factor out what is just a multiply/division. imo this only makes sense if the intent is to add extra validity checks to the functions.

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