💬 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
💬 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
(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
(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
(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.
(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
(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.
(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.
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255181268)
The comment was in multiple places, I've removed it now.
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255181611)
Done
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255181611)
Done
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255182702)
Done here and throughout the file
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255182702)
Done here and throughout the file
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255183692)
Done
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255183692)
Done
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255184775)
I've removed it.
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255184775)
I've removed it.