wallet: Drop unused OldKey #16502

pull promag wants to merge 1 commits into bitcoin:master from promag:2019-07-drop-oldkey changing 2 files +7 −33
  1. promag commented at 9:53 PM on July 30, 2019: member

    Replaces #16494, OldKey (previously CWalletKey) was never serialized in the code history which means that unserialization support is not required, so remove the code entirely.

  2. fanquake added the label Wallet on Jul 30, 2019
  3. achow101 commented at 10:56 PM on July 30, 2019: member

    ACK c1c0819a3d9fb6815dcda2a5f1680d704188dcb8

    Reviewed code. Looked through the git history and I could not find anywhere where wkey was written to the wallet, only where it was read. Thus I believe that it is safe to remove this and won't break compatibility with any wallets created using released versions.

  4. DrahtBot commented at 10:57 PM on July 30, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #16341 (Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)

    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.

  5. jnewbery commented at 11:32 PM on July 30, 2019: member

    Concept ACK

  6. laanwj commented at 6:03 AM on July 31, 2019: member

    I'm confused. So this "wkey" code has existed all the way from the beginning and was never used? That's a very interesting observation.

    One though though: please fail loading when you encounter one instead of ignoring it. So if someone (it's very unlikely, but still) happens to load a wallet with one of these old keys in it, this makes sure they get a compatibility error, instead of the keys silently disappearing.

  7. practicalswift commented at 7:25 AM on July 31, 2019: contributor

    Very nice find!

    Concept ACK assuming fail on encountering old key

  8. jonasschnelli commented at 7:50 AM on July 31, 2019: contributor

    Looks good and agree with @laanwj about that it should fail in case a wkey is found.

  9. fanquake added the label Waiting for author on Jul 31, 2019
  10. practicalswift commented at 9:29 AM on July 31, 2019: contributor

    utACK df577f7ed0ab06c756110060e01b5be73582f115 modulo squash

  11. in src/wallet/walletdb.cpp:427 in df577f7ed0 outdated 
     423 | @@ -428,7 +424,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
 424 |  
 425 |  bool WalletBatch::IsKeyType(const std::string& strType)
 426 |  {
 427 | -    return (strType == DBKeys::KEY || strType == DBKeys::OLD_KEY ||
 428 | +    return (strType == DBKeys::KEY ||
    promag commented at 1:19 PM on July 31, 2019:

    I could make this one line.

    fanquake commented at 3:54 AM on August 1, 2019:

    I think it's ok to leave this as is.

  12. in src/wallet/walletdb.cpp:259 in df577f7ed0 outdated 
     254 | @@ -256,7 +255,10 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
 255 |              ssValue >> fYes;
 256 |              if (fYes == '1')
 257 |                  pwallet->LoadWatchOnly(script);
 258 | -        } else if (strType == DBKeys::KEY || strType == DBKeys::OLD_KEY) {
 259 | +        } else if (strType == DBKeys::OLD_KEY) {
 260 | +            strErr = "Found unsupported record, try loading with version 0.18";
    promag commented at 1:19 PM on July 31, 2019:

    Is the error message OK?

    jnewbery commented at 1:56 PM on July 31, 2019:

    You could add the name of the unsupported record:

    strErr = "Found unsupported 'wkey' record, try loading with version 0.18";

    You could even deserialize the pubkey and print that, but it seems unnecessary.

    You might also want to move this if else block to the end, immediately before:

            } else if (strType != DBKeys::BESTBLOCK && strType != DBKeys::BESTBLOCK_NOMERKLE &&

    (intuitively it seems better to have unlikely error conditions handled at the end, rather than mixed with the mainline logic).

    promag commented at 5:36 PM on July 31, 2019:

    Done and done, thanks ;)

  13. promag commented at 1:22 PM on July 31, 2019: member

    I'll just wait for more feedback before squashing.

  14. jnewbery commented at 2:42 PM on July 31, 2019: member

    utACK df577f7ed0ab06c756110060e01b5be73582f115

    Suggested changes inline.

  15. wallet: Drop unused OldKey 0b1f4b3c66
  16. promag force-pushed on Jul 31, 2019
  17. promag commented at 5:37 PM on July 31, 2019: member

    Squashed.

  18. laanwj approved
  19. laanwj commented at 6:00 PM on July 31, 2019: member

    ACK 0b1f4b3c6685d0a6307926d43d166add538061b7

  20. jnewbery commented at 7:44 PM on July 31, 2019: member

    ACK 0b1f4b3c6685d0a6307926d43d166add538061b7

  21. fanquake removed the label Waiting for author on Jul 31, 2019
  22. fanquake approved
  23. fanquake commented at 3:55 AM on August 1, 2019: member

    ACK 0b1f4b3c6685d0a6307926d43d166add538061b7

  24. fanquake merged this on Aug 1, 2019
  25. fanquake closed this on Aug 1, 2019
  26. fanquake referenced this in commit b7fbf74b98 on Aug 1, 2019
  27. kittywhiskers referenced this in commit 0a6999f491 on Nov 6, 2021
  28. kittywhiskers referenced this in commit 160f8854ba on Nov 30, 2021
  29. kittywhiskers referenced this in commit fbb7161d54 on Nov 30, 2021
  30. kittywhiskers referenced this in commit 5782b24891 on Nov 30, 2021
  31. kittywhiskers referenced this in commit 3226d86d84 on Nov 30, 2021
  32. kittywhiskers referenced this in commit d01d124b00 on Dec 3, 2021
  33. kittywhiskers referenced this in commit 9572a7f8b1 on Dec 4, 2021
  34. kittywhiskers referenced this in commit 99e370f776 on Dec 5, 2021
  35. kittywhiskers referenced this in commit b65871367c on Dec 6, 2021
  36. kittywhiskers referenced this in commit d51358bf48 on Dec 8, 2021
  37. kittywhiskers referenced this in commit f5dd5eb436 on Dec 8, 2021
  38. kittywhiskers referenced this in commit 48ba294f55 on Dec 8, 2021
  39. kittywhiskers referenced this in commit c6650bbab9 on Dec 11, 2021
  40. kittywhiskers referenced this in commit d2b68964a3 on Dec 12, 2021
  41. kittywhiskers referenced this in commit 251c114375 on Dec 12, 2021
  42. kittywhiskers referenced this in commit 45f3ecd779 on Dec 12, 2021
  43. kittywhiskers referenced this in commit 6588122bc5 on Dec 12, 2021
  44. kittywhiskers referenced this in commit 891a9677a6 on Dec 12, 2021
  45. kittywhiskers referenced this in commit 7419f18be8 on Dec 12, 2021
  46. kittywhiskers referenced this in commit b14aa7460f on Dec 12, 2021
  47. kittywhiskers referenced this in commit f2af35c92e on Dec 12, 2021
  48. MarcoFalke locked this on Dec 16, 2021
Contributors
promagachow101DrahtBotjnewberylaanwjpracticalswiftjonasschnellifanquake
Labels
Wallet
Linked (view graph)
#16494 wallet: Drop support to serialize OldKey
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: 2026-04-17 09:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me