-
Notifications
You must be signed in to change notification settings - Fork 251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add WalletWrite::insert_address_with_diversifier_index
function
#1151
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ use zcash_primitives::{ | |
}, | ||
zip32::{AccountId, Scope}, | ||
}; | ||
use zip32::DiversifierIndex; | ||
|
||
use crate::{ | ||
address::{AddressMetadata, UnifiedAddress}, | ||
|
@@ -1019,6 +1020,27 @@ pub trait WalletWrite: WalletRead { | |
request: UnifiedAddressRequest, | ||
) -> Result<Option<UnifiedAddress>, Self::Error>; | ||
|
||
/// Generates and persists a new address for the specified account, with the specified | ||
/// diversifier index. | ||
/// | ||
/// Returns the new address, or an error if the account identifier does not correspond to a | ||
/// known account. | ||
/// A conflict with an existing row in the database is considered acceptable and no error is returned. | ||
/// If the diversifier index cannot produce a valid Sapling address, no sapling receiver will | ||
/// be included in the returned address. | ||
/// If the diversifier is outside the range for transparent addresses, no transparent receiver | ||
/// will be included in the returned address. | ||
/// | ||
/// This supports a more advanced use case than `get_next_available_address` where the caller | ||
/// simply gets the next diversified address sequentially. Mixing use of the two functions | ||
/// is not recommended because `get_next_available_address` will return the next address | ||
/// after the highest diversifier index in the database, which may leave gaps in the sequence. | ||
fn insert_address_with_diversifier_index( | ||
&mut self, | ||
account: AccountId, | ||
diversifier_index: DiversifierIndex, | ||
) -> Result<UnifiedAddress, Self::Error>; | ||
|
||
/// Updates the state of the wallet database by persisting the provided block information, | ||
/// along with the note commitments that were detected when scanning the block for transactions | ||
/// pertaining to this wallet. | ||
|
@@ -1105,6 +1127,7 @@ pub mod testing { | |
use secrecy::{ExposeSecret, SecretVec}; | ||
use shardtree::{error::ShardTreeError, store::memory::MemoryShardStore, ShardTree}; | ||
use std::{collections::HashMap, convert::Infallible, num::NonZeroU32}; | ||
use zip32::DiversifierIndex; | ||
|
||
use zcash_primitives::{ | ||
block::BlockHash, | ||
|
@@ -1323,6 +1346,14 @@ pub mod testing { | |
Ok(None) | ||
} | ||
|
||
fn insert_address_with_diversifier_index( | ||
&mut self, | ||
_account: AccountId, | ||
_diversifier_index: DiversifierIndex, | ||
) -> Result<UnifiedAddress, Self::Error> { | ||
todo!() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please either implement this, or add an issue to track the TODO There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would returning There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking that the mock could store a seed, but just adding an issue for this is fine. |
||
} | ||
|
||
#[allow(clippy::type_complexity)] | ||
fn put_blocks( | ||
&mut self, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,7 @@ use std::{ | |
borrow::Borrow, collections::HashMap, convert::AsRef, fmt, num::NonZeroU32, ops::Range, | ||
path::Path, | ||
}; | ||
use zcash_keys::keys::AddressGenerationError; | ||
|
||
use incrementalmerkletree::Position; | ||
use shardtree::{error::ShardTreeError, ShardTree}; | ||
|
@@ -445,6 +446,63 @@ impl<P: consensus::Parameters> WalletWrite for WalletDb<rusqlite::Connection, P> | |
) | ||
} | ||
|
||
fn insert_address_with_diversifier_index( | ||
&mut self, | ||
account: AccountId, | ||
diversifier_index: DiversifierIndex, | ||
) -> Result<UnifiedAddress, SqliteClientError> { | ||
self.transactionally(|wdb| { | ||
let keys = wdb.get_unified_full_viewing_keys()?; | ||
let ufvk = keys | ||
.get(&account) | ||
.ok_or(SqliteClientError::AccountUnknown(account))?; | ||
|
||
let has_orchard = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can't be merged as-is, because this will cause Orchard receivers to begin being generated before the wallet has There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That potentially presents a problem: if orchard cannot be guaranteed, and sapling may fail at a given diversifier index, that leaves me with just transparent in some cases. I can't construct a UA with just a transparent receiver. Yet the sqlite table I'm adding a row to has an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I should just create a new table to track the transparent addresses. It's not in this PR, but I have a follow-up change in my fork that adds a column to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a Sapling receiver is requested and no valid diversifier exists at the specified index, that should result in an error being returned; we should never generate an address that does not have all of the requested components. This is another reason to pass in the request from the caller. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That doesn't really answer my question. I need to be able to store addresses for which no valid sapling receiver exists. Since I can't store orchard receivers in this table without the caller's permission, I have to either store transparent addresses in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the use case you're after here the "transparent address zero" problem, wherein older wallets may have generated a transparent address at index zero but there is no corresponding Sapling address, and so you can't generate a UA to correspond? That is a use case that we do need to handle, and I'd rather not special-case it, so we could potentially handle it by making the The alternative is that in ZIP 316 Revision 1, we could permit transparent-only UAs. Ultimately this might be what we do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, this isn't directly related to the index 0 problem at all. This is simply about tracking which transparent addresses have been created/used so that sync knows which addresses to download transactions for. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So up until now, you could obtain this from the transparent parts of UAs saved to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. All my transparent addresses are generated completely independently of any UA. So I need to be able to represent all of them, even if their sapling counterpart is invalid. |
||
let mut has_sapling = true; | ||
let mut has_transparent = true; | ||
|
||
// Get the most comprehensive UA available for the given diversifier index. | ||
// We may have to drop the sapling and/or the transparent receiver if the diversifier index is invalid or out of range. | ||
let addr = loop { | ||
if let Some(addr) = match ufvk.address( | ||
diversifier_index, | ||
UnifiedAddressRequest::unsafe_new(has_orchard, has_sapling, has_transparent), | ||
) { | ||
Ok(addr) => Some(addr), | ||
Err(AddressGenerationError::InvalidSaplingDiversifierIndex(_)) => { | ||
has_sapling = false; | ||
None | ||
} | ||
Err(AddressGenerationError::InvalidTransparentChildIndex(_)) => { | ||
has_transparent = false; | ||
None | ||
} | ||
Err(_) => return Err(SqliteClientError::DiversifierIndexOutOfRange), | ||
} { | ||
break addr; | ||
} | ||
}; | ||
|
||
return match wallet::insert_address( | ||
wdb.conn.0, | ||
&wdb.params, | ||
account, | ||
diversifier_index, | ||
&addr, | ||
) { | ||
Ok(_) => Ok(addr), | ||
Err(rusqlite::Error::SqliteFailure( | ||
libsqlite3_sys::Error { | ||
code: libsqlite3_sys::ErrorCode::ConstraintViolation, | ||
.. | ||
}, | ||
_, | ||
)) => Ok(addr), // conflicts are ignorable | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we permit conflicts on insertion more generally, and just add an |
||
Err(e) => Err(e.into()), | ||
}; | ||
}) | ||
} | ||
|
||
#[tracing::instrument(skip_all, fields(height = blocks.first().map(|b| u32::from(b.height()))))] | ||
#[allow(clippy::type_complexity)] | ||
fn put_blocks( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this method should take a
UnifiedAddressRequest
instead of having this default behavior: https://github.com/zcash/librustzcash/blob/main/zcash_keys/src/keys.rs#L421-L425There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I left it out was that elsewhere, it was used to influence or check the diversifier index would work for the required receivers. In this case, the caller requires the exact index given to be inserted, so compatibility checks are moot. I didn't really need to return a UnifiedAddress from the function either. In my case I drop it on the floor.
My concern with taking
UnifiedAddressRequest
here is that it complicates the caller, as it now must describe what it expects to be the allowed receivers (and no more than that), when that isn't necessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The caller may be someone like the Brave browser, which will not be supporting the Sapling pool at all. So it must be up to the caller what receivers are generated, and they must handle the error that can occur if the diversifier index does not correspond to an index that is valid for the request they provided.