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

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

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

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

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

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

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

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

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

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

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

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

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255167614)
Done

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255168211)
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_r2255169474)
Done

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255169904)
Done

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255172700)
Done in both places where this was applicable.

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255174104)
Done

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255177206)
I've updated this test so it actually tests spending change now.

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255181268)
The comment was in multiple places, I've removed it now.