Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rpc, wallet: Expose database format in getwalletinfo #20125

Merged
merged 2 commits into from Oct 19, 2020
Merged

rpc, wallet: Expose database format in getwalletinfo #20125

merged 2 commits into from Oct 19, 2020

Conversation

promag
Copy link
Member

@promag promag commented Oct 11, 2020 •
edited

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
Copy link
Contributor

DrahtBot commented Oct 12, 2020 •
edited

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

This was referenced Oct 12, 2020
Closed
Merged
Closed
Merged
Merged
Closed
Merged
Closed
Merged
@achow101
Copy link
Member

Concept ACK

hebasto
hebasto approved these changes Oct 12, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK 56e1b81

Mind making the new Format member function const?

@promag
Copy link
Member Author

promag commented Oct 12, 2020

Mind making the new Format member function const?

@ryanofsky fine by you?

This was referenced Oct 12, 2020
Merged
Merged
@jonatack
Copy link
Contributor

jonatack commented Oct 14, 2020 •
edited

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
jonatack reviewed Oct 14, 2020
View reviewed changes
Copy link
Contributor

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

promag commented Oct 15, 2020

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

@promag promag marked this pull request as ready for review October 15, 2020 14:17
@promag
Copy link
Member Author

promag commented Oct 15, 2020

Rebased.

@jonatack
Copy link
Contributor

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

@promag
Copy link
Member Author

promag commented Oct 15, 2020

Thanks @jonatack, included as is.

@jonatack
Copy link
Contributor

Thanks @promag.

Tested ACK 624bab0

@jonatack
Copy link
Contributor

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

hebasto
hebasto approved these changes Oct 15, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

promag commented Oct 15, 2020

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

@laanwj laanwj added this to the 0.21.0 milestone Oct 15, 2020
jonatack
jonatack reviewed Oct 15, 2020
View reviewed changes
src/wallet/rpcwallet.cpp
@@ -2465,6 +2466,7 @@ static RPCHelpMan getwalletinfo()
int64_t kp_oldest = pwallet->GetOldestKeyPoolTime();
obj.pushKV("walletname", pwallet->GetName());
obj.pushKV("walletversion", pwallet->GetVersion());
obj.pushKV("format", pwallet->GetDatabase().Format());
Copy link
Contributor

@jonatack jonatack Oct 15, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@promag promag Oct 15, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

laanwj commented Oct 16, 2020

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

@maflcko
Copy link
Member

maflcko commented Oct 16, 2020

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
Copy link
Member

maflcko commented Oct 16, 2020

No objection obviously.

doesn't hurt ACK 624bab0

meshcollider
meshcollider reviewed Oct 19, 2020
View reviewed changes
Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 624bab0

@meshcollider meshcollider merged commit f5bd46a into bitcoin:master Oct 19, 2020
@promag promag deleted the 2020-10-walletinfoformat branch October 19, 2020 23:42
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 19, 2020
@meshcollider
Merge bitcoin#20125: rpc, wallet: Expose database format in getwallet…
b01329c 
…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
@maflcko maflcko mentioned this pull request Oct 21, 2020
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonatack jonatack jonatack left review comments

@meshcollider meshcollider meshcollider left review comments

@hebasto hebasto hebasto approved these changes

Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
0.21.0
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@promag @DrahtBot @achow101 @jonatack @laanwj @maflcko @meshcollider @hebasto @fanquake