When adding an account, check whether any component of its UFVK
(if available) collides with an existing imported or derived FVK. This does not check for collisions on IVK for `Incoming` accounts. Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>
This commit is contained in:
parent
130b23c92f
commit
bd38645da5
|
@ -1704,7 +1704,8 @@ impl AccountBirthday {
|
|||
/// ZIP 32 account indices from the same seed, a call to [`WalletWrite::create_account`]
|
||||
/// will use the ZIP 32 account index just after the highest-numbered existing account.
|
||||
/// - If an account is added to the wallet, and then a later call to one of the methods
|
||||
/// would produce the same UFVK, an error will be returned. This can occur in the
|
||||
/// would produce a UFVK that collides with that account on any FVK component (i.e.
|
||||
/// Sapling, Orchard, or transparent), an error will be returned. This can occur in the
|
||||
/// following cases:
|
||||
/// - An account is created via [`WalletWrite::create_account`] with an auto-selected
|
||||
/// ZIP 32 account index, and that index is later imported explicitly via either
|
||||
|
@ -1715,10 +1716,8 @@ impl AccountBirthday {
|
|||
/// via [`WalletWrite::create_account`], or explicitly via a call to
|
||||
/// [`WalletWrite::import_account_ufvk`] or [`WalletWrite::import_account_hd`].
|
||||
///
|
||||
/// Note that accounts having UFVKs that are not identical but have shared
|
||||
/// components (for example, two accounts having the same Sapling FVK, one
|
||||
/// of which also has an Orchard FVK while the other does not) are currently
|
||||
/// allowed. This will not be allowed in future.
|
||||
/// Note that an error will be returned on an FVK collision even if the UFVKs do not
|
||||
/// match exactly, e.g. if they have different subsets of components.
|
||||
///
|
||||
/// A future change to this trait might introduce a method to "upgrade" an imported
|
||||
/// account with derivation information. See [zcash/librustzcash#1284] for details.
|
||||
|
|
|
@ -74,6 +74,7 @@ pub enum SqliteClientError {
|
|||
AccountUnknown,
|
||||
|
||||
/// The account being added collides with an existing account in the wallet with the given ID.
|
||||
/// The collision can be on the seed and ZIP-32 account index, or a shared FVK component.
|
||||
AccountCollision(AccountId),
|
||||
|
||||
/// The account was imported, and ZIP-32 derivation information is not known for it.
|
||||
|
|
|
@ -2022,15 +2022,19 @@ extern crate assert_matches;
|
|||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use secrecy::{Secret, SecretVec};
|
||||
use secrecy::{ExposeSecret, Secret, SecretVec};
|
||||
use zcash_client_backend::data_api::{
|
||||
chain::ChainState, Account, AccountBirthday, AccountPurpose, AccountSource, WalletRead,
|
||||
WalletWrite,
|
||||
};
|
||||
use zcash_keys::keys::UnifiedSpendingKey;
|
||||
use zcash_keys::keys::{UnifiedFullViewingKey, UnifiedSpendingKey};
|
||||
use zcash_primitives::block::BlockHash;
|
||||
|
||||
use crate::{error::SqliteClientError, testing::TestBuilder, AccountId, DEFAULT_UA_REQUEST};
|
||||
use crate::{
|
||||
error::SqliteClientError,
|
||||
testing::{TestBuilder, TestState},
|
||||
AccountId, DEFAULT_UA_REQUEST,
|
||||
};
|
||||
|
||||
#[cfg(feature = "unstable")]
|
||||
use {
|
||||
|
@ -2135,8 +2139,55 @@ mod tests {
|
|||
AccountSource::Derived { seed_fingerprint: _, account_index } if account_index == zip32_index_2);
|
||||
}
|
||||
|
||||
fn check_collisions<C>(
|
||||
st: &mut TestState<C>,
|
||||
ufvk: &UnifiedFullViewingKey,
|
||||
birthday: &AccountBirthday,
|
||||
existing_id: AccountId,
|
||||
) {
|
||||
assert_matches!(
|
||||
st.wallet_mut().import_account_ufvk(ufvk, birthday, AccountPurpose::Spending),
|
||||
Err(SqliteClientError::AccountCollision(id)) if id == existing_id);
|
||||
|
||||
// Remove the transparent component so that we don't have a match on the full UFVK.
|
||||
// That should still produce an AccountCollision error.
|
||||
#[cfg(feature = "transparent-inputs")]
|
||||
{
|
||||
assert!(ufvk.transparent().is_some());
|
||||
let subset_ufvk = UnifiedFullViewingKey::new(
|
||||
None,
|
||||
ufvk.sapling().cloned(),
|
||||
#[cfg(feature = "orchard")]
|
||||
ufvk.orchard().cloned(),
|
||||
#[cfg(not(feature = "orchard"))]
|
||||
None, // see zcash/librustzcash#1488
|
||||
)
|
||||
.unwrap();
|
||||
assert_matches!(
|
||||
st.wallet_mut().import_account_ufvk(&subset_ufvk, birthday, AccountPurpose::Spending),
|
||||
Err(SqliteClientError::AccountCollision(id)) if id == existing_id);
|
||||
}
|
||||
|
||||
// Remove the Orchard component so that we don't have a match on the full UFVK.
|
||||
// That should still produce an AccountCollision error.
|
||||
#[cfg(feature = "orchard")]
|
||||
{
|
||||
assert!(ufvk.orchard().is_some());
|
||||
let subset_ufvk = UnifiedFullViewingKey::new(
|
||||
#[cfg(feature = "transparent-inputs")]
|
||||
ufvk.transparent().cloned(),
|
||||
ufvk.sapling().cloned(),
|
||||
None,
|
||||
)
|
||||
.unwrap();
|
||||
assert_matches!(
|
||||
st.wallet_mut().import_account_ufvk(&subset_ufvk, birthday, AccountPurpose::Spending),
|
||||
Err(SqliteClientError::AccountCollision(id)) if id == existing_id);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
pub(crate) fn import_account_hd_1_twice() {
|
||||
pub(crate) fn import_account_hd_1_then_conflicts() {
|
||||
let mut st = TestBuilder::new().build();
|
||||
|
||||
let birthday = AccountBirthday::from_parts(
|
||||
|
@ -2147,18 +2198,21 @@ mod tests {
|
|||
let seed = Secret::new(vec![0u8; 32]);
|
||||
let zip32_index_1 = zip32::AccountId::ZERO.next().unwrap();
|
||||
|
||||
let first = st
|
||||
let (first_account, _) = st
|
||||
.wallet_mut()
|
||||
.import_account_hd(&seed, zip32_index_1, &birthday)
|
||||
.unwrap();
|
||||
let ufvk = first_account.ufvk().unwrap();
|
||||
|
||||
assert_matches!(
|
||||
st.wallet_mut().import_account_hd(&seed, zip32_index_1, &birthday),
|
||||
Err(SqliteClientError::AccountCollision(id)) if id == first.0.id());
|
||||
Err(SqliteClientError::AccountCollision(id)) if id == first_account.id());
|
||||
|
||||
check_collisions(&mut st, ufvk, &birthday, first_account.id());
|
||||
}
|
||||
|
||||
#[test]
|
||||
pub(crate) fn import_account_ufvk() {
|
||||
pub(crate) fn import_account_ufvk_then_conflicts() {
|
||||
let mut st = TestBuilder::new().build();
|
||||
|
||||
let birthday = AccountBirthday::from_parts(
|
||||
|
@ -2166,9 +2220,11 @@ mod tests {
|
|||
None,
|
||||
);
|
||||
|
||||
let seed = vec![0u8; 32];
|
||||
let usk = UnifiedSpendingKey::from_seed(&st.wallet().params, &seed, zip32::AccountId::ZERO)
|
||||
.unwrap();
|
||||
let seed = Secret::new(vec![0u8; 32]);
|
||||
let zip32_index_0 = zip32::AccountId::ZERO;
|
||||
let usk =
|
||||
UnifiedSpendingKey::from_seed(&st.wallet().params, seed.expose_secret(), zip32_index_0)
|
||||
.unwrap();
|
||||
let ufvk = usk.to_unified_full_viewing_key();
|
||||
|
||||
let account = st
|
||||
|
@ -2186,10 +2242,16 @@ mod tests {
|
|||
purpose: AccountPurpose::Spending
|
||||
}
|
||||
);
|
||||
|
||||
assert_matches!(
|
||||
st.wallet_mut().import_account_hd(&seed, zip32_index_0, &birthday),
|
||||
Err(SqliteClientError::AccountCollision(id)) if id == account.id());
|
||||
|
||||
check_collisions(&mut st, &ufvk, &birthday, account.id());
|
||||
}
|
||||
|
||||
#[test]
|
||||
pub(crate) fn create_account_then_conflicting_import_account_ufvk() {
|
||||
pub(crate) fn create_account_then_conflicts() {
|
||||
let mut st = TestBuilder::new().build();
|
||||
|
||||
let birthday = AccountBirthday::from_parts(
|
||||
|
@ -2198,13 +2260,16 @@ mod tests {
|
|||
);
|
||||
|
||||
let seed = Secret::new(vec![0u8; 32]);
|
||||
let zip32_index_0 = zip32::AccountId::ZERO;
|
||||
let seed_based = st.wallet_mut().create_account(&seed, &birthday).unwrap();
|
||||
let seed_based_account = st.wallet().get_account(seed_based.0).unwrap().unwrap();
|
||||
let ufvk = seed_based_account.ufvk().unwrap();
|
||||
|
||||
assert_matches!(
|
||||
st.wallet_mut().import_account_ufvk(ufvk, &birthday, AccountPurpose::Spending),
|
||||
st.wallet_mut().import_account_hd(&seed, zip32_index_0, &birthday),
|
||||
Err(SqliteClientError::AccountCollision(id)) if id == seed_based.0);
|
||||
|
||||
check_collisions(&mut st, ufvk, &birthday, seed_based.0);
|
||||
}
|
||||
|
||||
#[cfg(feature = "transparent-inputs")]
|
||||
|
|
|
@ -357,6 +357,14 @@ pub(crate) fn add_account<P: consensus::Parameters>(
|
|||
viewing_key: ViewingKey,
|
||||
birthday: &AccountBirthday,
|
||||
) -> Result<Account, SqliteClientError> {
|
||||
if let Some(ufvk) = viewing_key.ufvk() {
|
||||
// Check whether any component of this UFVK collides with an existing imported or derived FVK.
|
||||
if let Some(existing_account) = get_account_for_ufvk(conn, params, ufvk)? {
|
||||
return Err(SqliteClientError::AccountCollision(existing_account.id()));
|
||||
}
|
||||
}
|
||||
// TODO(#1490): check for IVK collisions.
|
||||
|
||||
let (hd_seed_fingerprint, hd_account_index, spending_key_available) = match kind {
|
||||
AccountSource::Derived {
|
||||
seed_fingerprint,
|
||||
|
@ -427,8 +435,9 @@ pub(crate) fn add_account<P: consensus::Parameters>(
|
|||
rusqlite::Error::SqliteFailure(f, s)
|
||||
if f.code == rusqlite::ErrorCode::ConstraintViolation =>
|
||||
{
|
||||
// An account conflict occurred.
|
||||
// Make a best effort to determine the AccountId of the pre-existing row
|
||||
// An account conflict occurred. This should already have been caught by
|
||||
// the check using `get_account_for_ufvk` above, but in case it wasn't,
|
||||
// make a best effort to determine the AccountId of the pre-existing row
|
||||
// and provide that to our caller.
|
||||
if let Ok(id) = conn.query_row(
|
||||
"SELECT id FROM accounts WHERE ufvk = ?",
|
||||
|
|
Loading…
Reference in New Issue