wallet: move lock to the top of ReleaseWallet #29143

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.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

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

<!--174a7506f384e20aa4161008e828411d-->

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.

cc @maflcko @fanquake

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

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

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

auto it = g_unloading_wallet_set.insert(name);
assert(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.

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

auto it = g_unloading_wallet_set.insert(name);
assert(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.

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

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?

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.

#!/usr/bin/env python3
from threading import Thread
import os
import time

from test_framework.authproxy import JSONRPCException
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
    assert_equal,
    get_rpc_proxy,
)

got_loading_unloading_error = False

def test_load_unload(node, name):
    global got_loading_unloading_error
    while True:
        if got_loading_unloading_error:
            return
        try:
            node.loadwallet(name)
            node.unloadwallet(name)
            node.unloadwallet(name)
            threads = []
            for _ in range(2):
                t = Thread(target=unload_wallet, args=(node, name))
                t.start()
                threads.append(t)
            for t in threads:
                t.join()
        except JSONRPCException as e:
            is_wallet_already_loading = lambda ex : ex.error['code'] == -4 and 'Wallet already loading' in ex.error['message']
            is_wallet_already_marked_for_unloading = lambda ex : ex.error['code'] == -4 and 'Wallet already marked for unloading' in ex.error['message']
            if is_wallet_already_marked_for_unloading(e):
                got_loading_unloading_error = True
                return

def unload_wallet(node, wallet_name):
    node.unloadwallet(wallet_name)

class MultiWalletTest(BitcoinTestFramework):
    def set_test_params(self):
        self.setup_clean_chain = True
        self.num_nodes = 2
        self.rpc_timeout = 120
        self.extra_args = [["-nowallet"], []]

    def add_options(self, parser):
        self.add_wallet_options(parser)

    def run_test(self):
        node = self.nodes[0]
        to_create = ['w1']
        for wallet_name in to_create:
            self.nodes[0].createwallet(wallet_name)
        node.unloadwallet('w1')
        threads = []
        for _ in range(3):
            n = node.cli if self.options.usecli else get_rpc_proxy(node.url, 1, timeout=600, coveragedir=node.coverage_dir)
            t = Thread(target=test_load_unload, args=(n, 'w1'))
            t.start()
            threads.append(t)
        for t in threads:
            t.join()
        global got_loading_unloading_error
        assert_equal(got_loading_unloading_error, True)

if __name__ == '__main__':
    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.

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