💬 hebasto commented on issue "Having qt(@6) breaks build for qt@5 on macOS 15.0 and 13.7":
(https://github.com/bitcoin/bitcoin/issues/31009#issuecomment-2706655187)
> I assume this is related to [`AUTOMOC`](https://cmake.org/cmake/help/latest/manual/cmake-qt.7.html#automoc), and CMake getting confused and picking the wrong tooling to use.
From my perspective, Qt messes with included directories when compiling `qt/bitcoinqt_autogen/mocs_compilation.cpp`.
(https://github.com/bitcoin/bitcoin/issues/31009#issuecomment-2706655187)
> I assume this is related to [`AUTOMOC`](https://cmake.org/cmake/help/latest/manual/cmake-qt.7.html#automoc), and CMake getting confused and picking the wrong tooling to use.
From my perspective, Qt messes with included directories when compiling `qt/bitcoinqt_autogen/mocs_compilation.cpp`.
👍 hebasto approved a pull request: "doc: warn against having qt6 installed on macOS"
(https://github.com/bitcoin/bitcoin/pull/32017#pullrequestreview-2667531271)
ACK d79dab0fa999002a0c5b70c1688240e2a5032ce1.
It seems OK as a temporary measure until the [regression](https://github.com/bitcoin/bitcoin/issues/31009) is fixed or we switch to Qt 6.
(https://github.com/bitcoin/bitcoin/pull/32017#pullrequestreview-2667531271)
ACK d79dab0fa999002a0c5b70c1688240e2a5032ce1.
It seems OK as a temporary measure until the [regression](https://github.com/bitcoin/bitcoin/issues/31009) is fixed or we switch to Qt 6.
💬 hebasto commented on issue "Having qt(@6) breaks build for qt@5 on macOS 15.0 and 13.7":
(https://github.com/bitcoin/bitcoin/issues/31009#issuecomment-2706679598)
> I opened [#32017](https://github.com/bitcoin/bitcoin/pull/32017) to document this behavior. I suggest we add that PR to the milestone and remove this issue from it. The actual problem can be solved later.
In that case, this issue shouldn't be a blocker for the v29.0 release.
(https://github.com/bitcoin/bitcoin/issues/31009#issuecomment-2706679598)
> I opened [#32017](https://github.com/bitcoin/bitcoin/pull/32017) to document this behavior. I suggest we add that PR to the milestone and remove this issue from it. The actual problem can be solved later.
In that case, this issue shouldn't be a blocker for the v29.0 release.
💬 tnndbtc commented on pull request "miniscript: fixes #29098 by only use first k valid signatures":
(https://github.com/bitcoin/bitcoin/pull/31719#discussion_r1985228860)
@hodlinator Ah, sorry, overlooked the comment part. Thank you for the example and I have made the change accordingly. Please have a look.
(https://github.com/bitcoin/bitcoin/pull/31719#discussion_r1985228860)
@hodlinator Ah, sorry, overlooked the comment part. Thank you for the example and I have made the change accordingly. Please have a look.
💬 sipa commented on pull request "miniscript: fixes #29098 by only use first k valid signatures":
(https://github.com/bitcoin/bitcoin/pull/31719#issuecomment-2706695834)
@tnndbtc Have you considered my [suggestion](https://github.com/bitcoin/bitcoin/issues/29098#issuecomment-1860568169) to instead of just picking the first `k` signatures, keep using the existing algorithm, but stop trying things as soon as the optimal size is reached? I suspect that that will in practice have the same performance as first-k, but won't lose the optimality property the current algorithm has.
(https://github.com/bitcoin/bitcoin/pull/31719#issuecomment-2706695834)
@tnndbtc Have you considered my [suggestion](https://github.com/bitcoin/bitcoin/issues/29098#issuecomment-1860568169) to instead of just picking the first `k` signatures, keep using the existing algorithm, but stop trying things as soon as the optimal size is reached? I suspect that that will in practice have the same performance as first-k, but won't lose the optimality property the current algorithm has.
👍 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)