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

💬 darosior commented on pull request "p2p: never check tx rejections by txid":
(https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3155611529)
I still think this PR is preferable to #33105. The latter will inevitably introduce false-positive witness stripped errors, whereby we won't add some transactions with consensus/standardness errors to the reject filter. Since we already don't add all transactions with consensus/standardness errors to the reject filter, if we also don't care about these additional false positives, we might as well just to the cleaner thing and get rid of this filter entirely as is done here.

💬 0xB10C commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3155627414)
I've published a chart showing the share of sub 1 sat/vByte transactions on [mainnet.observer/charts/fees-sub-1-sat-vbyte-transactions](https://mainnet.observer/charts/fees-sub-1-sat-vbyte-transactions/).

I also generated a recent export of my [stats on compact block reconstructions](https://delvingbitcoin.org/t/stats-on-compact-block-reconstructions/1052) over the last two months:

<img width="1093" height="584" alt="image" src="https://github.com/user-attachments/assets/fb15af92-325b-4669
...

💬 darosior commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3155651614)
> In June, we were requesting less than 10 kB worth of transactions per block on average. Now, we are requesting close to 0.8 MB worth of transactions per block on average.

Crazy. Although this is a significant diff we might want to backport this PR at least to 29.

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

Two things:

- It might be nice to make `CountP2SHSigOps` function more similar to `CountWitnessSigOps` and accept a `scriptPubKey` argument. It looks like this change also makes call sites simpler.
- I noticed with BIP54 the `CScript::CountSigOps` comment when to use `fAccurate=false` will not be right anymore. Would suggest qualifying the comment with "When enforcing the `MAX_BLOCK_SIGO
...