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

New RPC API #220

Closed
jrick opened this Issue Mar 31, 2015 · 13 comments

Comments

Projects
None yet
4 participants
@jrick
Copy link
Member

jrick commented Mar 31, 2015

btcwallet functionality has diverged significantly enough from Bitcoin Core wallet that it no longer makes sense to copy each and every RPC API.

One big issue I see with the Bitcoin Core wallet API is that it is a very leaky abstraction and requires nearly perfect duplication of the wallet implementation to return the same results. This is out of the question for btcwallet, so some methods simply can not be supported.

Additionally, even if btcwallet does have the capability to return the same responses as Bitcoin Core wallet, some response formats are arcane and do not correctly reflect the nature of how Bitcoin works. As btcwallet is primarily intended to be used by other clients, the less lossy the results are, the better.

General philosophies that this API should cater to:

  • Addresses are only displayed once (to discourage address reuse)
  • Addresses are grouped by account
  • Spendable outputs are group by account
  • Account funds are never mingled
  • There is no from address
  • Bitcoin is very different than many other currencies and money software. Embrace its quirks rather than hiding them.

Development of the new API must be done in conjunction with a new client. A one-shot command line client is the easiest and quickest to implement, but we will want to consider the needs of long-running and graphical clients as well.

Decisions such as encoding format are still undecided. JSON is the obvious choice, but Cap'n Proto is being considered as well. Ideally these decisions will be orthogonal to the information contained in each request and response.

The old Bitcoin Core-compatible API will remain implemented but deprecated. Users will be encouraged to use the new API where possible.

@jrick jrick added the rpc-api label Mar 31, 2015

@davecgh

This comment has been minimized.

Copy link
Member

davecgh commented Mar 31, 2015

I'd like to hear from other consumers of wallet as to any requirements they might have, since I know there are now several.

One of things I'd like to move away from in the new API is JSON-RPC. Particularly version 1.0 which makes things like optional parameters extremely cumbersome to work with.

@arnuschky

This comment has been minimized.

Copy link

arnuschky commented Apr 18, 2015

I haven't used ProtoBuffers yet, but they seem to have a number of compelling advantages. PF-serialized data is small and fast, so a PF-based API could form the base with other, optional access schemes (eg, REST / JSON or the bitcoin core legacy API) sitting on top of that.

Whichever scheme/encoding will be used, it would be great if the same scheme is used for both, btcd and btcwallet. Similarly, data and calls should follow a common logic.

Regarding the requirements in terms of functionality, I'll try to compile a list of calls that we regularly need We're still using bitcoin core in production, but as the APIs still match that's not a problem I guess. One could take inspiration from the many APIs out there, as they typically cover the most common features (see here for an incomplete list: https://bitcointalk.org/index.php?topic=942633.0, although only a few offer wallet-related API methods).

@jrick

This comment has been minimized.

Copy link
Member

jrick commented Apr 18, 2015

Cap'n Proto is developed by the primary author of Protocol Buffers v2 as a "better PB". I haven't looked at it extensively, but I'd be inclined to take his word for it. There have been some security concerns with capn, but as far as I'm aware all of these are OOB errors in the C++ implementation. I've taken a brief look at the Go implementation and it does not use any unsafe so at worst an implementation mistake may result in a runtime panic (but that's the case for anything).

That said, discussing a data format at this point is of pretty low importance. What really matters are the calls and functionality of the API.

Thanks ahead of time for checking the functionality you require! Exactly the kind of feedback we're looking for. I have some ideas of things we do and do not want, but good to compare this with others' requirements and opinions.

There's quite a bit of other tasks on the table before any of us are able to get started on this, namely SPV support. I'd like to see this in before we finalize a client API and include details that wallet may not have access to. So that gives plenty of time to bikeshed over an API.

@arnuschky

This comment has been minimized.

Copy link

arnuschky commented Apr 23, 2015

Not sure which level of comments you need, but here's a very broad overview over what we typically use. It's also a bit of a wishlist. ;)

blockchain

  • get new block event via push (currently polling)
  • get chain reorg (currently polling tip and comparing hashes)
  • get block incl all tx
  • get tx (raw, we never use the other one)

wallet

  • get new tx towards wallet via push (currently polling)
  • get all tx for a subset of addresses S, since block x
  • get unspents for a subset of addresses S, filtered by confirmation
  • wallet lock functions
  • sign tx
  • send tx

We don't use accounts or tags. If get balance is available and fast, we might use it, but typically it's not necessary. All the for a subset of addresses S can be of course generalized over arbitrary number of addresses. We usually do unspent locking not in the daemon, but I can see it being useful.

Let me know if there's anything unclear or you need more detail.

@jrick

This comment has been minimized.

Copy link
Member

jrick commented Apr 23, 2015

Rather than filtering by address, would accounts work?

Filtering by address is expensive because btcwallet doesn't maintain any kind of an address index, mapping addresses to relevant transactions that include them. (See #231 for some more details regarding this issue)

@arnuschky

This comment has been minimized.

Copy link

arnuschky commented Apr 24, 2015

Depends on what accounts means in this case. :) Accounts as in Bitcoin Core? Tagging?

Bitcoin Core accounts would be rather limiting to the apps architecture as it would require that addresses are functionally disjoint subsets (an address can't be in two accounts). Tags would work, though.

Filtering by address is expensive because btcwallet doesn't maintain any kind of an address index

I am confused, would an address index not be required for any scalable wallet? Or do you currently keep only an account to address index?

What I am describing here is also server/enterprise usage. I can see desktop usage being a subset of enterprise usage, but maybe that's not accurate and it's not feasible to provide a wallet for enterprise and desktop at the same time.

@jrick

This comment has been minimized.

Copy link
Member

jrick commented Apr 24, 2015

Yes, accounts are disjoint groups of addresses. They behave more like sub-wallets than accounts from Bitcoin Core. Non-imported addresses are derived in the HD path m/44'/<coin type (main/test/simnet)>'/<account number>'/<branch (0 for external, 1 for internal)>/<address number>. There is more separation between our accounts than in Bitcoin Core wallet, since BC's accounts really are not accounts but tags with a very poor UI (although this is being improved, and "accounts" are being deprecated). Unlike Bitcoin Core, we do not allow creating transactions that spend outputs that cross account boundaries.

We recognize that this behavior isn't always desired so we plan on eventually adding per-account tagging as well, but don't have an ETA on that.

I am confused, would an address index not be required for any scalable wallet? Or do you currently keep only an account to address index?

We keep an index of addresses to the account number they belong to, but not from addresses to relevant transactions dealing with some address. Eventually transactions will be grouped by account (the current wtxmgr PR still treats all transaction history as global to the process the same way txstore does now).

Is it because you are reusing addresses that you need to query for transactions by address? If so, I'm not sure we can or would want to support that...

@jrick

This comment has been minimized.

Copy link
Member

jrick commented Apr 30, 2015

About waiting on SPV before attacking this: I think that I may begin planning out this API more in depth before SPV lands. The reason I wanted to avoid this previously was so that we wouldn't add something to the API that would be impossible or stupid to include without an RPC connection to btcd. But SPV support still cannot be added quite at this time (needs code changes in btcd first) and it should be mostly obvious what can and can't be included in the new API. We're also not guaranteeing any kind of API stability at this time, as we haven't even hit beta yet, let alone a 1.0 release.

@arnuschky

This comment has been minimized.

Copy link

arnuschky commented May 5, 2015

Sorry for the late feedback. I don't have to add much more to this. The account feature as described will do - if the need ever arises, one could split addresses in disjoint sets.

Yes, we're also managing services that reuse addresses. But as this is an edge case, one could work around it by sticking such an address (eg, a vanity address) into a separate account...

@ysangkok

This comment has been minimized.

Copy link
Contributor

ysangkok commented Jun 9, 2015

It would be interesting if it were possible to use the API of bitcore-wallet-service, a backend for wallets like bitcore-wallet (a user-friendly CLI wallet) and copay, which already has a big user base.

I am not suggesting that a btcwallet-only API shouldn't be implemented, I am just advocating additionally implementing an API which already has wallet client UI implementations.

@jrick

This comment has been minimized.

Copy link
Member

jrick commented Jun 10, 2015

We can look to other APIs for inspiration but I don't want to copy anymore. The API is a representation of what the program can and cannot do, and btcwallet is different enough from other wallets that not all aspects of it map well to another application's interface. For instance, even ignoring the inconsistencies and silly information provided by the Bitcoin Core JSON-RPC API, the ideas of accounts are totally different between us and them and there's no way to remain perfectly compatible in behavior even if the data representation in the API is the same.

@jrick

This comment has been minimized.

Copy link
Member

jrick commented Dec 8, 2015

The infrastructure for this is part of #337. The API itself is still unstable and only has the minimum implemented that I required to get a working GUI client.

@jrick

This comment has been minimized.

Copy link
Member

jrick commented Mar 9, 2016

Closing this issue since the new API is now available in master. Please open new issues for any feature requests or changes to the API.

@jrick jrick closed this Mar 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment