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

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

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2261578300)
Thanks, done

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2261571418)
Not after the fact, but that the commited code contained the added comments already explaining - not important, it's done :)

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

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2261493776)
Reworded the commit message, thanks

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

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2261477694)
Brought the change one commit earlier to avoid modifying the same thing twice.

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2261570975)
If you say you find it more readable, I don't mind - changed