💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254180036)
In commit "refactor: pull `IsPayToScriptHash` to header" (0d51088d1daa15ade3bf3c57410a0c12df235615)

It doesn't seem like WITNESS_V0_KEYHASH_SIZE is really the right constant to use for a p2sh script. Maybe should introduce a SCRIPT_HASH_SIZE or similar constant instead? Could also make sense to use the same constant for the memcpy in IsToScriptID

📝 maflcko opened a pull request: "ci: Remove unused CI_FAILFAST_TEST_LEAVE_DANGLING"
(https://github.com/bitcoin/bitcoin/pull/33137)
`CI_FAILFAST_TEST_LEAVE_DANGLING` was added in commit 26d98d51f2cfc902df35f855590c052e16a5ce12 to run a bash `trap`. However, now that the `trap` was removed in commit 904631e0fc00ac9c8a03d1ce226d071bf88c00db, this can be removed as well.

If there is need for this in the future, it seems simpler to just explicitly execute `test_runner.py` in a new session via `setsid` instead of using this config/env juggling.

💬 marcofleon commented on pull request "refactor: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/33116#discussion_r2254283425)
fixed

💬 marcofleon commented on pull request "refactor: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/33116#discussion_r2254286215)
Left this one as is because `hash` seems to be used quite a bit throughout the gui and I don't want to change more code than I do currently.

💬 marcofleon commented on pull request "refactor: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/33116#discussion_r2254287360)
fixed, along with the other ones in the rpc code

💬 marcofleon commented on pull request "refactor: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/33116#issuecomment-3155138166)
Addressed @janb84's comments and rebased after https://github.com/bitcoin/bitcoin/pull/32941.

✅ fanquake closed an issue: "ci: don't pass GIT_EXEC_PATH inside test container"
(https://github.com/bitcoin/bitcoin/issues/32935)

🚀 fanquake merged a pull request: "ci: Only pass documented env vars"
(https://github.com/bitcoin/bitcoin/pull/33002)

📝 fanquake converted_to_draft a pull request: "ci: Use mlc `v1` and ignore `depends/work`"
(https://github.com/bitcoin/bitcoin/pull/33125)
Otherwise you might see failures like this, if there are build artifacts in your depends dir:
```bash
[Err ] ./depends/work/build/x86_64-w64-mingw32/qt/6.7.3-64961f1d77c/qtbase/tests/auto/corelib/serialization/qxmlstream/XML-Test-Suite/xmlconf/xmlconf-20031030.htm (39, 55) => /Member/#confidential - Target not found.
[Err ] ./depends/work/build/x86_64-w64-mingw32/qt/6.7.3-64961f1d77c/qtbase/tests/auto/corelib/serialization/qxmlstream/XML-Test-Suite/xmlconf/xmlconf-20020521.htm (39, 55) => /Me
...

💬 m3dwards commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2254396245)
Added a comment. The docker build container uses gha style cache which is available on the host at http://127.0.0.1:12321/

💬 darosior commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2254398401)
> I would prefer returning witness-stripped in that scenario.

Sure, ok. That's my preference as well, so if people agree i'll do that then.

> Shouldn't this mimic VerifyScript more closely?

Pushonly is already checked before in `IsStandardTx()`. Checking the redeemscript hash and clean stack make sense. But so would checking every other standardness / consensus rule first. In fact i think your point hints that we should instead check for this after every other check.

What do you (bot
...

💬 maflcko commented on pull request "ci: Remove unused CI_FAILFAST_TEST_LEAVE_DANGLING":
(https://github.com/bitcoin/bitcoin/pull/33137#issuecomment-3155286213)
Induced failure works and looks as expected: https://github.com/bitcoin/bitcoin/runs/47420378191 (Just adding a new `Exit status: 137` at the end)

🤔 Jacksdad2015 reviewed a pull request: "[29.x] doc: minor rel notes changes"
(https://github.com/bitcoin/bitcoin/pull/32252#pullrequestreview-3088402937)
Don't know

💬 darosior commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3155344377)
> Why would this case ever occur?

Most likely never because we have the detection. If we didn't, someone may try to exploit this case to hinder propagation of 1p1c packages (see OP).

> Seems like a lot of code to handle an unexpected path.

Yeah. Note however this is *replacing* expensive code. I think it's always fair to question adding more code, but i hope in this instance it is pretty clear that replacing the expensive code by an inexpensive, albeit slightly more verbose, one is wort
...

💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2254489248)
Thx, but this reminds me that the setting the min port is missing? https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236668156

💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2254490171)
Actually, with `network=host`, this is needed, no?

💬 darosior commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2254514636)
Hmm and if we do so, we don't even need to carve out an exception for P2A, because using P2A with an empty witness can never result in a failure.

I'm going to implement that now.

💬 pinheadmz commented on pull request "Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications":
(https://github.com/bitcoin/bitcoin/pull/33117#issuecomment-3155500225)
concept ACK, tested the mainnet snapshot load with the QML gui branch built on top of this

💬 darosior commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2254614655)
Right, i should have double checked. Fixed in upcoming push, thanks.

💬 darosior commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2254632919)
Changing the call site to where we currently perform detection in master is not going to avoid all behaviour changes. Short of checking all consensus rules for all other inputs preceding the one that failed the check, we can't guarantee that we won't return a false-positive witness-stripped error. For instance if we receive a transaction with two inputs, the first a legacy P2PKH, the second a Segwit P2WPKH, then if the transaction comes in without witness but also fails the signature check for t
...