wallet: move lock to the top of ReleaseWallet

dimitaracev commented at 6:29 pm on December 26, 2023: contributor

Fixes #29073 Calling unloadwallet first marks that wallet for unloading and then unloads it. If the unloading does not finish in time and unloadwallet is called again for the same wallet an assertion error is thrown wallet/wallet.cpp:246: void wallet::UnloadWallet(std::shared_ptr<CWallet> &&): Assertion it.second which is not descriptive. The test in issue #29073 is sometimes failing due to that assertion. This pull request fixes that by changing the error returned from RPC to Wallet already marked for unloading and takes that into account for the test. Maybe it even makes sense to do nothing if the case above happens which will not require changes to the test since no error is returned.

DrahtBot commented at 6:29 pm on December 26, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25390 (sync: introduce a thread-safe generic container and use it to remove a bunch of “GlobalMutex"es by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Wallet on Dec 26, 2023

dimitaracev commented at 6:30 pm on December 26, 2023: contributor

cc @maflcko @fanquake

brunoerg commented at 6:36 pm on December 26, 2023: contributor

0test/functional/wallet_multiwallet.py:38:9: F841 local variable 'e' is assigned to but never used
1test/functional/wallet_multiwallet.py:39:50: F821 undefined name 'e'
2test/functional/wallet_multiwallet.py:39:104: F821 undefined name 'e'
3test/functional/wallet_multiwallet.py:40:63: F821 undefined name 'e'
4test/functional/wallet_multiwallet.py:40:130: F821 undefined name 'e'

dimitaracev force-pushed on Dec 26, 2023

DrahtBot added the label CI failed on Dec 26, 2023

furszy commented at 10:27 pm on December 26, 2023: member

How is this possible if RemoveWallet() locks the context wallets’ mutex, checks if the wallet is there (failing if not) and then removes it.

dimitaracev commented at 11:01 pm on December 26, 2023: contributor

How is this possible if RemoveWallet() locks the context wallets’ mutex, checks if the wallet is there (failing if not) and then removes it.

From my understanding, the statements

0auto it = g_unloading_wallet_set.insert(name);
1assert(it.second);

assume that the wallet will always be correctly inserted into g_unloading_wallet_set where g_unloading_wallet_set.insert(name) returns a pair (it.second is 1 if the wallet is successfully inserted into the set to be used by ReleaseWallet(), or 0 if it is already present in the set). At least to me it appears that it can in some rare cases come into a collision where the wallet name is not yet removed from the g_unloading_wallet_set and g_unloading_wallet_set.insert(name) returns it.second as 0. Correct me if I’m wrong but I’m not very familiar with the codebase and trying to slowly learn.

furszy commented at 11:19 pm on December 26, 2023: member

How is this possible if RemoveWallet() locks the context wallets’ mutex, checks if the wallet is there (failing if not) and then removes it.

From my understanding, the statements
0auto it = g_unloading_wallet_set.insert(name);
1assert(it.second);
assume that the wallet will always be correctly inserted into g_unloading_wallet_set where g_unloading_wallet_set.insert(name) returns a pair (it.second is 1 if the wallet is successfully inserted into the set to be used by ReleaseWallet(), or 0 if it is already present in the set). At least to me it appears that it can in some rare cases come into a collision where the wallet name is not yet removed from the g_unloading_wallet_set and g_unloading_wallet_set.insert(name) returns it.second as 0. Correct me if I’m wrong but I’m not very familiar with the codebase and trying to slowly learn.

Before calling UnloadWallet(), unloadwallet calls RemoveWallet() which locks the context wallets’ mutex and tries to remove the wallet. Failing if the wallet is being unloaded (aka not able to remove it from the context wallets’ vector).

In other words, the code shouldn’t reach the UnloadWallet() call. Should verify that we are not pushing two pointers of the same wallet into the context vector.

dimitaracev commented at 11:38 pm on December 26, 2023: contributor

@furszy Oh I see, will continue to look into this further. Thanks.

maflcko commented at 8:51 am on December 27, 2023: member

Do you have steps to reproduce the test failure (usually a race can be reproduced by adding a sleep in the right place in the C++ code)?

Also, an assertion crash fix, is not a “test” fix. I presume this can happen in production, no?

dimitaracev commented at 7:18 pm on December 27, 2023: contributor

Also, an assertion crash fix, is not a “test” fix. I presume this can happen in production, no?

Initially I thought that this was the expected behavior, which is why this pull request just returns a different error message so that it can be caught in the test. But as @furszy said, the issue appears to be something completely different that needs fixing.

Do you have steps to reproduce the test failure (usually a race can be reproduced by adding a sleep in the right place in the C++ code)?

Did not have time to check where exactly to add a sleep in the code, but with the following test I’m able to come into a state where the previous assertion fail happened and in my case the ‘Wallet already marked for unloading’ RPC error.

 0#!/usr/bin/env python3
 1from threading import Thread
 2import os
 3import time
 4
 5from test_framework.authproxy import JSONRPCException
 6from test_framework.test_framework import BitcoinTestFramework
 7from test_framework.util import (
 8    assert_equal,
 9    get_rpc_proxy,
10)
11
12got_loading_unloading_error = False
13
14def test_load_unload(node, name):
15    global got_loading_unloading_error
16    while True:
17        if got_loading_unloading_error:
18            return
19        try:
20            node.loadwallet(name)
21            node.unloadwallet(name)
22            node.unloadwallet(name)
23            threads = []
24            for _ in range(2):
25                t = Thread(target=unload_wallet, args=(node, name))
26                t.start()
27                threads.append(t)
28            for t in threads:
29                t.join()
30        except JSONRPCException as e:
31            is_wallet_already_loading = lambda ex : ex.error['code'] == -4 and 'Wallet already loading' in ex.error['message']
32            is_wallet_already_marked_for_unloading = lambda ex : ex.error['code'] == -4 and 'Wallet already marked for unloading' in ex.error['message']
33            if is_wallet_already_marked_for_unloading(e):
34                got_loading_unloading_error = True
35                return
36
37def unload_wallet(node, wallet_name):
38    node.unloadwallet(wallet_name)
39
40class MultiWalletTest(BitcoinTestFramework):
41    def set_test_params(self):
42        self.setup_clean_chain = True
43        self.num_nodes = 2
44        self.rpc_timeout = 120
45        self.extra_args = [["-nowallet"], []]
46
47    def add_options(self, parser):
48        self.add_wallet_options(parser)
49
50    def run_test(self):
51        node = self.nodes[0]
52        to_create = ['w1']
53        for wallet_name in to_create:
54            self.nodes[0].createwallet(wallet_name)
55        node.unloadwallet('w1')
56        threads = []
57        for _ in range(3):
58            n = node.cli if self.options.usecli else get_rpc_proxy(node.url, 1, timeout=600, coveragedir=node.coverage_dir)
59            t = Thread(target=test_load_unload, args=(n, 'w1'))
60            t.start()
61            threads.append(t)
62        for t in threads:
63            t.join()
64        global got_loading_unloading_error
65        assert_equal(got_loading_unloading_error, True)
66
67if __name__ == '__main__':
68    MultiWalletTest().main()

Note: The error message can be seen when the test is ran with ‘–tracerpc’ argument. I’m currently looking further into the issue, trying to debug how it can come into this situation where the assertion fails.

wallet: move lock at the top of ReleaseWallet c3d4463efe

dimitaracev force-pushed on Dec 29, 2023

dimitaracev renamed this:
~~wallet: add meaningful error message and fix test~~
wallet: move lock to the top of ReleaseWallet
on Dec 29, 2023

dimitaracev closed this on Dec 29, 2023

dimitaracev deleted the branch on Dec 29, 2023

dimitaracev commented at 9:23 pm on December 29, 2023: contributor

Closed in favor of #29155 due to the branch name being misleading.

wallet: move lock to the top of ReleaseWallet #29143

Code Coverage

Reviews

Conflicts