💬 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?
💬 w0xlt commented on pull request "wallet: prevent accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2255129545)
Wouldn't it be better to have this logic inside the miniscript files?
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2255129545)
Wouldn't it be better to have this logic inside the miniscript files?
💬 Sjors commented on issue "wallet: render BIP388 wallet policies":
(https://github.com/bitcoin/bitcoin/issues/32659#issuecomment-3156303824)
#33008 now has some very rudimentary code that loops through our descriptors and constructs a BIP388 policy if possible.
(https://github.com/bitcoin/bitcoin/issues/32659#issuecomment-3156303824)
#33008 now has some very rudimentary code that loops through our descriptors and constructs a BIP388 policy if possible.
💬 Sjors commented on pull request "wallet: prevent accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2255140424)
Maybe, but I'm worried it would add a ton of code churn in order to either:
1. bubble these values up; or
2. have the descriptor and miniscript parser take an `unsafe` argument to change its behavior (or some more general `parse_options` argument)
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2255140424)
Maybe, but I'm worried it would add a ton of code churn in order to either:
1. bubble these values up; or
2. have the descriptor and miniscript parser take an `unsafe` argument to change its behavior (or some more general `parse_options` argument)
💬 darosior commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3156334619)
I changed the call site for `SpendsNonAnchorWitnessProg` to be where we detect witness stripping today. Besides being a smaller diff this minimizes the potential for behaviour changes. Note this PR still slightly changes behaviour (see last commit's message and https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2254632919).
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3156334619)
I changed the call site for `SpendsNonAnchorWitnessProg` to be where we detect witness stripping today. Besides being a smaller diff this minimizes the potential for behaviour changes. Note this PR still slightly changes behaviour (see last commit's message and https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2254632919).
💬 darosior commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2255158075)
I updated the `return false` to `continue` because it makes more sense and is actually not a change in behaviour (see edit in https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2254398401).
I did not add the check to `return false` if redeem script mismatches, or `EvalScript` doesn't result in a clean stack. This is because those would be confusing to have in a function checking if inputs are Segwit while only partially reducing the change in behaviour (see https://github.com/bitcoin/b
...
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2255158075)
I updated the `return false` to `continue` because it makes more sense and is actually not a change in behaviour (see edit in https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2254398401).
I did not add the check to `return false` if redeem script mismatches, or `EvalScript` doesn't result in a clean stack. This is because those would be confusing to have in a function checking if inputs are Segwit while only partially reducing the change in behaviour (see https://github.com/bitcoin/b
...
💬 ajtowns commented on pull request "validation: detect witness stripping without re-running Script checks":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2255161296)
The logic when we see "WITNESS_STRIPPED" is "this peer sent us garbage, that we have to ignore" -- to me, it's better to spend as little CPU as possible on garbage, so that means we should detect WITNESS_STRIPPED as soon and as cheaply as possible, then just ignore the tx. If we could have drawn some useful conclusions from the garbage (eg "INPUTS_NOT_STANDARD" or any other policy/consensus failure that different witness data couldn't fix), that's fine, but probably not at the cost of doing much
...
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2255161296)
The logic when we see "WITNESS_STRIPPED" is "this peer sent us garbage, that we have to ignore" -- to me, it's better to spend as little CPU as possible on garbage, so that means we should detect WITNESS_STRIPPED as soon and as cheaply as possible, then just ignore the tx. If we could have drawn some useful conclusions from the garbage (eg "INPUTS_NOT_STANDARD" or any other policy/consensus failure that different witness data couldn't fix), that's fine, but probably not at the cost of doing much
...
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255166434)
I've fixed how these variables are named.
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255166434)
I've fixed how these variables are named.
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255167142)
I've removed it.
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255167142)
I've removed it.
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255167614)
Done
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255167614)
Done