Fixes bitcoin-core/gui#571.
CWallet::Create() notifies about wallet loading too early, that results the notification goes before DescriptorScriptPubKeyMans were created and added to an encrypted wallet.
And interfaces::Wallet::taprootEnabled() in https://github.com/bitcoin/bitcoin/blob/ecf692b466860f44334a1da967fc2559da913bec/src/qt/receivecoinsdialog.cpp#L100-L102 erroneously returns false for just created encrypted descriptor wallets.
This change is a prerequisite for the following bugfix.
777@@ -778,34 +778,6 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
778 BOOST_CHECK_EQUAL(addtx_count, 4);
779
780
781- TestUnloadWallet(std::move(wallet));
It got broken. Cannot point out the reasons though.
Happy to accept any help with this test.
The test fails because TestLoadWallet uses CWallet::Create directly. As that function no longer calls the wallet loading functions, the things that are supposed to happen in the test case don’t happen and causes it to fail.
Since CWallet::Create sets up unencrypted wallets, I think a reasonable alternative approach is to have it also do the same with born encrypted wallets.
Or the test can use LoadWallet rather than CWallet::Create directly.
Or the test can use
LoadWalletrather thanCWallet::Createdirectly.
Have tried the suggested approach for the removed subtest without success – UnloadWallet waits infinitely.
NotifyWalletLoaded in TestLoadWallet.
Another alternative is to just put
NotifyWalletLoadedinTestLoadWallet.
0error: in "wallet_tests/CreateWallet": check addtx_count == 4 has failed [3 != 4]
This diff works for me:
0diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
1index efb19518d0..683f0eb327 100644
2--- a/src/wallet/test/wallet_tests.cpp
3+++ b/src/wallet/test/wallet_tests.cpp
4@@ -54,6 +54,7 @@ static const std::shared_ptr<CWallet> TestLoadWallet(WalletContext& context)
5 std::vector<bilingual_str> warnings;
6 auto database = MakeWalletDatabase("", options, status, error);
7 auto wallet = CWallet::Create(context, "", std::move(database), options.create_flags, error, warnings);
8+ NotifyWalletLoaded(context, wallet);
9 if (context.chain) {
10 wallet->postInitProcess();
11 }
12@@ -778,6 +779,34 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
13 BOOST_CHECK_EQUAL(addtx_count, 4);
14
15
16+ TestUnloadWallet(std::move(wallet));
17+
18+
19+ // Load wallet again, this time creating new block and mempool transactions
20+ // paying to the wallet as the wallet finishes loading and syncing the
21+ // queue so the events have to be handled immediately. Releasing the wallet
22+ // lock during the sync is a little artificial but is needed to avoid a
23+ // deadlock during the sync and simulates a new block notification happening
24+ // as soon as possible.
25+ addtx_count = 0;
26+ auto handler = HandleLoadWallet(context, [&](std::unique_ptr<interfaces::Wallet> wallet) {
27+ BOOST_CHECK(rescan_completed);
28+ m_coinbase_txns.push_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
29+ block_tx = TestSimpleSpend(*m_coinbase_txns[2], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
30+ m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
31+ mempool_tx = TestSimpleSpend(*m_coinbase_txns[3], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
32+ BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error));
33+ SyncWithValidationInterfaceQueue();
34+ });
35+ wallet = TestLoadWallet(context);
36+ BOOST_CHECK_EQUAL(addtx_count, 4);
37+ {
38+ LOCK(wallet->cs_wallet);
39+ BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_tx.GetHash()), 1U);
40+ BOOST_CHECK_EQUAL(wallet->mapWallet.count(mempool_tx.GetHash()), 1U);
41+ }
42+
43+
44 TestUnloadWallet(std::move(wallet));
45 }
46
Too early NotifyWalletLoaded() call in CWallet::Create() results the
notification goes before DescriptorScriptPubKeyMans were created and
added to an encrypted wallet.
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
Not sure about the last commit. I think it makes sense to limit the scope of locks. And given that this is a backport bugfix, we might want to minimize the diff.
The last commit has been dropped.
Post-merge tACK 0c12f01
Got to this when going through v23.0rc3 testing. I’ve checked against this PR and now it works as expected.
