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

jnewbery commented at 6:03 pm on June 15, 2017: member

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

jnewbery force-pushed on Jun 15, 2017

ryanofsky commented at 6:17 pm on June 15, 2017: member

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 commented at 6:21 pm on June 15, 2017: member

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

Also more discussion in https://github.com/bitcoin/bitcoin/pull/8788

jnewbery commented at 7:01 pm on June 15, 2017: member

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 commented at 7:05 pm on June 15, 2017: member

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 label Wallet on Jun 16, 2017

laanwj commented at 12:53 pm on June 22, 2017: member

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 force-pushed on Jun 27, 2017

jnewbery commented at 1:52 pm on June 27, 2017: member

@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 renamed this:
~~Expose multiwallet in getwalletinfo and add multiwallet test~~
Add listwallets RPC and add multiwallet test
on Jun 27, 2017

jnewbery force-pushed on Jun 27, 2017

jnewbery renamed this:
~~Add listwallets RPC and add multiwallet test~~
[wallet] [tests] Add listwallets RPC and add multiwallet test
on Jun 30, 2017

instagibbs commented at 6:11 pm on July 3, 2017: member

will review

jnewbery force-pushed on Jul 4, 2017

jnewbery renamed this:
~~[wallet] [tests] Add listwallets RPC and add multiwallet test~~
[wallet] [tests] Add listwallets RPC, include wallet name in `getwalletinfo` and add multiwallet test
on Jul 4, 2017

jnewbery commented at 7:48 am on July 4, 2017: member

rebased

instagibbs commented at 12:59 pm on July 5, 2017: member

API makes sense to me

tACK https://github.com/bitcoin/bitcoin/pull/10604/commits/74882ca0ebc070ba80304e910af4562659e44a1f

jonasschnelli commented at 4:32 pm on July 5, 2017: contributor

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 commented at 4:41 pm on July 5, 2017: member

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?

gmaxwell approved

gmaxwell commented at 10:04 pm on July 12, 2017: contributor

utACK

in src/wallet/wallet.h:1057 in 754434a327 outdated

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

morcos commented at 10:11 pm on July 12, 2017:

ultranit: extra white space

jnewbery commented at 11:44 am on July 13, 2017:

Thanks. fixed

in src/wallet/rpcwallet.cpp:2454 in a952c04dde outdated

2449+            "\nExamples:\n"
2450+            + HelpExampleCli("listwallets", "")
2451+            + HelpExampleRpc("listwallets", "")
2452+        );
2453+
2454+    LOCK(cs_main);

ryanofsky commented at 10:28 pm on July 12, 2017:

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 commented at 11:45 am on July 13, 2017:

Done. Thanks!

in src/wallet/rpcwallet.cpp:2446 in a952c04dde outdated

2441+            "listwallets\n"
2442+            "Returns a list of currently loaded wallets.\n"
2443+            "For full information on the wallet, use \"getwalletinfo\"\n"
2444+            "\nResult:\n"
2445+            "[\n"
2446+            "  \"walletname\": xxxxx,             (string) the wallet name\n"

ryanofsky commented at 10:30 pm on July 12, 2017:

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

jnewbery commented at 11:45 am on July 13, 2017:

yup. Fixed

in src/wallet/rpcwallet.cpp:2446 in 74882ca0eb outdated

2441+            "listwallets\n"
2442+            "Returns a list of currently loaded wallets.\n"
2443+            "For full information on the wallet, use \"getwalletinfo\"\n"
2444+            "\nResult:\n"
2445+            "[\n"
2446+            "  \"walletname\": xxxxx,             (string) the wallet name\n"

morcos commented at 10:31 pm on July 12, 2017:

it just prints the wallet name, not a pair

jnewbery commented at 11:45 am on July 13, 2017:

quite right. Fixed

ryanofsky commented at 10:32 pm on July 12, 2017: member

utACK 74882ca0ebc070ba80304e910af4562659e44a1f

ryanofsky commented at 10:35 pm on July 12, 2017: member

Also, should maybe clean up the PR description (remove all caps “EDITED”) now that #10786 is merged.

jnewbery force-pushed on Jul 13, 2017

jnewbery commented at 11:45 am on July 13, 2017: member

@ryanofsky’s and @morcos’s comments addressed

morcos commented at 1:59 pm on July 13, 2017: member

ACK 011a860

will be awesome to show people all the wallets they can’t access!

instagibbs commented at 2:06 pm on July 13, 2017: member

re-ACK https://github.com/bitcoin/bitcoin/pull/10604/commits/011a860db94b179ca158dfc38acf7beefca2bed1

jnewbery commented at 3:06 pm on July 13, 2017: member

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 force-pushed on Jul 14, 2017

jnewbery commented at 2:48 pm on July 14, 2017: member

Rebased on the latest #10650

instagibbs commented at 2:59 pm on July 14, 2017: member

re-re-ACK https://github.com/bitcoin/bitcoin/pull/10604/commits/5baca3ad700e748d08b0b8f08ef014ea2c478d13

[wallet] fix comment for CWallet::Verify() 09eacee6b2

[wallet] [rpc] print wallet name in getwalletinfo 4a057152d2

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

9508761ed6

[wallet] [tests] Add listwallets to multiwallet test 3707fcd94e

jnewbery force-pushed on Jul 20, 2017

jnewbery commented at 9:07 pm on July 20, 2017: member

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 milestone 0.15.0 on Jul 20, 2017

sipa commented at 9:37 pm on July 20, 2017: member

utACK 3707fcd94e6251384235d16faafc975853d49e3d

instagibbs commented at 1:49 am on July 21, 2017: member

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 commented at 1:18 pm on July 21, 2017: member

bah! Unrelated intermittent travis failure. Job restarted.

in src/wallet/rpcwallet.cpp:2514 in 3707fcd94e

2509+        );
2510+
2511+    UniValue obj(UniValue::VARR);
2512+
2513+    for (CWalletRef pwallet : vpwallets) {
2514+

jonasschnelli commented at 1:19 pm on July 21, 2017:

nit newline

jnewbery commented at 1:24 pm on July 21, 2017:

Yes, agree, but in the interests of getting this merged, I won’t change this now that https://github.com/bitcoin/bitcoin/commit/3707fcd94e6251384235d16faafc975853d49e3d has two ACKs

jonasschnelli commented at 1:20 pm on July 21, 2017: contributor

utACK 3707fcd94e6251384235d16faafc975853d49e3d

instagibbs commented at 1:28 pm on July 21, 2017: member

ACK https://github.com/bitcoin/bitcoin/pull/10604/commits/3707fcd94e6251384235d16faafc975853d49e3d

laanwj merged this on Jul 21, 2017

laanwj closed this on Jul 21, 2017

laanwj referenced this in commit 420238d310 on Jul 21, 2017

MarcoFalke commented at 1:43 pm on July 21, 2017: member

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

On Fri, Jul 21, 2017 at 3:38 PM, Wladimir J. van der Laan < notifications@github.com> wrote:

Merged #10604 https://github.com/bitcoin/bitcoin/pull/10604.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10604#event-1173886839, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv-fIgKq7O65mkNaKFX2GC14F91Rvks5sQKnjgaJpZM4N7jWT .

jnewbery commented at 1:51 pm on July 21, 2017: member

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

Oops. Good catch!

MarcoFalke referenced this in commit 0c173a15ca on Jul 22, 2017

jnewbery deleted the branch on Oct 10, 2017

PastaPastaPasta referenced this in commit a676e0db0d on Aug 6, 2019

PastaPastaPasta referenced this in commit c374ca57fb on Aug 6, 2019

PastaPastaPasta referenced this in commit 00124b96c1 on Aug 6, 2019

PastaPastaPasta referenced this in commit e3e5b016cd on Aug 6, 2019

PastaPastaPasta referenced this in commit 24f5767922 on Aug 6, 2019

PastaPastaPasta referenced this in commit 2e8f8384e5 on Aug 6, 2019

PastaPastaPasta referenced this in commit ffc08e135d on Aug 7, 2019

PastaPastaPasta referenced this in commit 6738da138c on Aug 7, 2019

PastaPastaPasta referenced this in commit 56212875b4 on Aug 8, 2019

PastaPastaPasta referenced this in commit e8d0df3063 on Aug 8, 2019

PastaPastaPasta referenced this in commit af4450ffeb on Aug 12, 2019

PastaPastaPasta referenced this in commit d10e4e769e on Aug 12, 2019

barrystyle referenced this in commit 7cb3fe34bb on Jan 22, 2020

barrystyle referenced this in commit 7940758ce0 on Jan 22, 2020

random-zebra referenced this in commit 2d50b6e3b9 on Jun 9, 2021

MarcoFalke locked this on Sep 8, 2021

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

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