Closed gmale closed 3 years ago
@adityapk00 @str4d @gtank @braddmiller @lindanlee @pacu :
Hopefully, we can collaborate here and begin fleshing out something concrete in the next week or two.
If I take zcashlc.h as a reference, almost every function is affected, except
void zcashlc_vec_string_free(char **v, uintptr_t len);
void zcashlc_string_free(char *s);;
int32_t zcashlc_last_error_length(void);
void zcashlc_clear_last_error(void);
which are clearly auxiliary functions
After reading through the code in the zecwallet-light-cli I am even more convinced that we need to do this. That project is using sodiumoxide::crypto::secretbox
to unlock the wallet data file in order to load its contents into memory. Similarly, in the Android SDK, we'd like to use SQLCipher for encrypted data storage. Ensuring that SQLCipher can be used in both Kotlin and Rust will be brittle, at best, and impossible, at worst.
Creating a data access API solves both problems. Effectively, we move as much logic as we can into zcash_client_backend and get rid of zcash_client_sqlite. From there, the mobile SDKs can implement secure storage via SQLCipher and zecwallet-light-cli
can use secretbox
combined with storing data in memory.
If done well, then all the core logic like solving reorgs or checkpoint validation can live in one place inside zcash_client_backend
rather than reinventing the wheel at every turn. Librustzcash
becomes completely stateless and creating tools that leverage it becomes trivially simple.
Do we want to expose our decision of using SQLite to developers? I think we should keep that to ourselves and manage everything through the API. Probably if we want to cipher the store contents we should be asking the developer for a cipher passphrase, key or whatever SQLiteCipher uses. But not much more. We will have problems to get out of it if we want to once the mobile SDK has some adoption.
I got a little confused after this last comment.
To borrow from @gtank 's fun diagram approach :grin:, it might look something like this:
+-------------------------------------+
| |
+-------+------+ | +----+----+ Android OR |
| |-----|-----> | Data | iOS OR |
| librustzcash + | + Access | zecwallet-light |
| |<----|------ | API | Implementation |
+--------------+ | +----+----+ |
| +------+-----+ | +------------+
| | Wallet API |<-|--> | Developer |
| +------+-----+ | +------------+
+----------------+--------------------+
So, to your point @pacu, the SQLite decision is hidden from the developer. Android can use SQLCipher and neither librustzcash
nor the developer
need to know about it or care. Similarly, zecwallet-light-cli can do everything in memory and the developer doesn't care. They just know that when they call list
they get a JSON list of transactions.
Awesome! It got confusing at some point. I totally agree on that. Great ASCII art by the way
Pinging @dconnolly and @hdevalence, who may be interested in this question for Zebra.
I think the criteria and the need for this are both pretty clear, I'm comfortable putting it on the Core backlog to move this conversation along.
I'm a big fan of this proposal. It will simplify the lives of wallet developers significantly!
Right now, Zecwallet-Lite uses a fork of librustzcash
with several changes, bug and small. It also contains ~1000 lines of code that scans blocks, updates witnesses, does rollbacks etc... which I'd rather have librustzcash
do so I don't have to maintain it.
In my head, the wallet API will provide an interface to the underlying "zcash primitives" and allow wallet wallet developers to manipulate these "primitives". In my mind, the "zcash primitives" are:
Then the Wallet API allows a wallet developer to manipulate these primitives by providing new Compact blocks.
The wallet API:
The API needs to be quite details, allowing wallet developer sufficient control over the notes & Utxos. For example, the construct_sapling_transaction
API should:
This way, the wallet developer can make different tradeoffs depending on what they want their wallet to do.
Also, as has already been mentioned, the Wallet API should provide raw access to the zcash primivites - The notes, utxos, blocks - So that the wallet developer can persist/serialize them in whatever fashion.
I totally agree. To piggyback on @adityapk00's points around "Zcash Primitives" it seems we can have two layers of abstraction:
getBlock(520400)
and storeBlock(520400)
and these implementations would be both trivial and natural for developers. Default implementations (sql, in-memory, etc.) could even be made for each platform and included as optional dependencies by wallet developers.Some key goals for these layers would be:
If we can let the app control all data reads and writes it makes wallet development easier in a variety of ways. The trick will be to do this without adding a lot of complexity or inefficiency and without decreasing security or privacy.
It seems like the first step would be to create a few PoCs that just demonstrate full round-trip storage and access of a primitive object.
increment
functiongetItem
which returns the app objectsaveItem
which saves that item on the app sideblocks
containing 3 compact blocksscan
functionforEach
style of interaction where rust can just supply logic to apply to each block?storeTransaction(tx)
Another benefit of this work will be that it is easier for wallet apps to detect progress in chain scanning. In other words, instead of doing nothing during a scan, an app can indicate progress.
Currently, the scanning represents a significant portion of the sync time but we cannot give any visibility into its progress. In theory, we could monitor the dataDB for the "last scanned block" but this doesn't work in practice because of the way the database file gets locked for reading. Since scanning is continually using the sqlite db file, trying to concurrently read will not work well on Android.
Another, possibly easier, way of looking at this might be to change librustzcash
so that it:
From there the Android/iOS/JavaScript SDK's job is simply to wrap librustzcash
and manage state.
After a discussion with @adityapk00 today, it's worth mentioning that this feature also has an added bonus: decoupling the networking layer. The data access API is what allows the networking layer to live separately, which is essential for making Zcash useful across a variety of platforms.
.====================================.
| Wallet SDK Implementation |
.====================================.
| +---------+--------+ |
| | Networking Layer |<-. |
| +------------------+ | |
+-------+------+ | +----+----+ | |
| +--------> | Data | | |
| librustzcash | | | Access | +------+-----+ | +------------+
| |<---------+ API |<----| Wallet API |<--->| Developer |
+--------------+ | +----+----+ +------+-----+ | +------------+
+------------------------------------+
I've updated the ticket and diagram to reflect this. Note: this is not a change, rather we are just explicitly labeling something that was previously implicit.
Recently, there has been significant discussion on how viewing keys are stored:
here: https://github.com/zcash/zcash-android-wallet/issues/173
here: https://github.com/zcash/librustzcash/pull/246
and on the Zcash foundation discord #cryptography
channel.
This all underscores the concept that wallets should own key storage and management while libruszcash remains stateless. Then wallets can decide whether they store ovk
s and simply pass keys to the underlying libraries when scanning occurs.
It makes a lot of sense to me that librustzcash remain stateless, and delegate state management to its caller. I'm picking this issue up now on the core team side, so @gmale if you have time to have a conversation about the wallet team's needs I have bandwidth to start working on implementation.
@nuttycom that's great news! let's chat. I'll ping you.
@nuttycom any update on progress of this?
linking related WIP: https://github.com/zcash/librustzcash/pull/307
Update: Initial implementation of this has been completed on the Rust side and is nearly ready to merge. The next step is for the Wallet Team to pair with the Core Team and flesh out an end-to-end prototype similar to this comment. Once that's complete and any issues are ironed out, both mobile SDKs can move over to using this. From there, integrating t-address support is, most likely, the first new feature that will make heavy use of the Data Access API.
This issue tracks the discussion around creating an abstraction over the data that librustzcash needs to use. The goal is to define an API for this and then implement it.
As a wallet developer, I want to have a standard way to interact with the librustzcash library that gives me flexibility in how I interact with and store data so that librustzcash does not manage state.
Criteria:
Data that probably needs to be managed:
Diagram:
Diagram Legend:
Wallet SDK Implementation : Android SDK, iOS SDK, zecwallet-light, etc. that simply implement the glue that connects everything together in order to expose high level APIs for shielded wallet developers to leverage. These implementations should be as lightweight as possible, allowing everything complex to live within the robust core
librustzcash
code. networking layer : the component responsible for fetching compact blocks librustzcash : Rust crates likezcash_client_backend
which contain the core logic needed for interacting with Zcash Data Access API : interface through whichlibrustzcash
and theWallet API
communicate Wallet API : high level functionality exposed to wallet developers such asgetBalance
andgetAddress
Developer : you (probably)Background
One of the reasons we did not originally design it this way is that taking a functional approach across a language boundary (JNI) is expensive in terms of object allocation (memory), processing (cpu) and time. These are all the resources we want to conserve on mobile! This optimization may have been premature and now that we have a working product, we can create a more functional approach and benchmark its performance. Getting it right will provide a huge amount of benefits in usability and testability.