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

furszy commented at 12:59 pm on September 6, 2022: member

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.

furszy force-pushed on Sep 6, 2022

Sjors commented at 1:57 pm on September 6, 2022: member

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 commented at 2:53 pm on September 6, 2022: member

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?

DrahtBot added the label Wallet on Sep 6, 2022

Sjors commented at 3:46 pm on September 6, 2022: member

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

achow101 added this to the milestone 24.0 on Sep 6, 2022

in src/wallet/walletdb.cpp:844 in 8c52bbe592 outdated

838@@ -832,6 +839,9 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
839                     // Set tx_corrupt back to false so that the error is only printed once (per corrupt tx)
840                     wss.tx_corrupt = false;
841                     result = DBErrors::CORRUPT;
842+                } else if (wss.descriptor_corrupt) {
843+                    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());
844+                    return DBErrors::CORRUPT;

achow101 commented at 3:54 pm on September 6, 2022:

In 8c52bbe5923cb3d78bfd37e429d85b9727cf2dae “wallet: bugfix, load wallet with an unknown descriptor cause fatal error”

I think this should mention that the descriptor is either unknown or corrupt as this will occur for unknown future descriptors. I think it is far more likely that a user downgrades and runs into this error than actually has data corruption in the wallet file.

Also, I think it would be better to added a DBErrors::UNKNOWN_DESCRIPTOR so that the real error gets propagated to the user interface rather than directing them to find the log file.

luke-jr commented at 6:59 pm on September 6, 2022:

DBErrors::TOO_NEW might be a good fit here

furszy commented at 7:01 pm on September 6, 2022:

sounds good. Pushed.

achow101 commented at 4:50 pm on September 6, 2022: member

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:

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

luke-jr commented at 6:54 pm on September 6, 2022: member

@furszy I think a “partial wallet load” would be a footgun and shouldn’t be supported.

furszy commented at 6:58 pm on September 6, 2022: member

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.

in src/wallet/walletdb.cpp:843 in f59489cb7b outdated

838@@ -832,6 +839,9 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
839                     // Set tx_corrupt back to false so that the error is only printed once (per corrupt tx)
840                     wss.tx_corrupt = false;
841                     result = DBErrors::CORRUPT;
842+                } else if (wss.descriptor_corrupt) {
843+                    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());

luke-jr commented at 7:00 pm on September 6, 2022:

This is way too complicated for a typical user.

luke-jr commented at 7:00 pm on September 6, 2022:

Probably should export message via strErr

furszy commented at 7:17 pm on September 6, 2022:

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.

furszy commented at 7:37 pm on September 6, 2022:

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).

luke-jr commented at 9:11 pm on September 6, 2022:

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

luke-jr commented at 9:11 pm on September 6, 2022:

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

achow101 commented at 9:28 pm on September 6, 2022:

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.

furszy commented at 1:00 pm on September 7, 2022:

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.

furszy commented at 2:28 pm on September 7, 2022:

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?

achow101 commented at 3:34 pm on September 7, 2022:

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.

furszy commented at 4:57 pm on September 7, 2022:

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

furszy commented at 5:23 pm on September 7, 2022:

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.

furszy commented at 7:55 pm on September 6, 2022: member

@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 commented at 8:47 pm on September 6, 2022: member

@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 commented at 11:19 pm on September 6, 2022: contributor

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.

furszy force-pushed on Sep 7, 2022

in src/wallet/wallet.cpp:2824 in e6d4425ac2 outdated

2818@@ -2819,8 +2819,12 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
2819             warnings.push_back(strprintf(_("Error reading %s! Transaction data may be missing or incorrect."
2820                                            " Rescanning wallet."), walletFile));
2821             rescan_required = true;
2822-        }
2823-        else {
2824+        } else if (nLoadWalletRet == DBErrors::UNKNOWN_DESCRIPTOR) {
2825+            error = strprintf(_("Error loading %s: Unrecognized descriptor found.\n\n"
2826+                                "The wallet might had been created on a newer version. Downgrading is incompatible with descriptor wallets\n"

achow101 commented at 2:24 pm on September 7, 2022:

I don’t think it is correct to say Downgrading is incompatible with descriptor wallets. Not all downgrading is incompatible, just this particular one with new descriptors. I think you should just leave that sentence out.

Same down below.

furszy commented at 4:46 pm on September 7, 2022:

I wrote it in that way because at this point of the flow we already tried to parse the descriptor and failed, knowing for sure that either the downgrade is incompatible or the db is corrupted. But sure, will remove the line.

furszy force-pushed on Sep 7, 2022

furszy commented at 8:27 pm on September 7, 2022: member

Thanks for the review, feedback tackled.

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

in src/wallet/walletdb.cpp:849 in 645fd6d3b1 outdated

844@@ -832,6 +845,16 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
845                     // Set tx_corrupt back to false so that the error is only printed once (per corrupt tx)
846                     wss.tx_corrupt = false;
847                     result = DBErrors::CORRUPT;
848+                } else if (wss.descriptor_unknown) {
849+                    strErr = "Error: Unrecognized descriptor found in wallet %s\n";

achow101 commented at 2:05 am on September 8, 2022:

Missing strprintf for the %s here.

Also I think we should avoid having \ns here, makes the log look weird. If you want to have multiline, then each of these needs to be WalletLogPrintf’d separately.

furszy commented at 12:06 pm on September 8, 2022:

thx pushed.

Also I think we should avoid having \ns here, makes the log look weird. If you want to have multiline, then each of these needs to be WalletLogPrintf’d separately.

pushed as well, it was a remanent of the error propagation work.

Note: same as happen with the other db load errors, we log the messages twice in some flows (e.g. loading an unrecognized descriptor at startup). The text that is formatted with multiline for the GUI (the one inside CWallet::Create) will be logged right after the WalletBatch::LoadWallet logging. This is due the ThreadSafeMessageBox slot that logs the string if it’s marked “insecure” as well. Will keep the plan and tackle this in a follow-up PR.

furszy force-pushed on Sep 8, 2022

in src/wallet/walletdb.cpp:852 in ca30759b90 outdated

844@@ -832,6 +845,13 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
845                     // Set tx_corrupt back to false so that the error is only printed once (per corrupt tx)
846                     wss.tx_corrupt = false;
847                     result = DBErrors::CORRUPT;
848+                } else if (wss.descriptor_unknown) {
849+                    strErr = strprintf("Error: Unrecognized descriptor found in wallet %s.", pwallet->GetName());
850+                    strErr += (last_client > CLIENT_VERSION) ? "The wallet might had been created on a newer version." :
851+                            "The database might be corrupted or the software version is not compatible with one of your wallet descriptors.";
852+                    strErr += "Please try running the latest software version";

achow101 commented at 5:14 pm on September 8, 2022:

Needs a space before or after each sentence. Otherwise they run together.

furszy commented at 6:33 pm on September 8, 2022:

pushed

furszy force-pushed on Sep 8, 2022

Sjors commented at 1:49 pm on September 9, 2022: member

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):

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

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

furszy commented at 6:31 pm on September 9, 2022: member

@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.

wallet: bugfix, load wallet with an unknown descriptor cause fatal error

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.

d26c3cc444

wallet: coverage for loading an unknown descriptor

Previously, this was crashing the wallet.

e06676377d

furszy force-pushed on Sep 9, 2022

furszy commented at 6:37 pm on September 9, 2022: member

Feedback tackled per @Sjors review. Thanks

No code diff, only commits messages were changed.

achow101 commented at 1:34 am on September 10, 2022: member

ACK e06676377d935c69f0ee51fc18eb0772d524aba5

Sjors commented at 8:06 am on September 12, 2022: member

re-utACK e06676377d935c69f0ee51fc18eb0772d524aba5

achow101 merged this on Sep 13, 2022

achow101 closed this on Sep 13, 2022

sidhujag referenced this in commit 34db3dc495 on Sep 13, 2022

furszy deleted the branch on May 27, 2023

bitcoin locked this on May 26, 2024

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

Conflicts