From d0062a87d4be6ad435974644828796d004a0a1f1 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 13 Sep 2022 16:43:04 -0600 Subject: [PATCH 1/3] Add WalletWrite::create_account function --- zcash_client_backend/CHANGELOG.md | 1 + zcash_client_backend/Cargo.toml | 1 + zcash_client_backend/src/data_api.rs | 30 +++++++-- zcash_client_backend/src/data_api/chain.rs | 4 +- zcash_client_backend/src/data_api/error.rs | 5 ++ zcash_client_backend/src/data_api/wallet.rs | 4 +- zcash_client_sqlite/src/error.rs | 7 ++- zcash_client_sqlite/src/lib.rs | 26 +++++++- zcash_client_sqlite/src/wallet.rs | 54 ++++++++++++++++ zcash_client_sqlite/src/wallet/init.rs | 63 +++++-------------- .../wallet/init/migrations/addresses_table.rs | 5 +- 11 files changed, 143 insertions(+), 57 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index ade2268be..ce03b8e3b 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/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`: diff --git a/zcash_client_backend/Cargo.toml b/zcash_client_backend/Cargo.toml index 4e0c87e46..4f2f002fd 100644 --- a/zcash_client_backend/Cargo.toml +++ b/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" diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 388cde921..c5228f23a 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -1,5 +1,6 @@ //! Interfaces for wallet data persistence & low-level wallet utilities. +use secrecy::SecretVec; use std::cmp; use std::collections::HashMap; use std::fmt::Debug; @@ -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,14 @@ pub struct SentTransactionOutput<'a> { /// This trait encapsulates the write capabilities required to update stored /// wallet data. pub trait WalletWrite: WalletRead { + /// Creates a new account-level spending authority by derivation from the provided + /// seed using the next available unused account identifier, and returns this identifier + /// along with the generated unified spending key. + fn create_account( + &mut self, + seed: SecretVec, + ) -> Result<(AccountId, UnifiedSpendingKey), Self::Error>; + /// Generates and persists the next available diversified address, given the current /// addresses known to the wallet. /// @@ -353,6 +362,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 +370,7 @@ pub mod testing { use zcash_primitives::{ block::BlockHash, - consensus::BlockHeight, + consensus::{BlockHeight, Network}, legacy::TransparentAddress, memo::Memo, merkle_tree::{CommitmentTree, IncrementalWitness}, @@ -371,7 +381,7 @@ pub mod testing { use crate::{ address::UnifiedAddress, - keys::UnifiedFullViewingKey, + keys::{UnifiedFullViewingKey, UnifiedSpendingKey}, proto::compact_formats::CompactBlock, wallet::{SpendableNote, WalletTransparentOutput}, }; @@ -402,7 +412,9 @@ pub mod testing { } } - pub struct MockWalletDb {} + pub struct MockWalletDb { + pub network: Network, + } impl WalletRead for MockWalletDb { type Error = Error; @@ -521,6 +533,16 @@ pub mod testing { } impl WalletWrite for MockWalletDb { + fn create_account( + &mut self, + seed: SecretVec, + ) -> 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, diff --git a/zcash_client_backend/src/data_api/chain.rs b/zcash_client_backend/src/data_api/chain.rs index b5d9567a2..f3a4039f1 100644 --- a/zcash_client_backend/src/data_api/chain.rs +++ b/zcash_client_backend/src/data_api/chain.rs @@ -29,7 +29,9 @@ //! # fn test() -> Result<(), Error> { //! 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. //! diff --git a/zcash_client_backend/src/data_api/error.rs b/zcash_client_backend/src/data_api/error.rs index 22b81ae1f..d116fc849 100644 --- a/zcash_client_backend/src/data_api/error.rs +++ b/zcash_client_backend/src/data_api/error.rs @@ -71,6 +71,10 @@ pub enum Error { /// 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 fmt::Display for Error { 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), } } } diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 42128d3c3..e6d1e6b3c 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/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, diff --git a/zcash_client_sqlite/src/error.rs b/zcash_client_sqlite/src/error.rs index 536f9536b..a9afc4a7b 100644 --- a/zcash_client_sqlite/src/error.rs +++ b/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,10 @@ 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), } impl error::Error for SqliteClientError { @@ -108,6 +112,7 @@ 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), } } } diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 5e0a8de0a..3113feebc 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/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,29 @@ 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, + ) -> 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)); + + 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, diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 4bde63fcf..2bc002653 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -179,6 +179,60 @@ pub fn get_address( }) } +pub(crate) fn get_max_account_id

( + wdb: &WalletDb

, +) -> Result, 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( + wdb: &WalletDb

, + account: AccountId, + key: &UnifiedFullViewingKey, +) -> Result<(), SqliteClientError> { + add_account_internal(&wdb.conn, &wdb.params, "accounts", account, key) +} + +pub(crate) fn add_account_internal>( + 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", &::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", &::from(account)), + (":diversifier_index_be", &&idx.0[..]), + (":address", &address_str), + ], + )?; + + Ok(()) +} + pub(crate) fn get_current_address( wdb: &WalletDb

, account: AccountId, diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 75ab95782..0a45ee416 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/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,17 @@ fn init_wallet_db_internal( 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 [`zcash_client_backend::data_api::WalletWrite::create_account`] instead. /// /// 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** -/// be arranged in account-order; that is, the [`UnifiedFullViewingKey`] for ZIP 32 -/// account `i` **MUST** be at `extfvks[i]`. +/// [`get_address`], [`scan_cached_blocks`], and [`create_spend_to_address`]. Account identifiers +/// in `keys` **MUST** form a consecutive sequence beginning at account 0, and the +/// [`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 /// @@ -578,53 +587,13 @@ pub fn init_accounts_table( // Insert accounts atomically wdb.conn.execute("BEGIN IMMEDIATE", NO_PARAMS)?; for (account, key) in keys.iter() { - add_account_internal::( - &wdb.params, - &wdb.conn, - "accounts", - *account, - key, - )?; + wallet::add_account(wdb, *account, key)?; } wdb.conn.execute("COMMIT", NO_PARAMS)?; Ok(()) } -fn add_account_internal>( - 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", &::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", &::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 diff --git a/zcash_client_sqlite/src/wallet/init/migrations/addresses_table.rs b/zcash_client_sqlite/src/wallet/init/migrations/addresses_table.rs index 9b22f3eff..84417af2c 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/addresses_table.rs +++ b/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 RusqliteMigration for AddressesTableMigration

} add_account_internal::( - &self.params, transaction, + &self.params, "accounts_new", account, &ufvk, From 75eb082203898bf6ceb355d976993d03b18c3fa3 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 14 Sep 2022 12:20:39 -0600 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: str4d --- zcash_client_backend/src/data_api.rs | 22 +++++++++++++++++----- zcash_client_sqlite/src/lib.rs | 2 +- zcash_client_sqlite/src/wallet/init.rs | 4 +++- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index c5228f23a..7d188411c 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -270,12 +270,24 @@ pub struct SentTransactionOutput<'a> { /// This trait encapsulates the write capabilities required to update stored /// wallet data. pub trait WalletWrite: WalletRead { - /// Creates a new account-level spending authority by derivation from the provided - /// seed using the next available unused account identifier, and returns this identifier - /// along with the generated unified spending key. + /// 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, + seed: &SecretVec, ) -> Result<(AccountId, UnifiedSpendingKey), Self::Error>; /// Generates and persists the next available diversified address, given the current @@ -535,7 +547,7 @@ pub mod testing { impl WalletWrite for MockWalletDb { fn create_account( &mut self, - seed: SecretVec, + seed: &SecretVec, ) -> Result<(AccountId, UnifiedSpendingKey), Self::Error> { let account = AccountId::from(0); UnifiedSpendingKey::from_seed(&self.network, seed.expose_secret(), account) diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 3113feebc..4d8e1707f 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -405,7 +405,7 @@ impl<'a, P: consensus::Parameters> DataConnStmtCache<'a, P> { impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> { fn create_account( &mut self, - seed: SecretVec, + seed: &SecretVec, ) -> Result<(AccountId, UnifiedSpendingKey), Self::Error> { self.transactionally(|stmts| { let account = wallet::get_max_account_id(stmts.wallet_db)? diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 0a45ee416..37d7141b2 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -523,7 +523,9 @@ fn init_wallet_db_internal( /// 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 [`zcash_client_backend::data_api::WalletWrite::create_account`] instead. +/// 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`]. Account identifiers From d086c57f2f105f2260670f36452956fa38249edc Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 14 Sep 2022 13:38:00 -0600 Subject: [PATCH 3/3] Make init_accounts_table only permit sequential account identifiers. Also, check to ensure that account creation does not exceed the maximum account identifier value. --- zcash_client_backend/src/data_api.rs | 2 +- zcash_client_sqlite/CHANGELOG.md | 4 +++ zcash_client_sqlite/src/error.rs | 9 +++++++ zcash_client_sqlite/src/lib.rs | 4 +++ zcash_client_sqlite/src/wallet/init.rs | 37 ++++++++++++++++++++++++++ 5 files changed, 55 insertions(+), 1 deletion(-) diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 7d188411c..a2eea7f09 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -1,6 +1,5 @@ //! Interfaces for wallet data persistence & low-level wallet utilities. -use secrecy::SecretVec; use std::cmp; use std::collections::HashMap; use std::fmt::Debug; @@ -8,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, diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index 1ec3f1e4c..751eb92eb 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/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. diff --git a/zcash_client_sqlite/src/error.rs b/zcash_client_sqlite/src/error.rs index a9afc4a7b..c7315c4cf 100644 --- a/zcash_client_sqlite/src/error.rs +++ b/zcash_client_sqlite/src/error.rs @@ -73,6 +73,13 @@ pub enum SqliteClientError { /// 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 { @@ -113,6 +120,8 @@ impl fmt::Display for SqliteClientError { 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."), } } } diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 4d8e1707f..8ee156e74 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -412,6 +412,10 @@ impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> { .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(), diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 37d7141b2..9a8f2720f 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -586,6 +586,13 @@ pub fn init_accounts_table( 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() { @@ -677,6 +684,7 @@ mod tests { }; use crate::{ + error::SqliteClientError, tests::{self, network}, wallet::get_address, AccountId, WalletDb, @@ -1370,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::>() + }; + + // 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();