💬 hebasto commented on pull request "depends: fix libevent `_WIN32_WINNT` usage":
(https://github.com/bitcoin/bitcoin/pull/32837#issuecomment-3027626421)
> ACK [524dd98](https://github.com/bitcoin/bitcoin/commit/524dd98cb1cfa3a7917b94e13a91827842e7aba8), tested on macOS 15.5 (x64) using Homebrew's mingw-w64 13.0.0.

Retracting my ACK basing on non-reproducibilty of the Windows builds.

🤔 janb84 reviewed a pull request: "cmake: Move internal binaries from bin/ to libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2978891971)
Concept ACK

This PR changes the target of some executables on install (different folder). This achieves that not all the executables are added to the PATH (which I agree with)

Result of install ✅
<details>

```shell
cmake --install build --prefix $PWD/install
-- Install configuration: "RelWithDebInfo"
-- Installing: /Users/jan/Projects/bitcoin/install/bin/bitcoin-wallet
-- Installing: /Users/jan/Projects/bitcoin/install/share/man/man1/bitcoin-wallet.1
-- Installing: /Users/jan/Pro
...

🤔 rkrux reviewed a pull request: "wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs"
(https://github.com/bitcoin/bitcoin/pull/32618#pullrequestreview-2978731688)
Concept ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055

Thanks for splitting this PR out of the other one, it's easier to review now. I have left few minor suggestions.

> Descriptor wallets do not use the watchonly behavior as it is not possible to mix watchonly and non-watchonly in a descriptor wallet.

For this^, I verified that descriptors lacking all the private keys can't be imported in non-watch-only (descriptor) wallets.

https://github.com/bitcoin/bitcoin/blob/7fa9b58bd9078809037d
...

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

Similar to this, on line 386 `include_watchonly` can be removed for the `watchonly` wallet because that, too, is asserted after migration on the master node. I tested on local that it works fine.

```diff
- assert_equal(len(watchonly.listtransactions(include_watchonly=True)), 4)
+ assert_equal(len(watchonly.listtransactions()), 4)
```

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