💬 jonatack commented on pull request "rpc: Make v2transport default for addnode RPC when enabled":
(https://github.com/bitcoin/bitcoin/pull/29239#discussion_r1450777060)
Perhaps good to disambiguate the config option from the rpc option.
```suggestion
{"v2transport", RPCArg::Type::BOOL, RPCArg::DefaultHint{"set by -v2transport configuration option"}, "Attempt to connect using BIP324 v2 transport protocol (ignored for 'remove' command)"},
```

💬 Sjors commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450778587)
@jamesob I think you can just close the wallet after this call?

💬 jonatack commented on pull request "rpc: Make v2transport default for addnode RPC when enabled":
(https://github.com/bitcoin/bitcoin/pull/29239#discussion_r1450782082)
fwiw for the v2transport config option help

current
```
-v2transport
Support v2 transport (default: 0)
```

suggestion
```
-v2transport
Support BIP324 v2 transport (default: 0)
```
or
```
-v2transport
Support BIP324 v2 transport protocol (default: 0)
```

💬 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.

💬 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

💬 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.

💬 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)

💬 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?

💬 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.

💬 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.

💬 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.

💬 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.

💬 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.

💬 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.

💬 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.

💬 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.

💬 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.

💬 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.

💬 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.

💬 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.