💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2179856066)
I can confirm that I got the exact same result. I can also confirm that we didn't have any such block in the past decade.

Note that the biggest sigop per tx is 19971.

<img width="1000" alt="image" src="https://github.com/user-attachments/assets/b0a45fc6-98cb-4566-937f-e91234823505" />

sigops vs heights:
```
Found 2585 sigops in transaction 659135664894e50040830edb516a76f704fd2be409ecd8d1ea9916c002ab28a2 in block 228538
Found 4020 sigops in transaction bea1c2b87fee95a203c5b5d9f3e5d0f4
...

💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2179860454)
> bumped into this limit, all either pathological or/and DoSy, all already non-standard by current rules

Question: is the reason against such massive coinjoins that they currently take too long to validate? Wouldn't forking based on that prohibit such transactions in a few decades when we could expect them not to be ddosy anymore?

💬 fanquake commented on pull request "depends: fix libevent `_WIN32_WINNT` usage":
(https://github.com/bitcoin/bitcoin/pull/32837#issuecomment-3027621061)
Guix Build:
```bash
6a14c0d7863fcf63ad31f7327ba8bb6c05a8f187b23caf63031e4b383dafef6b guix-build-524dd98cb1cf/output/aarch64-linux-gnu/SHA256SUMS.part
fcb06e922b7a4e00f51f0517fbfbfa45fc0a9bb015f7f0dcead7e653c3fbfc93 guix-build-524dd98cb1cf/output/aarch64-linux-gnu/bitcoin-524dd98cb1cf-aarch64-linux-gnu-debug.tar.gz
c8f14621286d373d61490c0fbf07529c900cd88145c0d17fe6665220330f0fdc guix-build-524dd98cb1cf/output/aarch64-linux-gnu/bitcoin-524dd98cb1cf-aarch64-linux-gnu.tar.gz
9f65d8ace745d8f5
...

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