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 support for transparent-source-only (TEX) addresses #1257

Merged
merged 56 commits into from
Jul 17, 2024
Merged
Changes from 1 commit
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
2fae4bb
ZIP 320 implementation.
daira Jun 22, 2024
0f3de63
Apply documentation suggestions from code review.
daira Jun 22, 2024
549fe0b
In `reserve_next_n_ephemeral_addresses`, exclude addresses observed in
daira Jun 22, 2024
eb88461
Address review comment: `EphemeralIvk` should not implement `Incoming…
daira Jun 22, 2024
c6520cf
Change the protobuf schema to explicitly specify whether a `ChangeValue`
daira Jun 22, 2024
2f521d7
If a change memo is supplied, it should not be used in the second step
daira Jun 23, 2024
0f49dae
`mark_ephemeral_address_as_mined` now prefers setting `mined_in_tx` to
daira Jun 23, 2024
637ae92
Add a migration test for the `ephemeral_addresses` migration.
daira Jun 23, 2024
994f6ff
Change type of `n` in `reserve_next_n_ephemeral_addresses`.
daira Jun 24, 2024
25f07da
Add a constraint on the range of `ephemeral_addresses(address_index)`.
daira Jun 24, 2024
e164b59
Move most ephemeral address index handling into helper functions in
daira Jun 24, 2024
914acb5
Move most remaining code for wallet support of ephemeral addresses into
daira Jun 24, 2024
745054b
`find_account_for_transparent_output` now searches unreserved ephemeral
daira Jun 24, 2024
6b465b7
Document the mapping functions on `zcash_client_backend::wallet::Reci…
daira Jun 24, 2024
4f43a01
Refactor transparent address metadata lookups. This is correct as-is but
daira Jun 25, 2024
5a90fff
Factor out the conversion of the `diversifier_index_be` field in the
daira Jun 25, 2024
7fb3557
Implement `WalletRead::get_transparent_address_metadata` for
daira Jun 25, 2024
7d8a96f
Don't cache metadata between steps; it's not an important optimization.
daira Jun 25, 2024
bd6c9f3
Apply documentation suggestions from code review.
daira Jun 26, 2024
0735390
Rename `amount` to `transfer_amount` in `send_multi_step_proposed_tra…
daira Jun 26, 2024
c926e7c
Filter ephemeral transparent `ChangeValue`s by `is_ephemeral()` as we…
daira Jun 26, 2024
ffb2ddf
`zcash_client_backend::fees::ChangeValue` is now an enum, allowing only
daira Jun 26, 2024
b778139
Refactor ephemeral output-related parameters to balance calculation.
daira Jun 26, 2024
7a05b44
Make mutable inputs to closures in `create_proposed_transaction` expl…
daira Jun 26, 2024
6471d4c
Don't assume that prior step outputs are ephemeral iff they are
daira Jun 27, 2024
ec4a6d0
Documentation improvements.
daira Jun 27, 2024
81a2846
Simpler way to calculate `has_shielded_inputs`.
daira Jun 27, 2024
f0e5aab
Improve discrimination of proposal errors.
daira Jun 27, 2024
feabe6d
Remove complicated code to calculate the number of dust spends.
daira Jun 26, 2024
baccb43
Restore the logic to determine whether we are spending inputs that are
daira Jun 28, 2024
9c082dc
`zcash_primitives::transaction::fees::zip317::FeeRule::non_standard` has
daira Jun 28, 2024
3829663
Change note selection query to select notes > 5000 zats, not >= 5000 …
daira Jun 28, 2024
25006ab
Documentation improvement from code review.
daira Jun 28, 2024
3922d71
Change the type of `n` in `reserve_next_n_ephemeral_addresses` back t…
daira Jun 28, 2024
d32b7db
Remove `ChangeValue::new`. Also document `ChangeValue::is_ephemeral` as
daira Jun 28, 2024
286439a
Define a constant `EphemeralParameters::NONE` instead of deriving `De…
daira Jun 29, 2024
bc38f2a
Document the `possible_change` parameter to `check_for_uneconomic_inp…
daira Jun 29, 2024
7838c04
Make `ephemeral_parameters` and `EphemeralParameters::NONE` available
daira Jun 29, 2024
14bdcde
We cannot spend prior outputs at all when "transparent-inputs" is not
daira Jun 29, 2024
8636daa
Tiny simplification.
daira Jun 29, 2024
01ff201
Minor changes responding to review comments.
daira Jul 3, 2024
b63ff5b
Rename `get_reserved_ephemeral_addresses` to `get_known_ephemeral_add…
daira Jul 3, 2024
e97da43
Refactoring to address review comments.
daira Jul 3, 2024
a01588b
Ensure that `mark_ephemeral_address_as_mined` correctly handles indices
daira Jul 4, 2024
86428c4
Refactor `find_account_for_transparent_output` (now called
daira Jul 4, 2024
b48f627
Minor simplification in a test.
daira Jul 4, 2024
6bc22f4
Documentation fixes and improvements.
daira Jul 4, 2024
27ca6e4
Remove unneeded `impl SealedChangeLevelKey for EphemeralIvk`.
daira Jul 4, 2024
bda6451
Change `unwrap`s to `expect`s when constructing `NonHardenedChildIndex`.
daira Jul 4, 2024
22b8cff
The `TxId` argument to `EphemeralAddressReuse` does not need to be op…
daira Jul 4, 2024
9856a70
Simpler handling of a potential overflow.
daira Jul 4, 2024
56aa348
Extend the `send_multi_step_proposed_transfer` test to check the beha…
daira Jul 4, 2024
aa43123
Use `EphemeralBalance` instead of `EphemeralParameters`
nuttycom Jul 16, 2024
dbb5eeb
Fix a potential crash related to varying behavior between change stra…
nuttycom Jul 16, 2024
24b6d50
Apply suggestions from code review
nuttycom Jul 16, 2024
f8bedd8
Make ephemeral_addresses.address unique
nuttycom Jul 17, 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
Prev Previous commit
Next Next commit
Change type of n in reserve_next_n_ephemeral_addresses.
Signed-off-by: Daira-Emma Hopwood <[email protected]>
  • Loading branch information
daira committed Jun 25, 2024
commit 994f6ff3877527d458ab65e6fc33b1ee80f88304
7 changes: 5 additions & 2 deletions zcash_client_backend/src/data_api.rs
Original file line number Diff line number Diff line change
@@ -1599,12 +1599,14 @@ pub trait WalletWrite: WalletRead {
/// the given number of addresses, or if the account identifier does not correspond
/// to a known account.
///
/// Precondition: `n >= 0`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is 0 a valid value?

Copy link
Contributor Author

@daira daira Jun 28, 2024

Choose a reason for hiding this comment

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

There's no reason to exclude it; you're allocating 0 addresses which always succeeds. As it happens the current code relies on this toward the end of create_proposed_transaction: it doesn't bother to explicitly test whether there are any ephemeral addresses because the zero case just does the right thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the calling code enforce that the number of addresses returned is equal to the number of addresses that was requested?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nonblocking, but I really don't see why we need to allocate more than one at a time. It adds error cases that need to be handled; if the method only returned a single address, then we could make failure to generate the address an error; it would be able to return Result<(TransparentAddress, TransparentAddressMetadata), Self::Error> directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the calling code enforce that the number of addresses returned is equal to the number of addresses that was requested?

Yes.

Nonblocking, but I really don't see why we need to allocate more than one at a time. It adds error cases that need to be handled; if the method only returned a single address, then we could make failure to generate the address an error; it would be able to return Result<(TransparentAddress, TransparentAddressMetadata), Self::Error> directly.

You may be right, but it works as-is and I don't think it would be much of a simplification.

///
/// [BIP 44]: https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki#user-content-Address_gap_limit
#[cfg(feature = "transparent-inputs")]
fn reserve_next_n_ephemeral_addresses(
&mut self,
account_id: Self::AccountId,
n: u32,
n: i32,
daira marked this conversation as resolved.
Show resolved Hide resolved
) -> Result<Vec<(TransparentAddress, TransparentAddressMetadata)>, Self::Error>;
daira marked this conversation as resolved.
Show resolved Hide resolved
}

@@ -2010,8 +2012,9 @@ pub mod testing {
fn reserve_next_n_ephemeral_addresses(
&mut self,
_account_id: Self::AccountId,
_n: u32,
n: i32,
daira marked this conversation as resolved.
Show resolved Hide resolved
) -> Result<Vec<(TransparentAddress, TransparentAddressMetadata)>, Self::Error> {
assert!(n >= 0);
daira marked this conversation as resolved.
Show resolved Hide resolved
Err(())
}
}
2 changes: 1 addition & 1 deletion zcash_client_backend/src/data_api/wallet.rs
Original file line number Diff line number Diff line change
@@ -1157,7 +1157,7 @@ where
.filter(|(_, change_value)| matches!(change_value.output_pool(), PoolType::Transparent))
daira marked this conversation as resolved.
Show resolved Hide resolved
.collect();
let num_ephemeral_outputs =
u32::try_from(ephemeral_outputs.len()).map_err(|_| Error::ProposalNotSupported)?;
i32::try_from(ephemeral_outputs.len()).map_err(|_| Error::ProposalNotSupported)?;

let addresses_and_metadata = wallet_db
.reserve_next_n_ephemeral_addresses(account_id, num_ephemeral_outputs)
daira marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion zcash_client_sqlite/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1482,7 +1482,7 @@ impl<P: consensus::Parameters> WalletWrite for WalletDb<rusqlite::Connection, P>
fn reserve_next_n_ephemeral_addresses(
&mut self,
account_id: Self::AccountId,
n: u32,
n: i32,
) -> Result<Vec<(TransparentAddress, TransparentAddressMetadata)>, Self::Error> {
self.transactionally(|wdb| {
wallet::transparent::reserve_next_n_ephemeral_addresses(wdb, account_id, n)
4 changes: 3 additions & 1 deletion zcash_client_sqlite/src/wallet/transparent.rs
Original file line number Diff line number Diff line change
@@ -804,6 +804,8 @@ pub(crate) fn get_reserved_ephemeral_addresses<P: consensus::Parameters>(
/// Returns a vector with the next `n` previously unreserved ephemeral addresses for
/// the given account.
///
/// Precondition: `n >= 0`
///
/// # Errors
///
/// * `SqliteClientError::AccountUnknown`, if there is no account with the given id.
@@ -817,7 +819,7 @@ pub(crate) fn get_reserved_ephemeral_addresses<P: consensus::Parameters>(
pub(crate) fn reserve_next_n_ephemeral_addresses<P: consensus::Parameters>(
wdb: &mut WalletDb<SqlTransaction<'_>, P>,
account_id: AccountId,
n: u32,
n: i32,
) -> Result<Vec<(TransparentAddress, TransparentAddressMetadata)>, SqliteClientError> {
if n == 0 {
return Ok(vec![]);