rpc, wallet: Expose database format in getwalletinfo by promag · Pull Request #20125 · bitcoin/bitcoin

promag · 2020-10-11T22:26:36Z

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

DrahtBot · 2020-10-12T06:31:42Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Don't call lsn_reset in periodic flush #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.

achow101 · 2020-10-12T15:04:06Z

Concept ACK

hebasto

Approach ACK 56e1b81ef8efdcfe26b6fe22fea5354952afcead

Mind making the new Format member function const?

promag · 2020-10-12T15:52:51Z

Mind making the new Format member function const?

@ryanofsky fine by you?

jonatack · 2020-10-14T22:18:22Z

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.

jonatack

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)
...

promag · 2020-10-15T00:39:53Z

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

promag · 2020-10-15T14:20:42Z

Rebased.

jonatack · 2020-10-15T14:49:21Z

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

promag · 2020-10-15T15:01:32Z

Thanks @jonatack, included as is.

jonatack · 2020-10-15T15:10:08Z

Thanks @promag.

Tested ACK 624bab0

jonatack · 2020-10-15T15:14:19Z

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

hebasto

ACK 624bab0, 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. 😃

promag · 2020-10-15T16:46:00Z

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

jonatack · 2020-10-15T19:38:27Z

src/wallet/rpcwallet.cpp

    int64_t kp_oldest = pwallet->GetOldestKeyPoolTime();
    obj.pushKV("walletname", pwallet->GetName());
    obj.pushKV("walletversion", pwallet->GetVersion());
+    obj.pushKV("format", pwallet->GetDatabase().Format());


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

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.

laanwj · 2020-10-16T09:52:44Z

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

maflcko · 2020-10-16T09:56:54Z

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?

maflcko · 2020-10-16T09:58:42Z

No objection obviously.

doesn't hurt ACK 624bab0

meshcollider

utACK 624bab0

…info 624bab0 test: add coverage for getwalletinfo format field (Jon Atack) 5e737a0 rpc, wallet: Expose database format in getwalletinfo (João Barbosa) Pull request description: Support for sqlite based wallets was added in bitcoin#19077. This PR adds the `format` key in `getwalletinfo` response, that can be `bdb` or `sqlite`. ACKs for top commit: jonatack: Tested ACK 624bab0 laanwj: Code review ACK 624bab0. MarcoFalke: doesn't hurt ACK 624bab0 hebasto: ACK 624bab0, tested on Linux Mint 20 (x86_64). meshcollider: utACK 624bab0 Tree-SHA512: a81f8530f040f6381d33e073a65f281993eccfa717424ab6e651c1203cbaf27794dcb7175570459e7fdaa211565bc060d0a3ecbe70d2b6f9c49b8d5071e4441c

fanquake added the Wallet label Oct 11, 2020

hebasto approved these changes Oct 12, 2020

View reviewed changes

This was referenced Oct 12, 2020

Wallet: remove db mode string #20130

Merged

Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks #19502

Merged

rpc, wallet: Expose database format in getwalletinfo

5e737a0

promag force-pushed the 2020-10-walletinfoformat branch from 56e1b81 to 5e737a0 Compare October 14, 2020 20:48

jonatack reviewed Oct 14, 2020

View reviewed changes

promag marked this pull request as ready for review October 15, 2020 14:17

promag force-pushed the 2020-10-walletinfoformat branch from 5e737a0 to f33d6e1 Compare October 15, 2020 14:18

test: add coverage for getwalletinfo format field

624bab0

promag force-pushed the 2020-10-walletinfoformat branch from 1083e8c to 624bab0 Compare October 15, 2020 15:00

hebasto approved these changes Oct 15, 2020

View reviewed changes

laanwj added this to the 0.21.0 milestone Oct 15, 2020

jonatack reviewed Oct 15, 2020

View reviewed changes

meshcollider reviewed Oct 19, 2020

View reviewed changes

meshcollider merged commit f5bd46a into bitcoin:master Oct 19, 2020

promag deleted the 2020-10-walletinfoformat branch October 19, 2020 23:42

maflcko mentioned this pull request Oct 21, 2020

Show name, format and if uses descriptors in bitcoin-wallet tool #20198

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

promag commented Oct 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

achow101 commented Oct 12, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Oct 12, 2020

Uh oh!

jonatack commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Oct 15, 2020

Uh oh!

promag commented Oct 15, 2020

Uh oh!

jonatack commented Oct 15, 2020

Uh oh!

promag commented Oct 15, 2020

Uh oh!

jonatack commented Oct 15, 2020

Uh oh!

jonatack commented Oct 15, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Oct 15, 2020

Uh oh!

jonatack Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 16, 2020

Uh oh!

maflcko commented Oct 16, 2020

Uh oh!

maflcko commented Oct 16, 2020

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

promag commented Oct 11, 2020 •

edited

Loading

DrahtBot commented Oct 12, 2020 •

edited

Loading

jonatack commented Oct 14, 2020 •

edited

Loading

jonatack Oct 15, 2020 •

edited

Loading

promag Oct 15, 2020 •

edited

Loading