💬 achow101 commented on pull request "ci: Use previous releases in tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2064565125)
Why restrict to 22.0 and above?
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2064565125)
Why restrict to 22.0 and above?
💬 achow101 commented on pull request "ci: Use previous releases in tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2064544864)
Please put these with their respective versions above.
If we're going to do Windows testing as well, then we should be downloading all of the versions above, not just this small subset. This subset doesn't even make sense, there's no single test that uses just these versions. `wallet_migration.py` uses only 28.0, the rest require the older versions too.
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2064544864)
Please put these with their respective versions above.
If we're going to do Windows testing as well, then we should be downloading all of the versions above, not just this small subset. This subset doesn't even make sense, there's no single test that uses just these versions. `wallet_migration.py` uses only 28.0, the rest require the older versions too.
💬 achow101 commented on pull request "ci: Use previous releases in tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2064578902)
What is the purpose of this host triple? We don't make Windows binaries with any other host triple.
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2064578902)
What is the purpose of this host triple? We don't make Windows binaries with any other host triple.
💬 achow101 commented on pull request "ci: Use previous releases in tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2064585207)
What is the purpose of this change? It seems entirely unnecessary and unrelated to the purpose of this PR.
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2064585207)
What is the purpose of this change? It seems entirely unnecessary and unrelated to the purpose of this PR.
💬 1ma commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2836538928)
> Concept ACK.
>
> It's time to come to terms with the fact that Bitcoin is desirable for some people to use as a data anchor and they will find a way to use it as such, thus we should be asking what the preferred means of data anchoring is and how to incentivize it over less preferable options.
@jlopp I think it would be appropriate for you to disclose that you're one of the VCs backing Citrea, a company that is currently abusing unspendable Taproot outputs to embed arbitrary data onchain
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2836538928)
> Concept ACK.
>
> It's time to come to terms with the fact that Bitcoin is desirable for some people to use as a data anchor and they will find a way to use it as such, thus we should be asking what the preferred means of data anchoring is and how to incentivize it over less preferable options.
@jlopp I think it would be appropriate for you to disclose that you're one of the VCs backing Citrea, a company that is currently abusing unspendable Taproot outputs to embed arbitrary data onchain
...
💬 BullishNode commented on issue "Allow sending untrusted utxos in the sendtoaddress api":
(https://github.com/bitcoin/bitcoin/issues/32034#issuecomment-2836545266)
@achow101 I know you once nacked this conecpt so tagging you here for discussion maybe I can change your mind.
(https://github.com/bitcoin/bitcoin/issues/32034#issuecomment-2836545266)
@achow101 I know you once nacked this conecpt so tagging you here for discussion maybe I can change your mind.
💬 achow101 commented on pull request "ci: Use previous releases in tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2064603713)
Why does `wallet_migration.py` need to be run separately, and none of the other tests that require previous releases? These can all be run at the same time from the `test_runner.py` invocation if the `PREVIOUS_RELEASES_DIR` environment variable is set.
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2064603713)
Why does `wallet_migration.py` need to be run separately, and none of the other tests that require previous releases? These can all be run at the same time from the `test_runner.py` invocation if the `PREVIOUS_RELEASES_DIR` environment variable is set.
💬 achow101 commented on pull request "ci: Use previous releases in tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2064604835)
Unnecessary change?
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2064604835)
Unnecessary change?
💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2064644621)
> That's why I believe `sig.empty()` is added separately.
Yes, that's why.
> any reason why the verification flags don't also include `SCRIPT_VERIFY_LOW_S`?
Low S is a standardness rule, not a consensus rule. PSBTs may be produced by software other than Bitcoin Core, and these may sign with high S, for whatever reason. Since these are consensus valid signatures, we need to also accept them when decoding a PSBT.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2064644621)
> That's why I believe `sig.empty()` is added separately.
Yes, that's why.
> any reason why the verification flags don't also include `SCRIPT_VERIFY_LOW_S`?
Low S is a standardness rule, not a consensus rule. PSBTs may be produced by software other than Bitcoin Core, and these may sign with high S, for whatever reason. Since these are consensus valid signatures, we need to also accept them when decoding a PSBT.
💬 BitcoinMechanic commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2836629220)
The above comment (that has been marked "abuse") alerting us to Lopp's conflict of interest is entirely appropriate to point out. I am happy to disclose my affiliation with OCEAN for anyone that feels it is relevant to the input I have given here.
The moderation on this thread - and the suggestion that only Core contributors may participate in this discussion - is completely unacceptable.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2836629220)
The above comment (that has been marked "abuse") alerting us to Lopp's conflict of interest is entirely appropriate to point out. I am happy to disclose my affiliation with OCEAN for anyone that feels it is relevant to the input I have given here.
The moderation on this thread - and the suggestion that only Core contributors may participate in this discussion - is completely unacceptable.
💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2064733664)
Done
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2064733664)
Done
💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2064735230)
I don't think a separate function is necessary. This code is unlikely to be reused.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2064735230)
I don't think a separate function is necessary. This code is unlikely to be reused.
💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2064737641)
I've added size checks. I did not gate behind a `IsPayToTaproot` as the fields should only be set if the output script is taproot anyways.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2064737641)
I've added size checks. I did not gate behind a `IsPayToTaproot` as the fields should only be set if the output script is taproot anyways.
💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2064738574)
Meh, maybe someone can do that in a followup.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2064738574)
Meh, maybe someone can do that in a followup.
💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2064740358)
I don't think it's really necessary as these log lines are in the PSBT test, and the only way to sign with PSBTs is with `*processpsbt`.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2064740358)
I don't think it's really necessary as these log lines are in the PSBT test, and the only way to sign with PSBTs is with `*processpsbt`.
💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2064741305)
Added a sentence to the commit message.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2064741305)
Added a sentence to the commit message.
🚀 glozow merged a pull request: "test: Test that migration automatically repairs corrupted metadata with doubled derivation path"
(https://github.com/bitcoin/bitcoin/pull/29124)
(https://github.com/bitcoin/bitcoin/pull/29124)
🤔 achow101 reviewed a pull request: "descriptors: Reject + sign while parsing unsigned"
(https://github.com/bitcoin/bitcoin/pull/32365#pullrequestreview-2800976553)
ACK fa655da159876861251d1149a5c31a830bd35596
(https://github.com/bitcoin/bitcoin/pull/32365#pullrequestreview-2800976553)
ACK fa655da159876861251d1149a5c31a830bd35596
💬 achow101 commented on pull request "descriptors: Reject + sign while parsing unsigned":
(https://github.com/bitcoin/bitcoin/pull/32365#discussion_r2064772334)
In fa6f77ed3c152e0ff695c36b7a4ebb2c2efe916f "descriptors: Reject + sign in ParseKeyPathNum"
Perhaps also a test for `-`?
(https://github.com/bitcoin/bitcoin/pull/32365#discussion_r2064772334)
In fa6f77ed3c152e0ff695c36b7a4ebb2c2efe916f "descriptors: Reject + sign in ParseKeyPathNum"
Perhaps also a test for `-`?
💬 achow101 commented on pull request "test: Add missing check for empty stderr in util tester":
(https://github.com/bitcoin/bitcoin/pull/32327#issuecomment-2836763846)
ACK fadf12a56c294696052c4cb6ee5284030ada7498
(https://github.com/bitcoin/bitcoin/pull/32327#issuecomment-2836763846)
ACK fadf12a56c294696052c4cb6ee5284030ada7498