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.
Officially, RPC API deprecations in Bitcoin Core now follow the process
described in
JSON-RPC-interface.md#versioning
since commit
fa74749
(âdoc: Clarify RPC versioningâ) merged in April 2019.
Nevertheless, Bitcoin Core also has RPCs that contain âDEPRECATEDâ warnings in
capital letters in the help documentation, but which are not actually
deprecated. Why? Because they have not begun the -deprecatedrpc flag
process.
Other RPCs with âDEPRECATEDâ warnings are in the official deprecation
process.
Here are two examples of PRs that launch the deprecation process:
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.
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.
Is the getunconfirmedbalance RPC deprecated? How about the getwalletinfo
balance fields? Explain.
Give an example of a recent Bitcoin Core API deprecation? And removal?
You are the PR author: how would you implement laanwjâs request?
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?
<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.
<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."
<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).
<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.
<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.
<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.
<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 ;)
<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.
<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)
<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)
<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>
<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)
<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
<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
<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.
<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?
<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
<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
<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.
<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?
<sipa> ecurrencyhodler: the getinfo RPC commamd was just one RPC, but it reached into various otherwise unrelated subsystems in the code, which was messy
<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
<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".
<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
<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
<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
<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
<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.
<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.
<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
<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
<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
<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,
<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.
<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)
<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)
<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
<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?
<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.