💬 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

💬 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_r2254953402)
in 0c420772247ff51b510d44061a2e94e3f5195899: this pattern of creating the keyagg cache and verifying it against a given aggregate pubkey appears more often in later commits (in `CKey::CreateMuSig2Nonce` and `CKey::CreateMuSig2PartialSig`), so could refactor it into an own helper (can be done in a follow-up though)

💬 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_r2254957752)
could use `MUSIG_CHAINCODE` the constant, here and in `SignTaproot` below

💬 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_r2254970515)
in 0c420772247ff51b510d44061a2e94e3f5195899: nit: since the size is known at compile-time, could alternatively return a `std::array` (OTOH, at the call-site a std::vector is still needed due to potential adding of the sighash byte)

💬 furszy commented on pull request "index: remove unnecessary locater cleaning in BaseIndex::Init()":
(https://github.com/bitcoin/bitcoin/pull/32882#discussion_r2254973724)
Better to `return std::nullopt`.

💬 sipa commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-3156091497)
Rebased.

💬 maflcko commented on issue "OpenBSD, NetBSD: `-reindex` is broken":
(https://github.com/bitcoin/bitcoin/issues/33128#issuecomment-3156113524)
> > ... showing the size of the blk dat file.
>
> You mean `blkdat.GetPos()`?

A bit like commit 734737b5930df7cebab83cf0dbe5fd390143f2be

💬 theuni commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-3156150296)
> The final commit changes the behavior to only using the cache for non-consensus script validation. I'm open to feedback about whether adding this commit is worth it.

I'm not sure I understand the rationale for this. Consensus validation will still be affected by the new midstate caches by virtue of hitting a cached entry in the script execution cache which used them previously, no?

💬 sipa commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-3156194943)
> Consensus validation will still be affected by the new midstate caches by virtue of hitting a cached entry in the script execution cache which used them previously, no?

No, the cache only lives for the duration of one script execution. It does not survive beyond script execution, let alone across multiple validations of the same transaction. With the last commit, the cache object just won't be present in consensus-critical validations.

I'm ambivalent about the last commit, though. It's a
...

💬 darosior commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-3156205790)
> > Consensus validation will still be affected by the new midstate caches by virtue of hitting a cached entry in the script execution cache which used them previously, no?
>
> No, the cache only lives for the duration of one script execution.

Worth noting (as i had to convince myself during review) that it also does not affect affect consensus validation by means of the signature cache. The signature cache is looked up by sighash (among other things), so if the sighash differs because the
...

👍 ryanofsky approved a pull request: "test,refactor: extract script template helpers & widen sigop count coverage"
(https://github.com/bitcoin/bitcoin/pull/32729#pullrequestreview-3089148272)
Code review ACK f8208e92bd103359f8c3ceb3361eb7904099e994. I finished reviewing this and didn't see any problems, just left various suggestions. The changes overall seem nice for making this code less confusing and adding better test & benchmark coverage.

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254943653)
In commit "test: add `CountSigOps` edge-cases & known-template coverage" (c194618bb587a2e15ed11f268898599c74f1aa74)

Maybe add document boolean parameter with `/*fCompressed=*/` here and below

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254951939)
In commit "test: add `CountSigOps` edge-cases & known-template coverage" (c194618bb587a2e15ed11f268898599c74f1aa74)

Maybe reorder the CountSigOpsKnownTemplates before CountSigOpsErrors to make it easier to understand how the fake scripts differ from real scripts.

💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254970939)
In commit "test: make sure OP_CHECKSIGADD isn't considered valid (legacy) sigop" (f8208e92bd103359f8c3ceb3361eb7904099e994)

Would suggest dropping "test:" prefix from this commit since it is changing non-test code. Could also split this up into a renaming and a test commit.

Also I'm not sure it is good to use LEGACY to mean pre-taproot here. In the OutputType enum, legacy seems to means "pre-segwit" and in GetLegacySigOpCount legacy seems to mean "pre-p2sh", so adding a third definition of
...