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

> Is `isFalse()` equivalent to the former `IsArgNegated()` use?

Yes exactly,, if you look at the definition of IsArgNegated() it is just returning `isFalse()`:

https://github.com/bitcoin/bitcoin/blob/2638fdb4f934be96b7c798dbac38ea5ab8a6374a/src/common/args.cpp#L454

This works because of the translation of negated values to `false` in `InterpretValue`:

https://github.com/bitcoin/bitcoin/blob/2638fdb4f934be96b7
...

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

> How come we keep the default value in one of the cases here but not the other, both are using .GetBoolArg in the original?

The scripted diff will only assign default values to Setting types if the same default value is used every place a setting is retrieved.

In this case the `-dnsseed` setting is retrieved 5 different places, and 4 of the places look like `args.GetBoolArg("-dnsseed", DEFAULT_DNSSEED)` where `DEF
...

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

> What is `StringLiteral(std::nullptr_t) ->`?

This is a template deduction guide used to be able to pass nullptr_t as a string literal (used for a few settings that are only retrieved and never registered). I could explain more but chatgpt is much more great at this type of question: https://chatgpt.com/share/6740f058-e900-800a-afdf-7b56760db068

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

> In [b51a156](https://github.com/bitcoin/bitcoin/commit/b51a156f388926175afdbc50c2d563db404e3b81): I've learned to accept `static constexpr` (+`inline`...) in headers, but these other non-`constexpr` statics seem like they end up being "file-local" static vars in _every compilation unit_ that includes them. Seems off?

Good catch. I wanted to avoid changing these to keep the commit move-only as much as possible but it
...

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