💬 Sjors commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450784477)
From reading the test again (it's a been a while) I don't think we care about this loaded wallet `w` anymore.
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450784477)
From reading the test again (it's a been a while) I don't think we care about this loaded wallet `w` anymore.
💬 Sjors commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889752866)
Left an alternative suggestion here that (hopefully) doesn't require an exception for Windows: https://github.com/bitcoin/bitcoin/pull/28838/files#r1450778587
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889752866)
Left an alternative suggestion here that (hopefully) doesn't require an exception for Windows: https://github.com/bitcoin/bitcoin/pull/28838/files#r1450778587
💬 achow101 commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450785203)
I don't think the problem is that the original wallet is open. The error is the path for the restored wallet on node1, not the original on node0.
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450785203)
I don't think the problem is that the original wallet is open. The error is the path for the restored wallet on node1, not the original on node0.
💬 Sjors commented on pull request "rpc: add path to gethdkey":
(https://github.com/bitcoin/bitcoin/pull/22341#issuecomment-1889756175)
Given that #26728 was closed, I'll just modify this PR to build on top of #29130.
(bit busy with stratum v2 stuff, but it will happen)
(https://github.com/bitcoin/bitcoin/pull/22341#issuecomment-1889756175)
Given that #26728 was closed, I'll just modify this PR to build on top of #29130.
(bit busy with stratum v2 stuff, but it will happen)
💬 Sjors commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450789885)
I see (just saw the log in #29234).
Sounds like the node that made the backup holds on to it after writing? Maybe restart the node? Or copy the backup file?
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450789885)
I see (just saw the log in #29234).
Sounds like the node that made the backup holds on to it after writing? Maybe restart the node? Or copy the backup file?
💬 jamesob commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889762142)
> Left an alternative suggestion here that (hopefully) doesn't require an exception for Windows: https://github.com/bitcoin/bitcoin/pull/28838/files#r1450778587
Sadly (per the thread), not so simple. I think we should probably unbreak and then reassess but I'll leave that as a call for the maintainers.
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889762142)
> Left an alternative suggestion here that (hopefully) doesn't require an exception for Windows: https://github.com/bitcoin/bitcoin/pull/28838/files#r1450778587
Sadly (per the thread), not so simple. I think we should probably unbreak and then reassess but I'll leave that as a call for the maintainers.
💬 Sjors commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889763775)
Skipping Windows for now is fine by me. Best just leave #29234 open until it's more properly solved.
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889763775)
Skipping Windows for now is fine by me. Best just leave #29234 open until it's more properly solved.
💬 achow101 commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450792644)
> Sounds like the node that made the backup holds on to it after writing? Maybe restart the node? Or copy the backup file?
We don't remove the backup. The failure is in cleaning up the restored wallet after it fails to restore. The backup file is copied to a new path, and that is what `remove_all` is trying to remove.
I think it's more likely that the database isn't fully closed yet even though the `CWallet` should be destructed.
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450792644)
> Sounds like the node that made the backup holds on to it after writing? Maybe restart the node? Or copy the backup file?
We don't remove the backup. The failure is in cleaning up the restored wallet after it fails to restore. The backup file is copied to a new path, and that is what `remove_all` is trying to remove.
I think it's more likely that the database isn't fully closed yet even though the `CWallet` should be destructed.
💬 achow101 commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#discussion_r1450795240)
I think it would be better if we used `skip_if_no_module` so that the test is reported as skipped instead of silently "passing" on windows. Could also use `skip_if_platform_not_posix` for that.
(https://github.com/bitcoin/bitcoin/pull/29238#discussion_r1450795240)
I think it would be better if we used `skip_if_no_module` so that the test is reported as skipped instead of silently "passing" on windows. Could also use `skip_if_platform_not_posix` for that.
💬 Sjors commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450796060)
Ok, so it has nothing to do with what n0 did. I'm surprised none of the other wallet backup & restore tests have this issue. But I guess it's a different failure condition.
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450796060)
Ok, so it has nothing to do with what n0 did. I'm surprised none of the other wallet backup & restore tests have this issue. But I guess it's a different failure condition.
💬 jamesob commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#discussion_r1450796816)
Oh, good call. Will do.
(https://github.com/bitcoin/bitcoin/pull/29238#discussion_r1450796816)
Oh, good call. Will do.
💬 achow101 commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450797858)
It looks like we don't have any tests that have a failure in `CWallet::Create`, which I don't think is that surprising since generally the wallet needs to be corrupted for that to happen.
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450797858)
It looks like we don't have any tests that have a failure in `CWallet::Create`, which I don't think is that surprising since generally the wallet needs to be corrupted for that to happen.
💬 m3dwards commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889773750)
I agree with @achow101 that there looks like there is a problem with the cleanup when the wallet restore fails.
As a quick fix, what about just removing line 127 which calls `assert_raises_rpc_error`. It's intended to fail and cleanup anyway and so shouldn't affect the rest of the test.
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889773750)
I agree with @achow101 that there looks like there is a problem with the cleanup when the wallet restore fails.
As a quick fix, what about just removing line 127 which calls `assert_raises_rpc_error`. It's intended to fail and cleanup anyway and so shouldn't affect the rest of the test.
💬 achow101 commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889776317)
ACK f6140e47736a1b105196ba91080c7c1d203f2a77
> As a quick fix, what about just removing line 127 which calls `assert_raises_rpc_error`. It's intended to fail and cleanup anyway and so shouldn't affect the rest of the test.
That failure is an important part of the test since we need to be testing that wallets don't load while background ibd is ongoing.
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889776317)
ACK f6140e47736a1b105196ba91080c7c1d203f2a77
> As a quick fix, what about just removing line 127 which calls `assert_raises_rpc_error`. It's intended to fail and cleanup anyway and so shouldn't affect the rest of the test.
That failure is an important part of the test since we need to be testing that wallets don't load while background ibd is ongoing.
💬 Sjors commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889777696)
> As a quick fix, what about just removing line 127 which calls `assert_raises_rpc_error`.
That would make the test worse on all platforms. The point of that `assert_raises_rpc_error` is to prevent a regression where we allow the user to restore a wallet too early in the IBD process. That's probably not a rare occurrence: get new computer, start Bitcoin Core, load snapshot, load wallet backup -> oops.
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889777696)
> As a quick fix, what about just removing line 127 which calls `assert_raises_rpc_error`.
That would make the test worse on all platforms. The point of that `assert_raises_rpc_error` is to prevent a regression where we allow the user to restore a wallet too early in the IBD process. That's probably not a rare occurrence: get new computer, start Bitcoin Core, load snapshot, load wallet backup -> oops.
💬 m3dwards commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889779254)
> That failure is an important part of the test since we need to be testing that wallets don't load while background ibd is ongoing.
Understood, was thinking just as a temporary measure until the remove_all call was fixed on windows. Perhaps just disable that line on windows then.
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889779254)
> That failure is an important part of the test since we need to be testing that wallets don't load while background ibd is ongoing.
Understood, was thinking just as a temporary measure until the remove_all call was fixed on windows. Perhaps just disable that line on windows then.
💬 achow101 commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450807950)
> I think it's more likely that the database isn't fully closed yet even though the `CWallet` should be destructed.
Maybe it's because of `-unsafesqlitesync`, going to try turning that off for the test.
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450807950)
> I think it's more likely that the database isn't fully closed yet even though the `CWallet` should be destructed.
Maybe it's because of `-unsafesqlitesync`, going to try turning that off for the test.
💬 mjdietzx commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-1889791034)
Thank you @michaelfolkson and @Eunovo for the reviews! I agree with both of your feedback re improving `descriptors.md` docs to accompany this test. I will have a commit addressing your feedback in the coming days.
> I think it is probably overkill to create a new functional test file for this. I'm not sure why it can't go in wallet_miniscript.py
My intention with this test is to provide a "reference example" with the same motivations and flow/architecture as #22067 (and the original issue
...
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-1889791034)
Thank you @michaelfolkson and @Eunovo for the reviews! I agree with both of your feedback re improving `descriptors.md` docs to accompany this test. I will have a commit addressing your feedback in the coming days.
> I think it is probably overkill to create a new functional test file for this. I'm not sure why it can't go in wallet_miniscript.py
My intention with this test is to provide a "reference example" with the same motivations and flow/architecture as #22067 (and the original issue
...
💬 Sjors commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450812828)
I wonder why it's called unsafe :-)
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450812828)
I wonder why it's called unsafe :-)
💬 alfonsoromanz commented on pull request "Fix typos":
(https://github.com/bitcoin-core/gui/pull/787#issuecomment-1889805504)
Concept ACK. Maybe it's better to squash both commits into a single one? Not sure what others think
(https://github.com/bitcoin-core/gui/pull/787#issuecomment-1889805504)
Concept ACK. Maybe it's better to squash both commits into a single one? Not sure what others think