Return false on corrupt tx rather than asserting #23142

pull meshcollider wants to merge 1 commits into bitcoin:master from meshcollider:202109_no_assert_corruption changing 1 files +13 −1
  1. 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.

  2. meshcollider added the label Wallet on Sep 30, 2021
  3. meshcollider requested review from ryanofsky on Sep 30, 2021
  4. 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.

  5. 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
  6. 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) {
    
  7. jonatack commented at 10:19 am on September 30, 2021: member
    Concept/light code review ACK. A test might be good.
  8. 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.

  9. ryanofsky approved
  10. 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.
  11. achow101 commented at 8:58 pm on September 30, 2021: member
    ACK 0ab4c3b27265401c59e40adc494041927dc9dbe3
  12. 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

  13. laanwj merged this on Oct 1, 2021
  14. laanwj closed this on Oct 1, 2021

  15. 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:

  16. sidhujag referenced this in commit 946ad90881 on Oct 1, 2021
  17. DrahtBot locked this on Oct 30, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 21:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me