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

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

Similar to previous, could replace `34` with `WITNESS_V1_TAPROOT_SIZE + 2` but no strong preference

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254707312)
In commit "refactor: add `IsPayToPubKeyHash` helper to script.h" (5d23e9a4c6fa4aab530889c54565954486572afd)

Again it seems a little off to use a segwit constant in pre-segwit output. Might be better to stick with literal 20 or maybe introduce a HASH160_SIZE or PUBKEY_HASH_SIZE constant. No strong preference though and technically this should be ok.

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

Changing `34` to `WITNESS_V0_SCRIPTHASH_SIZE + 2` might make this a little more self-documenting.

(I don't mean to bikeshed on use of literals vs constants and front/back in these functions though. I could imagine other developers preferring the literals for concreteness even if I like the constants, and I don't have a strong preference.)

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254731153)
In commit "refactor: add `IsCompressedPayToPubKey` helper to script.h" (017f58821fe857a2bb7ac14f2b00b349936391d8)

Seems reasonable to include pubkey.h but since we are only using constants, it might be better to move constants somewhere.

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254716286)
In commit "refactor: add `IsCompressedPayToPubKey` helper to script.h" (017f58821fe857a2bb7ac14f2b00b349936391d8)

I guess it is intentional to replace `CPubKey::COMPRESSED_SIZE + 2` with 35 here?

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254732750)
In commit "refactor: add `IsCompressedPayToPubKey` helper to script.h" (017f58821fe857a2bb7ac14f2b00b349936391d8)

Might be good to document that this method does not check the pubkey prefix (even/odd/uncompressed) and verify this is even or odd.

✅ maflcko closed a pull request: "ci: Remove unused CI_FAILFAST_TEST_LEAVE_DANGLING"
(https://github.com/bitcoin/bitcoin/pull/33137)

💬 maflcko commented on pull request "ci: Remove unused CI_FAILFAST_TEST_LEAVE_DANGLING":
(https://github.com/bitcoin/bitcoin/pull/33137#issuecomment-3155890738)
No idea why neither kill nor setsid works on macos, but both work on Linux. I guess this means `CI_FAILFAST_TEST_LEAVE_DANGLING` is still needed.

✅ w0xlt closed a pull request: "util: C++20 `ToIntegral()` Improvement"
(https://github.com/bitcoin/bitcoin/pull/32522)

💬 Aris-Ritz commented on something "":
(https://github.com/bitcoin/bitcoin/commit/6135cc3f07ae91cab91927a2b610aaed2fb38858#commitcomment-163470703)
Test

📝 maflcko opened a pull request: "ci: Pass CI_FAILFAST_TEST_LEAVE_DANGLING into container"
(https://github.com/bitcoin/bitcoin/pull/33138)
After commit fd813bf863b1ffa91429de6342285b35bab2bfa4, the env var `CI_FAILFAST_TEST_LEAVE_DANGLING` is no longer passed into the container.

This is harmless, because it isn't needed for the Linux containers and macos doesn't use containers at all.

However, it would be nice to document it as an allowed setting and consistently pass it on, when set. So do that here.

🤔 janb84 reviewed a pull request: "refactor: Convert uint256 to Txid"
(https://github.com/bitcoin/bitcoin/pull/33116#pullrequestreview-3089109923)
re ACK a20724d926d5844168c6a13fa8293df8c8927efe

changes since last ACK:
- Rebase
- Changes related to my Nit (Thanks for incorporating my suggestions!)

💬 l0rinc commented on pull request "index: remove unnecessary locater cleaning in BaseIndex::Init()":
(https://github.com/bitcoin/bitcoin/pull/32882#issuecomment-3156019542)
ACK 04044e554e6982977f9d82f9bb60659315a45f92

PR title still has the typo

💬 achow101 commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2254964882)
Potentially moving multiple files to a single file is not a good idea. Using a wildcard here makes this much more fragile and prone to error.

💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2254818672)
```suggestion
* So this class handles the secure allocation of the secp256k1_musig_secnonce object
```

💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2254807599)
in commit c78d25b89074325ea37185ca87bb00385ad4ab20: alternatively, could pass `tweak_out` as pointer (set to `nullptr` by default, in which case no memcpy would happen) here and for `CExtPubkey::Derive` to avoid the method overloading for the latter