wallet: Enumerate walletdb keys #16475

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:1907-walletDbKeys changing 4 files +121 −113
  1. MarcoFalke commented at 8:00 PM on July 27, 2019: member

    It is nice to see all the keys that exists in a single enum

    Also, rename CWalletKey to OldKey and update the outdated documentation

  2. MarcoFalke force-pushed on Jul 27, 2019
  3. MarcoFalke force-pushed on Jul 27, 2019
  4. wallet: Enumerate walletdb keys fa6dc7fa5f
  5. wallet: Rename CWalletKey to OldKey fa6f22bf44
  6. MarcoFalke force-pushed on Jul 27, 2019
  7. MarcoFalke added the label Refactoring on Jul 27, 2019
  8. MarcoFalke added the label Wallet on Jul 27, 2019
  9. DrahtBot commented at 10:25 PM on July 27, 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:

    • #16451 (Remove CMerkleTx by jnewbery)
    • #16341 (Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)
    • #15931 (Remove GetDepthInMainChain dependency on locked chain interface by ariard)
    • #15709 (Do not add setting as unknown by Bushstar)
    • #9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

    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.

  10. fanquake requested review from achow101 on Jul 28, 2019
  11. fanquake requested review from meshcollider on Jul 28, 2019
  12. jnewbery commented at 7:14 PM on July 28, 2019: member

    Concept ACK. This seems like an improvement to code self-documentation.

    I think this would be even more helpful if the DBKeys namespace had documentation for each of the keys and what they're used for.

    We could go further and change this to an enum, which would ensure that only known keys are read/written, and would allow us to tidy the long sequence of if-else statments in ReadKeyValue() into a switch statement.

  13. in src/wallet/wallet.h:685 in fa6f22bf44 
     692 | -    explicit CWalletKey(int64_t nExpires=0);
 693 | -
 694 |      ADD_SERIALIZE_METHODS;
 695 |  
 696 |      template <typename Stream, typename Operation>
 697 |      inline void SerializationOp(Stream& s, Operation ser_action) {
    promag commented at 9:22 PM on July 28, 2019:

    fa6f22bf44c0f741285f27f27ac18e9679802e5e

    Could go further and just implement Unserialize?

    MarcoFalke commented at 12:36 PM on July 29, 2019:

    I kept the changes to a minimum. Happy to add a commit for this, if other think it is useful.

  14. promag commented at 9:35 PM on July 28, 2019: member

    ACK fa6f22bf44c0f741285f27f27ac18e9679802e5e. @jnewbery suggestions are great followups, I think this is good enough.

    I think you should change commit order because then it makes sense to name wkey as OLD_KEY.

  15. meshcollider commented at 12:58 PM on July 29, 2019: contributor

    utACK fa6f22bf44c0f741285f27f27ac18e9679802e5e

    Agree with John though, the PR title says enumerate so why not use an enum?

  16. MarcoFalke commented at 1:08 PM on July 29, 2019: member

    why not use an enum

    The underlying type on an enum in cpp can only be integer, not std::string.

    I could switch it to an enum and then add helper methods for std::string -> enum and enum -> std::string, but I don't see the benefit.

    We could go further and change this to an enum, which would ensure that only known keys are read/written

    I don't understand how this would prevent anyone from inlining a string literal (bypassing the enum).

  17. MarcoFalke commented at 1:11 PM on July 29, 2019: member

    and would allow us to tidy the long sequence of if-else statments in ReadKeyValue() into a switch statement.

    You'd still have to do the if-else statements elsewhere, so I don't see the benefit. I know a log(N) map lookup would also work, but I think the computational complexity shouldn't be a concern here.

  18. laanwj commented at 1:31 PM on July 29, 2019: member

    ACK fa6f22bf44c0f741285f27f27ac18e9679802e5e, I'm a big fan of this kind of change as it prevents typos, which can happen with 'magic' strings in the code.

  19. jnewbery commented at 1:50 PM on July 29, 2019: member

    I don't understand how this would prevent anyone from inlining a string literal (bypassing the enum).

    The idea would be to change WriteIC and EraseIC to take the enum rather than a string.

    Changing this to an enum was a suggested follow-up. This PR is a useful change by itself.

  20. in src/wallet/walletdb.cpp:409 in fa6dc7fa5f outdated 
     416 |                  strErr = "Error reading wallet database: Unknown non-tolerable wallet flags found";
 417 |                  return false;
 418 |              }
 419 | -        } else if (strType != "bestblock" && strType != "bestblock_nomerkle" &&
 420 | -                strType != "minversion" && strType != "acentry" && strType != "version") {
 421 | +        } else if (strType != DBKeys::BESTBLOCK && strType != DBKeys::BESTBLOCK_NOMERKLE &&
    achow101 commented at 1:58 PM on July 29, 2019:

    (maybe for a followup) I think it would be better to define a const set of the types that are unused but kept around to avoid triggering unknown record warnings. Then we could change this if to be checking membership in that set.

  21. in src/wallet/walletdb.cpp:429 in fa6dc7fa5f outdated 
     425 | @@ -431,8 +426,8 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
 426 |  
 427 |  bool WalletBatch::IsKeyType(const std::string& strType)
 428 |  {
 429 | -    return (strType== "key" || strType == "wkey" ||
 430 | -            strType == "mkey" || strType == "ckey");
 431 | +    return (strType == DBKeys::KEY || strType == DBKeys::OLD_KEY ||
    achow101 commented at 1:58 PM on July 29, 2019:

    (maybe for a followup) I think it would be better to define a const set of the types that are keys. Then we could change this function to be returning membership in that set.

  22. achow101 commented at 1:59 PM on July 29, 2019: member

    Code review ACK fa6f22bf44c0f741285f27f27ac18e9679802e5e

  23. MarcoFalke commented at 10:38 PM on July 29, 2019: member

    Thanks for the plethora of suggestions of follow-up refactorings. To not invalidate the ACKs and to not scope creep this change, which should be minimal, but still an improvement, I will leave them for later.

  24. fanquake approved
  25. fanquake commented at 3:35 AM on July 30, 2019: member

    ACK fa6f22bf44c0f741285f27f27ac18e9679802e5e - I had a quick look over, definitely prefer this to strings floating around everywhere.

    5 ACKs, including both the wallet maintainers, means I'll merge this now and all the follow ups can be done in a new PR. I like the idea of using a const set of types.

  26. fanquake merged this on Jul 30, 2019
  27. fanquake closed this on Jul 30, 2019
  28. fanquake referenced this in commit 478fe328a7 on Jul 30, 2019
  29. MarcoFalke deleted the branch on Jul 30, 2019
  30. jnewbery commented at 3:55 PM on July 30, 2019: member

    post-merge ACK fa6f22bf44c0f741285f27f27ac18e9679802e5e

    Potential follow-up PRs:

  31. kittywhiskers referenced this in commit a773a9df5a on Nov 6, 2021
  32. kittywhiskers referenced this in commit 663c4bf8da on Nov 30, 2021
  33. kittywhiskers referenced this in commit d8325fafa5 on Nov 30, 2021
  34. kittywhiskers referenced this in commit 1829d6771f on Nov 30, 2021
  35. kittywhiskers referenced this in commit f81625d5ee on Nov 30, 2021
  36. kittywhiskers referenced this in commit 7b1cb6346d on Dec 3, 2021
  37. kittywhiskers referenced this in commit 9dc421d219 on Dec 4, 2021
  38. kittywhiskers referenced this in commit 10e5ef7ea7 on Dec 5, 2021
  39. kittywhiskers referenced this in commit db3abe7d73 on Dec 6, 2021
  40. kittywhiskers referenced this in commit 849a50d35a on Dec 8, 2021
  41. kittywhiskers referenced this in commit bfda472302 on Dec 8, 2021
  42. kittywhiskers referenced this in commit 548e335742 on Dec 8, 2021
  43. kittywhiskers referenced this in commit 57ceaa4e36 on Dec 11, 2021
  44. kittywhiskers referenced this in commit 9744e60555 on Dec 12, 2021
  45. kittywhiskers referenced this in commit 64755d33ae on Dec 12, 2021
  46. kittywhiskers referenced this in commit fcab753598 on Dec 12, 2021
  47. kittywhiskers referenced this in commit d04345d335 on Dec 12, 2021
  48. kittywhiskers referenced this in commit 1ffca5e643 on Dec 12, 2021
  49. kittywhiskers referenced this in commit c88e060461 on Dec 12, 2021
  50. kittywhiskers referenced this in commit b9310fc427 on Dec 12, 2021
  51. kittywhiskers referenced this in commit 1f61b27399 on Dec 12, 2021
  52. UdjinM6 referenced this in commit 6af131f825 on Dec 12, 2021
  53. MarcoFalke locked this on Dec 16, 2021
Contributors
MarcoFalkeDrahtBotjnewberypromagmeshcolliderlaanwjachow101fanquake
Labels
RefactoringWallet
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 06:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me