The PR branch HEAD was a5ea571 at the time of this review club meeting.
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
API removal. What is the process for deprecating APIs? When should they be
deprecated, when are they actually deprecated, and when should they be
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
“RPC” is a frequently used acronym for Remote Procedure
Call, a form of
client–server interaction and request–response protocol.
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
Other RPCs with “DEPRECATED” warnings are in the official deprecation
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
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
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
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
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.
Bitcoin CLI and -getinfo
Bitcoin CLI calls are in
-getinfo code is in the
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
In the first
-getinfo is changed to replace using
getwalletinfo.balance with using getbalances.ismine.trusted.
In the second
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
from the command line to use and test it.
The third and final
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> 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.
<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)
<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>
<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
<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.
<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
<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> 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.
<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
<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)