💬 ismaelsadeeq commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179747303)
nit: I think it should be clear that when using this constructor we also have 0 virtual bytes, I think it is possible to have 0 fee rate per n virtual bytes I think.
```suggestion
/** Fee rate of 0 satoshis per 0 vB */
```

💬 ismaelsadeeq commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179744660)
nit:
```suggestion
* Fee rate in satoshis per virtualbyte: CAmount / vB
* the feerate is represented internally as FeeFrac
```

💬 ismaelsadeeq commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179823586)
Also same at `fee_rate` fuzz test.

👍 hebasto approved a pull request: "depends: fix libevent `_WIN32_WINNT` usage"
(https://github.com/bitcoin/bitcoin/pull/32837#pullrequestreview-2978812923)
ACK 524dd98cb1cfa3a7917b94e13a91827842e7aba8, tested on macOS 15.5 (x64) using Homebrew's mingw-w64 13.0.0.

✅ hebasto closed a pull request: "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows"
(https://github.com/bitcoin/bitcoin/pull/31158)

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