Precisely document same-UFVK errors on account creation and ensure they are per-component

[daira]

This is an issue with #1465 found in post-hoc review.

Originally posted by @daira in #1465 (comment)

Should this say "If an account created via [WalletWrite::create_account] or imported via [WalletWrite::import_account_ufvk] or [WalletWrite::import_account_hd] already exists in the wallet with the same UFVK (which will also be the case for accounts with the same seed), an error will be returned." ?

This differs from the current wording because (I think) it is not the case that the creation or import has to be in any particular order; any of these three methods will fail if it produces a duplicate UFVK, regardless of which of them was used to create the existing account. At least, if that's not the case then it is a bug.

Also, do we actually mean "same UFVK" here, or do we mean that there will be a failure if any component of the UFVK matches that of an existing account? In the case of an account where we don't know the seed, the imported UFVK can't include components that are not known yet, and may not include some existing components (I don't know whether the latter is supported).

Originally posted by @daira in #1465 (comment)

I think the doc for each of the three account creation/import methods should call out that it fails when there is a duplicate UFVK, even if that's redundant with the WalletWrite doc. Note that it is possible to hit this case even if you only use one of the three methods, and there might be implementors who only read the documentation of the methods they are using (because they haven't reached this page yet).

[str4d]

Also, do we actually mean "same UFVK" here, or do we mean that there will be a failure if any component of the UFVK matches that of an existing account? In the case of an account where we don't know the seed, the imported UFVK can't include components that are not known yet, and may not include some existing components (I don't know whether the latter is supported).

Currently, the three uniqueness constraints that zcash_client_sqlite implements are:

• same (seed, ZIP 32 account index)
• same UFVK
• same UIVK (which we can consider equivalent to "same UFVK")
  

  librustzcash/zcash_client_sqlite/src/wallet/db.rs

  Lines 25 to 34 in 7f7b685

  	CREATE TABLE "accounts" (
  	id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT,
  	account_kind INTEGER NOT NULL DEFAULT 0,
  	hd_seed_fingerprint BLOB,
  	hd_account_index INTEGER,
  	ufvk TEXT,
  	uivk TEXT NOT NULL,
  	orchard_fvk_item_cache BLOB,
  	sapling_fvk_item_cache BLOB,
  	p2pkh_fvk_item_cache BLOB,

  
  

  librustzcash/zcash_client_sqlite/src/wallet/db.rs

  Lines 55 to 60 in 7f7b685

  	pub(super) const INDEX_ACCOUNTS_UFVK: &str =
  	r#"CREATE UNIQUE INDEX accounts_ufvk ON "accounts" (ufvk)"#;
  	pub(super) const INDEX_ACCOUNTS_UIVK: &str =
  	r#"CREATE UNIQUE INDEX accounts_uivk ON "accounts" (uivk)"#;
  	pub(super) const INDEX_HD_ACCOUNT: &str =
  	r#"CREATE UNIQUE INDEX hd_account ON "accounts" (hd_seed_fingerprint, hd_account_index)"#;

  
  

  librustzcash/zcash_client_sqlite/src/wallet.rs

  Lines 418 to 431 in 7f7b685

  	.map_err(|e| match e {
  	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
  	// and provide that to our caller.
  	if let Ok(id) = conn.query_row(
  	"SELECT id FROM accounts WHERE ufvk = ?",
  	params![ufvk_encoded],
  	|row| Ok(AccountId(row.get(0)?)),
  	) {
  	return SqliteClientError::AccountCollision(id);
  	}

We do cache the individual components in the table, but only the ufvk and uivk columns are forced to be unique.

Specifically, what this means is that we will correctly detect conflicts between WalletWrite::create_account and WalletWrite::import_account_hd (because they only rely on (seed, ZIP 32 account index)), but we will not reliably detect conflicts between those two and WalletWrite::import_account_ufvk. We should decide what we want to do in the latter case, before the next release.

[nuttycom]

I think that what we should do is use a query to check against the individual components. We may also want to add a uniqueness constraint, but I prefer not to rely upon constraint violation errors for control flow; reporting to the user that a conflict has been identified should be the result of an explicit check.