💬 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)
(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
(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)
(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`.
(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.
(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
(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?
(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
...
(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
...
(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.
(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
(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.
(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
...
(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
...
💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254983086)
In commit "test: add `CountSigOps` edge-cases & known-template coverage" (c194618bb587a2e15ed11f268898599c74f1aa74)
Forgot to check sigops here?
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254983086)
In commit "test: add `CountSigOps` edge-cases & known-template coverage" (c194618bb587a2e15ed11f268898599c74f1aa74)
Forgot to check sigops here?
💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255009999)
In commit "test: add `CountSigOps` edge-cases & known-template coverage" (c194618bb587a2e15ed11f268898599c74f1aa74)
Maybe it would make sense to check that some (most?) of these opcodes actually do satisfy `op_name == "OP_UNKNOWN"`, if that is the case.
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255009999)
In commit "test: add `CountSigOps` edge-cases & known-template coverage" (c194618bb587a2e15ed11f268898599c74f1aa74)
Maybe it would make sense to check that some (most?) of these opcodes actually do satisfy `op_name == "OP_UNKNOWN"`, if that is the case.
💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255016916)
In commit "test: add `CountSigOps` edge-cases & known-template coverage" (c194618bb587a2e15ed11f268898599c74f1aa74)
Maybe use `CScript{}` instead of `CScript()` since already using braced initialization for the variable
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255016916)
In commit "test: add `CountSigOps` edge-cases & known-template coverage" (c194618bb587a2e15ed11f268898599c74f1aa74)
Maybe use `CScript{}` instead of `CScript()` since already using braced initialization for the variable
💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255022936)
In commit "test: add `CountSigOps` edge-cases & known-template coverage" (c194618bb587a2e15ed11f268898599c74f1aa74)
The checks in this test are very repetitive. You might be able to make it easier understand and maintain by building a `std::vector<CScript>` of invalid scripts and looping over it.
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255022936)
In commit "test: add `CountSigOps` edge-cases & known-template coverage" (c194618bb587a2e15ed11f268898599c74f1aa74)
The checks in this test are very repetitive. You might be able to make it easier understand and maintain by building a `std::vector<CScript>` of invalid scripts and looping over it.
💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255054508)
In commit "test: make sure OP_CHECKSIGADD isn't considered valid (legacy) sigop" (f8208e92bd103359f8c3ceb3361eb7904099e994)
I'm confused about whether code in this commit is actually doing the right thing currently, of if some of it should be improved to support taproot opcodes. Would be helpful if commit message could clarify.
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255054508)
In commit "test: make sure OP_CHECKSIGADD isn't considered valid (legacy) sigop" (f8208e92bd103359f8c3ceb3361eb7904099e994)
I'm confused about whether code in this commit is actually doing the right thing currently, of if some of it should be improved to support taproot opcodes. Would be helpful if commit message could clarify.
💬 glozow commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3156213089)
Yeah compact block reconstruction looks horrendous. Thanks @0xB10C!
I looked a little bit at the distribution of sub-1sat/vB feerates to try make sure we're capturing the ones that are causing poor reconstruction. If a significant portion of transactions are even lower, I think we could lower this to 50sat/kvB while staying solidly within the DoS budget, but it doesn't seem necessary.
This is individual feerate from the last day's worth of blocks, showing 97% of them are >= 0.1sat/vB.
I
...
(https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3156213089)
Yeah compact block reconstruction looks horrendous. Thanks @0xB10C!
I looked a little bit at the distribution of sub-1sat/vB feerates to try make sure we're capturing the ones that are causing poor reconstruction. If a significant portion of transactions are even lower, I think we could lower this to 50sat/kvB while staying solidly within the DoS budget, but it doesn't seem necessary.
This is individual feerate from the last day's worth of blocks, showing 97% of them are >= 0.1sat/vB.
I
...
💬 l0rinc commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33094#issuecomment-3156224078)
Thanks @fanquake, I wanted to leave out `QMap` values, but since it enables adding `readability-container-contains` to `clang-tidy`, I'm all for it.
Done in the latest push in the last commit (since it's a non-trivial map type) - where I also rebased to make sure we're capturing all potential recent additions.
Should be easy to re-review.
(https://github.com/bitcoin/bitcoin/pull/33094#issuecomment-3156224078)
Thanks @fanquake, I wanted to leave out `QMap` values, but since it enables adding `readability-container-contains` to `clang-tidy`, I'm all for it.
Done in the latest push in the last commit (since it's a non-trivial map type) - where I also rebased to make sure we're capturing all potential recent additions.
Should be easy to re-review.