💬 Eunovo commented on pull request "wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs":
(https://github.com/bitcoin/bitcoin/pull/32618#issuecomment-3026599126)
ACK https://github.com/bitcoin/bitcoin/pull/32618/commits/b1a8ac07e91dd1d305fcbc16ea931d60e46c0055

💬 Eunovo commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179306743)
Try `ConsumeIntegralInRange(0, std::numeric_limits<int32_t>::max())`

💬 Eunovo commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179291875)
https://github.com/bitcoin/bitcoin/pull/32750/commits/c4af90f4a80832cbaa925d33dbf4ba0f0eab6374: Consider

```diff
diff --git a/src/test/amount_tests.cpp b/src/test/amount_tests.cpp
index 40f3119cf1..65278aaa41 100644
--- a/src/test/amount_tests.cpp
+++ b/src/test/amount_tests.cpp
@@ -78,13 +78,15 @@ BOOST_AUTO_TEST_CASE(GetFeeTest)
BOOST_CHECK(CFeeRate(CAmount(-1), 1000) == CFeeRate(-1));
BOOST_CHECK(CFeeRate(CAmount(0), 1000) == CFeeRate(0));
BOOST_CHECK(CFeeRate(CAmoun
...

💬 Eunovo commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179302609)
https://github.com/bitcoin/bitcoin/pull/32750/commits/c4af90f4a80832cbaa925d33dbf4ba0f0eab6374:
```diff
diff --git a/src/test/fuzz/fee_rate.cpp b/src/test/fuzz/fee_rate.cpp
index 20578c325e..26146bc617 100644
--- a/src/test/fuzz/fee_rate.cpp
+++ b/src/test/fuzz/fee_rate.cpp
@@ -20,8 +20,8 @@ FUZZ_TARGET(fee_rate)
const CFeeRate fee_rate{satoshis_per_k};

(void)fee_rate.GetFeePerK();
- const auto bytes = fuzzed_data_provider.ConsumeIntegral<int32_t>();
- if (bytes >= 0
...

💬 maflcko commented on something "":
(https://github.com/bitcoin/bitcoin/commit/be3435c2ff82f69db11ba032452614404dac15f9#r161257474)
nit in be3435c2ff82f69db11ba032452614404dac15f9: Any reason to rename `data` to `test_data` and change the unit? Seems fine to just keep `bench.batch(data.size()).unit("byte").run([&] {`?

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-3026764342)
> removing the benchmark

My understanding is that changing the benchmark is required. Maybe this can be explained better in the commit message to mention:

* 31-byte xor-keys are not used in the codebase, so using the common size (8) makes more sense.

I am not sure about changing the size to 10_MiB, because database obfuscation still runs on the values individually, which are smaller in reality. Also, the buffered reader will read smaller chunks than that in reality. So maybe the data si
...

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2179333243)
nit in be3435c2ff82f69db11ba032452614404dac15f9: Any reason to rename `data` to `test_data` and change the unit? Seems fine to just keep `bench.batch(data.size()).unit("byte").run([&] {`?

💬 maflcko commented on pull request "test: check P2SH sigop count for coinbase tx":
(https://github.com/bitcoin/bitcoin/pull/32850#issuecomment-3026825528)
the branch is dead code outside of tests, so it could also be removed/disabled. Though, it may be better to add a test than to refactor the consensus code here. So lgtm

⚠️ maflcko opened an issue: "intermittent timeout in wallet_signer.py : 'createwallet' RPC took longer than 1200.000000 seconds"
(https://github.com/bitcoin/bitcoin/issues/32855)
in `self.nodes[1].createwallet(wallet_name='hww_disconnect', disable_private_keys=True, external_signer=True)`,

https://cirrus-ci.com/task/5704849158832128?logs=ci#L2967:

```
node1 2025-06-26T14:05:40.072184Z [msghand] [net_processing.cpp:3452] [ProcessMessage] [net] received: pong (8 bytes) peer=0
test 2025-06-26T14:05:43.222938Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "/ci_contai
...

💬 maflcko commented on pull request "wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs":
(https://github.com/bitcoin/bitcoin/pull/32618#issuecomment-3026946485)
re-ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055 🌈

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK b1a8ac07e91dd1d305fc
...

💬 rkrux commented on pull request "wallet, test: best block locator matches scan state follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32580#discussion_r2179522247)
Thanks, fixed.

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3027077942)
[Posted](https://gnusha.org/pi/bitcoindev/49dyqqkf5NqGlGdinp6SELIoxzE_ONh3UIj6-EB8S804Id5yROq-b1uGK8DUru66eIlWuhb5R3nhRRutwuYjemiuOOBS2FQ4KWDnEh0wLuA=@protonmail.com/T/#u) about this to the mailing list this morning.

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