wallet: bugfix, load a wallet with an unknown/corrupt descriptor causes a fatal error #26021

furszy · 2022-09-06T12:59:37Z

If the descriptor entry is unrecognized (due a soft downgrade) or corrupt, the
unserialization fails and LoadWallet, instead of stop there and return the error,
continues reading all the db records. As other records tied to the unrecognized
or corrupt descriptor are scanned, a fatal error is being thrown.

This fixes it by catching the descriptor parse failure and return which wallet failed.
Logging its name/path, so the user can remove it from the settings file, to prevent
its load at startup.

Note: added the test in a separate file intentionally.
Will continue adding coverage for the wallet load process in follow-up PRs.

Sjors · 2022-09-06T13:57:59Z

This also happens when loading a wallet containing a miniscript descriptor.

Catching the error is good, though I still wonder if we should use a flag or something like that to prevent loading future descriptor types.

#24148 (comment)

furszy · 2022-09-06T14:53:32Z

This also happens when loading a wallet containing a miniscript descriptor.

Catching the error is good, though I still wonder if we should use a flag or something like that to prevent loading future descriptor types.

#24148 (comment)

Hmm, if you load unrecognized descriptor types, the parsing process should always fail with the current flow. It's not different to parsing corrupted data.

Will add test coverage for both scenarios and expand the returned error mentioning the last soft version that opened the wallet if it is newer than the current one.

A new feature that could be handy for users is "partial wallet loads" (or "force wallet loads"), meaning that previous soft versions will be able to load every wallet (until where they can), without loading the unrecognized descriptors/data. Where, implementation wise, could be a startup flag that tells the wallet to skip all the entries related to any unrecognized descriptor instead of aborting right away.

So, end result would look like this:

If the descriptor parse fails for any reason: the app will, by default, abort and notify the user about the unrecognized descriptor; mentioning the wallet version if it was created/opened by a newer software version before or, if not, just saying that the file is corrupt.
In case of the user wanting to partially/force load wallets with all the information that can be parsed: The user will add a startup flag "-partial-wallet-load=true" (or something like that). So he/she is aware of the behavior as it can/will only be able to see a partial view of the owned/watched transactions.

What do you think?

Sjors · 2022-09-06T15:46:56Z

I would add an argument to the loadwallet RPC instead, but that's more of a followup thing.

src/wallet/walletdb.cpp

achow101 · 2022-09-06T16:50:15Z

When attempting to load a wallet with an unknown descriptor via the GUI, there is a segfault. This does not occur when using the loadwallet RPC.

Backtrace:

Thread 107 "b-qt-walletctrl" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffe1effd6c0 (LWP 2463919)]
0x00005555556f1b26 in WalletModel::WalletModel (this=0x7ffe0c003d00, wallet=std::unique_ptr<interfaces::Wallet> = {...}, client_model=..., platformStyle=0x5555571f8180, parent=0x0) at qt/walletmodel.cpp:55
55	   fHaveWatchOnly = m_wallet->haveWatchOnly();
(gdb) bt
#0  0x00005555556f1b26 in WalletModel::WalletModel (this=0x7ffe0c003d00, wallet=std::unique_ptr<interfaces::Wallet> = {...}, client_model=..., platformStyle=0x5555571f8180, 
    parent=0x0) at qt/walletmodel.cpp:55
#1  0x00005555556bd84a in WalletController::getOrCreateWallet (this=0x555557a6bd60, wallet=std::unique_ptr<interfaces::Wallet> = {...}) at qt/walletcontroller.cpp:130
#2  0x00005555556bf8c2 in operator() (__closure=0x555557b18980) at qt/walletcontroller.cpp:357
#3  0x00005555556c464e in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, OpenWalletActivity::open(const std::string&)::<lambda()> >::call(struct {...} &, void **) (f=..., arg=0x55555791cd68) at /usr/include/qt/QtCore/qobjectdefs_impl.h:146
#4  0x00005555556c4265 in QtPrivate::Functor<OpenWalletActivity::open(const std::string&)::<lambda()>, 0>::call<QtPrivate::List<>, void>(struct {...} &, void *, void **) (
    f=..., arg=0x55555791cd68) at /usr/include/qt/QtCore/qobjectdefs_impl.h:256
#5  0x00005555556c3b3a in QtPrivate::QFunctorSlotObject<OpenWalletActivity::open(const std::string&)::<lambda()>, 0, QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase *, QObject *, void **, bool *) (which=1, this_=0x555557b18970, r=0x555557a86050, a=0x55555791cd68, ret=0x0) at /usr/include/qt/QtCore/qobjectdefs_impl.h:443
#6  0x00007ffff7ab12f0 in QObject::event(QEvent*) () from /usr/lib/libQt5Core.so.5
#7  0x00007ffff7178b3c in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /usr/lib/libQt5Widgets.so.5
#8  0x00007ffff7a8d978 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /usr/lib/libQt5Core.so.5
#9  0x00007ffff7a8e483 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () from /usr/lib/libQt5Core.so.5
#10 0x00007ffff7ad4478 in ?? () from /usr/lib/libQt5Core.so.5
#11 0x00007ffff546fc6b in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
#12 0x00007ffff54c6001 in ?? () from /usr/lib/libglib-2.0.so.0
#13 0x00007ffff546d392 in g_main_context_iteration () from /usr/lib/libglib-2.0.so.0
#14 0x00007ffff7ad825c in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/libQt5Core.so.5
#15 0x00007ffff7a8611c in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/libQt5Core.so.5
#16 0x00007ffff78e74bf in QThread::exec() () from /usr/lib/libQt5Core.so.5
#17 0x00007ffff78e42ea in ?? () from /usr/lib/libQt5Core.so.5
#18 0x00007ffff61b778d in ?? () from /usr/lib/libc.so.6
#19 0x00007ffff62388e4 in clone () from /usr/lib/libc.so.6

luke-jr · 2022-09-06T18:54:54Z

@furszy I think a "partial wallet load" would be a footgun and shouldn't be supported.

furszy · 2022-09-06T18:58:47Z

When attempting to load a wallet with an unknown descriptor via the GUI, there is a segfault. This does not occur when using the loadwallet RPC.

Same cause as #26005, "bug 2". util::Result treats the wallet nullptr as the result obj.
It's solved there.

luke-jr · 2022-09-06T19:00:06Z

src/wallet/walletdb.cpp

@@ -832,6 +839,9 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
                    // Set tx_corrupt back to false so that the error is only printed once (per corrupt tx)
                    wss.tx_corrupt = false;
                    result = DBErrors::CORRUPT;
+                } else if (wss.descriptor_corrupt) {
+                    pwallet->WalletLogPrintf("Error: Corrupt wallet descriptor found in wallet %s. This can be fixed by removing wallet from the settings so it's not loaded at startup.\n", pwallet->GetName());


This is way too complicated for a typical user.

Are you talking about GUI users?
For them, I think that the best would be to trigger a dialog and ask if want to continue without this wallet. But, that would require a bit more work that could tackle here or in a follow-up work

For bitcoind users, as the app cannot proceed without further interaction, shouldn't be a problem for them to remove a line in the settings file. But.. another option could be to not load/open certain wallet/s, even if they are on the settings file, by providing a startup flag "-unloadwallet=<wallet_name>" (need to check if we have an available flag for this already or will have to create one). What do you think?

Note: In both cases, I don't think that skipping certain wallet/s from loading at startup without the user interaction (nor the user knowing about it) is any better than this (would actually say that it's worst).

I mean both should get the error details without jumping through extra hoops

Pushed a different text version.

At this point of the code we don't have much information aside from the "unknown wallet descriptor found" (at least for now, will work on it on a follow-up PR). We just know that a certain descriptor parse failed. Thus why added the extra information at the end, to provide the user a workaround if still want to run other wallets (just need to unload the failing one).

But.. for the sake of simplicity, removed the extra information as well. Let me know what you think.

Better to work on the GUI "unload wallet confirmation dialog" and the "unload wallet" startup flag on a different PR so this bugfix can get merged for this release.

luke-jr · 2022-09-06T19:00:30Z

src/wallet/walletdb.cpp

@@ -832,6 +839,9 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
                    // Set tx_corrupt back to false so that the error is only printed once (per corrupt tx)
                    wss.tx_corrupt = false;
                    result = DBErrors::CORRUPT;
+                } else if (wss.descriptor_corrupt) {
+                    pwallet->WalletLogPrintf("Error: Corrupt wallet descriptor found in wallet %s. This can be fixed by removing wallet from the settings so it's not loaded at startup.\n", pwallet->GetName());


Probably should export message via strErr

yeah, briefly thought about it as well and.. by doing it, remove code duplication that comes with it. But.. as this involves the introduction of the util::Result class and.. I have tendencies of wanting to change too much in single PRs was balancing on whether that should be here or not.
Let me see how it will looks like.

strErr is already used here, it doesn't need util::Result

Yes, this should just use strErr. Any error put there should already make it out to the user on the RPC or trigger an error dialog in the GUI. This shouldn't need any other changes.

strErr isn't propagated to the GUI nor RPC. Is internal to WalletBatch::LoadWallet, declared prior to call ReadKeyValue and only used to log the error at the end of every record read while we walk through all the db records.

To propagate the error to the upper layers need to add a ref string into the function signature (or use the result class and kill two birds with the same stone, cleaning more code in the way).

If you check the line below this logging, I’m returning right away, not letting the process continue reading all the db records. The reason is that you can have a corrupted descriptor that has an “active descriptor“ entry, which will crash the wallet once the process calls to LoadActiveScriptPubKeyMan (on the map.at(id) line) as the descriptor which the active entry points was never added to the wallet.

This is something that was going to push later but essentially you can crash the wallet at startup by adding an ACTIVEEXTERNALSPK or ACTIVEINTERNALSPK record to the db that points to a random spkm id.

Extra note: checked how the util::result connection would look like and changes aren't straightforward to do. It needs further code reorganizations as DBErrors enum mixes errors, warnings and the good result (I think that the best would be to decouple this enum into two enums errors and warnings or.. even get rid of the enum completely like it was done in #25308. but.. will leave that for another PR).

So, options:

Move forward by adding the ref string arg here. The same error string will be propagated to the upper layers, removing one line of code duplication but having to add the std::string ref arg in all the WalletBatch::LoadWallet calls (which we do have a good number of them).

Leave it as is now, which essentially means to act the same as with all the corrupted wallet errors: having the string duplicated inside WalletBatch::LoadWallet and its caller CWallet::Create (wallet.cpp). And implement the proper changes all at once in a follow-up PR.

Personally, I'm inclined to go with option (2) as it's inlined with what we currently have and doesn't introduce changes that will most likely be removed later.
Thoughts?

For a bugfix for this release, we should just do the minimum to get a message out to the user. We can do more granulated error message propagation in follow-ups.

sounds good, let's continue with option 2 then.

furszy · 2022-09-06T19:55:15Z

@furszy I think a "partial wallet load" would be a footgun and shouldn't be supported.

Conceptually, it's similar to how we treat other compatibility issues. Old versions aren't aware of new stuff but are still able to run what they have (skipping / not verifying some data). While on this scenario, previous versions are simply not compatible with newer wallets.

It's a debatable topic. Might be good to continue being incompatible actually.

achow101 · 2022-09-06T20:47:46Z

@furszy I think a "partial wallet load" would be a footgun and shouldn't be supported.

Conceptually, it's similar to how we treat other compatibility issues. Old versions aren't aware of new stuff but are still able to run what they have (skipping / not verifying some data). While on this scenario, previous versions are simply not compatible with newer wallets.

It's a debatable topic. Might be good to continue being incompatible actually.

I agree with @luke-jr, it's a footgun and we should avoid doing that. The difference is that a partial load may be missing critical information in things that it cannot understand and so may be allowing spending where it shouldn't, or failing to detect transactions when it should. There is a reason that those things return errors.

With our backwards compatibility, the newly added things are specifically designed so that they do not induce errors in older versions and are also designed such that they are not critical to operation. Furthermore, we do actually have downgrade protection to prevent loading of newer wallets if new critical data has been added.

New/unknown descriptors are part of the critical data and so must cause older software to fail to load the wallet. The decision to use descriptor parsing failures as the downgrade protection method was intentional as using more wallet flags for each new descriptor type is not scalable.

DrahtBot · 2022-09-06T23:19:03Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25766 (wallet: Include a signature with encrypted keys to mitigate a wallet scam by achow101)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#24914 (wallet: Load database records in a particular order by achow101)
#24897 ([Draft / POC] Silent Payments by w0xlt)

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.

src/wallet/wallet.cpp

furszy · 2022-09-07T20:27:06Z

Thanks for the review, feedback tackled.

Plus, for the sake of completeness, added test coverage for it as well.
Can run 1703678 without the fix commit and the test will crash.

src/wallet/walletdb.cpp

Sjors · 2022-09-09T13:49:25Z

tACK a76f9b2

@achow101 wrote:

When attempting to load a wallet with an unknown descriptor via the GUI, there is a segfault. This does not occur when using the loadwallet RPC.

This happens to me as well. That's not new in this PR though, running without this PR:

2022-09-09T13:52:07Z [qt-walletctrl] GUI: Qt has caught an exception thrown from an event handler. Throwing ...

cc @hebasto

Don't forget to update the PR description.

I also suggest moving this comment out of the commit and into the PR description (since there's no guarantee such "follow-up commits" get merged):

Note: added the test in a separate file intentionally.
Will continue adding coverage for the wallet load
process in follow-up commits.

You could also drop ### Issue Explanation from the first commit message.

furszy · 2022-09-09T18:31:15Z

@achow101 wrote:

When attempting to load a wallet with an unknown descriptor via the GUI, there is a segfault. This does not occur when using the loadwallet RPC.

This happens to me as well. That's not new in this PR though, running without this PR:

2022-09-09T13:52:07Z [qt-walletctrl] GUI: Qt has caught an exception thrown from an event handler. Throwing ...
cc @hebasto

Already answered it. @Sjors, please check #26021 (comment)

Don't forget to update the PR description.

I also suggest moving this comment out of the commit and into the PR description (since there's no guarantee such "follow-up commits" get merged):

Note: added the test in a separate file intentionally.
Will continue adding coverage for the wallet load
process in follow-up commits.
You could also drop ### Issue Explanation from the first commit message.

Ok, will do.

If the descriptor entry is unrecognized/corrupt, the unserialization fails and `LoadWallet` instead of stop there and return the error, continues reading all the db records. As other records tied to the unrecognized/corrupted descriptor are scanned, a fatal error is thrown.

Previously, this was crashing the wallet.

furszy · 2022-09-09T18:37:56Z

Feedback tackled per @Sjors review. Thanks

No code diff, only commits messages were changed.

achow101 · 2022-09-10T01:34:37Z

ACK e066763

Sjors · 2022-09-12T08:06:35Z

re-utACK e066763

…rrupt descriptor causes a fatal error e066763 wallet: coverage for loading an unknown descriptor (furszy) d26c3cc wallet: bugfix, load wallet with an unknown descriptor cause fatal error (furszy) Pull request description: Fixes bitcoin#26015 If the descriptor entry is unrecognized (due a soft downgrade) or corrupt, the unserialization fails and `LoadWallet`, instead of stop there and return the error, continues reading all the db records. As other records tied to the unrecognized or corrupt descriptor are scanned, a fatal error is being thrown. This fixes it by catching the descriptor parse failure and return which wallet failed. Logging its name/path, so the user can remove it from the settings file, to prevent its load at startup. Note: added the test in a separate file intentionally. Will continue adding coverage for the wallet load process in follow-up PRs. ACKs for top commit: achow101: ACK e066763 Sjors: re-utACK e066763 Tree-SHA512: d1f1a5d7e944c89c97a33b25b4411a36a11edae172c22f8524f69c84a035f84c570b284679f901fe60f1300f781b76a6c17b015a8e7ad44ebd25a0c295ef260f

furszy force-pushed the 2022_wallet_fix_descriptor_unserialization branch from 12b0169 to f59489c Compare September 6, 2022 13:25

DrahtBot added the Wallet label Sep 6, 2022

achow101 added this to the 24.0 milestone Sep 6, 2022

achow101 reviewed Sep 6, 2022

View reviewed changes

src/wallet/walletdb.cpp Outdated Show resolved Hide resolved

luke-jr reviewed Sep 6, 2022

View reviewed changes

This was referenced Sep 7, 2022

wallet: Include a signature with encrypted keys to mitigate a wallet scam #25766

Closed

refactor: Use util::Result class for wallet loading #25722

Draft

furszy force-pushed the 2022_wallet_fix_descriptor_unserialization branch from f59489c to e6d4425 Compare September 7, 2022 14:07

achow101 reviewed Sep 7, 2022

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

furszy force-pushed the 2022_wallet_fix_descriptor_unserialization branch from e6d4425 to b574285 Compare September 7, 2022 16:59

DrahtBot mentioned this pull request Sep 7, 2022

wallet: Load database records in a particular order #24914

Merged

furszy force-pushed the 2022_wallet_fix_descriptor_unserialization branch 2 times, most recently from b7e9f0f to 1703678 Compare September 7, 2022 20:22

achow101 reviewed Sep 8, 2022

View reviewed changes

src/wallet/walletdb.cpp Outdated Show resolved Hide resolved

furszy force-pushed the 2022_wallet_fix_descriptor_unserialization branch from 1703678 to 6cbe5bd Compare September 8, 2022 11:31

achow101 reviewed Sep 8, 2022

View reviewed changes

src/wallet/walletdb.cpp Outdated Show resolved Hide resolved

furszy force-pushed the 2022_wallet_fix_descriptor_unserialization branch from 6cbe5bd to a76f9b2 Compare September 8, 2022 18:33

furszy added 2 commits September 9, 2022 15:35

wallet: coverage for loading an unknown descriptor

e066763

Previously, this was crashing the wallet.

furszy force-pushed the 2022_wallet_fix_descriptor_unserialization branch from a76f9b2 to e066763 Compare September 9, 2022 18:35

achow101 merged commit c856883 into bitcoin:master Sep 13, 2022

DrahtBot mentioned this pull request Sep 13, 2022

[Draft / POC] Silent Payments #24897

Closed

furszy deleted the 2022_wallet_fix_descriptor_unserialization branch May 27, 2023 01:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: bugfix, load a wallet with an unknown/corrupt descriptor causes a fatal error #26021

wallet: bugfix, load a wallet with an unknown/corrupt descriptor causes a fatal error #26021

furszy commented Sep 6, 2022 •

edited

Sjors commented Sep 6, 2022

furszy commented Sep 6, 2022 •

edited

Sjors commented Sep 6, 2022

achow101 commented Sep 6, 2022

luke-jr commented Sep 6, 2022

furszy commented Sep 6, 2022

luke-jr Sep 6, 2022

furszy Sep 6, 2022 •

edited

luke-jr Sep 6, 2022

furszy Sep 7, 2022

luke-jr Sep 6, 2022

furszy Sep 6, 2022 •

edited

luke-jr Sep 6, 2022

achow101 Sep 6, 2022 •

edited

furszy Sep 7, 2022

furszy Sep 7, 2022 •

edited

achow101 Sep 7, 2022

furszy Sep 7, 2022

furszy commented Sep 6, 2022

achow101 commented Sep 6, 2022 •

edited

DrahtBot commented Sep 6, 2022 •

edited

furszy commented Sep 7, 2022

Sjors commented Sep 9, 2022 •

edited

furszy commented Sep 9, 2022 •

edited

furszy commented Sep 9, 2022

achow101 commented Sep 10, 2022

Sjors commented Sep 12, 2022

wallet: bugfix, load a wallet with an unknown/corrupt descriptor causes a fatal error #26021

wallet: bugfix, load a wallet with an unknown/corrupt descriptor causes a fatal error #26021

Conversation

furszy commented Sep 6, 2022 • edited

Sjors commented Sep 6, 2022

furszy commented Sep 6, 2022 • edited

Sjors commented Sep 6, 2022

achow101 commented Sep 6, 2022

luke-jr commented Sep 6, 2022

furszy commented Sep 6, 2022

Choose a reason for hiding this comment

furszy Sep 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furszy Sep 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 Sep 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furszy Sep 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furszy commented Sep 6, 2022

achow101 commented Sep 6, 2022 • edited

DrahtBot commented Sep 6, 2022 • edited

Conflicts

furszy commented Sep 7, 2022

Sjors commented Sep 9, 2022 • edited

furszy commented Sep 9, 2022 • edited

furszy commented Sep 9, 2022

achow101 commented Sep 10, 2022

Sjors commented Sep 12, 2022

furszy commented Sep 6, 2022 •

edited

furszy commented Sep 6, 2022 •

edited

furszy Sep 6, 2022 •

edited

furszy Sep 6, 2022 •

edited

achow101 Sep 6, 2022 •

edited

furszy Sep 7, 2022 •

edited

achow101 commented Sep 6, 2022 •

edited

DrahtBot commented Sep 6, 2022 •

edited

Sjors commented Sep 9, 2022 •

edited

furszy commented Sep 9, 2022 •

edited