wallet: Postpone wallet loading notification for encrypted wallets

[hebasto]

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

bitcoin/src/qt/receivecoinsdialog.cpp

Lines 100 to 102 in ecf692b

	if (model->wallet().taprootEnabled()) {
	add_address_type(OutputType::BECH32M, "Bech32m (Taproot)", "Bech32m (BIP-350) is an upgrade to Bech32, wallet support is still limited.");
	}

erroneously returns false for just created encrypted descriptor wallets.

[Sjors]

tACK bf01a51 modulo mysteriously disappearing test

[achow101]

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.

[hebasto]

Or the test can use LoadWallet rather than CWallet::Create directly.

Have tried the suggested approach for the removed subtest without success -- UnloadWallet waits infinitely.

[achow101]

Another alternative is to just put NotifyWalletLoaded in TestLoadWallet.

[hebasto]

Another alternative is to just put NotifyWalletLoaded in TestLoadWallet.

error: in "wallet_tests/CreateWallet": check addtx_count == 4 has failed [3 != 4]

[achow101]

This diff works for me:

diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index efb19518d0..683f0eb327 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -54,6 +54,7 @@ static const std::shared_ptr<CWallet> TestLoadWallet(WalletContext& context)
     std::vector<bilingual_str> warnings;
     auto database = MakeWalletDatabase("", options, status, error);
     auto wallet = CWallet::Create(context, "", std::move(database), options.create_flags, error, warnings);
+    NotifyWalletLoaded(context, wallet);
     if (context.chain) {
         wallet->postInitProcess();
     }
@@ -778,6 +779,34 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
     BOOST_CHECK_EQUAL(addtx_count, 4);
 
 
+    TestUnloadWallet(std::move(wallet));
+
+
+    // Load wallet again, this time creating new block and mempool transactions
+    // paying to the wallet as the wallet finishes loading and syncing the
+    // queue so the events have to be handled immediately. Releasing the wallet
+    // lock during the sync is a little artificial but is needed to avoid a
+    // deadlock during the sync and simulates a new block notification happening
+    // as soon as possible.
+    addtx_count = 0;
+    auto handler = HandleLoadWallet(context, [&](std::unique_ptr<interfaces::Wallet> wallet) {
+            BOOST_CHECK(rescan_completed);
+            m_coinbase_txns.push_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
+            block_tx = TestSimpleSpend(*m_coinbase_txns[2], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
+            m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
+            mempool_tx = TestSimpleSpend(*m_coinbase_txns[3], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
+            BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error));
+            SyncWithValidationInterfaceQueue();
+        });
+    wallet = TestLoadWallet(context);
+    BOOST_CHECK_EQUAL(addtx_count, 4);
+    {
+        LOCK(wallet->cs_wallet);
+        BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_tx.GetHash()), 1U);
+        BOOST_CHECK_EQUAL(wallet->mapWallet.count(mempool_tx.GetHash()), 1U);
+    }
+
+
     TestUnloadWallet(std::move(wallet));
 }
 

[hebasto]

@achow101

This diff works for me:

Thanks! Updated.

[maflcko]

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.

[hebasto]

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.

[Sjors]

utACK 0c12f01

[achow101]

ACK 0c12f01

[fanquake]

Backported in #24725.

[dunxen]

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.