wallet: guard and alert about a wallet invalid state during chain sync #25272

furszy · 2022-06-03T14:06:15Z

Follow-up work to my comment in #25239.

Guarding and alerting the user about a wallet invalid state during chain synchronization.

Explanation

if the AddToWallet tx write fails, the method returns a wtx nullptr without removing the recently added transaction from the wallet's map.

Which makes that AddToWalletIfInvolvingMe return false (even when the tx is on the wallet's map already), --> which makes SyncTransaction skip the MarkInputsDirty call --> which leads to a wallet invalid state where the inputs of this new transaction are not marked dirty, while the transaction that spends them still exist on the in-memory wallet tx map.

Plus, as we only store the arriving transaction inside AddToWalletIfInvolvingMe when we synchronize/scan block/s from the chain and nowhere else, it makes sense to treat the transaction db write error as a runtime error to notify the user about the problem. Otherwise, the user will lose all the not stored transactions after a wallet shutdown (without be able to recover them automatically on the next startup because the chain sync would be above the block where the txs arrived).

Note:
On purpose, the first commit adds test coverage for it. Showing how the wallet can end up in an invalid state. The second commit corrects it with the proposed solution.

furszy · 2022-06-03T15:53:26Z

Update:
Added test coverage for it in the first commit. Showing how the wallet can end up in the invalid state. The second commit corrects it with the proposed solution.

DrahtBot · 2022-06-04T15:58:07Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25297 (wallet: speedup transactions sync, rescan and load by grouping all independent db writes on a single batched db transaction by furszy)
#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.

jonatack · 2022-06-06T15:11:53Z

Concept ACK

ryanofsky

Code review ACK d15e9f7. Nice find and test, and clear description of the problem. This change will raise an explicit error in a failure case that was previously ignored, so in theory it could lead to worse outcomes in some cases. But status quo results in inconsistent in-memory state, and the error seems pretty unlikely to happen to begin with so this is probably the right move.

src/wallet/test/wallet_tests.cpp

ryanofsky · 2022-06-06T17:19:46Z

src/wallet/test/wallet_tests.cpp

+class FailDatabase : public WalletDatabase
+{
+public:
+    bool m_pass{true}; // false when this db should fail


In commit "test: add case for wallet invalid state where the inputs (prev-txs) of a new arriving transaction are not marked dirty, while the transaction that spends them exist inside the in-memory wallet tx map (not stored on db due a db write failure)." (742cd0b)

Default is true here, but false in the other class. Would suggest sticking choosing one default just to avoid confusion and having to remember which class has which default.

(Also maybe use a shorter commit subject, https://cbea.ms/git-commit/ has nice guidelines)

Default is true here, but false in the other class. Would suggest sticking choosing one default just to avoid confusion and having to remember which class has which default.

yeah, good catch. I wrote the test differently first, this is a remnant of the first approach.

(Also maybe use a shorter commit subject, https://cbea.ms/git-commit/ has nice guidelines)

awesome reading :), thanks!

furszy · 2022-06-06T19:31:29Z

Thanks for the feedback!

Applied the following changes:

Moved dynamic_cast to static_cast.
Set same default value for "m_pass" field in both classes FailDatabase and FailBatch.
Shorted the commit subject.

achow101

It's preferable that each commit passes the test suite. Currently the first commit fails as the code currently fails the test that you have added. So it would be preferable if that test were added after the fix is implemented.

src/wallet/test/wallet_tests.cpp

furszy · 2022-06-14T12:36:39Z

It's preferable that each commit passes the test suite. Currently the first commit fails as the code currently fails the test that you have added. So it would be preferable if that test were added after the fix is implemented.

Ok. I actually did that on purpose so it was easier for reviewers to replicate/understand the problem (I wrote it in the PR description); just compiling the first commit, see/verify the failure, then compile the fix and see it passing (Test Driven Approach).

At least for me, it make more sense (and feels more natural) than have tests always passing when we actually have problems in the sources.

But.. that is probably not a discussion for this PR, will just re-order the commits :).

furszy · 2022-06-14T12:56:44Z

Updated per achow101 feedback:

Re-ordered the commits, so test suite always passes.
Note about this change: now we only have the final version of the test. The one that fails due a runtime_error and not the intermediary one that was showing the wallet invalid state error (which, for reviewers, can still be found at furszy@c9c9466).

ryanofsky

Code review ACK 7027e94. Only changes since last review are internal changes to commits, and making suggested static_cast and m_pass default changes

src/wallet/wallet.cpp

achow101 · 2022-06-27T19:27:42Z

ACK 7027e94

-Context: If `AddToWallet` db write fails, the method returns a wtx nullptr without removing the recently added transaction from the wallet's map. -Problem: When a db write error occurs, `AddToWalletIfInvolvingMe` return false even when the tx is on the wallet's map already --> which makes `SyncTransaction` skip the `MarkInputsDirty` call --> which leads to a wallet invalid state where the inputs of this new transaction are not marked dirty, while the transaction that spends them still exist on the in-memory wallet tx map. Plus, as we only store arriving transaction inside `AddToWalletIfInvolvingMe` when we synchronize/scan blocks from the chain and nowhere else, it makes sense to treat the tx db write error as a runtime error to notify the user about the problem. Otherwise, the user will lose all the not stored transactions after a wallet shutdown (without be able to recover them automatically on the next startup because the chain sync would be above the block where the txs arrived).

When a transaction arrives, the wallet mark its inputs (prev-txs) as dirty. Clearing the wallet transaction cache, triggering a balance recalculation. If this does not happen due a db write error during `AddToWallet`, the wallet will be in an invalid state: The transaction that spends certain wallet UTXO will exist inside the in-memory wallet tx map, having the credit/debit calculated, while its inputs will still have the old cached data (like if them were never spent).

furszy · 2022-07-18T15:10:28Z

Sadly had to rebase it on master, got a silent merge conflict with the GetNewDestination changes.

Ready to go again.

achow101 · 2022-07-18T20:21:38Z

re-ACK 9e04cfa

jonatack

Quick review of the patch only LGTM with one question. Did not build, nor review/verify the passing/failing test yet, will do.

jonatack · 2022-07-19T11:31:19Z

src/wallet/wallet.cpp

+            if (!wtx) {
+                // Can only be nullptr if there was a db write error (missing db, read-only db or a db engine internal writing error).
+                // As we only store arriving transaction in this process, and we don't want an inconsistent state, let's throw an error.
+                throw std::runtime_error("DB error adding transaction to wallet, write failed");


Would it make sense to follow this existing error message format in src/wallet/wallet.cpp:2157, e.g. provide the function name?

throw std::runtime_error(std::string(__func__) + ": Wallet db error, transaction commit failed");

(modulo s/db/DB/)

jonatack · 2022-07-20T18:30:31Z

ACK 9e04cfa

modulo #25272 (comment) regarding the error message

furszy · 2022-07-25T15:23:40Z

Hey, thanks for the review.

about your point @jonatack:

Would it make sense to follow this existing error message format in src/wallet/wallet.cpp:2157, e.g. provide the function name?

Probably would make more sense to go into the opposite direction and change line 2157 to describe the context where the runtime error occurred properly. Something like "DB error committing transaction to wallet, write failed".

So if this edge case happens, and the runtime error string gets presented to the user before shutdown, the text is more descriptive than having the function name on it (which, looking it at the user's perspective, provides zero information).

Still, we could probably merge this PR as is now and tackle it in a short follow-up PR.

…tate du…

w0xlt

Post-merge ACK 9e04cfa

DrahtBot added the Wallet label Jun 3, 2022

furszy force-pushed the 2022_wallet_invalid_state branch from d964c9d to 712627a Compare June 3, 2022 15:47

furszy force-pushed the 2022_wallet_invalid_state branch from 712627a to d15e9f7 Compare June 4, 2022 14:14

DrahtBot mentioned this pull request Jun 4, 2022

[Draft / POC] Silent Payments #24897

Closed

furszy mentioned this pull request Jun 6, 2022

wallet: 'CommitTransaction', remove extra wtx lookup and add exception for db write error #25239

Merged

ryanofsky approved these changes Jun 6, 2022

View reviewed changes

furszy force-pushed the 2022_wallet_invalid_state branch 2 times, most recently from 097e911 to 4bd0d01 Compare June 6, 2022 19:26

DrahtBot mentioned this pull request Jun 8, 2022

wallet: group independent db writes on single batched db transaction #25297

Closed

achow101 reviewed Jun 13, 2022

View reviewed changes

src/wallet/test/wallet_tests.cpp Show resolved Hide resolved

furszy force-pushed the 2022_wallet_invalid_state branch from 4bd0d01 to 7027e94 Compare June 14, 2022 12:48

ryanofsky approved these changes Jun 15, 2022

View reviewed changes

luke-jr reviewed Jun 23, 2022

View reviewed changes

src/wallet/wallet.cpp Show resolved Hide resolved

furszy added 2 commits July 18, 2022 11:29

furszy force-pushed the 2022_wallet_invalid_state branch from 7027e94 to 9e04cfa Compare July 18, 2022 15:07

achow101 requested review from ryanofsky and jonatack July 18, 2022 20:21

jonatack reviewed Jul 19, 2022

View reviewed changes

achow101 merged commit de3c46c into bitcoin:master Aug 2, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 2, 2022

Merge bitcoin#25272: wallet: guard and alert about a wallet invalid s…

4d6eeb0

…tate du…

w0xlt reviewed Aug 2, 2022

View reviewed changes

furszy deleted the 2022_wallet_invalid_state branch May 27, 2023 01:50

bitcoin locked and limited conversation to collaborators May 26, 2024

wallet: guard and alert about a wallet invalid state during chain sync #25272

wallet: guard and alert about a wallet invalid state during chain sync #25272

Uh oh!

Conversation

furszy commented Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

Uh oh!

furszy commented Jun 3, 2022

Uh oh!

DrahtBot commented Jun 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonatack commented Jun 6, 2022

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky Jun 6, 2022 • 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!

furszy Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

furszy commented Jun 6, 2022

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy commented Jun 14, 2022

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Jun 27, 2022

Uh oh!

furszy commented Jul 18, 2022

Uh oh!

achow101 commented Jul 18, 2022

Uh oh!

jonatack 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!

jonatack Jul 19, 2022 • 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!

jonatack commented Jul 20, 2022

Uh oh!

furszy commented Jul 25, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy commented Jun 3, 2022 •

edited

Loading

DrahtBot commented Jun 4, 2022 •

edited

Loading

ryanofsky Jun 6, 2022 •

edited

Loading

furszy commented Jun 14, 2022 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack Jul 19, 2022 •

edited

Loading