💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-3027512988)
Thank you for comments @achow101 and @maflcko!

I've applied both suggestions - making sure the benchmark is closer to the original, explained it in the commit message as well. I have re-measured each commit accordingly with the updated benchmarks - see the commit messages and PR description.

I have also reworked the renames to use simple scripted diffs for the renames (it even revealed a `xor_key` to `obfuscation` rename I missed and in one of the commit messages was still using the `XOR`
...

💬 ismaelsadeeq commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179737580)
nit licence update

💬 ismaelsadeeq commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179741353)
nit: since we are changing the datatype, also change the name to virtual_bytes?

🤔 ismaelsadeeq reviewed a pull request: "refactor: CFeeRate encapsulates FeeFrac internally"
(https://github.com/bitcoin/bitcoin/pull/32750#pullrequestreview-2978665608)
Code review 0aaeba9fe79df5ed51b3802056f93c0e7857a9fe

I think you should document the current behavior in `CFeeRate` docstring

1. Passing any virtual bytes less than or equal to 0 to the constructor will result in 0 fee rate per 0 size.
2. Passing negative virtual bytes to `GetFee` will return -1 as the fee.

💬 ismaelsadeeq commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179806415)
why the assume, is it not better to just return the -1? and add a test for it?

Also it was added in first commit and removed here, the two commits should be squashed.

💬 ismaelsadeeq commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179800839)
you can remove the type conversion, I think it is no-op because div is implicitly `int32_t`

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