Merge pull request #637 from nuttycom/wallet/create_account

Add WalletWrite::create_account function

zcash_client_backend/CHANGELOG.md

@@ -34,6 +34,7 @@ and this library adheres to Rust's notion of
   - `WalletRead::get_unified_full_viewing_keys`
   - `WalletRead::get_current_address`
   - `WalletRead::get_all_nullifiers`
+  - `WalletWrite::create_account`
   - `WalletWrite::remove_unmined_tx` (behind the `unstable` feature flag).
   - `WalletWrite::get_next_available_address`
 - `zcash_client_backend::proto`:

zcash_client_backend/Cargo.toml

@@ -35,6 +35,7 @@ rand_core = "0.6"
 rayon = "1.5"
 ripemd = { version = "0.1", optional = true }
 secp256k1 = { version = "0.21", optional = true }
+secrecy = "0.8"
 sha2 = { version = "0.10.1", optional = true }
 subtle = "2.2.3"
 time = "0.2"

zcash_client_backend/src/data_api.rs

@@ -7,6 +7,7 @@ use std::fmt::Debug;
 #[cfg(feature = "transparent-inputs")]
 use std::collections::HashSet;
 
+use secrecy::SecretVec;
 use zcash_primitives::{
     block::BlockHash,
     consensus::BlockHeight,
@@ -20,7 +21,7 @@ use zcash_primitives::{
 use crate::{
     address::{RecipientAddress, UnifiedAddress},
     decrypt::DecryptedOutput,
-    keys::UnifiedFullViewingKey,
+    keys::{UnifiedFullViewingKey, UnifiedSpendingKey},
     proto::compact_formats::CompactBlock,
     wallet::{SpendableNote, WalletTx},
 };
@@ -269,6 +270,26 @@ pub struct SentTransactionOutput<'a> {
 /// This trait encapsulates the write capabilities required to update stored
 /// wallet data.
 pub trait WalletWrite: WalletRead {
+    /// Tells the wallet to track the next available account-level spend authority, given
+    /// the current set of [ZIP 316] account identifiers known to the wallet database.
+    ///
+    /// Returns the account identifier for the newly-created wallet database entry, along
+    /// with the associated [`UnifiedSpendingKey`].
+    ///
+    /// If `seed` was imported from a backup and this method is being used to restore a
+    /// previous wallet state, you should use this method to add all of the desired
+    /// accounts before scanning the chain from the seed's birthday height.
+    ///
+    /// By convention, wallets should only allow a new account to be generated after funds
+    /// have been received by the currently-available account (in order to enable
+    /// automated account recovery).
+    ///
+    /// [ZIP 316]: https://zips.z.cash/zip-0316
+    fn create_account(
+        &mut self,
+        seed: &SecretVec<u8>,
+    ) -> Result<(AccountId, UnifiedSpendingKey), Self::Error>;
+
     /// Generates and persists the next available diversified address, given the current
     /// addresses known to the wallet.
     ///
@@ -353,6 +374,7 @@ pub trait BlockSource {
 
 #[cfg(feature = "test-dependencies")]
 pub mod testing {
+    use secrecy::{ExposeSecret, SecretVec};
     use std::collections::HashMap;
 
     #[cfg(feature = "transparent-inputs")]
@@ -360,7 +382,7 @@ pub mod testing {
 
     use zcash_primitives::{
         block::BlockHash,
-        consensus::BlockHeight,
+        consensus::{BlockHeight, Network},
         legacy::TransparentAddress,
         memo::Memo,
         merkle_tree::{CommitmentTree, IncrementalWitness},
@@ -371,7 +393,7 @@ pub mod testing {
 
     use crate::{
         address::UnifiedAddress,
-        keys::UnifiedFullViewingKey,
+        keys::{UnifiedFullViewingKey, UnifiedSpendingKey},
         proto::compact_formats::CompactBlock,
         wallet::{SpendableNote, WalletTransparentOutput},
     };
@@ -402,7 +424,9 @@ pub mod testing {
         }
     }
 
-    pub struct MockWalletDb {}
+    pub struct MockWalletDb {
+        pub network: Network,
+    }
 
     impl WalletRead for MockWalletDb {
         type Error = Error<u32>;
@@ -521,6 +545,16 @@ pub mod testing {
     }
 
     impl WalletWrite for MockWalletDb {
+        fn create_account(
+            &mut self,
+            seed: &SecretVec<u8>,
+        ) -> Result<(AccountId, UnifiedSpendingKey), Self::Error> {
+            let account = AccountId::from(0);
+            UnifiedSpendingKey::from_seed(&self.network, seed.expose_secret(), account)
+                .map(|k| (account, k))
+                .map_err(|_| Error::KeyDerivationError(account))
+        }
+
         fn get_next_available_address(
             &mut self,
             _account: AccountId,

zcash_client_backend/src/data_api/chain.rs

@@ -29,7 +29,9 @@
 //! # fn test() -> Result<(), Error<u32>> {
 //! let network = Network::TestNetwork;
 //! let db_cache = testing::MockBlockSource {};
-//! let mut db_data = testing::MockWalletDb {};
+//! let mut db_data = testing::MockWalletDb {
+//!     network: Network::TestNetwork
+//! };
 //!
 //! // 1) Download new CompactBlocks into db_cache.
 //!

zcash_client_backend/src/data_api/error.rs

@@ -71,6 +71,10 @@ pub enum Error<NoteId> {
 
     /// It is forbidden to provide a memo when constructing a transparent output.
     MemoForbidden,
+
+    /// An error occurred deriving a spending key from a seed and an account
+    /// identifier.
+    KeyDerivationError(AccountId),
 }
 
 impl ChainInvalid {
@@ -122,6 +126,7 @@ impl<N: fmt::Display> fmt::Display for Error<N> {
             Error::Protobuf(e) => write!(f, "{}", e),
             Error::SaplingNotActive => write!(f, "Could not determine Sapling upgrade activation height."),
             Error::MemoForbidden => write!(f, "It is not possible to send a memo to a transparent address."),
+            Error::KeyDerivationError(acct_id) => write!(f, "Key derivation failed for account {:?}", acct_id),
         }
     }
 }

zcash_client_backend/src/data_api/wallet.rs

@@ -149,7 +149,9 @@ where
 /// let extsk = sapling::spending_key(&[0; 32][..], COIN_TYPE, account);
 /// let to = extsk.default_address().1.into();
 ///
-/// let mut db_read = testing::MockWalletDb {};
+/// let mut db_read = testing::MockWalletDb {
+///     network: Network::TestNetwork
+/// };
 ///
 /// create_spend_to_address(
 ///     &mut db_read,

zcash_client_sqlite/CHANGELOG.md

@@ -18,6 +18,10 @@ and this library adheres to Rust's notion of
     rewinds exceed supported bounds.
   - `SqliteClientError::DiversifierIndexOutOfRange`, to report when the space
     of available diversifier indices has been exhausted. 
+  - `SqliteClientError::AccountIdDiscontinuity`, to report when a user attempts
+    to initialize the accounts table with a noncontiguous set of account identifiers.
+  - `SqliteClientError::AccountIdOutOfRange`, to report when the maximum account
+    identifier has been reached.
 - An `unstable` feature flag; this is added to parts of the API that may change
   in any release. It enables `zcash_client_backend`'s `unstable` feature flag.
 - New summary views that may be directly accessed in the sqlite database.

zcash_client_sqlite/src/error.rs

@@ -7,7 +7,7 @@ use zcash_client_backend::{
     data_api,
     encoding::{Bech32DecodeError, TransparentCodecError},
 };
-use zcash_primitives::consensus::BlockHeight;
+use zcash_primitives::{consensus::BlockHeight, zip32::AccountId};
 
 use crate::{NoteId, PRUNING_HEIGHT};
 
@@ -69,6 +69,17 @@ pub enum SqliteClientError {
     /// The space of allocatable diversifier indices has been exhausted for
     /// the given account.
     DiversifierIndexOutOfRange,
+
+    /// An error occurred deriving a spending key from a seed and an account
+    /// identifier.
+    KeyDerivationError(AccountId),
+
+    /// A caller attempted to initialize the accounts table with a discontinuous
+    /// set of account identifiers.
+    AccountIdDiscontinuity,
+
+    /// A caller attempted to construct a new account with an invalid account identifier.
+    AccountIdOutOfRange,
 }
 
 impl error::Error for SqliteClientError {
@@ -108,6 +119,9 @@ impl fmt::Display for SqliteClientError {
             SqliteClientError::InvalidMemo(e) => write!(f, "{}", e),
             SqliteClientError::BackendError(e) => write!(f, "{}", e),
             SqliteClientError::DiversifierIndexOutOfRange => write!(f, "The space of available diversifier indices is exhausted"),
+            SqliteClientError::KeyDerivationError(acct_id) => write!(f, "Key derivation failed for account {:?}", acct_id),
+            SqliteClientError::AccountIdDiscontinuity => write!(f, "Wallet account identifiers must be sequential."),
+            SqliteClientError::AccountIdOutOfRange => write!(f, "Wallet account identifiers must be less than 0x7FFFFFFF."),
         }
     }
 }

zcash_client_sqlite/src/lib.rs

@@ -32,6 +32,7 @@
 // Catch documentation errors caused by code changes.
 #![deny(rustdoc::broken_intra_doc_links)]
 
+use secrecy::{ExposeSecret, SecretVec};
 use std::collections::HashMap;
 use std::fmt;
 use std::path::Path;
@@ -56,7 +57,7 @@ use zcash_client_backend::{
     data_api::{
         BlockSource, DecryptedTransaction, PrunedBlock, SentTransaction, WalletRead, WalletWrite,
     },
-    keys::UnifiedFullViewingKey,
+    keys::{UnifiedFullViewingKey, UnifiedSpendingKey},
     proto::compact_formats::CompactBlock,
     wallet::SpendableNote,
 };
@@ -402,6 +403,33 @@ impl<'a, P: consensus::Parameters> DataConnStmtCache<'a, P> {
 
 #[allow(deprecated)]
 impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> {
+    fn create_account(
+        &mut self,
+        seed: &SecretVec<u8>,
+    ) -> Result<(AccountId, UnifiedSpendingKey), Self::Error> {
+        self.transactionally(|stmts| {
+            let account = wallet::get_max_account_id(stmts.wallet_db)?
+                .map(|a| AccountId::from(u32::from(a) + 1))
+                .unwrap_or_else(|| AccountId::from(0));
+
+            if u32::from(account) >= 0x7FFFFFFF {
+                return Err(SqliteClientError::AccountIdOutOfRange);
+            }
+
+            let usk = UnifiedSpendingKey::from_seed(
+                &stmts.wallet_db.params,
+                seed.expose_secret(),
+                account,
+            )
+            .map_err(|_| SqliteClientError::KeyDerivationError(account))?;
+            let ufvk = usk.to_unified_full_viewing_key();
+
+            wallet::add_account(stmts.wallet_db, account, &ufvk)?;
+
+            Ok((account, usk))
+        })
+    }
+
     fn get_next_available_address(
         &mut self,
         account: AccountId,

zcash_client_sqlite/src/wallet.rs

@@ -179,6 +179,60 @@ pub fn get_address<P: consensus::Parameters>(
         })
 }
 
+pub(crate) fn get_max_account_id<P>(
+    wdb: &WalletDb<P>,
+) -> Result<Option<AccountId>, SqliteClientError> {
+    // This returns the most recently generated address.
+    Ok(wdb
+        .conn
+        .query_row("SELECT MAX(account) FROM accounts", NO_PARAMS, |row| {
+            row.get::<_, u32>(0).map(AccountId::from)
+        })
+        .optional()?)
+}
+
+pub(crate) fn add_account<P: consensus::Parameters>(
+    wdb: &WalletDb<P>,
+    account: AccountId,
+    key: &UnifiedFullViewingKey,
+) -> Result<(), SqliteClientError> {
+    add_account_internal(&wdb.conn, &wdb.params, "accounts", account, key)
+}
+
+pub(crate) fn add_account_internal<P: consensus::Parameters, E: From<rusqlite::Error>>(
+    conn: &rusqlite::Connection,
+    network: &P,
+    accounts_table: &'static str,
+    account: AccountId,
+    key: &UnifiedFullViewingKey,
+) -> Result<(), E> {
+    let ufvk_str: String = key.encode(network);
+    conn.execute_named(
+        &format!(
+            "INSERT INTO {} (account, ufvk) VALUES (:account, :ufvk)",
+            accounts_table
+        ),
+        &[(":account", &<u32>::from(account)), (":ufvk", &ufvk_str)],
+    )?;
+
+    // Always derive the default Unified Address for the account.
+    let (address, mut idx) = key.default_address();
+    let address_str: String = address.encode(network);
+    // the diversifier index is stored in big-endian order to allow sorting
+    idx.0.reverse();
+    conn.execute_named(
+        "INSERT INTO addresses (account, diversifier_index_be, address)
+        VALUES (:account, :diversifier_index_be, :address)",
+        &[
+            (":account", &<u32>::from(account)),
+            (":diversifier_index_be", &&idx.0[..]),
+            (":address", &address_str),
+        ],
+    )?;
+
+    Ok(())
+}
+
 pub(crate) fn get_current_address<P: consensus::Parameters>(
     wdb: &WalletDb<P>,
     account: AccountId,

zcash_client_sqlite/src/wallet/init.rs

@@ -1,5 +1,5 @@
 //! Functions for initializing the various databases.
-use rusqlite::{self, params, types::ToSql, Connection, NO_PARAMS};
+use rusqlite::{self, params, types::ToSql, NO_PARAMS};
 use schemer::{migration, Migration, Migrator, MigratorError};
 use schemer_rusqlite::{RusqliteAdapter, RusqliteMigration};
 use secrecy::{ExposeSecret, SecretVec};
@@ -19,7 +19,11 @@ use zcash_client_backend::{
     keys::{UnifiedFullViewingKey, UnifiedSpendingKey},
 };
 
-use crate::{error::SqliteClientError, wallet::PoolType, WalletDb};
+use crate::{
+    error::SqliteClientError,
+    wallet::{self, PoolType},
+    WalletDb,
+};
 
 #[cfg(feature = "transparent-inputs")]
 use {
@@ -516,12 +520,19 @@ fn init_wallet_db_internal<P: consensus::Parameters + 'static>(
     Ok(())
 }
 
-/// Initialises the data database with the given [`UnifiedFullViewingKey`]s.
+/// Initialises the data database with the given set of account [`UnifiedFullViewingKey`]s.
+///
+/// **WARNING** This method should be used with care, and should ordinarily be unnecessary.
+/// Prefer to use [`WalletWrite::create_account`] instead.
+///
+/// [`WalletWrite::create_account`]: zcash_client_backend::data_api::WalletWrite::create_account
 ///
 /// The [`UnifiedFullViewingKey`]s are stored internally and used by other APIs such as
-/// [`get_address`], [`scan_cached_blocks`], and [`create_spend_to_address`]. `extfvks` **MUST**
+/// [`get_address`], [`scan_cached_blocks`], and [`create_spend_to_address`]. Account identifiers
-/// be arranged in account-order; that is, the [`UnifiedFullViewingKey`] for ZIP 32
+/// in `keys` **MUST** form a consecutive sequence beginning at account 0, and the
-/// account `i` **MUST** be at `extfvks[i]`.
+/// [`UnifiedFullViewingKey`] corresponding to a given account identifier **MUST** be derived from
+/// the wallet's mnemonic seed at the BIP-44 `account` path level as described by
+/// [ZIP 316](https://zips.z.cash/zip-0316)
 ///
 /// # Examples
 ///
@@ -575,56 +586,23 @@ pub fn init_accounts_table<P: consensus::Parameters>(
         return Err(SqliteClientError::TableNotEmpty);
     }
 
+    // Ensure that the account identifiers are sequential and begin at zero.
+    if let Some(account_id) = keys.keys().max() {
+        if usize::try_from(u32::from(*account_id)).unwrap() >= keys.len() {
+            return Err(SqliteClientError::AccountIdDiscontinuity);
+        }
+    }
+
     // Insert accounts atomically
     wdb.conn.execute("BEGIN IMMEDIATE", NO_PARAMS)?;
     for (account, key) in keys.iter() {
-        add_account_internal::<P, SqliteClientError>(
+        wallet::add_account(wdb, *account, key)?;
-            &wdb.params,
-            &wdb.conn,
-            "accounts",
-            *account,
-            key,
-        )?;
     }
     wdb.conn.execute("COMMIT", NO_PARAMS)?;
 
     Ok(())
 }
 
-fn add_account_internal<P: consensus::Parameters, E: From<rusqlite::Error>>(
-    network: &P,
-    conn: &Connection,
-    accounts_table: &'static str,
-    account: AccountId,
-    key: &UnifiedFullViewingKey,
-) -> Result<(), E> {
-    let ufvk_str: String = key.encode(network);
-    conn.execute_named(
-        &format!(
-            "INSERT INTO {} (account, ufvk) VALUES (:account, :ufvk)",
-            accounts_table
-        ),
-        &[(":account", &<u32>::from(account)), (":ufvk", &ufvk_str)],
-    )?;
-
-    // Always derive the default Unified Address for the account.
-    let (address, mut idx) = key.default_address();
-    let address_str: String = address.encode(network);
-    // the diversifier index is stored in big-endian order to allow sorting
-    idx.0.reverse();
-    conn.execute_named(
-        "INSERT INTO addresses (account, diversifier_index_be, address)
-        VALUES (:account, :diversifier_index_be, :address)",
-        &[
-            (":account", &<u32>::from(account)),
-            (":diversifier_index_be", &&idx.0[..]),
-            (":address", &address_str),
-        ],
-    )?;
-
-    Ok(())
-}
-
 /// Initialises the data database with the given block.
 ///
 /// This enables a newly-created database to be immediately-usable, without needing to
@@ -706,6 +684,7 @@ mod tests {
     };
 
     use crate::{
+        error::SqliteClientError,
         tests::{self, network},
         wallet::get_address,
         AccountId, WalletDb,
@@ -1399,6 +1378,35 @@ mod tests {
         init_accounts_table(&db_data, &ufvks).unwrap_err();
     }
 
+    #[test]
+    fn init_accounts_table_allows_no_gaps() {
+        let data_file = NamedTempFile::new().unwrap();
+        let mut db_data = WalletDb::for_path(data_file.path(), network()).unwrap();
+        init_wallet_db(&mut db_data, Some(Secret::new(vec![]))).unwrap();
+
+        // allow sequential initialization
+        let seed = [0u8; 32];
+        let ufvks = |ids: &[u32]| {
+            ids.iter()
+                .map(|a| {
+                    let account = AccountId::from(*a);
+                    UnifiedSpendingKey::from_seed(&network(), &seed, account)
+                        .map(|k| (account, k.to_unified_full_viewing_key()))
+                        .unwrap()
+                })
+                .collect::<HashMap<_, _>>()
+        };
+
+        // should fail if we have a gap
+        assert!(matches!(
+            init_accounts_table(&db_data, &ufvks(&[0, 2])),
+            Err(SqliteClientError::AccountIdDiscontinuity)
+        ));
+
+        // should succeed if there are no gaps
+        assert!(init_accounts_table(&db_data, &ufvks(&[0, 1, 2])).is_ok());
+    }
+
     #[test]
     fn init_blocks_table_only_works_once() {
         let data_file = NamedTempFile::new().unwrap();

zcash_client_sqlite/src/wallet/init/migrations/addresses_table.rs

@@ -7,7 +7,8 @@ use uuid::Uuid;
 use zcash_client_backend::{address::RecipientAddress, keys::UnifiedFullViewingKey};
 use zcash_primitives::{consensus, zip32::AccountId};
 
-use super::super::{add_account_internal, WalletMigrationError};
+use super::super::WalletMigrationError;
+use crate::wallet::add_account_internal;
 
 #[cfg(feature = "transparent-inputs")]
 use zcash_primitives::legacy::keys::IncomingViewingKey;
@@ -153,8 +154,8 @@ impl<P: consensus::Parameters> RusqliteMigration for AddressesTableMigration<P>
             }
 
             add_account_internal::<P, WalletMigrationError>(
-                &self.params,
                 transaction,
+                &self.params,
                 "accounts_new",
                 account,
                 &ufvk,