Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

z_getbalance deprecation (question) #5925

Open
josef-v opened this issue May 5, 2022 · 4 comments
Open

z_getbalance deprecation (question) #5925

josef-v opened this issue May 5, 2022 · 4 comments
Labels
A-rpc-interface Area: RPC interface A-wallet Area: Wallet D-bitcoin-divergence Design issue: Divergence from Bitcoin (upstream code and/or architecture). usability
Milestone

Comments

@josef-v
Copy link

josef-v commented May 5, 2022

I am confused by the deprecation process and by deprecation of z_getbalance
Release notes state that:

(z_getbalanceforviewingkey) This API has been added to supplement (and largely supplant) z_getbalance

so it's not to supplement but to supplant only, if the z_getbalance gets deprecated?

What I have problem with is that z_getbalanceforviewingkey does a different thing than z_getbalance. With z_getbalance I have a simple API which lets me know what is an outstanding balance on my addresses including transparent addresses. With z_getbalanceforviewingkey, I have to get a viewing key for the address first and than use this method to emulate the same functionality. Also, I am not able to get a viewing key for a transparent address, am I?

This goes against usage of other methods like z_sendmany which uses address as a first argument. Typical use case would be checking that I have sufficient funds on that address by z_getbalance and then issuing z_sendmany.

What I understand from the changes in the last release, zcash wants to move user experience from addresses to accounts (my simplified view of that) but I am not sure if it's a good idea to deprecate the previously used approach/methods. And I admit that I am just being selfish in this because deprecation of these methods will raise a lot of new tasks for me to implement in our codebase where we use z_getbalance a lot :)

Note:
There is a document doc/book/src/user/deprecation.md merged into master which contains information about deprecated methods and z_getbalance is missing.

@str4d
Copy link
Contributor

str4d commented May 5, 2022

What I have problem with is that z_getbalanceforviewingkey does a different thing than z_getbalance. With z_getbalance I have a simple API which lets me know what is an outstanding balance on my addresses including transparent addresses.

This is an interesting problem. The zcashd wallet internally does not treat individual transparent addresses (returned by the now-deprecated getnewaddress) as individual "buckets of funds", instead treating all transparent addresses as part of a single "transparent bucket". This behaviour is inherited from Bitcoin Core, and corresponds to how APIs like sendtoaddress work. This is also why we haven't deprecated getbalance, as that correctly gets the balance of the entire legacy transparent wallet.

For shielded funds, we wanted each spending key to be an independent "bucket of funds", which is why z_getnewaddress and z_sendmany used addresses to indicate the funds source. But as a side-effect, they also enabled users to pretend that the wallet could treat individual transparent addresses as separate buckets of funds. In hindsight this was a mistake, as anyone who attempts to use the legacy transparent addresses in this way, will have their mental model of their wallet broken the moment they use any Bitcoin-inherted APIs (which do not respect per-address distinctions).

The new account-based APIs are intended to rectify this rather messy situation: each account is treated as a separate bucket of funds, and within that we can have multiple (diversified) addresses. The "bucket of funds" then becomes tied to the account (or equivalently, its full viewing key). We maintain z_sendmany support by selecting funds from the account tied to the given UA, rather than solely from funds sent to that UA (and then having the wallet decide how best to select funds within that account).

So my immediate question is, in what way are you using z_getbalance for transparent addresses? I'd like to figure out how your use case could be migrated to using accounts, in order to determine whether there is missing functionality we need to implement.

With z_getbalanceforviewingkey, I have to get a viewing key for the address first and than use this method to emulate the same functionality. Also, I am not able to get a viewing key for a transparent address, am I?

z_getbalanceforviewingkey only works for account viewing keys, and legacy shielded viewing keys. For the reasons above, we don't want to continue supporting per-address legacy transparent balance logic, so this API isn't going to ever support querying the balance of an address in the legacy transparent bucket.

This goes against usage of other methods like z_sendmany which uses address as a first argument. Typical use case would be checking that I have sufficient funds on that address by z_getbalance and then issuing z_sendmany.

Yep, this is indeed a problem with the way z_sendmany was designed. This was also why we never deployed diversified addresses for Sapling: it made the mental model too hard to follow with this API.

We plan to implement new RPC methods that more directly focus on a usable workflow for sending funds from accounts. In particular, this is likely to be a two-phase process:

  • First, call some RPC method with the desired transaction outcomes (account to send from, recipients, etc.). This will return an object representing the transaction effects, such as what funds will be selected, and what privacy properties the resulting transaction would have.
    • The caller would then check these effects against its own policies, display these effects to the user for confirmation, etc.
  • If the user agrees to the transaction effects, call a second RPC method that "confirms" the transaction, allowing the wallet to sign and send it.

With this model, the "check I have sufficient balance, then send" workflow would be entirely replaced by this new workflow; the returned transaction effects could include "you don't have sufficient balance to send this transaction". But also z_getbalanceforaccount is sufficient for this balance-checking.

What I understand from the changes in the last release, zcash wants to move user experience from addresses to accounts (my simplified view of that) but I am not sure if it's a good idea to deprecate the previously used approach/methods. And I admit that I am just being selfish in this because deprecation of these methods will raise a lot of new tasks for me to implement in our codebase where we use z_getbalance a lot :)

That's a fair consideration, and why I'd like to learn both what your use case is, and what kind of effort a migration would require. We definitely want to move people away from the legacy APIs (so we can deprecate and eventually remove the transparent-only wallet components), and the new deprecation process is meant to make this more concretely visible and with a more well-defined timeline.

Note: There is a document doc/book/src/user/deprecation.md merged into master which contains information about deprecated methods and z_getbalance is missing.

Whoops, we missed this, thanks! This is fixed in #5926.

@str4d str4d added A-wallet Area: Wallet A-rpc-interface Area: RPC interface D-bitcoin-divergence Design issue: Divergence from Bitcoin (upstream code and/or architecture). labels May 5, 2022
@str4d str4d added this to the Release 5.1.0 milestone May 5, 2022
@nuttycom
Copy link
Contributor

@josef-v Does this answer your question? If you need to have the ability to get the balance for a single transparent address, can you answer @str4d's question here:

So my immediate question is, in what way are you using z_getbalance for transparent addresses? I'd like to figure out how your use case could be migrated to using accounts, in order to determine whether there is missing functionality we need to implement.

@nuttycom nuttycom modified the milestones: Release 5.2.0, Release 5.3.0 Jul 15, 2022
@daira daira removed this from the Release 5.3.0 milestone Sep 19, 2022
@str4d
Copy link
Contributor

str4d commented Jan 5, 2023

Following the deprecation timeline, the earliest we could have disabled z_getbalance by default is 5.3.0. We are going to disable it by default in 5.4.0 with #6282. It can still be re-enabled with -allowdeprecated=z_getbalance, and we won't be removing it (#6326) until at least the third minor release after 5.4.0.

Per the above discussion, I would still like to hear your use-case @josef-v if you have time to write it up.

@josef-v
Copy link
Author

josef-v commented Jan 6, 2023

This doesn't concern me anymore as we have discontinued support for zcash.

Our whole codebase was based on bitcoind and it's rpc. Having similar methods (with different names only) for zcash was making things simple for us. Sure, there were some differences like shielding, but from the top level view, zcash behaved the same as bitcoin most of the time.
Once you have your whole workflow based on addresses, it might be quite a lot of work to switch to something else. I don't really think there was anything that couldn't be switched to accounts with some effort, but it still required effort.
Also having to treat shielded addresses differently than transparent addresses...

So the main problem with this change for me is that it breaks legacy behavior and I don't see a reason for it to be necessary to break it.

Sorry for not giving you any specific example, but zcash is out of my scope for some time already.

@daira daira added this to the Release 5.7.0 milestone Jan 26, 2023
@str4d str4d modified the milestones: Release 5.7.0, Release 5.8.0 Sep 21, 2023
@str4d str4d modified the milestones: Release 5.8.0, Post 5.8.0 Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc-interface Area: RPC interface A-wallet Area: Wallet D-bitcoin-divergence Design issue: Divergence from Bitcoin (upstream code and/or architecture). usability
Projects
None yet
Development

No branches or pull requests

4 participants