Return false on corrupt tx rather than asserting

meshcollider commented at 1:38 am on September 30, 2021: contributor

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

meshcollider added the label Wallet on Sep 30, 2021

meshcollider requested review from ryanofsky on Sep 30, 2021

in src/wallet/walletdb.cpp:314 in bbc0f511bb outdated

310@@ -311,6 +311,7 @@ class CWalletScanState {
311     std::map<std::pair<uint256, CKeyID>, CKey> m_descriptor_keys;
312     std::map<std::pair<uint256, CKeyID>, std::pair<CPubKey, std::vector<unsigned char>>> m_descriptor_crypt_keys;
313     std::map<uint160, CHDChain> m_hd_chains;
314+    bool tx_corrupt;

practicalswift commented at 8:07 am on September 30, 2021:

0    bool tx_corrupt{false};

To avoid potential use of uninitialized memory when the logic reading/writing tx_corrupt is changed over time.

Return false on corrupt tx rather than asserting

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: Andrew Chow <achow101-github@achow101.com>

0ab4c3b272

in src/wallet/walletdb.cpp:349 in 0ab4c3b272

345@@ -345,7 +346,13 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
346             // LoadToWallet call below creates a new CWalletTx that fill_wtx
347             // callback fills with transaction metadata.
348             auto fill_wtx = [&](CWalletTx& wtx, bool new_tx) {
349-                assert(new_tx);
350+                if(!new_tx) {

jonatack commented at 10:18 am on September 30, 2021:

0                if (!new_tx) {

jonatack commented at 10:19 am on September 30, 2021: member

Concept/light code review ACK. A test might be good.

in src/wallet/walletdb.cpp:352 in 0ab4c3b272

345@@ -345,7 +346,13 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
346             // LoadToWallet call below creates a new CWalletTx that fill_wtx
347             // callback fills with transaction metadata.
348             auto fill_wtx = [&](CWalletTx& wtx, bool new_tx) {
349-                assert(new_tx);
350+                if(!new_tx) {
351+                    // There's some corruption here since the tx we just tried to load was already in the wallet.
352+                    // We don't consider this type of corruption critical, and can fix it by removing tx data and
353+                    // rescanning.

rajarshimaitra commented at 12:52 pm on September 30, 2021:

Probably I am missing context here, so feel free to ignore my question.

Why would we consider “the tx we just tried to load was already in the wallet.” as a corrupted transaction? If I am missing context here, would it be helpful to clarify this in the above comment?

ryanofsky commented at 6:37 pm on September 30, 2021:

Why would we consider “the tx we just tried to load was already in the wallet.” as a corrupted transaction? If I am missing context here, would it be helpful to clarify this in the above comment?

It might not be bad to update the comment above or the error message below or the tx_corrupt variable name if you have suggestions, but this all seems ok to me. If there are multiple transactions with the same hash, the wallet is corrupt, and the transactions are maybe corrupt. In this case it is probably better to not load the wallet than to load it and make new writes, and to return a loading error instead of asserting false.

rajarshimaitra commented at 7:05 pm on September 30, 2021:

Hmm. The context I was missing then would be: !new suggests that the wallet already loaded the transaction with the given hash.

So maybe the comment above could read something like “There’s some corruption here since the wallet already knows about a transaction with the same Txid”? That would clarify why this case is an error a little clearly.

ryanofsky approved

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

Code review ACK 0ab4c3b27265401c59e40adc494041927dc9dbe3. 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.

achow101 commented at 8:58 pm on September 30, 2021: member

ACK 0ab4c3b27265401c59e40adc494041927dc9dbe3

laanwj commented at 8:31 am on October 1, 2021: member

Very much agree with this change, assert is not an error-handling mechanism.

Code review ACK 0ab4c3b27265401c59e40adc494041927dc9dbe3

laanwj merged this on Oct 1, 2021

laanwj closed this on Oct 1, 2021

laanwj commented at 8:33 am on October 1, 2021: member

Would be good to have a test for this, by the way.

Oh I see jonatack brought this up too:

A test might be good.

Do we need a “needs test” label :smile:

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

DrahtBot locked this on Oct 30, 2022

Return false on corrupt tx rather than asserting #23142