/bitcoin

[wallet] [tests] Add listwallets RPC, include wallet name in `getwalletinfo` and add multiwallet test #10604

Merged
merged 4 commits into from Jul 21, 2017
+61 −11

Conversation

Reviewers

@jonasschnelli jonasschnelli

@morcos morcos

@ryanofsky ryanofsky

@gmaxwell gmaxwell

Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
  0.15.0
10 participants
@jnewbery @ryanofsky @laanwj @instagibbs @jonasschnelli @morcos @sipa @MarcoFalke @gmaxwell @fanquake
@jnewbery
Member

jnewbery commented Jun 15, 2017 • edited

  • fix comment for CWallet::Verify (cleanup after #8694)
  • expose the wallet name in getwalletinfo rpc
  • add listwallets rpc - returns array of wallet names
  • add functional test for multiwallet using new rpc functionality
@ryanofsky
Contributor

ryanofsky commented Jun 15, 2017

It's good to add the wallet name to getwalletinfo but I don't see a reason to break backwards compatibility by returning an array in place of the previous wallet info object.

I thought the approach we were going to take for multiwallet RPC support was to add some kind of listwallets API that would return wallet names, and some kind of RPC session variable that would control which wallet is returned by GetWalletForJSONRPCRequest and used by all current wallet apis (including getwalletinfo).
@ryanofsky
Contributor

ryanofsky commented Jun 15, 2017 • edited

Jonas had a suggestion about how we could use different RPC endpoints to access individual wallets here: #8694 (comment)

Also more discussion in #8788
@jnewbery
Member

jnewbery commented Jun 15, 2017

Jonas had a suggestion about how we could use different RPC endpoints to access individual wallets

Sounds good, although I don't think that's relevant for this PR, since it doesn't touch any RPCs that are specific to a single wallet.

I thought the approach we were going to take for multiwallet RPC support was to add some kind of listwallets API

I missed that discussion (and can't find mention of listwallets in the PRs you've linked to). Can you point me at where this was discussed? It shouldn't be difficult to change this PR to implement a new listwallets RPC and use that in the test if that's what people want.
@ryanofsky
Contributor

ryanofsky commented Jun 15, 2017

Sounds good, although I don't think that's relevant for this PR, since it doesn't touch any RPCs that are specific to a single wallet.

Deriving the wallet from the RPC endpoint is relevant to this PR if you want getwalletinfo to be able to return information about all wallets without changing it to return an array.

I missed that discussion (and can't find mention of listwallets in the PRs you've linked to). Can you point me at where this was discussed? It shouldn't be difficult to change this PR to implement a new listwallets RPC and use that in the test if that's what people want.

There's no discussion about listwallets, I was just suggesting that as an alternative to making getwalletinfo return an array.

fanquake added the Wallet label Jun 16, 2017

@laanwj
Owner

laanwj commented Jun 22, 2017

It's good to add the wallet name to getwalletinfo but I don't see a reason to break backwards compatibility by returning an array in place of the previous wallet info object.
I thought the approach we were going to take for multiwallet RPC support was to add some kind of listwallets API that would return wallet names, and some kind of RPC session variable that would control which wallet is returned by GetWalletForJSONRPCRequest and used by all current wallet apis (including getwalletinfo).

I agree - last meeting we decided on using RPC endpoints to distinguish wallets. This means that the current wallet RPCs, including getwalletinfo, will take derive the wallet to be use from this. Adding the wallet name to the current API makes sense, of course.

Also adding a new listwallets RPC on a global (not per-wallet) level makes sense. This will keep backward compatibility but make it possible to list the currently loaded wallets.
@jnewbery
Member

jnewbery commented Jun 27, 2017

@ryanofsky @laanwj - thanks for the useful feedback on the API here.

I've updated this PR to add a new listwallets RPC, which simply lists the names of the currently loaded wallets. The command intentionally shows no information about the wallet. If/when we add multi-user multiwallet functionality, the listwallet RPC should not reveal the contents of the individual wallets.

The PR still adds walletname to the output of the getwalletinfo RPC.

Also rebased on master.

jnewbery changed the title from Expose multiwallet in getwalletinfo and add multiwallet test to Add listwallets RPC and add multiwallet test Jun 27, 2017

jnewbery referenced this pull request Jun 28, 2017

Closed

Simple, backwards compatible RPC multiwallet support (superseded by #10829) #10653

jnewbery changed the title from Add listwallets RPC and add multiwallet test to [wallet] [tests] Add listwallets RPC and add multiwallet test Jun 30, 2017

jnewbery referenced this pull request Jul 3, 2017

Closed

getwalletinfo: return path for wallet file #10733

@instagibbs
Member

instagibbs commented Jul 3, 2017

will review

jnewbery changed the title from [wallet] [tests] Add listwallets RPC and add multiwallet test to [wallet] [tests] Add listwallets RPC, include wallet name in `getwalletinfo` and add multiwallet test Jul 4, 2017

@jnewbery
Member

jnewbery commented Jul 4, 2017

rebased

jnewbery referenced this pull request Jul 4, 2017

Open

[WIP] [wallet] dynamic loading/unloading of wallets #10740

@instagibbs
Member

instagibbs commented Jul 5, 2017

API makes sense to me

tACK 74882ca
@jonasschnelli
Member

jonasschnelli commented Jul 5, 2017

Looks good.
I'm not sure if we should call it wallet_name on the RPC layer. It's actually the wallet_filename. Maybe we never do, but there is a chance that we once allow to give wallets a name (like other multiwallet software) and not sure if we want the filename char limits there.
@jnewbery
Member

jnewbery commented Jul 5, 2017

I'm not sure if we should call it wallet_name on the RPC layer

Yes, good point to bring up. I see there was some discussion of this here: #10650 (comment) . My personal opinion is that wallet name should always equal filename to avoid confusion, but I'm happy to discuss if other people have different ideas. Perhaps one to bring up in tomorrow's meeting?

ryanofsky referenced this pull request Jul 12, 2017

Open

Multiwallet: add RPC endpoint support #10650

@gmaxwell

gmaxwell approved these changes Jul 12, 2017
View changes

utACK

src/wallet/wallet.h
@@ -1053,7 +1053,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
//! Flush wallet (bitdb flush)
void Flush(bool shutdown=false);
- //! Verify the wallet database and perform salvage if required
+ //! Responsible for reading and validating the -wallet arguments and verifying the wallet database.
+ // This function will perform salvage on the wallet if requested, as long as only one wallet is
@morcos

morcos Jul 12, 2017

Contributor

ultranit: extra white space

@jnewbery

jnewbery Jul 13, 2017

Member

Thanks. fixed

src/wallet/rpcwallet.cpp
+ "For full information on the wallet, use \"getwalletinfo\"\n"
+ "\nResult:\n"
+ "[\n"
+ " \"walletname\": xxxxx, (string) the wallet name\n"
@morcos

morcos Jul 12, 2017

Contributor

it just prints the wallet name, not a pair

@jnewbery

jnewbery Jul 13, 2017

Member

quite right. Fixed

src/wallet/rpcwallet.cpp
+ "For full information on the wallet, use \"getwalletinfo\"\n"
+ "\nResult:\n"
+ "[\n"
+ " \"walletname\": xxxxx, (string) the wallet name\n"
@ryanofsky

ryanofsky Jul 12, 2017

Contributor

Should drop the :xxxx, here since an array is just a list of items, not key/value pairs.

@jnewbery

jnewbery Jul 13, 2017

Member

yup. Fixed

src/wallet/rpcwallet.cpp
+ + HelpExampleRpc("listwallets", "")
+ );
+
+ LOCK(cs_main);
@ryanofsky

ryanofsky Jul 12, 2017

Contributor

Probably should drop the cs_main lock. I don't think we decided what lock will be used to guard the vpwallets array, and no lock is used other places (it's not needed since the list doesn't change after initialization) so it would be inconsistent to add this here.

@jnewbery

jnewbery Jul 13, 2017

Member

Done. Thanks!

@ryanofsky
Contributor

ryanofsky commented Jul 12, 2017

Also, should maybe clean up the PR description (remove all caps "EDITED") now that #10786 is merged.
@jnewbery
Member

jnewbery commented Jul 13, 2017

@ryanofsky's and @morcos's comments addressed
@morcos
Contributor

morcos commented Jul 13, 2017

ACK 011a860

will be awesome to show people all the wallets they can't access!
@instagibbs
Member

instagibbs commented Jul 13, 2017

re-ACK 011a860
@jnewbery
Member

jnewbery commented Jul 13, 2017

This conflicts (slightly) with #10650. I have a branch of this rebased on top of that, so if both are going in to v0.15, #10650 can go in first and I'll push the rebased branch.
@jnewbery
Member

jnewbery commented Jul 14, 2017

Rebased on the latest #10650
@instagibbs
Member

instagibbs commented Jul 14, 2017

re-re-ACK 5baca3a

jnewbery added some commits Jun 15, 2017

@jnewbery jnewbery [wallet] [rpc] print wallet name in getwalletinfo 4a05715
@jnewbery jnewbery [wallet] fix comment for CWallet::Verify() 09eacee
@jnewbery jnewbery [wallet] [rpc] Add listwallets RPC 
This commit adds a listwallets RPC, which lists the names of the
currently loaded wallets. This command intentionally shows no
information about the wallet other then the name. Information on
individual wallets can be obtained using the getwalletinfo RPC.
 9508761
@jnewbery jnewbery [wallet] [tests] Add listwallets to multiwallet test
3707fcd
@jnewbery
Member

jnewbery commented Jul 20, 2017

rebased on master now that we have the endpoint PR merged.

@laanwj: Any chance of tagging this 0.15? It has 4 ACKs pre-rebase. I think it's useful multiwallet management functionality.

laanwj added this to the 0.15.0 milestone Jul 20, 2017

@sipa
Owner

sipa commented Jul 20, 2017

utACK 3707fcd
@instagibbs
Member

instagibbs commented Jul 21, 2017

test failing:
assert_raises_jsonrpc(-13, "Please enter the wallet passphrase with walletpassphrase first", self.nodes[0].dumpprivkey, address)
File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 87, in assert_raises_jsonrpc
raise AssertionError("No exception raised")

from walletencryption.py
@jnewbery
Member

jnewbery commented Jul 21, 2017

bah! Unrelated intermittent travis failure. Job restarted.
src/wallet/rpcwallet.cpp
+ UniValue obj(UniValue::VARR);
+
+ for (CWalletRef pwallet : vpwallets) {
+
@jonasschnelli

jonasschnelli Jul 21, 2017

Member

nit newline

@jnewbery

jnewbery Jul 21, 2017

Member

Yes, agree, but in the interests of getting this merged, I won't change this now that 3707fcd has two ACKs

@jonasschnelli
Member

jonasschnelli commented Jul 21, 2017

utACK 3707fcd
@instagibbs
Member

instagibbs commented Jul 21, 2017

ACK 3707fcd

@laanwj laanwj merged commit 3707fcd into bitcoin:master Jul 21, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@laanwj laanwj added a commit that referenced this pull request Jul 21, 2017

@laanwj laanwj Merge #10604: [wallet] [tests] Add listwallets RPC, include wallet na… 
…me in `getwalletinfo` and add multiwallet test


3707fcd [wallet] [tests] Add listwallets to multiwallet test (John Newbery)
9508761 [wallet] [rpc] Add listwallets RPC (John Newbery)
4a05715 [wallet] [rpc] print wallet name in getwalletinfo (John Newbery)
09eacee [wallet] fix comment for CWallet::Verify() (John Newbery)

Pull request description:

  - fix comment for CWallet::Verify (cleanup after #8694)
  - expose the wallet name in `getwalletinfo` rpc
  - add `listwallets` rpc - returns array of wallet names
  - add functional test for multiwallet using new rpc functionality

Tree-SHA512: 52f864726bf8a28421d4f3604a6cb95fffb3f4e19edbce18efaef06142c48dd4adb9e7a65a10de2955c80f13c00803ce27c78ccbc8434d92ef12cd36c4ccb4aa
This commit was signed with a verified signature.
@laanwj laanwj Wladimir J. van der Laan
GPG key ID: 1E4AED62986CD25D Learn about signing commits
420238d
@MarcoFalke
Member

MarcoFalke commented Jul 21, 2017

jonasschnelli referenced this pull request Jul 21, 2017

Merged

[QA] Avoid running multiwallet.py twice #10893

@jnewbery
Member

jnewbery commented Jul 21, 2017

Nit: No need to run multiwallet.py twice in the test runner ;)

Oops. Good catch!

jnewbery referenced this pull request Jul 21, 2017

Merged

Reject invalid wallets #10885

@MarcoFalke MarcoFalke added a commit that referenced this pull request Jul 22, 2017

@MarcoFalke MarcoFalke Merge #10893: [QA] Avoid running multiwallet.py twice 
44eb9d4 [QA] Avoid running multiwallet.py twice (Jonas Schnelli)

Pull request description:

  It's already on L92.

  Second script execution was introduced in #10604 3707fcd (probably rebase issue)

  Reported by @MarcoFalke

Tree-SHA512: cd2873df08e31cbf5b7a43b5e6713b643b758496d4357dcc99d1c3ad2da07e55f6d69996654d17d3f5484219cb5fd4e32da3bfd94701d1137bc955241d285e57
This commit was signed with a verified signature.
@MarcoFalke MarcoFalke
GPG key ID: D2EA4850E7528B25 Learn about signing commits
0c173a1

jnewbery referenced this pull request Jul 28, 2017

Open

TODO for release notes 0.15.0 #9889

0 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment