💬 rkrux commented on pull request "wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs":
(https://github.com/bitcoin/bitcoin/pull/32618#discussion_r2179803189)
In 4439bf4b41a6997d4d965f00a8c40efa9cf6895b "wallet, spend: Remove fWatchOnly from CCoinControl"

The `UseMaxSig` function now tests only for the input being externally selected. The only usage of this function is inside `MaxInputWeight` function. Maybe the following diff so that this function still remains generic? Also aligns well the function doc.

```diff
@@ -51,10 +51,10 @@ static bool IsSegwit(const Descriptor& desc) {
}

/** Whether to assume ECDSA signatures' will be high-r. *
...

💬 rkrux commented on pull request "wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs":
(https://github.com/bitcoin/bitcoin/pull/32618#discussion_r2179785578)
In 4439bf4b41a6997d4d965f00a8c40efa9cf6895b "wallet, spend: Remove fWatchOnly from CCoinControl"

```diff
- // Use max sig if watch only inputs were used or if this particular input is an external input
+ // Use max sig if this particular input is an external input
```

💬 rkrux commented on pull request "wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs":
(https://github.com/bitcoin/bitcoin/pull/32618#discussion_r2179815729)
In 1337c72198a7d32935431d64e9e58c12f9003abc "wallet, rpc: Remove watchonly from RPCs"

I find it unsettling that this field is shown to be `false` even for watch-only wallets. Can't we remove this field and mark this commit a breaking change?
Reference: [Conventional Commits - Breaking Change](https://www.conventionalcommits.org/en/v1.0.0/#summary:~:text=in%20Semantic%20Versioning).-,BREAKING%20CHANGE,-%3A%20a%20commit)

💬 fanquake commented on pull request "cmake: Fix `FindQt` module":
(https://github.com/bitcoin/bitcoin/pull/32709#discussion_r2179914291)
To be clear, I think these changes could be made, with the `XKB_` change dropped. If that's going to be kept, then there should be a link to an upstream issue.

💬 fanquake commented on pull request "cmake: Fix `FindQt` module":
(https://github.com/bitcoin/bitcoin/pull/32709#issuecomment-3027672609)
cc @purpleKarrot

💬 fanquake commented on pull request "doc: Add note for building on macOS (Intel) with CMake ≥ 4.0":
(https://github.com/bitcoin/bitcoin/pull/32289#issuecomment-3027673706)
What's the status of this? CMake `4.x` has been available on macOS via brew for > 3 months, and we haven't had any issues reported. I think we could just do nothing here.

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2179916095)
Yes, the maximum standard size was initially introduced because of quadratic hashing. It is also desirable for other reasons (see for instance [here](https://delvingbitcoin.org/t/non-confiscatory-transaction-weight-limit/1732/8?u=antoinep)). I don't expect the worst case with large legacy transactions to be reasonable even in a decade from now. Also, such large transactions can be done with Segwit v0 or v1 inputs instead and those concerns are moot.

💬 fanquake commented on pull request "build, docs: Fix Boost-related issues on NetBSD":
(https://github.com/bitcoin/bitcoin/pull/32828#issuecomment-3027678123)
> I wonder why no other projects run into this issue.

Yea. I guess nobody else is using Boost, via CMake, on NetBSD?

👋 fanquake's pull request is ready for review: "[28.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/32811)

🚀 fanquake merged a pull request: "build, docs: Fix Boost-related issues on NetBSD"
(https://github.com/bitcoin/bitcoin/pull/32828)

💬 fanquake commented on pull request "refactor: Drop unused `#include <boost/operators.hpp>`":
(https://github.com/bitcoin/bitcoin/pull/32668#issuecomment-3027713936)
Yea, I'm not sure what is the best approach here. However I don't think a good way forward is PR's like this, based on IWYU output, which simultaneously ignore other IWYU output (and no explanation for devs that might otherwise try to make changes based on the output). I think any changes should be a part of #31308.

💬 fanquake commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#issuecomment-3027724136)
cc @hodlinator

💬 maflcko commented on pull request "refactor: Drop unused `#include <boost/operators.hpp>`":
(https://github.com/bitcoin/bitcoin/pull/32668#issuecomment-3027737893)
>I think any changes should be a part of #31308.

I'd say it is also fine to do it in smaller steps, but no strong opinion.

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r2168558603)
```shell
₿ mypy contrib/devtools/reg-settings.py
contrib/devtools/reg-settings.py:99: SyntaxWarning: invalid escape sequence '\|'
"git", "grep", "-l", "AddArg(\|AddHiddenArgs(\|GetArg(\|GetPathArg(\|GetIntArg(\|GetBoolArg(\|GetArgs(\|IsArgSet(\|IsArgNegated(", "--", src_dir
```
Suggestion; make it an r-string:
```python
"git", "grep", "-l", r"AddArg(\|AddHiddenArgs(\|GetArg(\|GetPathArg(\|GetIntArg(\|GetBoolArg(\|GetArgs(\|IsArgSet(\|IsArgNegated(", "--", src_dir
```

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r2168428599)
1. You described it in the middle of https://github.com/bitcoin/bitcoin/pull/31260#pullrequestreview-2455631901 but please document what `legacy` is intended to signify in this context using a comment or at minimum the commit message.

2. The field is currently never read. Should make that clear too in comments/commit message. Or remove it altogether for now, or add the functionality that reads from it into this PR.

🤔 hodlinator reviewed a pull request: "scripted-diff: Type-safe settings retrieval"
(https://github.com/bitcoin/bitcoin/pull/31260#pullrequestreview-2960951696)
Overdue Concept ACK b3968352b292c8c69dffbaa9c54a17405e246289

Looking back at this, I realize there's actually not *that* much going on. No (intended) changes in behavior when executing the code, just installing compile time guardrails.

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r2168478592)
Commit message mentions *lint-format-strings.py* but commit doesn't touch it (any longer?).

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r2175982324)
nit: Tempting to go for:
```suggestion
consteval int SettingFlags(SettingOptions options)
```
But it's somewhat of a departure from the surrounding style.

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r2178368647)
nit: This indentation of the `for`-loops is not necessary, we can still access `content` after letting go of `f`. Less indentation => less intimidation. Same is already done in `replace_in_file()`.

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r2178376601)
nit: Would be good to follow with `assert data_type` to guard against the `None`-case.

Similar case:
https://github.com/bitcoin/bitcoin/blob/73a77f4f01fc479bf015605cb95894b440533a2d/contrib/devtools/reg-settings.py#L400-L407

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r2168340344)
nit:
```suggestion
//! Setting template struct used to declare name, type, and behavior of command
```