bitcoin-wallet salvage: Return false instead of asserting when a loaded tx isn't new

ryanofsky commented at 8:06 pm on August 24, 2020: member

Returning false will give us a recoverable error instead of killing the node entirely. In that situation, the wallet is still recoverable by deleting all txs and rescanning.

This commit was originally part of #19457.

Original motivation for this change was to avoid crashes with #19078, but a better fix for that problem is in #19793. This change is still beneficial because it removes an unsafe assert.

Return false instead of asserting when a loaded tx isn't new

Returning false will give us a recoverable error instead of killing the
node entirely. In that situation, the wallet is still recoverable by
deleting all txs and rescanning.

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

c4deddb3c5

DrahtBot added the label Wallet on Aug 24, 2020

in src/wallet/walletdb.cpp:292 in 8e734a13d8 outdated

286@@ -286,7 +287,12 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
287             // LoadToWallet call below creates a new CWalletTx that fill_wtx
288             // callback fills with transaction metadata.
289             auto fill_wtx = [&](CWalletTx& wtx, bool new_tx) {
290-                assert(new_tx);
291+                if (!new_tx) {
292+                    // There's probably some corruption here since the tx we just tried to load was already in the wallet
293+                    // This error is recoverable with zapwallettxs and is not a major failure

MarcoFalke commented at 8:41 am on August 25, 2020:

Isn’t zapwallettxs about to be removed?

achow101 commented at 3:42 pm on August 25, 2020:

I guess here’s an argument to keep it.

The question is really whether it is possible to hit this error in normal operation.

MarcoFalke commented at 4:21 pm on August 25, 2020:

The test #19078 is hitting it. Does salvagewallet count as “normal operation”? If not, it should probably be removed.

achow101 commented at 4:33 pm on August 25, 2020:

Looking at it further, it is surprising to me that #19078 hits this error because it shouldn’t have any transactions left after the salvage. Salvage is supposed to only extract keys. This indicates that salvage has an additional bug somewhere.

achow101 commented at 4:43 pm on August 25, 2020:

Oh, the error is because ReadKeyValue is called by salvage so it’s happening during the salvage, rather than during the loading of the wallet post-salvage.

achow101 commented at 5:30 pm on August 25, 2020:

#19805 is a fix to #19078’s failure that sidesteps this problem entirely.

ryanofsky commented at 6:14 pm on August 25, 2020:

re: #19793 (review)

#19805 is a fix to #19078’s failure that sidesteps this problem entirely.

#19805 seems like a better way to fix the test failure. In case we want to merge this PR as well to remove the bad assert #19793#pullrequestreview-474262517, I updated the comment, just changing “There’s probably some corruption” to “There’s some corruption”

MarcoFalke commented at 8:42 am on August 25, 2020: member

Concept ACK on removing an assert that should only be used for code internal logic errors and not user/disk input validation

ryanofsky marked this as a draft on Aug 25, 2020

ryanofsky force-pushed on Aug 25, 2020

ryanofsky marked this as ready for review on Aug 25, 2020

ryanofsky commented at 6:53 pm on August 25, 2020: member

Updated 8e734a13d8d3f055740826a7c433ab8c988331fc -> c4deddb3c5806d524e71c344be083b8bd6ea50aa (pr/badsalv.1 -> pr/badsalv.2, compare) just tweaking the comment.

achow101 commented at 4:55 pm on August 27, 2020: member

ACK c4deddb3c5806d524e71c344be083b8bd6ea50aa

fanquake requested review from meshcollider on Aug 27, 2020

meshcollider commented at 9:37 am on October 7, 2020: contributor

utACK c4deddb3c5806d524e71c344be083b8bd6ea50aa

Very sorry I missed this, nice simple PR shouldn’t take over a month to review 😅

in src/wallet/walletdb.cpp:738 in c4deddb3c5

731@@ -726,7 +732,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
732             {
733                 // losing keys is considered a catastrophic error, anything else
734                 // we assume the user can live with:
735-                if (IsKeyType(strType) || strType == DBKeys::DEFAULTKEY) {
736+                if (wss.corrupt || IsKeyType(strType) || strType == DBKeys::DEFAULTKEY) {
737                     result = DBErrors::CORRUPT;
738                 } else if (strType == DBKeys::FLAGS) {
739                     // reading the wallet flags can only fail if unknown flags are present

MarcoFalke commented at 11:29 am on October 7, 2020:

should the other corruption reasons also latch to true?

wss.corrupt would never be set to false, so the error TOO_NEW is translated to the error CORRUPT (after at least one CORRUPT error).

With a lost key this doesn’t seem to happen and the error might be rewritten to TOO_NEW.

Probably doesn’t matter in practice, but I wanted to ask to be sure.

ryanofsky commented at 10:53 pm on October 21, 2020:

re: #19793 (review)

should the other corruption reasons also latch to true?

wss.corrupt would never be set to false, so the error TOO_NEW is translated to the error CORRUPT (after at least one CORRUPT error).

With a lost key this doesn’t seem to happen and the error might be rewritten to TOO_NEW.

Probably doesn’t matter in practice, but I wanted to ask to be sure.

Looking at the calling code, in practice the wallet will fail to load either way if TOO_NEW or CORRUPT is returned, only the error text will be different. It’s not even a new thing for the CORRUPT error to take priority over the TOO_NEW error since it can also happen in other the places result = DBErrors::CORRUPT is set below.

But one somewhat ugly side effect of this is it can make gArgs.SoftSetBoolArg("-rescan", true) line not get called consistency. It would really be better not to do this and for rescan status were passed back more directly, but that requires more changes outside the file.

ryanofsky commented at 11:04 pm on October 21, 2020: member

Closing this since it’s out of date and has some odd behaviors. Things it would be nice to do if someone wants to clean this code up:

Replace assert(new_tx) with error handling that triggers the CWallet::Create “Wallet Corrupted” error
Fix the “This error is recoverable with zapwallettxs” comment now that zapwallettxs is removed.
Make the TOO_NEW error take precedence over the CORRUPT error or enforce some other consistent precedence
Replace SoftSetBoolArg("-rescan", true) with a rescan_required return value so one wallet requiring a rescan doesn’t cause other wallets to be rescanned

ryanofsky closed this on Oct 21, 2020

in src/wallet/walletdb.cpp:735 in c4deddb3c5

731@@ -726,7 +732,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
732             {
733                 // losing keys is considered a catastrophic error, anything else
734                 // we assume the user can live with:
735-                if (IsKeyType(strType) || strType == DBKeys::DEFAULTKEY) {
736+                if (wss.corrupt || IsKeyType(strType) || strType == DBKeys::DEFAULTKEY) {

luke-jr commented at 2:04 pm on October 25, 2020:

A corrupt/missing transaction isn’t supposed to follow this path, but rather the one below which simply triggers a rescan.

ryanofsky commented at 6:44 pm on September 30, 2021: member

Closing this since it’s out of date and has some odd behaviors. Things it would be nice to do if someone wants to clean this code up:

Replace assert(new_tx) with error handling that triggers the CWallet::Create “Wallet Corrupted” error

Fix the “This error is recoverable with zapwallettxs” comment now that zapwallettxs is removed.

Make the TOO_NEW error take precedence over the CORRUPT error or enforce some other consistent precedence

Replace SoftSetBoolArg("-rescan", true) with a rescan_required return value so one wallet requiring a rescan doesn’t cause other wallets to be rescanned

1-3 seems to be handled by #23142 and 4 seems to be handled by #23123

laanwj referenced this in commit 46b4937bc1 on Oct 1, 2021

sidhujag referenced this in commit 946ad90881 on Oct 1, 2021

DrahtBot locked this on Oct 30, 2022

bitcoin-wallet salvage: Return false instead of asserting when a loaded tx isn’t new #19793