Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions zcash_client_backend/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ and this library adheres to Rust's notion of
- `zcash_client_backend::fees::ChangeValue::orchard`
- `zcash_client_backend::wallet`:
- `Note::Orchard`
- `WalletWrite::insert_address_with_diversifier_index`

### Changed
- `zcash_client_backend::data_api`:
Expand Down
31 changes: 31 additions & 0 deletions zcash_client_backend/src/data_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ use zcash_primitives::{
},
zip32::{AccountId, Scope},
};
use zip32::DiversifierIndex;

use crate::{
address::{AddressMetadata, UnifiedAddress},
Expand Down Expand Up @@ -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.
Comment on lines +1029 to +1030
Copy link
Contributor

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-L425

Copy link
Contributor Author

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.

Copy link
Contributor

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.

/// 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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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!()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please either implement this, or add an issue to track the TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would returning Err(()) count? It's difficult to imagine a better implementation given this mock doesn't store anything at all.

Copy link
Contributor

Choose a reason for hiding this comment

The 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,
Expand Down
1 change: 1 addition & 0 deletions zcash_client_sqlite/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ shardtree = { workspace = true, features = ["legacy-api"] }
# CocoaPods, due to being bound to React Native. We need to ensure that the SQLite
# version required for `rusqlite` is a version that is available through CocoaPods.
rusqlite = { version = "0.29.0", features = ["bundled", "time", "array"] }
libsqlite3-sys = { version = "0.26.0", features = ["bundled"] }
schemer = "0.2"
schemer-rusqlite = "0.2.2"
time = "0.3.22"
Expand Down
58 changes: 58 additions & 0 deletions zcash_client_sqlite/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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 orchard support, which is a "the user can't see their received funds" risk. Instead, either use https://github.com/zcash/librustzcash/blob/main/zcash_client_sqlite/src/lib.rs#L110 or have the request passed in by the caller.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 address column which (at least so far) seems to only take UAs. Can I insert a transparent address there, or will code fall over?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 addresses table to record the last sync'd block for the transparent component of the address. If that isn't tolerable for this crate, then my creating a whole new table to store the data I need (and isn't wanted upstream) may be a better overall approach.

Copy link
Contributor

@nuttycom nuttycom Feb 7, 2024

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@AArnott AArnott Feb 8, 2024

Choose a reason for hiding this comment

The 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 address column, or create another table to store this data. Do you have a preference?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 address column nullable, and using the cached_transparent_receiver_address for this purpose. However, for that, you'd need a different entrypoint, as at present you can't create UA requests that don't have a shielded component.

The alternative is that in ZIP 316 Revision 1, we could permit transparent-only UAs. Ultimately this might be what we do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 addresses table, plus the 0 address (which I had forgotten I added handling for in #685). Is the thing that is changing is that you intend to generate transparent addresses that are not correlated to any UA? Sorry if I'm being obtuse, but I'm still having a bit of trouble understanding the use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we permit conflicts on insertion more generally, and just add an ON CONFLICT DO NOTHING clause to the SQL code in wallet::insert_address?

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(
Expand Down
Loading