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

💬 darosior commented on pull request "validation: detect witness stripping without re-running Script checks":
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3165605860)
Corrected commit message and OP as per @Crypt-iQ's [point](https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2260386539).

📝 cedwies converted_to_draft a pull request: "tests: cover abortrescan() in-flight True path with dynamic-tail retry"
(https://github.com/bitcoin/bitcoin/pull/33131)
This PR adds a test in wallet_transactiontime_rescan.py. Previously we only tested that abortrescan() returns False when no rescan was running. This change verifies that it returns True and actually interrupts a running scan.

How it works:
1. spawn a daemon background thread that rescans a tail of the wallet’s chain.
2. aggressively poll every 10 ms (for up to 5 s) until we see the node start scanning
3. Call 'abortrescan()' and assert to True
4. Threads are joined with short timeouts so
...

🤔 naiyoma reviewed a pull request: "validation: remove BLOCK_FAILED_CHILD"
(https://github.com/bitcoin/bitcoin/pull/32950#pullrequestreview-3098701894)
TestedACK 06db700a1315bb655ac7fa12578c626990a22ea5


Block 2 (valid)
/ \
(valid)Block height 3 Block height 3 (BLOCK_FAILED_CHILD)
| |
(valid) Block height 4 Block height 4(BLOCK_FAILED_CHILD)
| |

Before removing BLOCK_FAILED_CHILD,this test(https://github.com/bitcoin/bitcoin/issues/32173#issue-2960274990) would fail at: `self.log.info(self.nodes
...

💬 willcl-ark commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2261384462)
Yes I agree, the blob cache makes the action simpler, and the whole process easier to reason about (and doesnt require a carve out for the fallback if not using cirrus runners). But without it restoring newest first we are going to see worse cache hit-rates than with this approach.

Perhaps @fkorotkov could clarify here if oldest match is correct, and if there is a way to restore newest first like (as I understand) GHA does natively? My understanding, which may be incorrect, is that epoch seco
...