rpc: Add balances RPC

MarcoFalke commented at 2:24 pm on May 1, 2019: member

This exposes the CWallet::GetBalance() struct over RPC.

In the future, incorrectly named rpcs such as getunconfirmedbalance or rpcs redundant to this such as getbalance could be removed.

wallet: Use IsValidNumArgs in getwalletinfo rpc fad40ec915

fanquake added the label RPC/REST/ZMQ on May 1, 2019

MarcoFalke added the label Wallet on May 1, 2019

laanwj commented at 3:29 pm on May 1, 2019: member

Concept ACK

or rpcs redundant to this such as getbalance could be removed.

Dunno, I think having a dedicated call to get the balance is useful. If it’s not broken (like mis-named methods) I’d prefer keeping it.

instagibbs commented at 3:43 pm on May 1, 2019: member

concept ACK

in src/wallet/rpcwallet.cpp:2404 in fac5d96105 outdated

2405+            "      \"trusted\": xxx                 (numeric) trusted balance (outputs created by the wallet or confirmed outputs)\n"
2406+            "      \"untrusted_pending\": xxx       (numeric) untrusted pending balance (outputs created by others that are in the mempool)\n"
2407+            "      \"immature\": xxx                (numeric) balance from immature coinbase outputs\n"
2408+            "    },\n"
2409+            "  },\n"
2410+            "  \"balance\": xxxxxxx,                (numeric) Identical to balances.mine.trusted\n"

jnewbery commented at 3:52 pm on May 1, 2019:

Consider changing description to “alias for balances.mine.trusted for backwards compatibility. This filed may be removed in a future version.” and similar for below.

jnewbery commented at 3:55 pm on May 1, 2019: member

Looks good. I think this needs release notes to describe the new return object and document that getunconfirmedbalance is deprecated.

I think having a dedicated call to get the balance is useful.

How about we have a getbalances RPC that just returns the balances object added in this PR.

fanquake added the label Needs release note on May 1, 2019

MarcoFalke commented at 4:38 pm on May 1, 2019: member

How about we have a getbalances RPC that just returns the balances object added in this PR.

Then I’d rather not add the same struct to getwalletinfo. Imo a single place where to get this is enough.

jnewbery commented at 6:16 pm on May 1, 2019: member

I don’t have an issue with the same data appearing in multiple RPC methods, but if you think that’s an issue, then I’d have a slight preference for a getbalances RPC method to be separate from getwalletinfo. I think either is an improvement though.

I also think all wallet get- and list- methods should return a best block hash, so it’s clear at what point the returned values are valid.

in test/functional/wallet_balance.py:79 in fac5d96105 outdated

74+        assert_equal(self.nodes[0].getwalletinfo()['balances']['mine']['trusted'], 50)
75+        assert_equal(self.nodes[1].getwalletinfo()['balances']['mine']['trusted'], 50)
76+
77+        assert_equal(self.nodes[0].getwalletinfo()['balances']['watchonly']['immature'], 5000)
78+        assert 'watchonly' not in self.nodes[1].getwalletinfo()['balances']
79+

promag commented at 7:55 pm on May 1, 2019:

Could assert balances.min.trusted == balance as well as for unconfirmed_balance and immature_balance.

jonasschnelli commented at 8:41 am on May 2, 2019: contributor

Nice! Concept ACK.

I agree with @jnewbery about returning the new object through getbalance. Why? getwalletinfo, for historical and logical reasons, has the is_min is_spendable balance value in it and I think removing it makes little sense. We already have the same data type in getbalance and to me it would just be logical to switch from a single balance value to the new obect (in both calls).

I agree about depracating / removing getunconfirmedbalance

jonatack commented at 10:55 am on May 2, 2019: member

Concept ACK

Initial manual testing, to be completed later:

test/functional/wallet_balance.py passes locally -> EDIT: all unit + extended functional tests pass locally

getwalletinfo

0  "balances": {
1    "mine": {
2      "trusted": 0.00000000,
3      "untrusted_pending": 0.00000000,
4      "immature": 0.00000000
5    }
6  },

help getwalletinfo

 0getwalletinfo
 1Returns an object containing various wallet state info.
 2
 3Result:
 4{
 5  "walletname": xxxxx,               (string) the wallet name
 6  "walletversion": xxxxx,            (numeric) the wallet version
 7  "balances": {                      (object) the wallet balances in BTC
 8    "mine": {                        (object) balances from outputs that the wallet can sign
 9      "trusted": xxx                 (numeric) trusted balance (outputs created by the wallet or confirmed outputs)
10      "untrusted_pending": xxx       (numeric) untrusted pending balance (outputs created by others that are in the mempool)
11      "immature": xxx                (numeric) balance from immature coinbase outputs
12    },
13    "watchonly": {                   (object) watchonly balances (not present if wallet does not watch anything)
14      "trusted": xxx                 (numeric) trusted balance (outputs created by the wallet or confirmed outputs)
15      "untrusted_pending": xxx       (numeric) untrusted pending balance (outputs created by others that are in the mempool)
16      "immature": xxx                (numeric) balance from immature coinbase outputs
17    },
18  },
19  "balance": xxxxxxx,                (numeric) Identical to balances.mine.trusted
20  "unconfirmed_balance": xxx,        (numeric) Identical to balances.mine.untrusted_pending
21  "immature_balance": xxxxxx,        (numeric) Identical to balances.mine.immature
22  "txcount": xxxxxxx,                (numeric) the total number of transactions in the wallet
23  "keypoololdest": xxxxxx,           (numeric) the timestamp (seconds since Unix epoch) of the oldest pre-generated key in the key pool
24  "keypoolsize": xxxx,               (numeric) how many new keys are pre-generated (only counts external keys)
25  "keypoolsize_hd_internal": xxxx,   (numeric) how many new keys are pre-generated for internal use (used for change outputs, only appears if the wallet is using this feature, otherwise external keys are used)
26  "unlocked_until": ttt,             (numeric) the timestamp in seconds since epoch (midnight Jan 1 1970 GMT) that the wallet is unlocked for transfers, or 0 if the wallet is locked
27  "paytxfee": x.xxxx,                (numeric) the transaction fee configuration, set in BTC/kB
28  "hdseedid": "<hash160>"            (string, optional) the Hash160 of the HD seed (only present when HD is enabled)
29  "private_keys_enabled": true|false (boolean) false if privatekeys are disabled for this wallet (enforced watch-only wallet)
30}
31
32Examples:
33> bitcoin-cli getwalletinfo 
34> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "getwalletinfo", "params": [] }' -H 'content-type: text/plain;' http://127.0.0.1:8332/

getunconfirmedbalance

00.00000000

help getunconfirmedbalance

0getunconfirmedbalance
1DEPRECATED
2Identical to getwalletinfo().balances.mine.untrusted_pending

getunconfirmedbalance true

0error code: -1
1error message:
2getunconfirmedbalance
3DEPRECATED
4Identical to getwalletinfo().balances.mine.untrusted_pending

promag commented at 10:59 am on May 2, 2019: member

Concept ACK either way (getbalances or here). I tend to prefer getbalances though.

luke-jr commented at 12:15 pm on May 2, 2019: member

This seems redundant with JSON-RPC batching?

MarcoFalke commented at 12:24 pm on May 2, 2019: member

@luke-jr It would be horribly redundant to provide a separate rpc for each balance type, when it comes at no cost to return them all.

luke-jr commented at 12:43 pm on May 2, 2019: member

Perhaps just a getbalances as @jnewbery suggested then?

promag commented at 12:49 pm on May 2, 2019: member

Having a separate RPC makes getwalletinfo constant (after removing deprecated fields).

MarcoFalke commented at 12:59 pm on May 2, 2019: member

Ok, going to rework to make this a fresh getbalances call (not to be confused with getbalance)

MarcoFalke force-pushed on May 2, 2019

jnewbery commented at 1:47 pm on May 2, 2019: member

This seems redundant with JSON-RPC batching?

Note that batched RPCs are not atomic (and nor could they ever be, since the RPC server can’t take the main lock).

rpcwallet: Make helper methods const on CWallet fad13e925e

rpc: Add getbalances RPC 999931cf8f

MarcoFalke force-pushed on May 2, 2019

in src/wallet/rpcwallet.cpp:2454 in 999931cf8f outdated

2450@@ -2389,9 +2451,9 @@ static UniValue getwalletinfo(const JSONRPCRequest& request)
2451             "{\n"
2452             "  \"walletname\": xxxxx,               (string) the wallet name\n"
2453             "  \"walletversion\": xxxxx,            (numeric) the wallet version\n"
2454-            "  \"balance\": xxxxxxx,                (numeric) the total confirmed balance of the wallet in " + CURRENCY_UNIT + "\n"
2455-            "  \"unconfirmed_balance\": xxx,        (numeric) the total unconfirmed balance of the wallet in " + CURRENCY_UNIT + "\n"
2456-            "  \"immature_balance\": xxxxxx,        (numeric) the total immature balance of the wallet in " + CURRENCY_UNIT + "\n"
2457+            "  \"balance\": xxxxxxx,                (numeric) Identical to getbalances().mine.trusted\n"

promag commented at 2:40 pm on May 2, 2019:

Maybe Deprecated, use getbalances().mine.trusted instead?

jnewbery commented at 3:49 pm on May 2, 2019:

I agree this could be changed. My suggestion earlier was something like “Alias for getbalances().mine.trusted for backwards compatibility. This filed may be removed in a future version.”

MarcoFalke commented at 6:07 pm on May 2, 2019:

We have a track record of removing RPC features when users relied on them, so I’d rather keep them around redundantly instead of getting yelled at again and have users stick to EOL software instead of upgrading to a recent version.

promag commented at 1:54 pm on May 3, 2019:

@MarcoFalke agree it shouldn’t be removed, but marking deprecated encourages using getbalances instead.

MarcoFalke commented at 6:00 pm on May 3, 2019:

Done

in src/wallet/rpcwallet.cpp:2395 in 999931cf8f outdated

2390+            "    \"mine\": {                        (object) balances from outputs that the wallet can sign\n"
2391+            "      \"trusted\": xxx                 (numeric) trusted balance (outputs created by the wallet or confirmed outputs)\n"
2392+            "      \"untrusted_pending\": xxx       (numeric) untrusted pending balance (outputs created by others that are in the mempool)\n"
2393+            "      \"immature\": xxx                (numeric) balance from immature coinbase outputs\n"
2394+            "    },\n"
2395+            "    \"watchonly\": {                   (object) watchonly balances (not present if wallet does not watch anything)\n"

promag commented at 2:43 pm on May 2, 2019:

Any reason to not just send zeros?

MarcoFalke commented at 6:05 pm on May 2, 2019:

Seems too bloated

in src/wallet/rpcwallet.cpp:2411 in eeee1497ac outdated

2406+    if (request.fHelp || !help.IsValidNumArgs(request.params.size())) {
2407+        throw std::runtime_error(help.ToString());
2408+    }
2409+
2410+    // Make sure the results are valid at least up to the most recent block
2411+    // the user could have gotten from another RPC command prior to now

promag commented at 2:47 pm on May 2, 2019:

Unless a rescan is in progress?

MarcoFalke commented at 6:04 pm on May 2, 2019:

This boilerplate is copy-pasted, so I’d rather change it in another pull.

promag commented at 2:48 pm on May 2, 2019: member

I also think all wallet get- and list- methods should return a best block hash, so it’s clear at what point the returned values are valid.

Agree with @jnewbery, it can be useful.

jnewbery commented at 3:49 pm on May 2, 2019: member

utACK eeee1497aca36eea40e386d0694bbd062bd1e89d

in src/wallet/rpcwallet.cpp:73 in eeee1497ac outdated

69@@ -70,14 +70,14 @@ std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& reques
70     return wallets.size() == 1 || (request.fHelp && wallets.size() > 0) ? wallets[0] : nullptr;
71 }
72 
73-std::string HelpRequiringPassphrase(CWallet * const pwallet)
74+std::string HelpRequiringPassphrase(const CWallet* pwallet)

luke-jr commented at 5:58 pm on May 2, 2019:

Why are these losing pointer constness?

MarcoFalke commented at 6:03 pm on May 2, 2019:

We don’t really use pointer constness anywhere else in the code base. If it is required, the function should take a const reference instead.

sipa commented at 6:08 pm on May 2, 2019:

@luke-jr It is not the pointer losing constness, but the pwallet local variable losing constness.

CWallet * const pwallet means “Take a pointer-to-CWallet as argument, and store in the local variable pwallet which is const”. It’s not actually part of the function signature.

luke-jr commented at 6:27 pm on May 2, 2019:

It’s not part of the function signature, but it safeguards against accidentally changing the pointer within the function.

luke-jr commented at 6:28 pm on May 2, 2019: member

PR title/description needs update

in doc/release-notes.md:67 in eeee1497ac outdated

60@@ -61,6 +61,15 @@ platform.
61 Notable changes
62 ===============
63 
64+New RPCs
65+--------
66+
67+* `getbalances` returns an object with all balances (`mine`,

luke-jr commented at 6:29 pm on May 2, 2019:

We’ve been using a - here, not *

MarcoFalke commented at 6:08 pm on May 3, 2019:

Done: Replaced * with -

MarcoFalke renamed this:
~~rpc: Add balances to getwalletinfo~~
rpc: Add balances RPC
on May 2, 2019

rpc: Deprecate getunconfirmedbalance and getwalletinfo balances facfb4111d

MarcoFalke force-pushed on May 3, 2019

jnewbery commented at 6:56 pm on May 3, 2019: member

utACK facfb4111d14a3b06c46690a2cca7ca91cea8a96

Only change is adding “DEPRECATED” to the balance descriptions in getwalletinfo and replacing the despicable * with a glorious - in the release notes.

laanwj commented at 9:35 am on May 6, 2019: member

lightly tested ACK facfb4111d14a3b06c46690a2cca7ca91cea8a96

in doc/release-notes.md:69 in facfb4111d

60@@ -61,6 +61,15 @@ platform.
61 Notable changes
62 ===============
63 
64+New RPCs
65+--------
66+
67+- `getbalances` returns an object with all balances (`mine`,
68+  `untrusted_pending` and `immature`). Please refer to the RPC help of
69+  `getbalances` for details. The new RPC is intended to replace

laanwj commented at 9:36 am on May 6, 2019:

Good call on referring to the RPC help for details. This is what the release notes should do instead of substitute for documentation :+1:

laanwj merged this on May 6, 2019

laanwj closed this on May 6, 2019

laanwj referenced this in commit a3d2d6b067 on May 6, 2019

jonatack commented at 11:36 am on May 6, 2019: member

Post-merge tested ACK a3d2d6b0674c11e7c0cf227848322470b1cf11e9

MarcoFalke deleted the branch on May 6, 2019

MarcoFalke removed the label Needs release note on May 6, 2019

promag commented at 3:13 pm on May 6, 2019: member

utACK facfb41.

sidhujag referenced this in commit f0754725d5 on May 6, 2019

deadalnix referenced this in commit bb39798e5b on Jun 9, 2020

deadalnix referenced this in commit 4174deaa1b on Jun 9, 2020

deadalnix referenced this in commit 1b1b61fd39 on Jun 9, 2020

deadalnix referenced this in commit 0eddb715c4 on Jun 9, 2020

MarcoFalke locked this on Dec 16, 2021

rpc: Add balances RPC #15930