💬 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.
💬 l0rinc commented on pull request "index: remove unnecessary locater cleaning in BaseIndex::Init()":
(https://github.com/bitcoin/bitcoin/pull/32882#discussion_r2255095594)
Isn't that what we're doing already? I tried making it explicit in:
```C++
if (CBlockLocator locator; false) return locator;
return {};
```
which successfully enters:
```C++
if (!locator) {
SetBestBlockIndex(nullptr);
```
since `locator.has_value() == false` when running `txindex_tests/txindex_initial_sync`.
Or do you mean just that it's more aesthetic that way?
(https://github.com/bitcoin/bitcoin/pull/32882#discussion_r2255095594)
Isn't that what we're doing already? I tried making it explicit in:
```C++
if (CBlockLocator locator; false) return locator;
return {};
```
which successfully enters:
```C++
if (!locator) {
SetBestBlockIndex(nullptr);
```
since `locator.has_value() == false` when running `txindex_tests/txindex_initial_sync`.
Or do you mean just that it's more aesthetic that way?
💬 shocknet-justin commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3156258686)
> But the fact is that this feature is mostly only popular with spammers.
Important context is the prevalence of scaling scammers, fake L2's etc that claim to be "scaling ownership" by way of centralized transaction pooling to lower the cost-per-transaction.
Word games around "unlilateral exit" are the basis of these scams, these fake L2's target users who literally cannot afford a unilateral exit and so end up in these centralized schemes in the first place. By lowering the actual cost of
...
(https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3156258686)
> But the fact is that this feature is mostly only popular with spammers.
Important context is the prevalence of scaling scammers, fake L2's etc that claim to be "scaling ownership" by way of centralized transaction pooling to lower the cost-per-transaction.
Word games around "unlilateral exit" are the basis of these scams, these fake L2's target users who literally cannot afford a unilateral exit and so end up in these centralized schemes in the first place. By lowering the actual cost of
...
💬 theuni commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-3156283189)
> 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.
Heh, there are so many levels of caching here.. my question still stands though...
> Worth noting (as i had to convince myself during review) that it also does not affect affect consensus validation by means of the signat
...
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-3156283189)
> 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.
Heh, there are so many levels of caching here.. my question still stands though...
> Worth noting (as i had to convince myself during review) that it also does not affect affect consensus validation by means of the signat
...
💬 furszy commented on pull request "index: remove unnecessary locater cleaning in BaseIndex::Init()":
(https://github.com/bitcoin/bitcoin/pull/32882#discussion_r2255125446)
I think the fact that you had to run the `txindex_tests/txindex_initial_sync` test to assert the return value is kind of self-explanatory.
Using the standard `std::nullopt` is better in terms of style and clarity. It’s explicit and self-documenting.
In contrast, `return {};` is more ambiguous. One need to check the function’s signature. Is it returning an empty `CBlockLocator`? a null optional?
(https://github.com/bitcoin/bitcoin/pull/32882#discussion_r2255125446)
I think the fact that you had to run the `txindex_tests/txindex_initial_sync` test to assert the return value is kind of self-explanatory.
Using the standard `std::nullopt` is better in terms of style and clarity. It’s explicit and self-documenting.
In contrast, `return {};` is more ambiguous. One need to check the function’s signature. Is it returning an empty `CBlockLocator`? a null optional?