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

ryanofsky · 2020-08-24T20:06:26Z

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.

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 <[email protected]>

maflcko

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

src/wallet/walletdb.cpp

ryanofsky

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

src/wallet/walletdb.cpp

achow101 · 2020-08-27T16:55:42Z

ACK c4deddb

meshcollider

utACK c4deddb

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

maflcko · 2020-10-07T11:29:09Z

src/wallet/walletdb.cpp

+                if (wss.corrupt || IsKeyType(strType) || strType == DBKeys::DEFAULTKEY) {
                    result = DBErrors::CORRUPT;
                } else if (strType == DBKeys::FLAGS) {
                    // reading the wallet flags can only fail if unknown flags are present


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.

re: #19793 (comment)

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

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 · 2020-10-21T22:53:30Z

src/wallet/walletdb.cpp

+                if (wss.corrupt || IsKeyType(strType) || strType == DBKeys::DEFAULTKEY) {
                    result = DBErrors::CORRUPT;
                } else if (strType == DBKeys::FLAGS) {
                    // reading the wallet flags can only fail if unknown flags are present


re: #19793 (comment)

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.

luke-jr · 2020-10-25T14:04:17Z

src/wallet/walletdb.cpp

                // losing keys is considered a catastrophic error, anything else
                // we assume the user can live with:
-                if (IsKeyType(strType) || strType == DBKeys::DEFAULTKEY) {
+                if (wss.corrupt || IsKeyType(strType) || strType == DBKeys::DEFAULTKEY) {


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

ryanofsky · 2021-09-30T18:44:55Z

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

0ab4c3b Return false on corrupt tx rather than asserting (Samuel Dobson) Pull request description: Takes up #19793 Rather than asserting, we log an error and return CORRUPT so that the user is informed. This type of error isn't critical so it isn't worth `assert`ing. ACKs for top commit: achow101: ACK 0ab4c3b laanwj: Code review ACK 0ab4c3b ryanofsky: Code review ACK 0ab4c3b. There may be room for more improvements later like better error messages or easier recovery options, but changing from an assert to an error seems like a clear improvement, and this seems to avoid all the pitfalls of the last PR that tried this. Tree-SHA512: 4a1a412e7c473d176c4e09123b85f390a6b0ea195e78d28ebd50b13814b7852f8225a172511a2efb6affb555b11bd4e667c19eb8c78b060c5444b62f0fae5f7a

0ab4c3b Return false on corrupt tx rather than asserting (Samuel Dobson) Pull request description: Takes up bitcoin#19793 Rather than asserting, we log an error and return CORRUPT so that the user is informed. This type of error isn't critical so it isn't worth `assert`ing. ACKs for top commit: achow101: ACK 0ab4c3b laanwj: Code review ACK 0ab4c3b ryanofsky: Code review ACK 0ab4c3b. There may be room for more improvements later like better error messages or easier recovery options, but changing from an assert to an error seems like a clear improvement, and this seems to avoid all the pitfalls of the last PR that tried this. Tree-SHA512: 4a1a412e7c473d176c4e09123b85f390a6b0ea195e78d28ebd50b13814b7852f8225a172511a2efb6affb555b11bd4e667c19eb8c78b060c5444b62f0fae5f7a

ryanofsky mentioned this pull request Aug 24, 2020

wallet: Cleanup wallettool salvage and walletdb extraneous declarations #19457

Merged

DrahtBot added the Wallet label Aug 24, 2020

maflcko reviewed Aug 25, 2020

View reviewed changes

src/wallet/walletdb.cpp Show resolved Hide resolved

ryanofsky marked this pull request as draft August 25, 2020 16:37

achow101 mentioned this pull request Aug 25, 2020

test: Extend salvage test for wallet tool #19078

Closed

ryanofsky force-pushed the pr/badsalv branch from 8e734a1 to c4deddb Compare August 25, 2020 18:40

ryanofsky marked this pull request as ready for review August 25, 2020 18:41

ryanofsky commented Aug 25, 2020

View reviewed changes

src/wallet/walletdb.cpp Show resolved Hide resolved

fanquake requested a review from meshcollider August 27, 2020 22:36

meshcollider reviewed Oct 7, 2020

View reviewed changes

maflcko reviewed Oct 7, 2020

View reviewed changes

ryanofsky commented Oct 21, 2020

View reviewed changes

ryanofsky closed this Oct 21, 2020

luke-jr reviewed Oct 25, 2020

View reviewed changes

meshcollider mentioned this pull request Sep 30, 2021

Return false on corrupt tx rather than asserting #23142

Merged

bitcoin locked and limited conversation to collaborators Oct 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

ryanofsky commented Aug 24, 2020 •

edited

Loading

Uh oh!

maflcko left a comment

Uh oh!

Uh oh!

ryanofsky left a comment

Uh oh!

Uh oh!

achow101 commented Aug 27, 2020

Uh oh!

meshcollider left a comment •

edited

Loading

Uh oh!

maflcko Oct 7, 2020

Uh oh!

ryanofsky Oct 21, 2020

Uh oh!

ryanofsky left a comment

Uh oh!

ryanofsky Oct 21, 2020

Uh oh!

luke-jr Oct 25, 2020

Uh oh!

ryanofsky commented Sep 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

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

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

Uh oh!

Conversation

ryanofsky commented Aug 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Aug 27, 2020

Uh oh!

meshcollider left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

luke-jr Oct 25, 2020

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Sep 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ryanofsky commented Aug 24, 2020 •

edited

Loading

meshcollider left a comment •

edited

Loading