💬 maflcko commented on pull request "ci: Remove redundant RUN_UNIT_TESTS_SEQUENTIAL":
(https://github.com/bitcoin/bitcoin/pull/33136#issuecomment-3165130053)
(trivial rebase)

💬 w0xlt commented on issue "spendable is true for UTXO of private key disabled wallet":
(https://github.com/bitcoin/bitcoin/issues/33110#issuecomment-3165217540)
Considering that the legacy wallet can no longer be loaded, will this field always be true?

💬 maflcko commented on pull request "test: Run bench sanity checks in parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3165272701)
(trivial rebase)

💬 w0xlt commented on pull request "test: revive test verifying that `GetCoinsCacheSizeState` switches from OK→LARGE→CRITICAL":
(https://github.com/bitcoin/bitcoin/pull/33021#discussion_r2261145849)
Brace initialization will convert `size_t` to `int64_t`

```suggestion
constexpr int64_t MAX_BLOCK_COINSDB_USAGE_BYTES{10_MiB};
```

💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#issuecomment-3165392837)
> it could debug-assert that capacity is even

Added, thanks for the hint

👍 ryanofsky approved a pull request: "test,refactor: extract script template helpers & widen sigop count coverage"
(https://github.com/bitcoin/bitcoin/pull/32729#pullrequestreview-3097866742)
Code review ACK dd1a7ab428a7c7f87b2a02580aca0d593c2de68a. Thanks for the updates! I left a few more suggestions, but nothing important.

I think it might be good to link directly to your optimization commit d76d7531dfcc7ef2d104b8977a2239cb1fc89119 in the PR description since it helps explain the changes here and bring them together.

On style changes in the CScript methods I think they are an improvement overall, though personally I'd try to avoid front/back methods and magic numbers and wri
...

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2261132664)
re: https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255448800

> You still think it needs additional explanations?

It seems ok, but maybe consider adding your explanation to the commit message: "The `BOOST_CHECK_EQUAL` to `BOOST_CHECK` change is just to test the new helper method in a simpler way, now that we don't have to use `Solver`."

I wouldn't have asked the question if I had known what the intention behind the solver change was, so this wasn't obvious to me.

Also I do
...

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2260808130)
In commit "refactor: rename `GetSigOpCount` to `CountSigOps`" (006a428613710dde24866cf4f6efd7b68b6ee294)

This sentence should be prefixed with "When enforcing the `MAX_BLOCK_SIGOPS_COST` limit" to be correct since it doesn't apply in other cases. (Problem is fixed in the next commit but the change belongs this commit. Sorry my previous feedback was a bit jumbled and I mixed the commits together.)

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2260854463)
In commit "refactor: split off `P2SH` from `GetSigOpCount`" (6bf5dce801e0da9d372d42d2c0650c283ec787f5)

I still think you should consider simplifying these lines to:

```c++
nSigOps += CountP2SHSigOps(tx.vin[i].scriptSig, coin.out.scriptPubKey);
```

and simplifying CheckSigopsBIP54 to:

```c++
sigops += txin.scriptSig.CountSigOps(/*fAccurate=*/true);
sigops += prev_txo.scriptPubKey.CountSigOps(/*fAccurate=*/true);
sigops += CountP2SHSigOps(txin.scriptSig, prev_txo.scriptPubKey);
`
...

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2261095050)
In commit "refactor: make sure `OP_CHECKSIGADD` isn't considered valid (legacy) sigop" (a2cce66f11e635924c34b27b253214be8815b0bc)

Thanks, maybe also replace "(legacy)" in commit description with "pre-tapscript" or "base"

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2261177012)
re: https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254732750

To clarify I agree code is doing the right thing and the CScript class shouldn't be looking inside the keys to check the format. I just think short comments could be helpful to prevent misunderstandings. Maybe:

- `//! Detect P2PK script with a compressed public key. Doesn't check the 0x02/0x03 key prefix.`
- `//! Detect P2PK script with a uncompressed public key. Doesn't check the 0x04 key prefix.`

Feel free to le
...

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2260876336)
re: https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255358039

Thanks. I think I just dislike reviewing changes before I know what purpose / motivation of the change is. I know you can often guess motivations after the fact, but it is usually helpful imo to see them stated explicitly.

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2260899980)
re: https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255644392

> I played with this, but really dislike the new version, mostly for how awkward `CheckSigopsBIP54` has become after it.

I'm confused because the diff makes this code shorter than before, removes a conditional, and makes it match the BIP54 spec more closely (see other comment about BIP54 above). Also I liked the ternary version of this better than the current version. But whatever you prefer is fine with me and maybe
...

💬 l0rinc commented on pull request "test: revive test verifying that `GetCoinsCacheSizeState` switches from OK→LARGE→CRITICAL":
(https://github.com/bitcoin/bitcoin/pull/33021#discussion_r2261243522)
I don't think this works on every platform (e.g. 32 bits) - I probably had a version like this before

💬 TheCharlatan commented on pull request "RFC: Riscv bare metal CI job":
(https://github.com/bitcoin/bitcoin/pull/31425#issuecomment-3165544024)
Rebased f27760012b3e236933ea5d1b39a6ba74884df1a2 -> ec7c86c732c995d2c38e2a3f93ad55a451a8cf81 ([bare_metal_support_1](https://github.com/TheCharlatan/bitcoin/tree/bare_metal_support_1) -> [bare_metal_support_2](https://github.com/TheCharlatan/bitcoin/tree/bare_metal_support_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/bare_metal_support_1..bare_metal_support_2))

* Fixed a bunch of silent and proper merge conflicts.

🤔 mzumsande reviewed a pull request: "Adding alert for failure to prevent dead-end user crash"
(https://github.com/bitcoin/bitcoin/pull/33127#pullrequestreview-3098565003)
> Yeah agreed overall. However i cant repro and don't have the data dir. That being said this deadend the user to a crash with no explanation or logs. And the crash is repeated on every launch. This would just a lot of node runners to give up and move on. Atleast this way they have some way out. It's quite a soft change.

The problem is that this is added to such a central spot that you could imagine other ways leading to this assert failing, in particular ways that have nothing to do with db
...

💬 TheCharlatan commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#issuecomment-3165575610)
Rebased 946d794919b640a3e5f78e853b3291f6711aa98b -> 88d8bc9a1cc0aa8177424d411fe876aaca47e466 ([kernelRmClientversion_0](https://github.com/TheCharlatan/bitcoin/tree/kernelRmClientversion_0) -> [kernelRmClientversion_1](https://github.com/TheCharlatan/bitcoin/tree/kernelRmClientversion_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelRmClientversion_0..kernelRmClientversion_1))

* Fixed conflict with #33077

💬 TheCharlatan commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#issuecomment-3165591787)
Updated 88d8bc9a1cc0aa8177424d411fe876aaca47e466 -> 04bc2bb530f9e8cc4dd8321be5811a9760624e53 ([kernelRmClientversion_1](https://github.com/TheCharlatan/bitcoin/tree/kernelRmClientversion_1) -> [kernelRmClientversion_2](https://github.com/TheCharlatan/bitcoin/tree/kernelRmClientversion_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelRmClientversion_1..kernelRmClientversion_2))

* Addressed @maflcko's [comment](https://github.com/bitcoin/bitcoin/pull/32543#discussion_r2095321
...

📝 TheCharlatan converted_to_draft a pull request: "kernel: Remove dependency on clientversion"
(https://github.com/bitcoin/bitcoin/pull/32543)
Including a Bitcoin-Core specific version does not make sense if used by external applications.

💬 darosior commented on pull request "validation: detect witness stripping without re-running Script checks":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2261326385)
Thanks, done.