🤔 furszy reviewed a pull request: "init: handle empty settings file gracefully"
(https://github.com/bitcoin/bitcoin/pull/29144#pullrequestreview-1811179954)
Cool notification. The CI should be happy now.
(https://github.com/bitcoin/bitcoin/pull/29144#pullrequestreview-1811179954)
Cool notification. The CI should be happy now.
🤔 glozow reviewed a pull request: "[26.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/29011#pullrequestreview-1811251821)
ACK 7b79e54474b86864c81148c74824bfe4b732412d, matches mine
(https://github.com/bitcoin/bitcoin/pull/29011#pullrequestreview-1811251821)
ACK 7b79e54474b86864c81148c74824bfe4b732412d, matches mine
💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1446161874)
We are testing that they are marked as in-mempool wallet transactions.
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1446161874)
We are testing that they are marked as in-mempool wallet transactions.
💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#issuecomment-1883154232)
> Is there a reason both legacy and descriptor wallet tests aren't in the new wallet_rescan_unconfirmed test, though? That seems like a more logical grouping with less duplication?
They use different RPCs and different codepaths, so I don't really see why they need to be in the same file or tied together. I think it's more readable for them to be separate (having most the test as `if is_sqlite_compiled() elif is_bdb_compiled()` is pretty annoying). I think it's much easier to delete legacy wi
...
(https://github.com/bitcoin/bitcoin/pull/29179#issuecomment-1883154232)
> Is there a reason both legacy and descriptor wallet tests aren't in the new wallet_rescan_unconfirmed test, though? That seems like a more logical grouping with less duplication?
They use different RPCs and different codepaths, so I don't really see why they need to be in the same file or tied together. I think it's more readable for them to be separate (having most the test as `if is_sqlite_compiled() elif is_bdb_compiled()` is pretty annoying). I think it's much easier to delete legacy wi
...
✅ reardencode closed a pull request: "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)"
(https://github.com/bitcoin/bitcoin/pull/29198)
(https://github.com/bitcoin/bitcoin/pull/29198)
💬 reardencode commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1883158798)
Hi Sjors, thanks for your thoughts!
> It seems too early in the process to open a pull request like this. It's also possible to open a pull request against your own repo-fork of Bitcoin Core, or Inquisition as suggested above.
> For comparison, the first Taproot pull request to this repo #17977 was made in early 2020, which was _after_ about year of in person workshops studying the proposal. The BIP's themselves were first proposed in 2018. The first work-in-progress taproot branch, not a
...
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1883158798)
Hi Sjors, thanks for your thoughts!
> It seems too early in the process to open a pull request like this. It's also possible to open a pull request against your own repo-fork of Bitcoin Core, or Inquisition as suggested above.
> For comparison, the first Taproot pull request to this repo #17977 was made in early 2020, which was _after_ about year of in person workshops studying the proposal. The BIP's themselves were first proposed in 2018. The first work-in-progress taproot branch, not a
...
📝 reardencode reopened a pull request: "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)"
(https://github.com/bitcoin/bitcoin/pull/29198)
This pull request contains a the same implementation of OP_CHECKTEMPLATEVERIFY (BIP119) as @jamesob's [Covenant Tools](https://github.com/bitcoin/bitcoin/pull/28550), an implementation of [OP_CHECKSIGFROMSTACK(VERIFY)](https://github.com/bitcoin/bips/pull/1535) and of [OP_INTERNALKEY](https://github.com/bitcoin/bips/pull/1534).
There are no testnet or mainnet activation parameters proposed in this pull request. I am deeply uninterested in the details of activation semantics.
This combinati
...
(https://github.com/bitcoin/bitcoin/pull/29198)
This pull request contains a the same implementation of OP_CHECKTEMPLATEVERIFY (BIP119) as @jamesob's [Covenant Tools](https://github.com/bitcoin/bitcoin/pull/28550), an implementation of [OP_CHECKSIGFROMSTACK(VERIFY)](https://github.com/bitcoin/bips/pull/1535) and of [OP_INTERNALKEY](https://github.com/bitcoin/bips/pull/1534).
There are no testnet or mainnet activation parameters proposed in this pull request. I am deeply uninterested in the details of activation semantics.
This combinati
...
🤔 furszy reviewed a pull request: "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/28670#pullrequestreview-1811280860)
Would be good to add test coverage for this error.
Also, the "file size being too small" error message isn't really descriptive and it does not provide further guidance to solve the problem. The file could be corrupt or could had been crafted erroneously.
I would go simpler for now and change "file size being too small" for "Unable to load UTXO snapshot, the file %s cannot be parsed."
I see your alternative solution better. More comprehensive.
(https://github.com/bitcoin/bitcoin/pull/28670#pullrequestreview-1811280860)
Would be good to add test coverage for this error.
Also, the "file size being too small" error message isn't really descriptive and it does not provide further guidance to solve the problem. The file could be corrupt or could had been crafted erroneously.
I would go simpler for now and change "file size being too small" for "Unable to load UTXO snapshot, the file %s cannot be parsed."
I see your alternative solution better. More comprehensive.
💬 reardencode commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1883175973)
Hi Sjors, thanks for your thoughts!
> It seems too early in the process to open a pull request like this. It's also possible to open a pull request against your own repo-fork of Bitcoin Core, or Inquisition as suggested above.
I'm not aware of a well defined process for making changes to bitcoin consensus (nor do I think there should be one), so I'm not sure how we can be too early in the process.
CTV had (a year of?) workshops and review with Jeremy Rubin about 2 years ago. OP_INTERNA
...
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1883175973)
Hi Sjors, thanks for your thoughts!
> It seems too early in the process to open a pull request like this. It's also possible to open a pull request against your own repo-fork of Bitcoin Core, or Inquisition as suggested above.
I'm not aware of a well defined process for making changes to bitcoin consensus (nor do I think there should be one), so I'm not sure how we can be too early in the process.
CTV had (a year of?) workshops and review with Jeremy Rubin about 2 years ago. OP_INTERNA
...
💬 Ayush170-Future commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1446205449)
I've thought about it, and I think a global Singleton might not be the smartest way to conserve resources. The `wallet` object definitely mutates during execution. I think I've to remove the Singleton pattern, but doing so will make the file very slow, as creating the `Descriptor Wallet` is really time-consuming.
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1446205449)
I've thought about it, and I think a global Singleton might not be the smartest way to conserve resources. The `wallet` object definitely mutates during execution. I think I've to remove the Singleton pattern, but doing so will make the file very slow, as creating the `Descriptor Wallet` is really time-consuming.
💬 michaelfolkson commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1883215894)
From the [contributing guidelines](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review)
> **After** conceptual agreement on the change, code review can be provided.
I can only speak for myself obviously but Concept NACK from me for this repo at the current time. This pull request should be opened to bitcoin-inquisition or a fork of Core with its own custom signet consensus rules.
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1883215894)
From the [contributing guidelines](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review)
> **After** conceptual agreement on the change, code review can be provided.
I can only speak for myself obviously but Concept NACK from me for this repo at the current time. This pull request should be opened to bitcoin-inquisition or a fork of Core with its own custom signet consensus rules.
💬 darosior commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1446215062)
Yes, thankfully. :) Attempting to overwrite an unspent coin when `possible_overwrite` is `false` is always considered a fatal error elsewhere AFAICT.
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1446215062)
Yes, thankfully. :) Attempting to overwrite an unspent coin when `possible_overwrite` is `false` is always considered a fatal error elsewhere AFAICT.
💬 darosior commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1446216031)
It's been a while since i wrote this code and to be honest i don't remember what i was thinking. (Note-to-self: comments are good actually.)
As you point out it does change the behaviour of the existing target. Since i don't have a rationale i've updated it to simply keep the existing behaviour.
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1446216031)
It's been a while since i wrote this code and to be honest i don't remember what i was thinking. (Note-to-self: comments are good actually.)
As you point out it does change the behaviour of the existing target. Since i don't have a rationale i've updated it to simply keep the existing behaviour.
💬 jonatack commented on pull request "[26.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/29011#issuecomment-1883240726)
Suggest backporting #29200 that resolves https://github.com/bitcoin/bitcoin/issues/29197.
(https://github.com/bitcoin/bitcoin/pull/29011#issuecomment-1883240726)
Suggest backporting #29200 that resolves https://github.com/bitcoin/bitcoin/issues/29197.
💬 Ayush170-Future commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#issuecomment-1883240915)
> Spend.cpp coverage is not too bad already: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/wallet/spend.cpp.gcov.html
>
> It may be good to clarify the new coverage a bit.
I didn't understand. Are you suggesting that I reconsider what I want to increase coverage of in this file?
Also, I have a question about the blue lines in the coverage reports. Do they represent the line coverage of the code during fuzzing? For instance, if these functions are encountered in the process of runn
...
(https://github.com/bitcoin/bitcoin/pull/28236#issuecomment-1883240915)
> Spend.cpp coverage is not too bad already: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/wallet/spend.cpp.gcov.html
>
> It may be good to clarify the new coverage a bit.
I didn't understand. Are you suggesting that I reconsider what I want to increase coverage of in this file?
Also, I have a question about the blue lines in the coverage reports. Do they represent the line coverage of the code during fuzzing? For instance, if these functions are encountered in the process of runn
...
💬 moonsettler commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1883258307)
> From the [contributing guidelines](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review)
>
> > **After** conceptual agreement on the change, code review can be provided.
>
> I can only speak for myself obviously but Concept NACK from me for this repo at the current time. This pull request should be opened to bitcoin-inquisition or a fork of Core with its own custom signet consensus rules.
You are clearly commenting in bad faith. But in the off chance you want to
...
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1883258307)
> From the [contributing guidelines](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review)
>
> > **After** conceptual agreement on the change, code review can be provided.
>
> I can only speak for myself obviously but Concept NACK from me for this repo at the current time. This pull request should be opened to bitcoin-inquisition or a fork of Core with its own custom signet consensus rules.
You are clearly commenting in bad faith. But in the off chance you want to
...
💬 maflcko commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-1883272800)
@theuni See https://github.com/bitcoin/bitcoin/pull/29208
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-1883272800)
@theuni See https://github.com/bitcoin/bitcoin/pull/29208
💬 michaelfolkson commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1883279066)
@moonsettler: That PR is in draft, was opened to bitcoin-inquisition simultaneously and doesn't seem to have conceptual agreement at the current time either. Everyone opening their own pull request to this repo with their own favorite combination of opcodes and sighash flags wastes people's time. They will have to come to this pull request and tell you exactly what Sjors and I have already told you. But if you insist that is what they will have to do.
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1883279066)
@moonsettler: That PR is in draft, was opened to bitcoin-inquisition simultaneously and doesn't seem to have conceptual agreement at the current time either. Everyone opening their own pull request to this repo with their own favorite combination of opcodes and sighash flags wastes people's time. They will have to come to this pull request and tell you exactly what Sjors and I have already told you. But if you insist that is what they will have to do.
👍 hebasto approved a pull request: "build: Bump clang minimum supported version to 14"
(https://github.com/bitcoin/bitcoin/pull/29208#pullrequestreview-1811425126)
ACK fa223ba5eb764fe822229a58d4d44d3ea83d0793.
(https://github.com/bitcoin/bitcoin/pull/29208#pullrequestreview-1811425126)
ACK fa223ba5eb764fe822229a58d4d44d3ea83d0793.
💬 hebasto commented on pull request "build: Bump clang minimum supported version to 14":
(https://github.com/bitcoin/bitcoin/pull/29208#discussion_r1446254597)
nit: I understand that that was a clean commit revert. But these lines look really unneeded these days.
(https://github.com/bitcoin/bitcoin/pull/29208#discussion_r1446254597)
nit: I understand that that was a clean commit revert. But these lines look really unneeded these days.