👍 Sjors approved a pull request: "wallet: Disable creating and loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/31250#pullrequestreview-2667368942)
ACK ba2042c31f1630678a1f08bb68e01c1d98cc9abc
Though there's a few places where we could salvage test coverage (in a followup).
I ran the test suite on all commits.
`wallet_upgradewallet.py` is dropped. The test itself was only run for legacy wallets. However the `upgradewallet` RPC isn't legacy-only, even though it doesn't (currently) do anything useful for descriptor wallets. Maybe we should drop it here or in #28710? Or otherwise at least have some trivial coverage.
Should `test/fu
...
(https://github.com/bitcoin/bitcoin/pull/31250#pullrequestreview-2667368942)
ACK ba2042c31f1630678a1f08bb68e01c1d98cc9abc
Though there's a few places where we could salvage test coverage (in a followup).
I ran the test suite on all commits.
`wallet_upgradewallet.py` is dropped. The test itself was only run for legacy wallets. However the `upgradewallet` RPC isn't legacy-only, even though it doesn't (currently) do anything useful for descriptor wallets. Maybe we should drop it here or in #28710? Or otherwise at least have some trivial coverage.
Should `test/fu
...
💬 Sjors commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1985126112)
In 8afd005cbf9209e90a5cc1bcdb2c0ff400c2de33 "test: rpcs disabled for descriptor wallets were removed": I'm confused by the commit message. Should this commit go later? Or maybe reword it as "will go away in the next commits".
At this commit they still exist and there's other tests that refer to them, e.g. `rpcnestedtests.cpp`
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1985126112)
In 8afd005cbf9209e90a5cc1bcdb2c0ff400c2de33 "test: rpcs disabled for descriptor wallets were removed": I'm confused by the commit message. Should this commit go later? Or maybe reword it as "will go away in the next commits".
At this commit they still exist and there's other tests that refer to them, e.g. `rpcnestedtests.cpp`
💬 Sjors commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1985142758)
In 801d2b1c3f25c6922df04cce4b2baa2047173eab "test: Remove legacy wallet unit tests": for descriptor wallets, do we now rely entirely on functional tests for import and rescan coverage?
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1985142758)
In 801d2b1c3f25c6922df04cce4b2baa2047173eab "test: Remove legacy wallet unit tests": for descriptor wallets, do we now rely entirely on functional tests for import and rescan coverage?
💬 Sjors commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1985139917)
In 801d2b1c3f25c6922df04cce4b2baa2047173eab "test: Remove legacy wallet unit tests": we could include a bdb file, but there's probably enough coverage of `BerkeleyRO` through the migration tests - which use previous releases to generate them on the fly.
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1985139917)
In 801d2b1c3f25c6922df04cce4b2baa2047173eab "test: Remove legacy wallet unit tests": we could include a bdb file, but there's probably enough coverage of `BerkeleyRO` through the migration tests - which use previous releases to generate them on the fly.
💬 Sjors commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1985147350)
In 801d2b1c3f25c6922df04cce4b2baa2047173eab "test: Remove legacy wallet unit tests": could this be converted to using a descriptor wallet? Or is there really no interesting coverage here?
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1985147350)
In 801d2b1c3f25c6922df04cce4b2baa2047173eab "test: Remove legacy wallet unit tests": could this be converted to using a descriptor wallet? Or is there really no interesting coverage here?
💬 Sjors commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1985134129)
Testing the PSBT workflow would be still be useful though.
In 801d2b1c3f25c6922df04cce4b2baa2047173eab "test: Remove legacy wallet unit tests"
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1985134129)
Testing the PSBT workflow would be still be useful though.
In 801d2b1c3f25c6922df04cce4b2baa2047173eab "test: Remove legacy wallet unit tests"
💬 hodlinator commented on pull request "doc: add note to Windows build about stripping bins":
(https://github.com/bitcoin/bitcoin/pull/32002#discussion_r1985281749)
Don't understand why you switched from `--prefix` to `CMAKE_INSTALL_PREFIX`, the previous order of build instructions before install instructions felt more ordered.
Please amend the commit message with an explanation of this.
(https://github.com/bitcoin/bitcoin/pull/32002#discussion_r1985281749)
Don't understand why you switched from `--prefix` to `CMAKE_INSTALL_PREFIX`, the previous order of build instructions before install instructions felt more ordered.
Please amend the commit message with an explanation of this.
💬 fanquake commented on pull request "doc: add note to Windows build about stripping bins":
(https://github.com/bitcoin/bitcoin/pull/32002#discussion_r1985295727)
Shouldn't we be using CMake options now that we are using CMake?
(https://github.com/bitcoin/bitcoin/pull/32002#discussion_r1985295727)
Shouldn't we be using CMake options now that we are using CMake?
🤔 fjahr reviewed a pull request: "qa: Fix TxIndex race conditions"
(https://github.com/bitcoin/bitcoin/pull/32010#pullrequestreview-2667572104)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32010#pullrequestreview-2667572104)
Concept ACK
💬 fjahr commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1985242659)
Did you check if this has an performance impact and maybe that's why they were added here?
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1985242659)
Did you check if this has an performance impact and maybe that's why they were added here?
💬 fjahr commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1985310423)
Can this be a bit more clear what this means? I guess "Removing these files does not result in a startup error"? Would be great if you could spell that out because I needed a moment to understand. You could also put the commented out line below and this one together and above the variable declaration. I am not sure if having them in line and split up helps with understanding. Same below.
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1985310423)
Can this be a bit more clear what this means? I guess "Removing these files does not result in a startup error"? Would be great if you could spell that out because I needed a moment to understand. You could also put the commented out line below and this one together and above the variable declaration. I am not sure if having them in line and split up helps with understanding. Same below.
💬 fjahr commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1985243645)
Same as above, may check for performance impact on this test.
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1985243645)
Same as above, may check for performance impact on this test.
💬 tnndbtc commented on issue "Performance decrease after tapscript miniscript":
(https://github.com/bitcoin/bitcoin/issues/29098#issuecomment-2706831776)
> That sounds very plausible, [@darosior](https://github.com/darosior).
>
> If so, I think there is an approach that doesn't lose optimality: keep track of the optimal satisfaction size, and once reached, don't bother evaluation further keys?
@sipa I agree with you and @darosior that this suggestion would gain performance on many cases. However, in proposed unit test in https://github.com/bitcoin/bitcoin/pull/28212
`self.do_test_k_of_n(999, 999)`
This test would still timeout in most of mac
...
(https://github.com/bitcoin/bitcoin/issues/29098#issuecomment-2706831776)
> That sounds very plausible, [@darosior](https://github.com/darosior).
>
> If so, I think there is an approach that doesn't lose optimality: keep track of the optimal satisfaction size, and once reached, don't bother evaluation further keys?
@sipa I agree with you and @darosior that this suggestion would gain performance on many cases. However, in proposed unit test in https://github.com/bitcoin/bitcoin/pull/28212
`self.do_test_k_of_n(999, 999)`
This test would still timeout in most of mac
...
💬 jonatack commented on pull request "Add mainnet assumeutxo param at height 880,000":
(https://github.com/bitcoin/bitcoin/pull/31969#issuecomment-2706832777)
> Concept ACK, shasum of the torrent file matches up and will re-test soon after further reindexing. For now am seeing this:
>
> ```
> error code: -32603
> error message:
> Unable to load UTXO snapshot: The base block header (000000000000000000010b17283c3c400507969a9c2afd1dcf2082ec5cca2880) must appear in the headers chain. Make sure all headers are syncing, and call loadtxoutset again. (utxo-880000.dat)
> ```
Successfully activated the snapshot, once the node had the headers.
``
2
...
(https://github.com/bitcoin/bitcoin/pull/31969#issuecomment-2706832777)
> Concept ACK, shasum of the torrent file matches up and will re-test soon after further reindexing. For now am seeing this:
>
> ```
> error code: -32603
> error message:
> Unable to load UTXO snapshot: The base block header (000000000000000000010b17283c3c400507969a9c2afd1dcf2082ec5cca2880) must appear in the headers chain. Make sure all headers are syncing, and call loadtxoutset again. (utxo-880000.dat)
> ```
Successfully activated the snapshot, once the node had the headers.
``
2
...
💬 tnndbtc commented on pull request "miniscript: fixes #29098 by only use first k valid signatures":
(https://github.com/bitcoin/bitcoin/pull/31719#issuecomment-2706836700)
@sipa Yes, I considered your proposal and it would still make the proposed test to timeout.
`self.do_test_k_of_n(999, 999)`
I just responded your comment in the original thread [here](https://github.com/bitcoin/bitcoin/issues/29098#issuecomment-2706831776)
(https://github.com/bitcoin/bitcoin/pull/31719#issuecomment-2706836700)
@sipa Yes, I considered your proposal and it would still make the proposed test to timeout.
`self.do_test_k_of_n(999, 999)`
I just responded your comment in the original thread [here](https://github.com/bitcoin/bitcoin/issues/29098#issuecomment-2706831776)
💬 sipa commented on issue "Performance decrease after tapscript miniscript":
(https://github.com/bitcoin/bitcoin/issues/29098#issuecomment-2706846092)
@tnndbtc I don't understand why there is a difference at all in performance between the two approaches for 999-of-999; it needs 999 signing attempts regardless.
(https://github.com/bitcoin/bitcoin/issues/29098#issuecomment-2706846092)
@tnndbtc I don't understand why there is a difference at all in performance between the two approaches for 999-of-999; it needs 999 signing attempts regardless.
💬 achow101 commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1985347658)
This pattern is specifically not generalized because we want to restrict to at least post-segwit versions, hence beginning at 0.14.0.
Furthermore, while the tag is similar to the user agent, they do not match exactly. Specifically, since the version style change in 22.0, the tag is typically MAJOR.MINOR, but the user agent is always MAJOR.MINOR.PATCH. So 22.0 is 22.0.0 in the user agent. Release candidates additionally produce user agents without the rc suffix. Hence your test script is not r
...
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1985347658)
This pattern is specifically not generalized because we want to restrict to at least post-segwit versions, hence beginning at 0.14.0.
Furthermore, while the tag is similar to the user agent, they do not match exactly. Specifically, since the version style change in 22.0, the tag is typically MAJOR.MINOR, but the user agent is always MAJOR.MINOR.PATCH. So 22.0 is 22.0.0 in the user agent. Release candidates additionally produce user agents without the rc suffix. Hence your test script is not r
...
📝 hebasto opened a pull request: "cmake: Check for `makensis` tool before using it"
(https://github.com/bitcoin/bitcoin/pull/32019)
Fixes https://github.com/bitcoin/bitcoin/issues/32018.
(https://github.com/bitcoin/bitcoin/pull/32019)
Fixes https://github.com/bitcoin/bitcoin/issues/32018.
💬 achow101 commented on issue "Check if the wallet already contains the descriptor GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/issues/32013#issuecomment-2706880541)
If you believe you have a useful change, please just open a PR. There's no need to ask permission to do so, and doing code review in an issue is painful.
(https://github.com/bitcoin/bitcoin/issues/32013#issuecomment-2706880541)
If you believe you have a useful change, please just open a PR. There's no need to ask permission to do so, and doing code review in an issue is painful.
💬 hebasto commented on pull request "doc: add note to Windows build about stripping bins":
(https://github.com/bitcoin/bitcoin/pull/32002#discussion_r1985358929)
As I mentioned [above](https://github.com/bitcoin/bitcoin/pull/32002#pullrequestreview-2662658024), the `cmake --install` command does support the `--prefix` option.
(https://github.com/bitcoin/bitcoin/pull/32002#discussion_r1985358929)
As I mentioned [above](https://github.com/bitcoin/bitcoin/pull/32002#pullrequestreview-2662658024), the `cmake --install` command does support the `--prefix` option.