rpc, wallet: Expose database format in getwalletinfo #20125

1. 
   promag commented at 10:26 PM on October 11, 2020: member

   Support for sqlite based wallets was added in #19077. This PR adds the format key in getwalletinfo response, that can be bdb or sqlite.

2. fanquake added the label Wallet on Oct 11, 2020
3. 
   DrahtBot commented at 6:31 AM on October 12, 2020: 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:

   • #18904 (Don't call lsn_reset in periodic flush by bvbfan)

   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.

4. 
   achow101 commented at 3:04 PM on October 12, 2020: member

   Concept ACK

5. hebasto approved
6. 
   hebasto commented at 3:49 PM on October 12, 2020: member

   Approach ACK 56e1b81ef8efdcfe26b6fe22fea5354952afcead

   Mind making the new Format member function const?

7. 
   promag commented at 3:52 PM on October 12, 2020: member

   Mind making the new Format member function const? @ryanofsky fine by you?

8. rpc, wallet: Expose database format in getwalletinfo 5e737a0092
9. promag force-pushed on Oct 14, 2020
10. 
    jonatack commented at 10:18 PM on October 14, 2020: member

    Concept/Approach ACK, this was the first thing I wanted to see when testing #19077. Looks like it's only missing tests. Maybe also add it to -getinfo in single-wallet mode or when -rpcwallet= is passed.

11. 
    jonatack commented at 10:38 PM on October 14, 2020: member

    Tested ACK modulo test coverage

    RPC results

    $ ./src/bitcoin-cli -rpcwallet= getwalletinfo
    {
      "walletname": "",
      "walletversion": 169900,
      "format": "bdb",
    ...
    

    $ ./src/bitcoin-cli -rpcwallet=sqlite getwalletinfo
    {
      "walletname": "sqlite",
      "walletversion": 169900,
      "format": "sqlite",
    ...
    

    help

    Result:
    {                                         (json object)
      "walletname" : "str",                   (string) the wallet name
      "walletversion" : n,                    (numeric) the wallet version
      "format" : "str",                       (string) the database format (bdb or sqlite)
    ...
    

12. 
    promag commented at 12:39 AM on October 15, 2020: member

    @jonatack thanks for testing, will pick this after base is merged.

13. promag marked this as ready for review on Oct 15, 2020
14. promag force-pushed on Oct 15, 2020
15. 
    promag commented at 2:20 PM on October 15, 2020: member

    Rebased.

16. test: add coverage for getwalletinfo format field 624bab00dd
17. 
    jonatack commented at 2:49 PM on October 15, 2020: member

    Test coverage commit at https://github.com/jonatack/bitcoin/commits/test-getwalletinfo-format-field, feel free to pull in or use.

18. promag force-pushed on Oct 15, 2020
19. 
    promag commented at 3:01 PM on October 15, 2020: member

    Thanks @jonatack, included as is.

20. 
    jonatack commented at 3:10 PM on October 15, 2020: member

    Thanks @promag.

    Tested ACK 624bab00dd2cc8e2ebd77dc0a669bc8d507c3721

21. 
    jonatack commented at 3:14 PM on October 15, 2020: member

    Will need a release note here or added manually in https://github.com/bitcoin-core/bitcoin-devwiki/wiki.

22. hebasto approved
23. 
    hebasto commented at 4:35 PM on October 15, 2020: member

    ACK 624bab00dd2cc8e2ebd77dc0a669bc8d507c3721, tested on Linux Mint 20 (x86_64).

    Tested in the GUI console :)

    Mind making the new Format member function const?

    @ryanofsky fine by you?

    Didn't get what are concerns. :smiley:

24. 
    promag commented at 4:46 PM on October 15, 2020: member

    He previously said that he dislikes interfaces that enforce const. Probably not relevant in this case.

25. laanwj added this to the milestone 0.21.0 on Oct 15, 2020
26. in src/wallet/rpcwallet.cpp:2469 in 624bab00dd

    2465 | @@ -2465,6 +2466,7 @@ static RPCHelpMan getwalletinfo()
    2466 |      int64_t kp_oldest = pwallet->GetOldestKeyPoolTime();
    2467 |      obj.pushKV("walletname", pwallet->GetName());
    2468 |      obj.pushKV("walletversion", pwallet->GetVersion());
    2469 | +    obj.pushKV("format", pwallet->GetDatabase().Format());
    

    ---

    ---

    

    jonatack commented at 7:38 PM on October 15, 2020:

    Not sure, since it is related to the "descriptors" bool field, maybe place "format" at the end right after "descriptors". IDK.

    ---

    

    promag commented at 7:47 PM on October 15, 2020:

    For now it's related. But note that you can have descriptor wallets in bdb format.

    Actually you can have a bdb descriptors wallet if you use a build before #19077.

27. 
    laanwj commented at 9:52 AM on October 16, 2020: member

    I think this information is mildly useful for troubleshooting user issues, and for testing. Code review ACK 624bab00dd2cc8e2ebd77dc0a669bc8d507c3721.

28. 
    MarcoFalke commented at 9:56 AM on October 16, 2020: member

    How would it be possible to have a legacy-sqlite wallet or a descriptor-bdb wallet? This should only be useful for developers that ran master?

29. 
    MarcoFalke commented at 9:58 AM on October 16, 2020: member

    No objection obviously.

    doesn't hurt ACK 624bab00dd2cc8e2ebd77dc0a669bc8d507c3721

30. 
    meshcollider commented at 10:50 PM on October 19, 2020: contributor

    utACK 624bab00dd2cc8e2ebd77dc0a669bc8d507c3721

31. meshcollider merged this on Oct 19, 2020
32. meshcollider closed this on Oct 19, 2020

    ---

33. promag deleted the branch on Oct 19, 2020
34. sidhujag referenced this in commit b01329c2ec on Oct 19, 2020
35. DrahtBot locked this on Feb 15, 2022

Contributors

promagDrahtBotachow101hebastojonatacklaanwjMarcoFalkemeshcollider

Milestone
0.21.0