[Wallet] Do not parse ssValue in CWalletDB::Recover #9101

pull pstratem wants to merge 1 commits into bitcoin:master from pstratem:2016-11-07-cwalletdb-recover changing 1 files +6 −17

pstratem commented at 1:23 AM on November 8, 2016: contributor

This removes the warning in debug.log when a value fails to parse correctly.

Removes the last dependency from walletdb.cpp on CWallet.

I'm not sure this is a good idea, but the circular dependency being removed finally is nice.

Do not parse ssValue in CWalletDB::Recover

This removes the warning in debug.log when a value fails to parse correctly.

Removes the last dependency from walletdb.cpp on CWallet.

6f8a07a7b8

pstratem force-pushed on Nov 8, 2016
MarcoFalke added the label Wallet on Nov 8, 2016

in src/wallet/walletdb.cpp:None in 6f8a07a7b8

 904 | -                continue;
 905 | -            }
 906 | +                ssKey >> strType;
 907 | +                if (!IsKeyType(strType) && strType != "hdchain")
 908 | +                    continue;
 909 | +            }catch(...){continue;}

laanwj commented at 10:42 AM on November 11, 2016:

Please add a warning when this fails, don't ignore failures here silently.

in src/wallet/walletdb.cpp:None in 6f8a07a7b8
```
 889 | -            string strType, strErr;
 890 | -            bool fReadOK;
 891 | -            {
 892 | -                // Required in LoadKeyMetadata():
 893 | -                LOCK(dummyWallet.cs_wallet);
 894 | -                fReadOK = ReadKeyValue(&dummyWallet, ssKey, ssValue,
```
jonasschnelli commented at 12:48 PM on November 11, 2016:

Not sure about this. Removing the actual deserialization routing may result in not actually checking the correctness of the data. For example, currently each "key" record (pub- & priv-key), is checked in ReadKeyValue() with vchPubKey.IsValid(), also, corrupt keys or a corrupt hdchain dataset will be detected now (log output) , but not after this PR.

pstratem commented at 3:07 AM on November 15, 2016:

@jonasschnelli indeed that is why I'm not sure it's a good idea.
sipa commented at 3:55 PM on April 9, 2017: member

Concept ACK. I don't think that the attempt to parse the data is a particularly strong check anyway.
fanquake commented at 10:47 AM on April 26, 2017: member

Needs a rebase/nits addressed.
mchrostowski commented at 2:12 AM on May 29, 2017: contributor

NAK Looks like the method this PR modified no longer operates in the same way. ReadKeyValue() is no longer called, instead we call some callback if it's registered.

It does not appear that this PR can be applied to the current code in any way. I believe it can be closed.
TheBlueMatt commented at 7:15 PM on September 29, 2017: member

This was superceeded by the split between db.cpp and walletdb.cpp - db.cpp already has no other wallet dependancies.
sipa closed this on Sep 29, 2017
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#9143 Refactor ZapWalletTxes to avoid layer violations