Basic fix for the CI failure introduced in #28838 until something better can be devised.
Apparently Windows doesn't allow two processes to read the same wallet file. Note that this failure is unique to the tests and not something an end user would see (unless they were running two bitcoind instances...).
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | achow101 |
| Stale ACK | BrandonOdiwuor |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
lgtm, but wouldn't it be better to just unload the wallet for a short time?
129 | @@ -124,8 +130,14 @@ def run_test(self): 130 | assert_equal(n1.getblockchaininfo()["blocks"], SNAPSHOT_BASE_HEIGHT) 131 | 132 | self.log.info("Backup can't be loaded during background sync") 133 | + if is_windows: 134 | + n0.unloadwallet('w')
n0.unloadwallet('w') # Required, because on Windows only one process can access a file, when it is written
nit: Seems better to run the same test on all platforms, than to run different branches on different platforms, and thus make it harder to debug test failures?
test/functional/wallet_assumeutxo.py:14:1: F401 'platform' imported but unused
Code Review ACK 99399ae88fa23c0333bc2b0d59b48d0936a869c7
I'm struggling to exactly replicate this locally. I get a failure and on the same line but my error seems to be the assert failing that expects an exception:
2024-01-12T16:34:39.449000Z TestFramework (INFO): Backup can't be loaded during background sync
2024-01-12T16:34:39.496000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "C:\Users\Max\source\bitcoin\test\functional\test_framework\util.py", line 140, in try_rpc
fun(*args, **kwds)
File "C:\Users\Max\source\bitcoin\test\functional\test_framework\coverage.py", line 50, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "C:\Users\Max\source\bitcoin\test\functional\test_framework\authproxy.py", line 129, in __call__
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: remove_all: unknown error: "C:\Users\Max\AppData\Local\Temp\bitcoin_func_test_bud5txts\node1\regtest\w" (-1)
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "C:\Users\Max\source\bitcoin\test\functional\test_framework\test_framework.py", line 131, in main
self.run_test()
File "C:\Users\Max\source\bitcoin\test\functional\wallet_assumeutxo.py", line 134, in run_test
assert_raises_rpc_error(-4, "Wallet loading failed. Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order when using assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height 299", n1.restorewallet, "w", "backup_w.dat")
File "C:\Users\Max\source\bitcoin\test\functional\test_framework\util.py", line 131, in assert_raises_rpc_error
assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "C:\Users\Max\source\bitcoin\test\functional\test_framework\util.py", line 144, in try_rpc
raise AssertionError("Unexpected JSONRPC error code %i" % e.error["code"])
AssertionError: Unexpected JSONRPC error code -1
2024-01-12T16:34:39.552000Z TestFramework (INFO): Stopping nodes
I tried the changes in 99399ae88fa23c0333bc2b0d59b48d0936a869c7 but they didn't help my local failure. If I remove the assert_raises_rpc_error(-4, "Wallet loading failed... assertion the test passes for me locally.
edit: it could be that I'm getting the same problem but for whatever reason the reason is "unknown error" for me.
I don't think this fixes the problem.
Left an alternative suggestion here that (hopefully) doesn't require an exception for Windows: https://github.com/bitcoin/bitcoin/pull/28838/files#r1450778587
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.
53 | @@ -52,6 +54,11 @@ def run_test(self): 54 | Load the snapshot into the second, ensure it syncs to tip and completes 55 | background validation when connected to the first. 56 | """ 57 | + # Disable this test for windows currently because it doesn't allow two 58 | + # bitcoind processes simultaneously trying to read the same wallet file. 59 | + if platform.system() == "Windows": 60 | + return
I think it would be better if we used skip_test_if_missing_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.
Oh, good call. Will do.
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.
Test passes locally for me on windows without this line.
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.
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.
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.