Skip to content

wallet: Postpone wallet loading notification for encrypted wallets #24711

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 31, 2022

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Mar 29, 2022

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

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.

This change is a prerequisite for the following bugfix.
Copy link
Member

@Sjors Sjors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK bf01a51 modulo mysteriously disappearing test

@achow101
Copy link
Member

achow101 commented Mar 30, 2022

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
Copy link
Member Author

hebasto commented Mar 30, 2022

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
Copy link
Member

Another alternative is to just put NotifyWalletLoaded in TestLoadWallet.

@hebasto
Copy link
Member Author

hebasto commented Mar 30, 2022

Another alternative is to just put NotifyWalletLoaded in TestLoadWallet.

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

@achow101
Copy link
Member

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));
 }
 

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 <[email protected]>
@hebasto
Copy link
Member Author

hebasto commented Mar 30, 2022

@achow101

This diff works for me:

Thanks! Updated.

@maflcko
Copy link
Member

maflcko commented Mar 31, 2022

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
Copy link
Member Author

hebasto commented Mar 31, 2022

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
Copy link
Member

Sjors commented Mar 31, 2022

utACK 0c12f01

@achow101
Copy link
Member

ACK 0c12f01

@achow101 achow101 merged commit b7d78e6 into bitcoin:master Mar 31, 2022
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Mar 31, 2022
This change is a prerequisite for the following bugfix.

Github-Pull: bitcoin#24711
Rebased-From: aeee419
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Mar 31, 2022
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 <[email protected]>

Github-Pull: bitcoin#24711
Rebased-From: 0c12f01
@fanquake
Copy link
Member

Backported in #24725.

@hebasto hebasto deleted the 220329-taproot branch March 31, 2022 17:14
fujicoin pushed a commit to fujicoin/fujicoin-23.0 that referenced this pull request Apr 1, 2022
This change is a prerequisite for the following bugfix.

Github-Pull: bitcoin/bitcoin#24711
Rebased-From: aeee419c6aae085cacd75343c1ce23486b2b8916
fujicoin pushed a commit to fujicoin/fujicoin-23.0 that referenced this pull request Apr 1, 2022
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 <[email protected]>

Github-Pull: bitcoin/bitcoin#24711
Rebased-From: 0c12f0116ca802f55f5ab43e6c4842ac403b9889
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 3, 2022
… encrypted wallets

0c12f01 wallet: Postpone NotifyWalletLoaded() for encrypted wallets (Hennadii Stepanov)
aeee419 wallet, refactor: Add wallet::NotifyWalletLoaded() function (Hennadii Stepanov)

Pull request description:

  Fixes bitcoin-core/gui#571.

  `CWallet::Create()` notifies about wallet loading too early, that results the notification goes before `DescriptorScriptPubKeyMan`s 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.

ACKs for top commit:
  Sjors:
    utACK 0c12f01
  achow101:
    ACK 0c12f01

Tree-SHA512: 2694bacd12748cd5f6c95d9d3bf8bcf4502ee67fecd8d057f33236b72069c61401b08f49deb013fc71c3f1e51ae16bdfd827ddcbc2a083d7044589be7a78982e
Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

backpacker69 pushed a commit to peercoin/peercoin that referenced this pull request Jan 18, 2023
This change is a prerequisite for the following bugfix.

Github-Pull: bitcoin/bitcoin#24711
Rebased-From: aeee419c6aae085cacd75343c1ce23486b2b8916
backpacker69 pushed a commit to peercoin/peercoin that referenced this pull request Jan 18, 2023
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 <[email protected]>

Github-Pull: bitcoin/bitcoin#24711
Rebased-From: 0c12f0116ca802f55f5ab43e6c4842ac403b9889
@bitcoin bitcoin locked and limited conversation to collaborators Apr 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bech32m unavailable for encrypted wallets
6 participants