💬 darosior commented on pull request "test: check P2SH sigop count for coinbase tx":
(https://github.com/bitcoin/bitcoin/pull/32850#issuecomment-3027122136)
Yeah, i think it's preferable to remove dead code than to test it:
```diff
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 95466b759cb..5036e26e21a 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -125,8 +125,7 @@ unsigned int GetLegacySigOpCount(const CTransaction& tx)

unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& inputs)
{
- if (tx.IsCoinBase())
- return 0;
+ Assert(!tx.IsCoin
...

👍 darosior approved a pull request: "feature_taproot: sample tx version border values more"
(https://github.com/bitcoin/bitcoin/pull/32841#pullrequestreview-2978437124)
ACK 4be81e9746e9e18923386d6f4945a33885fd98a7

💬 fanquake commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2179642119)
> I think it's fine,

Not sure I agree that we should start installing this. It's an experimental, demo binary, not built by default, that nobody is using; other than a handful of developers working on the kernel (also experimental). There's every likelyhood it could just be removed tommorow.

💬 fanquake commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3027233699)
> Removing these esoteric and not generally useful binaries from bin/ and from the system PATH should make it easier for typical users to find and identify the binaries which are actually useful.

I don't really agree that `bench_bitcoin` and particularly `test_bitcoin` are "esoteric and not generally useful", and agree with @TheCharlatan's comment: `Maybe there is some utility in testing your platform before actually running bitcoin on it.`. If we do think that `test_bitcoin` is not useful, t
...

💬 maflcko commented on pull request "feature_taproot: sample tx version border values more":
(https://github.com/bitcoin/bitcoin/pull/32841#issuecomment-3027265968)
lgtm ACK 4be81e9746e9e18923386d6f4945a33885fd98a7

Also tested the failure on current master.

📝 hebasto opened a pull request: "Update `minisketch` subtree and switch to its build script"
(https://github.com/bitcoin/bitcoin/pull/32856)
This PR:
1. Updates the `minisketch` subtree to latest upstream, which includes:
- https://github.com/bitcoin-core/minisketch/pull/75

2. Switches to `minisketch` upstream build system.

💬 hebasto commented on pull request "Update `minisketch` subtree and switch to its build script":
(https://github.com/bitcoin/bitcoin/pull/32856#issuecomment-3027293159)
This PR concludes the joint efforts of @sipa, @purpleKarrot, @theuni, and me.

💬 Sjors commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3027353830)
Now that the wrapper binary landed in #31375 I think it's fine to ask users to use `bitcoin test` instead of `test_bitcoin`.

I agree `test_bitcoin` (and maybe `bench_bitcoin`) are useful enough to have them in the release.

But I doubt there are many _automated_ deployments out there that obtain the release binaries and run the tests. That's the only thing this PR would break.

🚀 fanquake merged a pull request: "feature_taproot: sample tx version border values more"
(https://github.com/bitcoin/bitcoin/pull/32841)

🤔 hebasto reviewed a pull request: "depends: fix libevent `_WIN32_WINNT` usage"
(https://github.com/bitcoin/bitcoin/pull/32837#pullrequestreview-2978694585)
My Guix build:
```
aarch64
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
9f65d8ac
...

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