💬 darosior commented on pull request "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only":
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-2759027670)
If the next block's (889707) hash represent an even uint256 i'll do it, if it's odd you do.

👍 ryanofsky approved a pull request: "test: create assert_not_equal util"
(https://github.com/bitcoin/bitcoin/pull/29500#pullrequestreview-2723044504)
Code review ACK d9cb032ec1aa7000fc3f5b9fa48405f269879497. Seems like a nice change, though needs to be rebased. I think this change is good because it makes tests more readable, prints more information on failures, avoids misusing the `assert` keyword, and makes the test framework api more consistent internally and consistent with other frameworks.

💬 martinus commented on pull request "Draft: CCoinMap Experiments":
(https://github.com/bitcoin/bitcoin/pull/32128#discussion_r2017405150)
I think boost maps might be a better choice, I believe they are better tested, and might be faster as well. Also for `boost::unordered_node_map` references are stable, which is not the case for `unordered_dense`, so it is easier to have this as a drop-in replacement without worrying about pointer stability.

💬 martinus commented on pull request "Draft: CCoinMap Experiments":
(https://github.com/bitcoin/bitcoin/pull/32128#discussion_r2017406465)
It seems it has far less of an effect in your test, maybe I should also run the benchmark on my machine until block 890k for comparison

💬 martinus commented on pull request "Draft: CCoinMap Experiments":
(https://github.com/bitcoin/bitcoin/pull/32128#discussion_r2017408749)
I think collision attacks can't be an issue because the hash implementation still uses the random seed, and the hash itself is of good quality.

💬 hebasto commented on issue "build: make macOS build Clang only":
(https://github.com/bitcoin/bitcoin/issues/30206#issuecomment-2759053713)
> > The last commit makes the macOS builds on Guix Clang only.
>
> Seems like that commit is missing the diff to actually remove the GCC toolchain from the macOS build? Applying that causes the build to fail:
>
> ```
> diff --git a/contrib/guix/manifest.scm b/contrib/guix/manifest.scm
> index 4e7e955218..f1e3db81c8 100644
> --- a/contrib/guix/manifest.scm
> +++ b/contrib/guix/manifest.scm
> @@ -544,7 +544,6 @@ inspecting signatures in Mach-O binaries.")
> gzip
> xz
>
...

📝 instagibbs reopened a pull request: "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only"
(https://github.com/bitcoin/bitcoin/pull/26398)
Since the original fix was set to be a "reasonable" transaction
to reduce allocations and the true motivation later revealed,
it makes sense to relax this check to something more principled.

There are more exotic transaction patterns that could take advantage
of a relaxed requirement, such as 1 input, 1 output OP_RETURN to burn
a utxo to fees for CPFP purposes when change isn't practical.

Two changes could be accomplished:

1) Anything not 64 bytes could be allowed

2) Anything abo
...

👍 ryanofsky approved a pull request: "depends: patch Qt rounding bugs"
(https://github.com/bitcoin/bitcoin/pull/32081#pullrequestreview-2723072817)
Code review ACK 9ca76aa4210d15060fe35eb95da0862af57a8cc9

Reviewed by comparing https://github.com/qt/qtbase/commit/8c8b9a4173f4add522ec13de85107deba7c82da0.patch and
https://github.com/fanquake/bitcoin/raw/refs/heads/fix_qt_32_O0/depends/patches/qt/normalize_round.patch and confirming they are effectively the same patch.

👍 ryanofsky approved a pull request: "refactor: Enforce readability-avoid-const-params-in-decls"
(https://github.com/bitcoin/bitcoin/pull/31650#pullrequestreview-2723116771)
Code review ACK fa8e57951abc5c737ab68694b8f6aa77551faa6b. Agree it would be nice to enforce this check to reduce noise and prevent potential bugs

💬 ryanofsky commented on pull request "refactor: Enforce readability-avoid-const-params-in-decls":
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2017450776)
In commit "refactor: Enforce readability-avoid-const-params-in-decls" (fa8e57951abc5c737ab68694b8f6aa77551faa6b)

Since these are definitions not declarations, I don't think these actually need to change to fix the warnings. Does seem good to change, though.

💬 ryanofsky commented on pull request "refactor: Enforce readability-avoid-const-params-in-decls":
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2017443670)
In commit "refactor: Avoid copies by using const references or by move-construction" (fafe09b0161c8c8d22a1e2e1bad0428b51fca813)

This change seems reasonable but would point out that sometimes short string copies like this can be intentional:
https://github.com/bitcoin/bitcoin/pull/24306#discussion_r804625724. I'd probably go with this change, but I don't actually know if copying might be better.

💬 l0rinc commented on pull request "Draft: CCoinMap Experiments":
(https://github.com/bitcoin/bitcoin/pull/32128#discussion_r2017460739)
Or maybe these are only measurable after the other changes are in place - I have compared all commits independently.

💬 janb84 commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#discussion_r2017471095)
Found this construction bid odd, asserting something that is known to fail. If the assertion is removed the unit test fails more gracefully, from 234 failures reported to 2 failures reported. Is this approach deliberate?


```suggestion
```

💬 sipa commented on pull request "Draft: CCoinMap Experiments":
(https://github.com/bitcoin/bitcoin/pull/32128#discussion_r2017482800)
I'm pretty skeptical about that claim.

👍 ryanofsky approved a pull request: "test: Add test coverage for rpcwhitelistdefault when unset"
(https://github.com/bitcoin/bitcoin/pull/32079#pullrequestreview-2723186603)
Code review ACK b77485cc7220314edc05adf3b2cb298af556e36c. This is a very good check to add. We should be making sure `-rpcwhitelist` settings are effective even when `-rpcwhitelistdefault` is unset.

I think the last commit could be shorter and clearer by reusing existing checks instead of copying them. Would suggest the following change to simplify:

<details><summary>diff</summary>
<p>

```diff
--- a/test/functional/rpc_whitelist.py
+++ b/test/functional/rpc_whitelist.py
@@ -102,7 +1
...

📝 darosior opened a pull request: "miner: timelock the coinbase to the mined block's height"
(https://github.com/bitcoin/bitcoin/pull/32155)
The Consensus Cleanup soft fork proposal includes enforcing that coinbase transactions set their
nLockTime field to the block height minus 1, as well as their nSequence such as to not disable the
timelock. If such a fork were to be activated by Bitcoin users, miners need to be ready to produce
compliant blocks at the risk of losing substantial amounts mining would-be invalid blocks. As miners
are unfamously slow to upgrade, it's good to make this change as early as possible.

Although Bitc
...

💬 instagibbs commented on issue "test: `p2p_message_capture.py` fails with GCC 14 & undefined sanitizer":
(https://github.com/bitcoin/bitcoin/issues/32016#issuecomment-2759249245)
I'm seeing this on 13.3.0 fwiw

💬 darosior commented on pull request "qa: make feature_assumeutxo.py test more robust":
(https://github.com/bitcoin/bitcoin/pull/32117#issuecomment-2759274663)
For what it's worth https://github.com/bitcoin/bitcoin/pull/32155 is the branch i was working on and the reason for this PR. I just wanted to avoid changing the error messages there and let reviewers figure out why it's fine (which involves learning the structure of the snapshot, familiarizing themselves with the offset, and understanding that actually the error message depends on the next txid in the snapshot and therefore may change if the txid change).

I usually find that "change the error
...

💬 sipa commented on pull request "Follow-ups for txgraph #31363":
(https://github.com/bitcoin/bitcoin/pull/32151#discussion_r2017510416)
This instantly crashes the fuzz tests for me:

```diff
- if (picked_num < todo.Size() && todo[picked_num]) {
+ if (picked_num < todo.Size()/* && todo[picked_num]*/) {
picked = picked_num;
}
```

💬 Rsed19901 commented on pull request "build: Remove Autotools-based build system":
(https://github.com/bitcoin/bitcoin/pull/30664#discussion_r2017511202)
> I'm still not sure how I feel about removing all of these. We are going to switch a lot between the versions with and without cmake over the next two years. Is it really desirable to have all these files appear in the untracked list?