Add multiwallet balances rpc, use it in -getinfo, no longer depend on getwalletinfo balance (rpc/rest/zmq, utils/log/libs)

https://github.com/bitcoin/bitcoin/pull/18453

Host: jonatack  -  PR author: jonatack

The PR branch HEAD was a5ea571 at the time of this review club meeting.

Notes

Introduction

This week’s PR is relatively approachable to review. It is also easy to manually test by building, launching bitcoind, and running bitcoin-cli -getinfo on the command line, preferably with multiple wallets loaded (you can use the createwallet and loadwallet RPCs to do that, if needed).

Yet the PR also brings up several practical topics on Bitcoin Core API process and design:

  • API removal. What is the process for deprecating APIs? When should they be deprecated, when are they actually deprecated, and when should they be removed?
  • Adding APIs. When should we add an RPC call versus extending an existing one?
  • Multiwallets. How should we extend the API in the future to handle multiple wallets?

Definitions

  • “RPC” is a frequently used acronym for Remote Procedure Call, a form of client–server interaction and request–response protocol.

  • The Bitcoin Core RPC API follows the JSON-RPC protocol. See the Bitcoin Core JSON-RPC-interface.md doc for more.

Context

Today’s PR and PR 18451 “rpc: remove deprecated getunconfirmedbalance” were both motivated by PR 15930 “rpc: Add balances RPC”, which added the getbalances RPC and contained a commit entitled “rpc: Deprecate getunconfirmedbalance and getwalletinfo balances”.

Bitcoin Core deprecation process

-deprecatedrpc and DEPRECATED comments

When to deprecate, and when to remove?

  • One way of thinking about this might be to distinguish between possibly minor deprecations, like removing an RPC input argument or result field, and removing an existing RPC completely.

  • Another approach is to think in terms of estimating or verifying current usage and how much upgrade pain it would ask of users.

Adding RPCs or extending them

Review is scarce, and one consequence of that is when a particular section of code is being touched and reviewed, it is often an opportune moment to make – or be asked to make – changes touching the same code, while effort and eyes are on it.

This PR is an illustration of that. It originally began as just one commit to enable -getinfo to call getbalances, in order to no longer depend on the getwalletinfo balance response that was marked as DEPRECATED a year earlier.

Then, Bitcoin Core maintainer laanwj proposed, while updating this code, to add displaying the balance per wallet.

  • For context, getinfo was previously part of the RPC API. It was first marked as deprecated in v0.14 (PR 8780) and removed in v0.16 (PR 10838), with a deprecation warning added in that release (PR 12198).

  • Now that it has been renamed to -getinfo and is no longer part of the RPC API, -getinfo is easier to change and improve without worrying about API stability, which enables initiatives like issue 17314 “Re-thinking bitcoin-cli -getinfo.” Displaying the balance for each wallet is point 2 in that issue list.

This outwardly fun and simple request ended up raising a few unexpected questions for me. While working on it, it struck me that it might not only be cleaner to implement by adding a new RPC, but also perhaps more useful. Ultimately we’ll want an RPC that can fetch all the wallet balances in one call.

The Shakespearean question then became whether ‘tis better to add an RPC or to extend an existing one, in the latter case by adding an argument to getbalances to iterate through the loaded wallets.

My initial intuition was that it’s (a) more convenient for users to have a dedicated call, (b) simpler to implement, and (c) faster to run and test, so I began with that.

Code

Bitcoin CLI and -getinfo

Bitcoin CLI calls are in src/bitcoin-cli.cpp. The -getinfo code is in the GetinfoRequestHandler class starting at line 224. Under the hood, -getinfo performs multiple requests to various RPC calls, namely to getnetworkinfo, getblockchaininfo, and getwalletinfo, and presents the results in a (hopefully) user-friendly output.

Wallet RPCs

Many of the wallet RPCs are in src/wallet/rpcwallet.cpp, including the ones we are interested in for this PR.

Commits

  • In the first commit, -getinfo is changed to replace using getwalletinfo.balance with using getbalances.ismine.trusted.

  • In the second commit, a small RPC, tentatively called getwalletbalances, is created to fetch the ismine.trusted balances from all the currently loaded wallets, and functional test coverage is added. The RPC is undocumented in the help (for now), but it can be called from the command line to use and test it.

  • The third and final commit changes -getinfo to call getwalletbalances instead of getbalances and adds functional test coverage for the change.

Questions

  1. Did you review the PRs? Concept ACK, approach ACK, ACK, or NACK? (Don’t forget to put your PR review on GitHub).

  2. Is the getunconfirmedbalance RPC deprecated? How about the getwalletinfo balance fields? Explain.

  3. Give an example of a recent Bitcoin Core API deprecation? And removal?

  4. You are the PR author: how would you implement laanwj’s request?

  5. How should we extend the Bitcoin Core API to handle multiple wallets? Do you prefer extending existing RPCs or adding dedicated ones for this? Which calls might be good candidates for updates? What new calls might be needed?

Meeting Log

  119:00 <jonatack> #startmeeting
  219:00 <jonatack> Hi all! Welcome to this week's episode of the Bitcoin Core PR Review club.
  319:00 <jonatack> Feel free to say "hi" to let everyone know you're here, even if you arrive in the middle of the meeting.
  419:00 <jnewbery> hi
  519:00 <jonatack> Special welcome to everyone here for the first time today.
  619:00 <emzy> Hi
  719:00 <kevin_gislason> hi
  819:00 <robot-visions> hi
  919:00 <pinheadmz> hi!
 1019:00 <jonatack> I'd like to encourage you to not be shy. Jump in at any time to comment or ask questions. There are no dumb questions here. We're all here to learn.
 1119:00 <jkczyz> hi
 1219:00 <jonatack> #topic This week, we are looking at PR #18453, which for the moment is called "rpc, cli: add multiwallet balances rpc, use it in -getinfo, no longer depend on getwalletinfo balance."
 1319:00 <vasild> Hi
 1419:00 <theStack> hi
 1519:00 <michaelfolkson> hi
 1619:00 <jonatack> Notes and questions in the usual place: https://bitcoincore.reviews/18453
 1719:00 <ecurrencyhodler> hi
 1819:00 <kanzure> hi
 1919:00 <jonatack> This week's PR is relatively approachable to review.
 2019:00 <andrewtoth> hi
 2119:00 <fjahr> hi
 2219:00 <jonatack> It should be easy to manually test by building, launching bitcoind, and then running `bitcoin-cli -getinfo` on the command line, preferably with multiple wallets loaded (you can use the `createwallet` and `loadwallet` RPCs to do that, if needed).
 2319:00 <nehan_> hi
 2419:00 <jonatack> Yet the PR also brings up several practical topics on Bitcoin Core API process and design.
 2519:00 <jonatack> - API removal: what is the process for deprecating APIs?
 2619:00 <willcl_ark> hi
 2719:00 <jonatack> - Adding APIs: when should we add an RPC call versus extending an existing one?
 2819:01 <jonatack> - Multiwallets: how should we extend the API to handle multiple wallets?
 2919:01 <jonatack> Today's PR is a great example of Thing Leads To Another (TM).
 3019:01 <jonatack> Experienced review is scarce, and one consequence of that is when a particular section of code is being touched and reviewed, it is often an opportune moment to make -- or be asked to make -- changes touching the same code, while effort and eyes are on it.
 3119:01 <jonatack> This PR originally began as a single commit to enable `-getinfo` to call `getbalances`, in order to no longer depend on the `getwalletinfo` balance response that was marked as deprecated a year earlier.
 3219:01 <jonatack> Then Bitcoin Core maintainer laanwj proposed, while this code was being updated, to add displaying the balance per wallet. The review comment is here: https://github.com/bitcoin/bitcoin/pull/18453#issuecomment-605431806
 3319:01 <jonatack> Bitcoin CLI calls are in src/bitcoin-cli.cpp: https://github.com/bitcoin/bitcoin/blob/master/src/bitcoin-cli.cpp
 3419:01 <jonatack> The -getinfo code is in the GetinfoRequestHandler class starting at line 224: https://github.com/bitcoin/bitcoin/blob/master/src/bitcoin-cli.cpp#L224
 3519:01 <jonatack> Under the hood, `-getinfo` performs multiple requests to various RPC calls (namely `getnetworkinfo`, `getblockchaininfo`, and `getwalletinfo`) and presents the results in a (hopefully) user-friendly output.
 3619:01 <jonatack> While adding multiwallet balances to -getinfo, it seemed to make sense to do it by adding a new dedicated RPC call.
 3719:01 <jonatack> Now... changing a CLI call is one thing; but changing the RPC API is quite another: the latter is subject to API stability constraints
 3819:02 <jonatack> which anyone who has ever maintained a legacy codebase with a large user base long enough to have battle scars from the experience can understand ;)
 3919:02 <jonatack> Let's get started!
 4019:02 <jonatack> Who had a chance to review the PR this week? Did you build and test it manually with multiple wallets?
 4119:02 <robot-visions> (y)
 4219:02 <pinheadmz> yeah, but having issues with multiwallet
 4319:02 <vasild> y
 4419:02 <fjahr> y and y
 4519:02 <jnewbery> 0.5 y review, but no testing
 4619:02 <willcl_ark> y and y
 4719:03 <vasild> pinheadmz: what issues?
 4819:03 <nehan_> a little review, started testing but didn't get far yet
 4919:03 <pinheadmz> well i created a second wallet, restarted bitcoind with -wallet=a adn -wallet=b
 5019:03 <pinheadmz> but when i call -getingo it shows me NO balances
 5119:03 <pinheadmz> unless i specify -rpcwallet=a or b
 5219:03 <pinheadmz> then it only shows me the one balance
 5319:04 <vasild> maybe you need "loadwallet"?
 5419:04 <theStack> did code-review, but no testing yet
 5519:04 <jonatack> anyone else have issues with multiwallets?
 5619:04 <jonatack> Question 2: Is the `getunconfirmedbalance` RPC deprecated? How about the `getwalletinfo` RPC "balance" fields? Explain.
 5719:04 <vasild> getunconfirmedbalance: yes - DEPRECATED\nIdentical to getbalances().mine.untrusted_pending
 5819:04 <robot-visions> Both `getunconfirmedbalance` and the `getwalletinfo` balance fields have been "soft deprecated", because the documentation indicates that they are deprecated, but `-deprecatedrpc=` is not yet needed to use them.
 5919:04 <vasild> getwalletinfoybalance fields: yes - DEPRECATED. Identical to getbalances().mine.*
 6019:04 <jnewbery> pinheadmz: (forgive the basic question) are you definitely running a newly built bitcoin-cli? (I've made mistakes before where I've rebuilt bitcoind but accidentally called the old bitcoin-cli executable)
 6119:05 <pinheadmz> yes i thought of that and just re-maked
 6219:05 <pinheadmz> hm but might not be on my pth, hang on
 6319:05 <pinheadmz> jonatack: those are both deprecated RPCs
 6419:05 <jonatack> pinheadmz: yes, or sometimes from running the command from root and not /src
 6519:05 <pinheadmz> but so is `getinfo` -- so I'm curious why bother adding features to it ?
 6619:05 <jonatack> (at least i've done that)
 6719:06 <theStack> from what i found out the `getunconfirmedbalance` deprecation was introduced with the introduction of the `getbalances` rpc (https://github.com/bitcoin/bitcoin/pull/15930)
 6819:06 <jonatack> robot-visions: correct!
 6919:06 <theStack> s/introduced/done
 7019:06 <jonatack> the commit *says* they are deprecated
 7119:06 <pinheadmz> jonatack: thanks, src/bitcoin-cli and src/bitcoind totally my bad
 7219:06 <jonatack> and the *do* have warnings in the help
 7319:07 <jonatack> but they are still usable without the need to pass the deprecation flag in the .conf file or when launching
 7419:08 <jonatack> TBH this state can be a bit confusing
 7519:08 <sipa> when were they made deprecatedm
 7619:08 <sipa> ?
 7719:08 <sipa> also: hi!
 7819:08 <vasild> maybe somebody (some app) could be using them and never noticed they are deprecated because apps don't read the help
 7919:08 <jonatack> and I think one of three things should happen:
 8019:08 <jnewbery> in my experience, the best assumption to make is that no user will ever read the help text or documentation, nor will they change client behaviour if there's a warning about deprecation. I think writing "deprecated" in help text is next-to-useless if the command is not functionally deprecated. </rant>
 8119:09 <MarcoFalke> That is my fault. I should not have added DEPRECATED to the doc
 8219:09 <sipa> vasild: yeah, we don't remove deprecated things without first going through a -deprecatedrpc cycle
 8319:09 <willcl_ark> What is the general idea behind deprecated `getinfo` being moved to `-getinfo`?
 8419:09 <jnewbery> great question, willcl_ark!
 8519:09 <jonatack> (a) deprecate them for real, (b) remove the warnings, or (c) update the doc to mention possible 2-stage ("soft then hard") deprecation process for some calls (like entire calls)
 8619:10 <jonatack> *for some things (like entire calls)
 8719:10 <jonatack> sipa: MarcoFalke: hi!
 8819:10 <MarcoFalke> hi
 8919:10 <pinheadmz> jonatack: but why add new features like multiwallet balance statistics to a deprecated rpc ?
 9019:11 <jnewbery> `getinfo` used to be a server-side RPC method. It was a PITA because it accessed data from many different components (wallet, node, etc) and locked everything up while it did that
 9119:11 <ecurrencyhodler> So are you saying people tend to look at release notes only if their app breaks because an rpc has been removed?
 9219:11 <theStack> -getinfo as cli option is not deprecated as far as i see it?
 9319:11 <jnewbery> it was deprecated and finally removed from the server a few releases ago
 9419:11 <jonatack> willcl_ark: my guess is that it is because RPC calls are intended to be consumed by applications, not people
 9519:11 <sipa> theStack: it is not
 9619:11 <sipa> ecurrencyhodler: even that is a stretch ;)
 9719:12 <jnewbery> however, the functionailty is useful for people (and particularly developers and testers), so a bitcoin-cli client-side function called getinfo was added to deliver that functionality
 9819:12 <jonatack> willcl_ark: as so getinfo is better as a CLI call for use by humans which frees it from API stability constraints
 9919:12 <jnewbery> bitcoin-cli -getinfo makes multiple RPC requests to the server and presents a nice summary to the user
10019:12 <pinheadmz> i see, so the server getinfo was removed, but on the CLI side, getinfo is still useful as a sort of macro that wraps several other calls
10119:12 <willcl_ark> jnewbery: hmm ok. I see. I always liked the command so was happy it was not "removed" completely
10219:12 <willcl_ark> thanks all
10319:12 <jnewbery> pinheadmz: exactly
10419:13 <jonatack> pinheadmz: RPC getinfo was deprecated in favor of CLI -getinfo (with a dash)
10519:13 <jonatack> pinheadmz: yes
10619:13 <robot-visions> In practice, does making `-getinfo` "non atomic" introduce any gotchas?
10719:13 <willcl_ark> Ok I see it now, it makes 4 calls, and aggregates them
10819:13 <kevin_gislason> What does a dash mean tbh?
10919:14 <sipa> robot-visions: it may mean that the wallet balance isn't for the exact block reported
11019:14 <theStack> (a little off-topic, but i always found arguments with more than one letter but *not* having two dashes confusing...)
11119:14 <jnewbery> ecurrencyholder: that's my experience from bitcoind and other projects I've worked on
11219:14 <sipa> theStack: feel free to use two dashes; i think that works too
11319:14 <MarcoFalke> jup, should work as well
11419:14 <jonatack> jnewbery: thanks, good point about why it was deprecated as an rpc
11519:15 <theStack> sipa: MarcoFalke: ah, indeed it does
11619:15 <ecurrencyhodler> thanks john and sipa for answering.
11719:16 <jnewbery> It should be possible to make it 'atomic' - have every RPC call return the block that it's reporting info for, and then ensure that all of them are the same before aggregating the data.
11819:16 <MarcoFalke> jnewbery: Is this a "good first issue"? :))
11919:16 <jnewbery> it could be! Probably worth asking around for concept ACKs first
12019:17 <vasild> infinite loop (during IBD)
12119:17 <MarcoFalke> Concept ACK
12219:17 <jonatack> Question 3: Give an example of a recent Bitcoin Core API deprecation? And removal?
12319:17 <robot-visions> 3. The "size" field of mempool related RPCs was deprecated in [#15637](https://github.com/bitcoin/bitcoin/pull/15637) and will be removed in [#18493](https://github.com/bitcoin/bitcoin/pull/18493)
12419:17 <MarcoFalke> Concept ACK on returning the block, no opinion on the cli change
12519:17 <jnewbery> vasild: yeah, you'd need to be careful. Perhaps only retry a few times and then report that tip height is changing rapidly so data could be inconsistent?
12619:17 <michaelfolkson> wallet generate deprecated and removed
12719:17 <theStack> not sure if that counts as "recent", but i found that the `generate` RPC was deprecated in 0.18 and finally removed in 0.19.1
12819:18 <jonatack> all good
12919:18 <vasild> jnewbery: +1
13019:18 <andrewtoth> labels were all deprecated fairly recently
13119:19 <jonatack> rpc bumpfee's totalFee argument was removed e.g. no more fee bumping using total fee
13219:19 <andrewtoth> getbalance for getbalances
13319:19 <jonatack> andrewtoth: rpc getaddressinfo label yes, labels was modified
13419:19 <andrewtoth> ah right
13519:20 <willcl_ark> Is an "immature" balance "trusted" or "untrusted"?
13619:20 <andrewtoth> willcl_ark could be either
13719:20 <vasild> willcl_ark: neither one
13819:20 <vasild> :)
13919:20 <willcl_ark> either and neither :)
14019:20 <jonatack> label was superseded by labels, and labels went from being a JSON object containing name and purpose, to an array containing the name
14119:20 <jnewbery> There have been many deprecations and removals over the last 3 or 4 releases, I think mainly because for years things were marked 'DEPRECATED' and then never actually deprecated/removed so there was a backlog and we're finally paying down that debt
14219:20 <sipa> immature balances are unspendable, by consensus rules
14319:21 <sipa> trusted/untrusted ones are (but with the untrusted ones you need to be careful)
14419:21 <willcl_ark> thanks sipa. I don't think I've ever seen one of those. I guess it's mainly coinbase transactions then?
14519:21 <jonatack> willcl_ark: run rpc getbalances
14619:21 <sipa> willcl_ark: it is solely coinbase transactions
14719:21 <sipa> willcl_ark: with leas than 100 confirmatiins
14819:22 <willcl_ark> That explains why then.
14919:22 <vasild> nLockTime?
15019:22 <sipa> (sorry, phone typing)
15119:22 <theStack> vasild: not having anything to do with nLockTime as far as i know
15219:22 <jonatack> willcl_ark: ah, misunderstood your question
15319:22 <sipa> vasild: non-final transactions are something else
15419:23 <sipa> vasild: they can't enter the mempool until they are minable
15519:23 <vasild> I see
15619:23 <MarcoFalke> In which case they won't enter the wallet either, or at least the wallet doesn't "count" them
15719:25 <jonatack> re jnewbery's point about debt:
15819:25 <sipa> arguably CSV transaction outputs to our wallet could be given a similar treatment - but since those aren't (yet) supported it isn't an issue
15919:25 <vasild> So, coinbase transactions cannot be spent if they have less than 100 confirmations, by consensus rules?
16019:25 <sipa> vasild: correct
16119:26 <vasild> ack
16219:26 <sipa> it may be 101, unsure
16319:27 <jonatack> how much of the accumulated debt was a result of backlash from the community on deprecating too quickly?
16419:27 <jonatack> (versus just no one bothering with it)
16519:28 <sipa> for a very long time in bitcoin core's history, rpc were never ever deprecated or removed or broken in impactful ways
16619:28 <MarcoFalke> I don't think we have great feedback mechanisms for this kind of thing in general. Though, account removal had a great backlash
16719:28 <sipa> i don't think that was as a result of user feedback, just an abundance of caution
16819:28 <andrewtoth> ah so an immature balance can never become untrusted, but can mature into trusted?
16919:28 <jnewbery> jonatack: I think it was a healthy caution about breaking things for users. We didn't have the -deprecatedrpc staged deprecation framework until ~0.16
17019:29 <sipa> andrewtoth: correct
17119:29 <kevin_gislason> Only tangentially related, but why doesn't the bitcoin core RPC use encryption?
17219:30 <sipa> kevin_gislason: it's only intended to be exposed to local, trusted, clients
17319:30 <jonatack> sipa: MarcoFalke: jnewbery: thanks
17419:30 <sipa> encrypting it (which was supported for a while) made people incorrectly assume it was safe to expose to the internet
17519:30 <vasild> what if a miner sends his reward to me after 50 confirmations, would that tx be allowed in mempool?
17619:30 <pinheadmz> kevin_gislason: bcoin and btcd offer rpc over SSL, but to sipas point its not very safe
17719:30 <sipa> vasild: no
17819:31 <vasild> ok
17919:31 <kevin_gislason> sipa why wouldn't encrypted rpc be safe to expose to the internet tbh?
18019:32 <sipa> kevin_gislason: because RPC is not safe to expose to untrusted parties
18119:32 <emzy> Also you would need again SSL/TLS in bitcoind.
18219:32 <sipa> it may have DoS vulnerabilities
18319:32 <sipa> it's just not designed to be used by untrusted parties
18419:32 <andrewtoth> it's not safe to expose period, and encrypting gave a false sense that it was
18519:32 <sipa> you can use stunnel if you really want to encrypt it
18619:32 <michaelfolkson> But DoS are the only vulnerabilities?
18719:33 <sipa> michaelfolkson: maybe?
18819:33 <nehan_> jonatack: how are #18451 and #18453 related? Did you start with the idea of fixing up deprecations?
18919:33 <kevin_gislason> Hmm interesting, makes sense
19019:33 <MarcoFalke> michaelfolkson: It used to be possible to guess the rpcpassword with a timing attack. Though now that is fixed.
19119:34 <MarcoFalke> And obvioulsy some users use rpcpassword=123456
19219:34 <MarcoFalke> No need for a timing attack there
19319:34 <michaelfolkson> Interesting, thanks. Yup that is a concern
19419:34 <ecurrencyhodler> Quick question back to the get-info PR.
19519:34 <jonatack> nehan_: yes! i (and 2 other contributors) tripped up a bit on the deprecation process lately
19619:35 <sipa> vasild: generally the mempool only accepts transactions that would be legal to include in the subsequent block
19719:35 <ecurrencyhodler> Do I have this correct? bitcoin-cli -getinfo means it's client side and so user can access it via terminal. But bitcoin-cli getinfo is on the server side and intended for apps. bitcoin-cli getinfo command included several other rpc calls which prevented the app from accessing them again until the task was completed.
19819:35 <willcl_ark> So will this PR get held up waiting for an "across all wallets" RPC naming convention (favoured in the comments), or is it fine to go in and get modified with that later change?
19919:35 <sipa> ecurrencyhodler: i don't understand the second half of your sentence
20019:36 <sipa> willcl_ark: well, up to reviewers!
20119:36 <pinheadmz> ecurrencyhodler: think you might be confusing RPC calls to bitcoind and CLI commands
20219:36 <ecurrencyhodler> Ah ic
20319:36 <willcl_ark> everyone got plenty of time for bikeshedding whilst under lockdown :)
20419:36 <jonatack> willcl_ark: i think that depends if the rpc should be public
20519:36 <ecurrencyhodler> Okay will need to read more. Thanks.
20619:37 <jonatack> ISTM that once an RPC is part of the public API, stability constraints are in force
20719:37 <sipa> ecurrencyhodler: the getinfo RPC commamd was just one RPC, but it reached into various otherwise unrelated subsystems in the code, which was messy
20819:38 <ecurrencyhodler> sipa: Thanks for clarifying that for me.
20919:38 <jnewbery> ecurrencyhodler: bitcoin-cli is a very simple client-side application. The starting point in the code is src/bitcoin-cli.cpp
21019:38 <sipa> ecurrencyhodler: so it was split into various more single-purpose RPCs (getblockchaininfo, getwalletinfo, getnetworkinfo, ...), and a CLI command (-getinfo) was added that just calls all those RPCs and aggregates the result, inside bitcoin-cli
21119:38 <jonatack> Question 4: *You are the PR author:* how would you personally implement laanwj's request to add multiwallet balances to -getinfo?
21219:39 <jonatack> Would you have done it in bitcoin-cli.cpp?
21319:39 <vasild> The "getbalances" output is already fixed to display a single/default wallet data. I would introduce a new multi-wallet RPC that would eventually replace "getbalances".
21419:39 <jnewbery> jonatack: a general principal in software projects is that it's *much* *much* harder to remove something than to add it, so I think we should generally be cautious about adding functionality to the RPC interface
21519:39 <jonatack> jnewbery: +1
21619:40 <nehan_> jnewebery: +1
21719:40 <nehan_> jnewbery: +1
21819:40 <robot-visions> 4. It looks like `getbalances` makes some assumptions that depend on working with a single wallet: (i) the result doesn't have any fields that support multiple baances, (ii) the RPC errors if multiple wallets are loaded and you don't specify one. I don't yet see a good case for completely overhauling `getbalances` to handle multiple wallets, so I
21919:40 <robot-visions> agree with jonatack's approach of adding a new RPC.
22019:40 <vasild> in bitcoin-cli.cpp - no because then it cannot be reused by RPC users
22119:40 <jnewbery> once users start relying on a feature, even if it's marked 'experimental' or 'debug-only' or whatever, they'll be upset if you try to remove or change it
22219:41 <nehan_> jonatack: is your next step here to actually remove usage of getwalletinfo() in the tests?
22319:41 <robot-visions> (That being said, I also agree with jnewbery's caveat about adding vs. removing so I'm not sure in the end)
22419:41 <willcl_ark> jonatack: yes. It seems to make sense to have small, logical RPC commands, then implement more complex stuff in bitcoin-cli.cpp
22519:41 <theStack> i agree to vasild that bitcoin-cli.cpp is not the proper place, for the same reason
22619:41 <jnewbery> vasild: this functionality is precisely for bitcoin-cli users. I don't think we want it re-used by other applications
22719:42 <willcl_ark> I think, if you are interfacing with the RPC you are able to (easily?) re-create any cli-specific stuff you want
22819:42 <jonatack> nehan_: i think getunconfirmedbalances and the getwalletinfo balance fields should be deprecated
22919:42 <vasild> jnewbery: you mean getting the balances of all wallets?
23019:43 <jnewbery> vaslid: yes. In the current PR the new RPC is hidden precisely because we don't want other clients to use it
23119:43 <jonatack> jnewbery: yes
23219:44 <nehan_> jonatack: ah, just those fields, right
23319:44 <jonatack> while getting feedback i also didn't see any point in adding documentation unless people want a public rpc
23419:44 <jnewbery> jonatack: my preferred approach is to implement this entirely in bitcoin-cli by looping over the wallets, rather than making changes to the RPC server
23519:44 <vasild> "get balances of all wallets" looks like a good RPC candidate to me, why not?
23619:45 <jonatack> nehan_: and maybe(?) getbalance at some point (superseded by getbalances)
23719:45 <jonatack> jnewbery: like what promag suggested in the PR?
23819:46 <jnewbery> your proposed new RPC `getwalletbalances` is the only RPC that exposes multiple wallets. There's `listwallets` but that only lists the wallets and whether they're loaded, rather than interact with wallet functionality.
23919:46 <vasild> I think either completely in bitcoin-cli.cpp or a public documented RPC. I don't like the idea of a "hidden" RPC because people will find it and will use it.
24019:47 <sipa> jnewbery: i don't know why it's intended to be hidden from other applications?
24119:47 <jonatack> jnewbery: is your approach to begin this way and attempt adding multiwallet rpcs separately? or that it there is no need for an rpc at all
24219:47 <MarcoFalke> Does it scale if we add for each wallet call another call that does the same for all wallets?
24319:47 <jnewbery> (there's no way for you to know this since it's not documented anywhere, but) some people would like to add per-wallet permissions (https://github.com/bitcoin/bitcoin/pull/10615), and there's another proposal to move the wallet to a separate process with its own RPC server (https://github.com/bitcoin/bitcoin/pull/10102). I think both would be more difficult if we started introducing RPCs
24419:47 <jnewbery> that access multiple wallets.
24519:47 <willcl_ark> (once you understand it) it seems easier to reason that each `wallet` command refers to a single wallet. Although the comments on the PR about adding specific `allwallets*` commands or endpoint also seems reasonable
24619:48 <theStack> so the alternative idea would be that users make one RPC call per wallet and assemble the informations themselves?
24719:48 <jnewbery> so I think the current model of RPCs only being able to access the wallet that they were called for is something we should strive to keep
24819:49 <sipa> jnewbery: in that case maybe it's better to not have a separate RPC at all, and have bitcoin-cli assembly the information?
24919:49 <wumpus> I'd also prefer this to be a client-only feature in bitcoin-cli at most
25019:49 <jnewbery> sipa: yes. That's what I'm proposing
25119:49 <wumpus> +1 sipa that's how I initially meant it as well
25219:50 <sipa> the per-wallet permissions/endpoints/separation argument would favor that approach
25319:50 <wumpus> I'm not sure why people suddenly want to change the server side, this was about -cli imporovements
25419:50 <jnewbery> MarcoFalke: I'd be a concept NACK on that for the permissions/endpoints argument
25519:50 <willcl_ark> so 27b81b25ab6c176ba84f69bf9c00fed13c2dca30 should move getbalances logic to bitcoin-cli.cpp?
25619:50 <jnewbery> wumpus: it seems like a cleaner solution if you don't have the context of permissions/endpoints
25719:51 <wumpus> I think it's useful if bitcoin-cli can show the balances for multiple wallets, if the complexity of this prohibitive, please just drop the entire idea
25819:51 <jonatack> wumpus: i thought the PR as-is it quite simple (except for the additional rpc, heh)
25919:52 <jnewbery> wumpus: I don't think that's it's too complex. We're using this meeting to explore different ideas
26019:53 <vasild> how about per wallet permissions and bitcoin-cli.cpp "get all wallet balances"? How would it behave?
26119:53 <wumpus> I know but I"m alomost sorry for suggesting it now
26219:53 <jonatack> maybe can revert back to the initial commit, and explore doing it in bitcoin-cli.cpp in a follow-on PR
26319:53 <willcl_ark> Apart form making more RPC calls, I don't see any difference about having it in bitcoin-cli.cpp vs rpcwallet.cpp?
26419:53 <jonatack> wumpus: i think in the worse case it's a good opportunity for everyone to learn/discuss
26519:54 <nehan_> so, to summarize (do i have everything?) in favor of new rpc: cleaner and simpler in favor of bitcoin-cli: server-side might make future wallet/server separation harder; might want to add permissioned wallets,
26619:54 <michaelfolkson> Yup agreed. I'm enjoying the discussion
26719:54 <jnewbery> jonatack: +1
26819:54 <ecurrencyhodler> jonatack: +1 I'm learning a lot.
26919:55 <robot-visions> nehan_: one more point in favor of bitcoin-cli, avoid adding to the API in a way that's hard to remove later?
27019:55 <jnewbery> willcl_ark: for nodes with multiple wallets, those wallet's RPC methods are accessed on a separate RPC endpoint. That maps nicely to the permissions/multiprocess model - if you used this endpoint, then you can access this wallet.
27119:55 <jonatack> robot-visions: indeed, i think that's the main worry
27219:55 <nehan_> robot-visio: good point! at least it's a hidden rpc.
27319:56 <jnewbery> the proposed RPC in this PR adds an RPC method to the global node endpoint which can access all the wallets. That breaks the mapping above (which again, is not well documented anywhere so there's no reason jonatack should have known this)
27419:56 <nehan_> it occurred to me that doing it bitcoin-cli side might make it harder to get consistency across wallets, though I'm not sure that's really true (the try again method could be used there; if at some point someone actually *wants* to lock the server to get a consistent snapshot that could only be done with a server-side rpc)
27519:56 <wumpus> the idea of a hidden RPC with specific API for our client is a bit strange, other people are bound to start using it too, if it can be avoided that would be preferable
27619:57 <pinheadmz> this hidden rpc is still accessible say from curl:
27719:57 <wumpus> do we really need this kind of 'private APIs'
27819:57 <pinheadmz> curl a:b@127.0.0.1:18443 -X POST --data '{"method":"getwalletbalances"}'
27919:57 <jonatack> agreed -- i think it should be public or not at all
28019:57 <jnewbery> wumpus: we already have hidden RPCs, but I guess those are mostly for testing?
28119:57 <wumpus> jnewbery: they are for testing only
28219:58 <wumpus> even there I don't really like them, but testing is really important
28319:58 <jonatack> and it would need careful thinking... so doing it in bitcoin-cli.cpp is certainly much less risk
28419:58 <nehan_> if my summary is roughly correct i'd vote cli side.
28519:58 <wumpus> (it's hard to avoid that)
28619:58 <wumpus> also the significant ones of the testing calls only work for regtest I think?
28720:00 <jonatack> any last thoughts/questions?
28820:00 <jonatack> #endmeeting
28920:00 <ecurrencyhodler> Thanks jonatack for hosting!
29020:00 <robot-visions> What are some common RPC use cases to consider (block explorers, 3rd-party wallets, etc.)? Who are the clients that will be affected by hard deprecations / removals?
29120:00 <jonatack> thanks everyone!
29220:00 <vasild> Thanks!
29320:00 <jnewbery> nehan_: Good summary, although "server side - cleaner and simpler" is a bit of a subjective call. We could argue that minimizing the changes to the server is simpler/cleaner.
29420:00 <pinheadmz> thanks jon, great work and thanks for your time
29520:00 <kevin_gislason> Thanks all, it's been interesting
29620:00 <nehan_> thanks jonatack!
29720:00 <theStack> thanks for hosting jon
29820:00 <michaelfolkson> 4 seconds for last thoughts and questions? ;)
29920:00 <andrewtoth> thanks jonatack!
30020:00 <jonatack> michaelfolkson: sure
30120:00 <nehan_> jnewbery: true!
30220:00 <robot-visions> Thanks jonatack, this was my first PR Review Club, I found it very interesting and educational!
30320:01 <michaelfolkson> Haha thanks jonatack. Great meeting
30420:01 <emzy> Thanks jonatack for hosting
30520:01 <jonatack> robot-visions: keep coming! hope to see you at the next ones
30620:01 <willcl_ark> thansk jonatack
30720:01 <wumpus> yes thanks jonatack
30820:01 <jonatack> if anyone is interested in hosting, or has ideas for good PRs to cover
30920:02 <jonatack> don't hesitate to post here or to comment in
31020:02 <jnewbery> We need a host for next week. Don't be shy!
31120:02 <jonatack> https://github.com/bitcoin-core-review-club/website/issues/14
31220:03 <jonatack> +1 (and you learn much more from hosting!)