Merge pull request #632 from nuttycom/data_api/get_next_address

Replace `get_address` with `get_current_address` and `get_next_available_address`

zcash_client_backend/CHANGELOG.md

@@ -32,8 +32,10 @@ and this library adheres to Rust's notion of
   - `RecipientAddress::Unified`
 - `zcash_client_backend::data_api`:
   - `WalletRead::get_unified_full_viewing_keys`
+  - `WalletRead::get_current_address`
   - `WalletRead::get_all_nullifiers`
   - `WalletWrite::remove_unmined_tx` (behind the `unstable` feature flag).
+  - `WalletWrite::get_next_available_address`
 - `zcash_client_backend::proto`:
   - `actions` field on `compact_formats::CompactTx`
   - `compact_formats::CompactOrchardAction`
@@ -79,8 +81,6 @@ and this library adheres to Rust's notion of
     a `min_confirmations` argument that is used to compute an upper bound on
     the anchor height being returned; this had previously been hardcoded to
     `data_api::wallet::ANCHOR_OFFSET`.
-  - `WalletRead::get_address` now returns a `UnifiedAddress` instead of a
-    `sapling::PaymentAddress`.
   - `WalletRead::get_spendable_notes` has been renamed to
     `get_spendable_sapling_notes`
   - `WalletRead::select_spendable_notes` has been renamed to
@@ -128,6 +128,8 @@ and this library adheres to Rust's notion of
 - `zcash_client_backend::data_api`:
   - `WalletRead::get_extended_full_viewing_keys` (use
     `WalletRead::get_unified_full_viewing_keys` instead).
+  - `WalletRead::get_address` (use `WalletRead::get_current_address` or
+    `WalletWrite::get_next_available_address` instead.)
 - The hardcoded `data_api::wallet::ANCHOR_OFFSET` constant.
 - `zcash_client_backend::wallet::AccountId` (moved to `zcash_primitives::zip32::AccountId`).
 

zcash_client_backend/src/data_api.rs

@@ -114,12 +114,15 @@ pub trait WalletRead {
     /// or `Ok(None)` if the transaction is not mined in the main chain.
     fn get_tx_height(&self, txid: TxId) -> Result<Option<BlockHeight>, Self::Error>;
 
-    /// Returns the unified address for the specified account, if the account
+    /// Returns the most recently generated unified address for the specified account, if the
-    /// identifier specified refers to a valid account for this wallet.
+    /// account identifier specified refers to a valid account for this wallet.
     ///
     /// This will return `Ok(None)` if the account identifier does not correspond
     /// to a known account.
-    fn get_address(&self, account: AccountId) -> Result<Option<UnifiedAddress>, Self::Error>;
+    fn get_current_address(
+        &self,
+        account: AccountId,
+    ) -> Result<Option<UnifiedAddress>, Self::Error>;
 
     /// Returns all unified full viewing keys known to this wallet.
     fn get_unified_full_viewing_keys(
@@ -266,6 +269,16 @@ pub struct SentTransactionOutput<'a> {
 /// This trait encapsulates the write capabilities required to update stored
 /// wallet data.
 pub trait WalletWrite: WalletRead {
+    /// Generates and persists the next available diversified address, given the current
+    /// addresses known to the wallet.
+    ///
+    /// Returns `Ok(None)` if the account identifier does not correspond to a known
+    /// account.
+    fn get_next_available_address(
+        &mut self,
+        account: AccountId,
+    ) -> Result<Option<UnifiedAddress>, Self::Error>;
+
     /// Updates the state of the wallet database by persisting the provided
     /// block information, along with the updated witness data that was
     /// produced when scanning the block for transactions pertaining to
@@ -283,6 +296,8 @@ pub trait WalletWrite: WalletRead {
         received_tx: &DecryptedTransaction,
     ) -> Result<Self::TxRef, Self::Error>;
 
+    /// Saves information about a transaction that was constructed and sent by the wallet to the
+    /// persistent wallet store.
     fn store_sent_tx(&mut self, sent_tx: &SentTransaction) -> Result<Self::TxRef, Self::Error>;
 
     /// Removes the specified unmined transaction from the persistent wallet store, if it
@@ -409,7 +424,10 @@ pub mod testing {
             Ok(None)
         }
 
-        fn get_address(&self, _account: AccountId) -> Result<Option<UnifiedAddress>, Self::Error> {
+        fn get_current_address(
+            &self,
+            _account: AccountId,
+        ) -> Result<Option<UnifiedAddress>, Self::Error> {
             Ok(None)
         }
 
@@ -503,6 +521,13 @@ pub mod testing {
     }
 
     impl WalletWrite for MockWalletDb {
+        fn get_next_available_address(
+            &mut self,
+            _account: AccountId,
+        ) -> Result<Option<UnifiedAddress>, Self::Error> {
+            Ok(None)
+        }
+
         #[allow(clippy::type_complexity)]
         fn advance_by_block(
             &mut self,

zcash_client_sqlite/CHANGELOG.md

@@ -16,6 +16,8 @@ and this library adheres to Rust's notion of
     transparent address decoding.
   - `SqliteClientError::RequestedRewindInvalid`, to report when requested
     rewinds exceed supported bounds.
+  - `SqliteClientError::DiversifierIndexOutOfRange`, to report when the space
+    of available diversifier indices has been exhausted. 
 - 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

@@ -65,6 +65,10 @@ pub enum SqliteClientError {
 
     /// Wrapper for errors from zcash_client_backend
     BackendError(data_api::error::Error<NoteId>),
+
+    /// The space of allocatable diversifier indices has been exhausted for
+    /// the given account.
+    DiversifierIndexOutOfRange,
 }
 
 impl error::Error for SqliteClientError {
@@ -103,6 +107,7 @@ impl fmt::Display for SqliteClientError {
             SqliteClientError::Io(e) => write!(f, "{}", e),
             SqliteClientError::InvalidMemo(e) => write!(f, "{}", e),
             SqliteClientError::BackendError(e) => write!(f, "{}", e),
+            SqliteClientError::DiversifierIndexOutOfRange => write!(f, "The space of available diversifier indices is exhausted"),
         }
     }
 }

zcash_client_sqlite/src/lib.rs

@@ -48,7 +48,7 @@ use zcash_primitives::{
     merkle_tree::{CommitmentTree, IncrementalWitness},
     sapling::{Node, Nullifier},
     transaction::{components::Amount, Transaction, TxId},
-    zip32::{AccountId, ExtendedFullViewingKey},
+    zip32::{AccountId, DiversifierIndex, ExtendedFullViewingKey},
 };
 
 use zcash_client_backend::{
@@ -153,8 +153,11 @@ impl<P: consensus::Parameters> WalletRead for WalletDb<P> {
         wallet::get_unified_full_viewing_keys(self)
     }
 
-    fn get_address(&self, account: AccountId) -> Result<Option<UnifiedAddress>, Self::Error> {
+    fn get_current_address(
-        wallet::get_address_ua(self, account)
+        &self,
+        account: AccountId,
+    ) -> Result<Option<UnifiedAddress>, Self::Error> {
+        wallet::get_current_address(self, account).map(|res| res.map(|(addr, _)| addr))
     }
 
     fn is_valid_account_extfvk(
@@ -276,8 +279,11 @@ impl<'a, P: consensus::Parameters> WalletRead for DataConnStmtCache<'a, P> {
         self.wallet_db.get_unified_full_viewing_keys()
     }
 
-    fn get_address(&self, account: AccountId) -> Result<Option<UnifiedAddress>, Self::Error> {
+    fn get_current_address(
-        self.wallet_db.get_address(account)
+        &self,
+        account: AccountId,
+    ) -> Result<Option<UnifiedAddress>, Self::Error> {
+        self.wallet_db.get_current_address(account)
     }
 
     fn is_valid_account_extfvk(
@@ -396,6 +402,34 @@ impl<'a, P: consensus::Parameters> DataConnStmtCache<'a, P> {
 
 #[allow(deprecated)]
 impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> {
+    fn get_next_available_address(
+        &mut self,
+        account: AccountId,
+    ) -> Result<Option<UnifiedAddress>, Self::Error> {
+        match self.get_unified_full_viewing_keys()?.get(&account) {
+            Some(ufvk) => {
+                let search_from = match wallet::get_current_address(self.wallet_db, account)? {
+                    Some((_, mut last_diversifier_index)) => {
+                        last_diversifier_index
+                            .increment()
+                            .map_err(|_| SqliteClientError::DiversifierIndexOutOfRange)?;
+                        last_diversifier_index
+                    }
+                    None => DiversifierIndex::default(),
+                };
+
+                let (addr, diversifier_index) = ufvk
+                    .find_address(search_from)
+                    .ok_or(SqliteClientError::DiversifierIndexOutOfRange)?;
+
+                self.stmt_insert_address(account, diversifier_index, &addr)?;
+
+                Ok(Some(addr))
+            }
+            None => Ok(None),
+        }
+    }
+
     #[allow(clippy::type_complexity)]
     fn advance_by_block(
         &mut self,
@@ -691,13 +725,6 @@ mod tests {
     use rusqlite::params;
     use std::collections::HashMap;
 
-    use zcash_client_backend::{
-        keys::{sapling, UnifiedFullViewingKey},
-        proto::compact_formats::{
-            CompactBlock, CompactSaplingOutput, CompactSaplingSpend, CompactTx,
-        },
-    };
-
     #[cfg(feature = "transparent-inputs")]
     use zcash_primitives::{legacy, legacy::keys::IncomingViewingKey};
 
@@ -714,7 +741,18 @@ mod tests {
         zip32::ExtendedFullViewingKey,
     };
 
-    use crate::{wallet::init::init_accounts_table, AccountId, WalletDb};
+    use zcash_client_backend::{
+        data_api::{WalletRead, WalletWrite},
+        keys::{sapling, UnifiedFullViewingKey},
+        proto::compact_formats::{
+            CompactBlock, CompactSaplingOutput, CompactSaplingSpend, CompactTx,
+        },
+    };
+
+    use crate::{
+        wallet::init::{init_accounts_table, init_wallet_db},
+        AccountId, WalletDb,
+    };
 
     use super::BlockDb;
 
@@ -930,6 +968,29 @@ mod tests {
             .unwrap();
     }
 
+    #[test]
+    pub(crate) fn get_next_available_address() {
+        use tempfile::NamedTempFile;
+
+        let data_file = NamedTempFile::new().unwrap();
+        let mut db_data = WalletDb::for_path(data_file.path(), network()).unwrap();
+
+        let account = AccountId::from(0);
+        init_wallet_db(&mut db_data, None).unwrap();
+        let _ = init_test_accounts_table_ufvk(&db_data);
+
+        let current_addr = db_data.get_current_address(account).unwrap();
+        assert!(current_addr.is_some());
+
+        let mut update_ops = db_data.get_update_ops().unwrap();
+        let addr2 = update_ops.get_next_available_address(account).unwrap();
+        assert!(addr2.is_some());
+        assert_ne!(current_addr, addr2);
+
+        let addr2_cur = db_data.get_current_address(account).unwrap();
+        assert_eq!(addr2, addr2_cur);
+    }
+
     #[cfg(feature = "transparent-inputs")]
     #[test]
     fn transparent_receivers() {

zcash_client_sqlite/src/prepared.rs

@@ -15,9 +15,11 @@ use zcash_primitives::{
     merkle_tree::{CommitmentTree, IncrementalWitness},
     sapling::{Diversifier, Node, Nullifier},
     transaction::{components::Amount, TxId},
-    zip32::AccountId,
+    zip32::{AccountId, DiversifierIndex},
 };
 
+use zcash_client_backend::address::UnifiedAddress;
+
 use crate::{error::SqliteClientError, wallet::PoolType, NoteId, WalletDb};
 
 #[cfg(feature = "transparent-inputs")]
@@ -64,6 +66,8 @@ pub struct DataConnStmtCache<'a, P> {
     stmt_insert_witness: Statement<'a>,
     stmt_prune_witnesses: Statement<'a>,
     stmt_update_expired: Statement<'a>,
+
+    stmt_insert_address: Statement<'a>,
 }
 
 impl<'a, P> DataConnStmtCache<'a, P> {
@@ -160,6 +164,10 @@ impl<'a, P> DataConnStmtCache<'a, P> {
                         WHERE id_tx = received_notes.spent AND block IS NULL AND expiry_height < ?
                     )",
                 )?,
+                stmt_insert_address: wallet_db.conn.prepare(
+                    "INSERT INTO addresses (account, diversifier_index_be, address)
+                    VALUES (:account, :diversifier_index_be, :address)",
+                )?,
             }
         )
     }
@@ -369,6 +377,27 @@ impl<'a, P: consensus::Parameters> DataConnStmtCache<'a, P> {
 
         Ok(rows)
     }
+
+    /// Adds the given address and diversifier index to the addresses table.
+    ///
+    /// Returns the database row for the newly-inserted address.
+    pub(crate) fn stmt_insert_address(
+        &mut self,
+        account: AccountId,
+        mut diversifier_index: DiversifierIndex,
+        address: &UnifiedAddress,
+    ) -> Result<i64, SqliteClientError> {
+        diversifier_index.0.reverse();
+        let sql_args: &[(&str, &dyn ToSql)] = &[
+            (":account", &u32::from(account)),
+            (":diversifier_index_be", &&diversifier_index.0[..]),
+            (":address", &address.encode(&self.wallet_db.params)),
+        ];
+
+        self.stmt_insert_address.execute_named(sql_args)?;
+
+        Ok(self.wallet_db.conn.last_insert_rowid())
+    }
 }
 
 impl<'a, P> DataConnStmtCache<'a, P> {

zcash_client_sqlite/src/wallet.rs

@@ -22,7 +22,7 @@ use zcash_primitives::{
     merkle_tree::{CommitmentTree, IncrementalWitness},
     sapling::{keys::DiversifiableFullViewingKey, Node, Note, Nullifier, PaymentAddress},
     transaction::{components::Amount, Transaction, TxId},
-    zip32::{AccountId, ExtendedFullViewingKey},
+    zip32::{AccountId, DiversifierIndex, ExtendedFullViewingKey},
 };
 
 use zcash_client_backend::{
@@ -158,10 +158,12 @@ pub fn get_address<P: consensus::Parameters>(
     wdb: &WalletDb<P>,
     account: AccountId,
 ) -> Result<Option<PaymentAddress>, SqliteClientError> {
-    // This returns the first diversified address, which will be the default one.
+    // This returns the most recently generated address.
     let addr: String = wdb.conn.query_row(
-        "SELECT address FROM addresses
+        "SELECT address
-        WHERE account = ?",
+        FROM addresses WHERE account = ?
+        ORDER BY diversifier_index_be DESC
+        LIMIT 1",
         &[u32::from(account)],
         |row| row.get(0),
     )?;
@@ -177,21 +179,31 @@ pub fn get_address<P: consensus::Parameters>(
         })
 }
 
-pub(crate) fn get_address_ua<P: consensus::Parameters>(
+pub(crate) fn get_current_address<P: consensus::Parameters>(
     wdb: &WalletDb<P>,
     account: AccountId,
-) -> Result<Option<UnifiedAddress>, SqliteClientError> {
+) -> Result<Option<(UnifiedAddress, DiversifierIndex)>, SqliteClientError> {
-    // This returns the first diversified address, which will be the default one.
+    // This returns the most recently generated address.
-    let addr: Option<String> = wdb
+    let addr: Option<(String, Vec<u8>)> = wdb
         .conn
         .query_row_named(
-            "SELECT address FROM addresses WHERE account = :account",
+            "SELECT address, diversifier_index_be
+            FROM addresses WHERE account = :account
+            ORDER BY diversifier_index_be DESC
+            LIMIT 1",
             &[(":account", &u32::from(account))],
-            |row| row.get(0),
+            |row| Ok((row.get(0)?, row.get(1)?)),
         )
         .optional()?;
 
-    addr.map(|addr_str| {
+    addr.map(|(addr_str, di_vec)| {
+        let mut di_be: [u8; 11] = di_vec.try_into().map_err(|_| {
+            SqliteClientError::CorruptedData(
+                "Diverisifier index is not an 11-byte value".to_owned(),
+            )
+        })?;
+        di_be.reverse();
+
         RecipientAddress::decode(&wdb.params, &addr_str)
             .ok_or_else(|| {
                 SqliteClientError::CorruptedData("Not a valid Zcash recipient address".to_owned())
@@ -203,6 +215,7 @@ pub(crate) fn get_address_ua<P: consensus::Parameters>(
                     addr_str,
                 ))),
             })
+            .map(|addr| (addr, DiversifierIndex(di_be)))
     })
     .transpose()
 }
@@ -619,7 +632,7 @@ pub fn get_block_hash<P>(
 pub fn get_rewind_height<P>(wdb: &WalletDb<P>) -> Result<Option<BlockHeight>, SqliteClientError> {
     wdb.conn
         .query_row(
-            "SELECT MIN(tx.block) 
+            "SELECT MIN(tx.block)
              FROM received_notes n
              JOIN transactions tx ON tx.id_tx = n.tx
              WHERE n.spent IS NULL",

zcash_client_sqlite/src/wallet/init.rs

@@ -608,14 +608,16 @@ fn add_account_internal<P: consensus::Parameters, E: From<rusqlite::Error>>(
     )?;
 
     // Always derive the default Unified Address for the account.
-    let (address, idx) = key.default_address();
+    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, address)
+        "INSERT INTO addresses (account, diversifier_index_be, address)
-        VALUES (:account, :diversifier_index, :address)",
+        VALUES (:account, :diversifier_index_be, :address)",
         &[
             (":account", &<u32>::from(account)),
-            (":diversifier_index", &&idx.0[..]),
+            (":diversifier_index_be", &&idx.0[..]),
             (":address", &address_str),
         ],
     )?;
@@ -730,10 +732,10 @@ mod tests {
             )",
             "CREATE TABLE addresses (
                 account INTEGER NOT NULL,
-                diversifier_index BLOB NOT NULL,
+                diversifier_index_be BLOB NOT NULL,
                 address TEXT NOT NULL,
                 FOREIGN KEY (account) REFERENCES accounts(account),
-                CONSTRAINT diversification UNIQUE (account, diversifier_index)
+                CONSTRAINT diversification UNIQUE (account, diversifier_index_be)
             )",
             "CREATE TABLE blocks (
                 height INTEGER PRIMARY KEY,

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

@@ -51,10 +51,10 @@ impl<P: consensus::Parameters> RusqliteMigration for AddressesTableMigration<P>
         transaction.execute_batch(
             "CREATE TABLE addresses (
                 account INTEGER NOT NULL,
-                diversifier_index BLOB NOT NULL,
+                diversifier_index_be BLOB NOT NULL,
                 address TEXT NOT NULL,
                 FOREIGN KEY (account) REFERENCES accounts(account),
-                CONSTRAINT diversification UNIQUE (account, diversifier_index)
+                CONSTRAINT diversification UNIQUE (account, diversifier_index_be)
             );
             CREATE TABLE accounts_new (
                 account INTEGER PRIMARY KEY,