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

💬 cedwies commented on pull request "tests: cover abortrescan() in-flight True path with dynamic-tail retry":
(https://github.com/bitcoin/bitcoin/pull/33131#discussion_r2261387883)
Thanks for the suggestion, I will look into this. Could work in theory, but my guess would be issues with portability, too. And maybe lack robustness. I will dig into this, and thank you for the review so far.

📝 l0rinc opened a pull request: "test: use local `CBlockIndex` in block read hash mismatch check"
(https://github.com/bitcoin/bitcoin/pull/33154)
Avoid mutating the shared active tip `CBlockIndex` in the `blockmanager_readblock_hash_mismatch` test.
Instead, construct a local `CBlockIndex` with only the required fields set, ensuring the test remains self-contained and hopefully eliminating the data race reported in https://github.com/bitcoin/bitcoin/issues/33150.

💬 sipa commented on pull request "validation: detect witness stripping without re-running Script checks":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2261412071)
I'm not convinced by the inclusion of the `&& !redeem_script.IsPayToAnchor()` part inside the P2SH check here.

P2SH-P2A is not a thing, as far as our codebase is concerned, just like P2SH-P2TR is not a thing. Highly unlikely of course, but P2SH-WITV1 could feasibly be assigned a consensus meaning in the future, which could require a witness. The code treating it as a P2A is incorrectly "guessing" that it will never need a witness to spend, and thus if we see it in a witness-stripped transacti
...

💬 l0rinc commented on issue "intermittent TSAN failure in lockmanager_tests::blockmanager_readblock_hash_mismatch":
(https://github.com/bitcoin/bitcoin/issues/33150#issuecomment-3165735263)
Not sure if this will fix it, but avoiding mutating shared state is desirable anyway - pushed https://github.com/bitcoin/bitcoin/pull/33154

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

💬 fkorotkov commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2261481446)
I actually email @m3dwards couple hours ago that we changed behaviour:

> "restore-keys" now should return the newest entry within the last 1000 cache entries in lexicographical order. There might be an edge case where one can accumulate more than 1000 keys before the eviction happens. We added some metrics to track this case and thought of lazy evicting entries based on some heuristic.

So it should now work as expected and return the freshest entity among first 1000 in lexicographical orde
...

🤔 l0rinc reviewed a pull request: "test,refactor: extract script template helpers & widen sigop count coverage"
(https://github.com/bitcoin/bitcoin/pull/32729#pullrequestreview-3098836449)
Pushed a new set of smaller, mostly test fixes - thanks for detailed review again!

> though personally I'd try to avoid front/back methods

Done, they're more uniform this way. I left the hard-coded indexes for redundancy.

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2261517398)
> Also I do see IsWitnessProgram followed by IsPayToTaproot in the header before https://github.com/bitcoin/bitcoin/commit/b71e7a07becb5d74db4bc1fdd60899b5ec5157f4 and IsPayToTaproot followed by IsWitnessProgram after that commit

Yes, the quick template helpers are grouped above the `IsWitnessProgram`, the move is deliberate.

> test the new helper method in a simpler way, now that we don't have to use Solver

I don't actually mind keeping both the solver and the new helpers - it's even b
...