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.

Sign up for GitHub

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

Declare and use NonHardenedChildIndex for transparent #1185

Merged
merged 6 commits into from
Feb 14, 2024
Merged

Declare and use NonHardenedChildIndex for transparent #1185

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1733af8
Declare `NonHardenedChildIndex` struct
AArnott Feb 10, 2024
918f5cc
Change `WalletRead::get_transparent_receivers` signature
AArnott Feb 10, 2024
8003a39
Add test coverage
AArnott Feb 10, 2024
9f221f8
Fail instead of swallow on database corruption
AArnott Feb 13, 2024
8f6afb2
Update changelogs
AArnott Feb 14, 2024
f1b6dd0
Switch out `AddressMetadata` for new struct
AArnott Feb 14, 2024
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 zcash_client_backend/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ and this library adheres to Rust's notion of
- Arguments to `ScannedBlock::from_parts` have changed.
- Changes to the `WalletRead` trait:
- Added `get_orchard_nullifiers`
- Changed `get_transparent_receivers` return type.
AArnott marked this conversation as resolved.
Show resolved Hide resolved
- `ShieldedProtocol` has a new `Orchard` variant.
- `zcash_client_backend::fees`:
- Arguments to `ChangeStrategy::compute_balance` have changed.
Expand Down
12 changes: 6 additions & 6 deletions zcash_client_backend/src/data_api.rs
View file
Open in desktop
AArnott marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use shardtree::{error::ShardTreeError, store::ShardStore, ShardTree};
use zcash_primitives::{
block::BlockHash,
consensus::BlockHeight,
legacy::TransparentAddress,
legacy::{NonHardenedChildIndex, TransparentAddress},
memo::{Memo, MemoBytes},
transaction::{
components::amount::{Amount, BalanceError, NonNegativeAmount},
Expand All @@ -24,7 +24,7 @@ use zcash_primitives::{
};

use crate::{
address::{AddressMetadata, UnifiedAddress},
address::UnifiedAddress,
decrypt::DecryptedOutput,
keys::{UnifiedAddressRequest, UnifiedFullViewingKey, UnifiedSpendingKey},
proto::service::TreeState,
Expand Down Expand Up @@ -560,7 +560,7 @@ pub trait WalletRead {
fn get_transparent_receivers(
&self,
account: AccountId,
) -> Result<HashMap<TransparentAddress, AddressMetadata>, Self::Error>;
) -> Result<HashMap<TransparentAddress, NonHardenedChildIndex>, Self::Error>;
Copy link
Contributor

Choose a reason for hiding this comment

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

If all we wanted to return from this API is the transparent address and the child index it was derived from, then I argue that NonHardenedChildIndex should be the key: it is trivial to derive the address from the full viewing key given the index (and providing the address is thus only an optimization), whereas the reverse requires brute forcing, so logically the child index is the unique key here.

However, given the existing usage of this API, the child index is being treated as metadata, so we should keep AddressMetadata as the return type here. This would allow for e.g. marking whether a receiver is external or internal (currently this API only returns external receivers because zcash_client_backend wallets never create transparent change, but if a third party wallet seed phrase is imported that did, we'd need to support scanning at least the default internal transparent receiver to detect it). If we keep AddressMetadata removed then in future we'd also need to change the map to be HashMap<(zip32::Scope, NonHardenedChildIndex), TransparentAddress>.

Separately, I note that AddressMetadata is currently only used for transparent addresses, and is really a detail of this specific WalletRead method. It should probably not have been moved into zcash_keys, and should be moved back into zcash_client_backend (and maybe renamed to be specific to transparent addresses, since we do not expose internal addresses for shielded keys).

After a rather deep rabbit-hole session with @nuttycom, our conclusion is we should:

  • Move AddressMetadata back into zcash_client_backend::address.
  • Change AddressMetadata to replace diversifier_index with NonHardenedChildIndex, remove account_id, and add zip32::Scope.

These changes don't need to happen in this PR, however this PR should be changed to not remove AddressMetadata from the return type of this method. So if you do want to make the other changes here, you are welcome to.

Copy link
Contributor

Choose a reason for hiding this comment

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

More specifically, we realized as part of the discussion that there is probably an existing bug related to the import of seeds from transparent-only wallets, where we are not at present scanning any of the internal key tree (which is where Bitcoin wallets regularly send change) so if you import a seed that was previously used with a transparent-only wallet, you're likely to be missing funds.

Then again, if you do that, then you're also not doing gap limit scanning at the moment, so both of these should be addressed at once. I'll open an issue for that and it can get stuck in the backlog somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AddressMetadata includes DiversifierIndex and the ZIP-32 AccountId. Both are problematic because we may not know the address index at all for a given receiver, and similarly we may not know the AccountId. At least, we won't be guaranteed to know these things after #1175 merges. And it is in preparation for that PR that I created this PR in the first place. Although now that I think about it, I guess we're not even guaranteed to know the NonHardenedChildIndex. So what do we do about a function whose primary purpose (based on its name, anyway) is to produce the transparent addresses for a given account, when it also provides metadata for that address which may or may not be available?
What about changing it to return a vector of tuples, where the first element is the address, and the second element is an Option<T> where T is whatever makes the most sense given what we may know?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it all of the metadata that may be known or unknown, or is it each field of AddressMetadata somewhat independently? If the former then use Option<AddressMetadata>, otherwise make the relevant accessors on AddressMetadata return Options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed. Please take another look.
I left the key=value order as it was because in the future when we support importing transparent private keys, we won't have derivation data.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although now that I think about it, I guess we're not even guaranteed to know the NonHardenedChildIndex.

If we can't even guarantee that, then we cannot spend any funds received to that address, because:

I left the key=value order as it was because in the future when we support importing transparent private keys, we won't have derivation data.

We do not import raw transparent spending keys; everything needs to be at a minimum tied back to a transparent full viewing key (which we define in ZIP 316 as a particular node in the BIP 44 key derivation path).

Importing arbitrary non-HD transparent private keys will break a bunch of assumptions the current wallet architecture makes, and no one has committed to supporting and maintaining that use case in the long term. From a user perspective, it is only necessary in the context of a full node wallet that explicitly supports importing an arbitrary zcashd wallet, and even in that case we should preferably instead have a migration pathway away from those transparent keys. Transparent-only mobile wallets have for basically all of Zcash's existence used HD-derived keys.

Copy link
Contributor

Choose a reason for hiding this comment

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

While I am strongly opposed to trait changes that point us in the direction of adding support for long-since-deprecated zcashd behaviours, I am not going to block this PR being merged. I have not fully re-reviewed the PR after the recent changes, but the trait returning Option<Metadata> is the least intrusive thing that could be done, and I will almost certainly just be .unwrap()ing everywhere downstream.


/// Returns a mapping from transparent receiver to not-yet-shielded UTXO balance,
/// for each address associated with a nonzero balance.
Expand Down Expand Up @@ -1109,14 +1109,14 @@ pub mod testing {
use zcash_primitives::{
block::BlockHash,
consensus::{BlockHeight, Network},
legacy::TransparentAddress,
legacy::{NonHardenedChildIndex, TransparentAddress},
memo::Memo,
transaction::{components::Amount, Transaction, TxId},
zip32::{AccountId, Scope},
};

use crate::{
address::{AddressMetadata, UnifiedAddress},
address::UnifiedAddress,
keys::{UnifiedAddressRequest, UnifiedFullViewingKey, UnifiedSpendingKey},
wallet::{Note, NoteId, ReceivedNote, WalletTransparentOutput},
ShieldedProtocol,
Expand Down Expand Up @@ -1284,7 +1284,7 @@ pub mod testing {
fn get_transparent_receivers(
&self,
_account: AccountId,
) -> Result<HashMap<TransparentAddress, AddressMetadata>, Self::Error> {
) -> Result<HashMap<TransparentAddress, NonHardenedChildIndex>, Self::Error> {
Ok(HashMap::new())
}

Expand Down
13 changes: 1 addition & 12 deletions zcash_client_backend/src/data_api/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use crate::data_api::wallet::input_selection::InputSelectorError;
use crate::PoolType;

#[cfg(feature = "transparent-inputs")]
use zcash_primitives::{legacy::TransparentAddress, zip32::DiversifierIndex};
use zcash_primitives::legacy::TransparentAddress;

use crate::wallet::NoteId;

Expand Down Expand Up @@ -70,9 +70,6 @@ pub enum Error<DataSourceError, CommitmentTreeError, SelectionError, FeeError> {

#[cfg(feature = "transparent-inputs")]
AddressNotRecognized(TransparentAddress),

#[cfg(feature = "transparent-inputs")]
ChildIndexOutOfRange(DiversifierIndex),
AArnott marked this conversation as resolved.
Show resolved Hide resolved
}

impl<DE, CE, SE, FE> fmt::Display for Error<DE, CE, SE, FE>
Expand Down Expand Up @@ -128,14 +125,6 @@ where
Error::AddressNotRecognized(_) => {
write!(f, "The specified transparent address was not recognized as belonging to the wallet.")
}
#[cfg(feature = "transparent-inputs")]
Error::ChildIndexOutOfRange(i) => {
write!(
f,
"The diversifier index {:?} is out of range for transparent addresses.",
i
)
}
}
}
}
Expand Down
10 changes: 3 additions & 7 deletions zcash_client_backend/src/data_api/wallet.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -639,17 +639,13 @@ where
for utxo in proposal.transparent_inputs() {
utxos.push(utxo.clone());

let diversifier_index = known_addrs
let child_index = known_addrs
.get(utxo.recipient_address())
.ok_or_else(|| Error::AddressNotRecognized(*utxo.recipient_address()))?
.diversifier_index();

let child_index = u32::try_from(*diversifier_index)
.map_err(|_| Error::ChildIndexOutOfRange(*diversifier_index))?;
.ok_or_else(|| Error::AddressNotRecognized(*utxo.recipient_address()))?;

let secret_key = usk
.transparent()
.derive_external_secret_key(child_index)
.derive_external_secret_key(*child_index)
.unwrap();

builder.add_transparent_input(
Expand Down
6 changes: 3 additions & 3 deletions zcash_client_sqlite/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ use shardtree::{error::ShardTreeError, ShardTree};
use zcash_primitives::{
block::BlockHash,
consensus::{self, BlockHeight},
legacy::TransparentAddress,
legacy::{NonHardenedChildIndex, TransparentAddress},
memo::{Memo, MemoBytes},
transaction::{
components::amount::{Amount, NonNegativeAmount},
Expand All @@ -58,7 +58,7 @@ use zcash_primitives::{
};

use zcash_client_backend::{
address::{AddressMetadata, UnifiedAddress},
address::UnifiedAddress,
data_api::{
self,
chain::{BlockSource, CommitmentTreeRoot},
Expand Down Expand Up @@ -346,7 +346,7 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters> WalletRead for W
fn get_transparent_receivers(
&self,
_account: AccountId,
) -> Result<HashMap<TransparentAddress, AddressMetadata>, Self::Error> {
) -> Result<HashMap<TransparentAddress, NonHardenedChildIndex>, Self::Error> {
#[cfg(feature = "transparent-inputs")]
return wallet::get_transparent_receivers(self.conn.borrow(), &self.params, _account);

Expand Down
41 changes: 25 additions & 16 deletions zcash_client_sqlite/src/wallet.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@
zip32::{AccountId, DiversifierIndex},
};

#[cfg(feature = "transparent-inputs")]
use zcash_primitives::legacy::NonHardenedChildIndex;

use zcash_client_backend::{
address::{Address, UnifiedAddress},
data_api::{
Expand All @@ -112,7 +115,7 @@
crate::UtxoId,
rusqlite::Row,
std::collections::BTreeSet,
zcash_client_backend::{address::AddressMetadata, wallet::WalletTransparentOutput},
zcash_client_backend::wallet::WalletTransparentOutput,
zcash_primitives::{
legacy::{keys::IncomingViewingKey, Script, TransparentAddress},
transaction::components::{OutPoint, TxOut},
Expand Down Expand Up @@ -349,7 +352,7 @@
conn: &rusqlite::Connection,
params: &P,
account: AccountId,
) -> Result<HashMap<TransparentAddress, AddressMetadata>, SqliteClientError> {
) -> Result<HashMap<TransparentAddress, NonHardenedChildIndex>, SqliteClientError> {
let mut ret = HashMap::new();

// Get all UAs derived
Expand All @@ -360,12 +363,12 @@
while let Some(row) = rows.next()? {
let ua_str: String = row.get(0)?;
let di_vec: Vec<u8> = row.get(1)?;
let mut di_be: [u8; 11] = di_vec.try_into().map_err(|_| {
let mut di: [u8; 11] = di_vec.try_into().map_err(|_| {
SqliteClientError::CorruptedData(
"Diverisifier index is not an 11-byte value".to_owned(),
)
})?;
di_be.reverse();
di.reverse(); // BE -> LE conversion

let ua = Address::decode(params, &ua_str)
.ok_or_else(|| {
Expand All @@ -380,16 +383,25 @@
})?;

if let Some(taddr) = ua.transparent() {
ret.insert(
*taddr,
AddressMetadata::new(account, DiversifierIndex::from(di_be)),
);
let index = NonHardenedChildIndex::from_index(
DiversifierIndex::from(di).try_into().map_err(|_| {
SqliteClientError::CorruptedData(
"Unable to get diversifier for transparent address.".to_string(),

Check warning on line 389 in zcash_client_sqlite/src/wallet.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_sqlite/src/wallet.rs#L388-L389 

Added lines #L388 - L389 were not covered by tests
)
})?,
)
.ok_or_else(|| {
SqliteClientError::CorruptedData(
"Unexpected hardened index for transparent address.".to_string(),

Check warning on line 395 in zcash_client_sqlite/src/wallet.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_sqlite/src/wallet.rs#L394-L395 

Added lines #L394 - L395 were not covered by tests
)
})?;

ret.insert(*taddr, index);
}
}

if let Some((taddr, diversifier_index)) = get_legacy_transparent_address(params, conn, account)?
{
ret.insert(taddr, AddressMetadata::new(account, diversifier_index));
if let Some((taddr, child_index)) = get_legacy_transparent_address(params, conn, account)? {
ret.insert(taddr, child_index);
}

Ok(ret)
Expand All @@ -400,7 +412,7 @@
params: &P,
conn: &rusqlite::Connection,
account: AccountId,
) -> Result<Option<(TransparentAddress, DiversifierIndex)>, SqliteClientError> {
) -> Result<Option<(TransparentAddress, NonHardenedChildIndex)>, SqliteClientError> {
// Get the UFVK for the account.
let ufvk_str: Option<String> = conn
.query_row(
Expand All @@ -418,10 +430,7 @@
ufvk.transparent()
.map(|tfvk| {
tfvk.derive_external_ivk()
.map(|tivk| {
let (taddr, child_index) = tivk.default_address();
(taddr, DiversifierIndex::from(child_index))
})
.map(|tivk| tivk.default_address())
.map_err(SqliteClientError::HdwalletError)
})
.transpose()
Expand Down
6 changes: 4 additions & 2 deletions zcash_client_sqlite/src/wallet/init/migrations/add_transaction_views.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,9 @@ mod tests {
#[cfg(feature = "transparent-inputs")]
fn migrate_from_wm2() {
use zcash_client_backend::keys::UnifiedAddressRequest;
use zcash_primitives::transaction::components::amount::NonNegativeAmount;
use zcash_primitives::{
legacy::NonHardenedChildIndex, transaction::components::amount::NonNegativeAmount,
};

use crate::UA_TRANSPARENT;

Expand Down Expand Up @@ -450,7 +452,7 @@ mod tests {
.and_then(|k| {
k.derive_external_ivk()
.ok()
.map(|k| k.derive_address(0).unwrap())
.map(|k| k.derive_address(NonHardenedChildIndex::ZERO).unwrap())
})
.map(|a| a.encode(&network));

Expand Down
10 changes: 6 additions & 4 deletions zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,13 +292,13 @@ mod tests {
use zcash_primitives::{
block::BlockHash,
consensus::{BlockHeight, Network, NetworkUpgrade, Parameters},
legacy::keys::IncomingViewingKey,
legacy::{keys::IncomingViewingKey, NonHardenedChildIndex},
memo::MemoBytes,
transaction::{
builder::{BuildConfig, BuildResult, Builder},
components::amount::NonNegativeAmount,
components::{amount::NonNegativeAmount, transparent},
fees::fixed,
},
transaction::{components::transparent, fees::fixed},
zip32::{AccountId, Scope},
};
use zcash_proofs::prover::LocalTxProver;
Expand Down Expand Up @@ -354,7 +354,9 @@ mod tests {
);
builder
.add_transparent_input(
usk0.transparent().derive_external_secret_key(0).unwrap(),
usk0.transparent()
.derive_external_secret_key(NonHardenedChildIndex::ZERO)
.unwrap(),
transparent::OutPoint::new([1; 32], 0),
transparent::TxOut {
value: NonNegativeAmount::const_from_u64(EXTERNAL_VALUE + INTERNAL_VALUE),
Expand Down
15 changes: 10 additions & 5 deletions zcash_keys/src/keys.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ use zcash_primitives::{

use crate::address::UnifiedAddress;

#[cfg(feature = "transparent-inputs")]
use zcash_primitives::legacy::NonHardenedChildIndex;

#[cfg(feature = "transparent-inputs")]
use {
std::convert::TryInto,
Expand Down Expand Up @@ -68,13 +71,13 @@ pub mod sapling {
}

#[cfg(feature = "transparent-inputs")]
fn to_transparent_child_index(j: DiversifierIndex) -> Option<u32> {
fn to_transparent_child_index(j: DiversifierIndex) -> Option<NonHardenedChildIndex> {
let (low_4_bytes, rest) = j.as_bytes().split_at(4);
let transparent_j = u32::from_le_bytes(low_4_bytes.try_into().unwrap());
if transparent_j > (0x7FFFFFFF) || rest.iter().any(|b| b != &0) {
if rest.iter().any(|b| b != &0) {
None
} else {
Some(transparent_j)
NonHardenedChildIndex::from_index(transparent_j)
}
}

Expand Down Expand Up @@ -388,7 +391,7 @@ impl UnifiedSpendingKey {
}

#[cfg(all(feature = "test-dependencies", feature = "transparent-inputs"))]
pub fn default_transparent_address(&self) -> (TransparentAddress, u32) {
pub fn default_transparent_address(&self) -> (TransparentAddress, NonHardenedChildIndex) {
self.transparent()
.to_account_pubkey()
.derive_external_ivk()
Expand Down Expand Up @@ -812,13 +815,15 @@ mod tests {
#[cfg(feature = "transparent-inputs")]
#[test]
fn pk_to_taddr() {
use zcash_primitives::legacy::NonHardenedChildIndex;

let taddr =
legacy::keys::AccountPrivKey::from_seed(&MAIN_NETWORK, &seed(), AccountId::ZERO)
.unwrap()
.to_account_pubkey()
.derive_external_ivk()
.unwrap()
.derive_address(0)
.derive_address(NonHardenedChildIndex::ZERO)
.unwrap()
.encode(&MAIN_NETWORK);
assert_eq!(taddr, "t1PKtYdJJHhc3Pxowmznkg7vdTwnhEsCvR4".to_string());
Expand Down
1 change: 1 addition & 0 deletions zcash_primitives/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ and this library adheres to Rust's notion of

## [Unreleased]
### Added
- `legacy::keys::NonHardenedChildIndex`
AArnott marked this conversation as resolved.
Show resolved Hide resolved
- Dependency on `bellman 0.14`.
- `zcash_primitives::consensus::sapling_zip212_enforcement`
- `zcash_primitives::transaction`:
Expand Down
Loading
Loading