Add transparent address gap limit handling & general address rotation functionality. #1673

nuttycom · 2024-12-23T20:43:44Z

No description provided.

codecov · 2024-12-28T05:55:12Z

Codecov Report

Attention: Patch coverage is 59.32203% with 312 lines in your changes missing coverage. Please review.

Project coverage is 51.86%. Comparing base (1b09741) to head (2b3f0d4).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
.../init/migrations/transparent_gap_limit_handling.rs	45.71%	76 Missing ⚠️
zcash_client_sqlite/src/wallet.rs	60.99%	55 Missing ⚠️
zcash_client_sqlite/src/lib.rs	51.37%	53 Missing ⚠️
zcash_client_sqlite/src/wallet/transparent.rs	74.12%	52 Missing ⚠️
zcash_client_sqlite/src/wallet/orchard.rs	0.00%	27 Missing ⚠️
.../src/wallet/init/migrations/ephemeral_addresses.rs	56.00%	11 Missing ⚠️
zcash_client_sqlite/src/wallet/sapling.rs	68.96%	9 Missing ⚠️
..._client_sqlite/src/wallet/transparent/ephemeral.rs	59.09%	9 Missing ⚠️
zcash_client_backend/src/data_api.rs	0.00%	3 Missing ⚠️
zcash_client_sqlite/src/error.rs	0.00%	3 Missing ⚠️
... and 10 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1673      +/-   ##
==========================================
- Coverage   51.89%   51.86%   -0.03%     
==========================================
  Files         180      182       +2     
  Lines       21169    21601     +432     
==========================================
+ Hits        10986    11204     +218     
- Misses      10183    10397     +214

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…handling-prep Preparatory refactoring for #1673

zcash_client_sqlite/src/wallet/db.rs

zcash_client_sqlite/src/wallet.rs

str4d · 2024-12-30T23:47:07Z

zcash_client_sqlite/src/wallet.rs

+            :cached_transparent_receiver_address,
+            :exposed_at_height
+        )
+        ON CONFLICT (account_id, diversifier_index_be, key_scope) DO UPDATE


In the case that this upsert is triggered by a note that was received in a pool that the conflicting row's address does not contain, we should upgrade the row to contain that receiver (similar to what we do in zcashd), because this is evidence of that receiver being exposed to the ecosystem. It's not technically evidence of other receivers in address being exposed, so we might only want to add the one receiver triggering the upsert to the conflicting row.

zcash_client_sqlite/src/wallet/transparent.rs

This is a large change that unifies the handling of ephemeral transparent addresses for ZIP 320 support with generalized "gap limit" handling for transparent wallet recovery. The best way to understand this commit is to start from the `transparent_gap_limit_handling` database migration that drives the change in behavior.

…limit handling.

Co-authored-by: Jack Grigg <[email protected]>

nuttycom · 2025-01-29T18:36:18Z

force-pushed to address comments from code review
then
force-pushed to rebase on main to address conflicts.

daira · 2025-01-29T19:02:07Z

zcash_client_backend/src/data_api.rs

@@ -1393,7 +1391,7 @@ pub trait WalletRead {
    /// This is equivalent to (but may be implemented more efficiently than):
    /// ```compile_fail
    /// Ok(
-    ///     if let Some(result) = self.get_transparent_receivers(account)?.get(address) {


I think this can now be equivalent to just Ok(self.get_transparent_receivers(account, true, true)?.get(address)). (The purpose of the else branch was only to include ephemeral addresses, and setting include_ephemeral to true does that. We also include change/internal addresses now, but that's correct. It could be clarified by saying "a given transparent receiver, which can also be a change or ephemeral address, ...".)

daira · 2025-01-29T19:05:35Z

zcash_client_backend/src/data_api.rs

@@ -1369,6 +1365,8 @@ pub trait WalletRead {
    fn get_transparent_receivers(
        &self,
        _account: Self::AccountId,
+        _include_change: bool,


Do we create, or can we reasonably anticipate creating, internal transparent addresses other than for change? I think auto-shielding and note management will not do so.

We will generate these as part of gap limit handling in order to make it possible to recover the full history for transparent-only wallets that follow the Bitcoin conventions for generation of change addresses (we'll advance the gap as soon as we see such addresses used in transactions, and not otherwise).

I think the intent of the question was missed. It was: is it correct to refer to all of these internal addresses as "change addresses", or are there / will there be non-change internal addresses (as there are in the shielded case)?

By definition these are addresses at the change child index.

daira · 2025-01-29T19:13:02Z

zcash_client_backend/src/data_api.rs

@@ -1414,7 +1412,10 @@ pub trait WalletRead {
    ) -> Result<Option<TransparentAddressMetadata>, Self::Error> {
        // This should be overridden.
        Ok(
-            if let Some(result) = self.get_transparent_receivers(account)?.get(address) {


Change the default implementation if we change the doc.

daira · 2025-01-29T19:16:31Z

zcash_client_backend/src/data_api.rs

+    /// be generated using all of the available receivers for the account's UFVK.
+    ///
+    /// In the case that the diversifier index is outside of the range of valid transparent address
+    /// indexes, no transparent receiver should be generated in the resulting unified address. If a


Suggested change

/// indexes, no transparent receiver should be generated in the resulting unified address. If a

/// indices, no transparent receiver should be generated in the resulting unified address. If a

(We use "indices" far more often than "indexes"; the latter is only used a couple of times in zcash_transparent/src/keys.rs.)

daira · 2025-01-29T19:28:54Z

zcash_client_backend/src/data_api/testing/pool.rs

+/// The number of ephemeral addresses that can be safely reserved without observing any
+/// of them to be mined.
+#[cfg(feature = "transparent-inputs")]
+pub const EPHEMERAL_ADDR_GAP_LIMIT: usize = 5;


I don't think we should change this from what was previously defined (i.e. 20). There is a potential compatibility problem with existing wallets: a wallet that previously worked might violate a smaller gap limit.

The reason to use a smaller gap limit is that ephemeral addresses should, modulo wallet failures, always receive at least one (and very frequently at most one) transaction. This is the same rationale as Bitcoin wallets use for specifying a small gap limit for change addresses; I think it holds here.

daira · 2025-01-29T19:33:07Z

zcash_client_backend/src/data_api/testing/pool.rs

+            let (h, _) = st.generate_next_block_including(*txid);
+            st.scan_cached_blocks(h, 1);
+        }
+
        // Check that there are sent outputs with the correct values.


Previously there were sent outputs even before the transactions had been mined; is that no longer the case? It was intended to check it, and also to check that the transaction appears in the history before being mined.

(Whether a status request is generated before the tx has been mined is not so important, I think.)

The lower end of the gap (and therefore the upper end of the gap) cannot be advanced until the transaction is actually mined. Specifically, the tested behavior that is deleted where the reserve_next_n_ephemeral_addresses call advances the start of the gap was testing a bug.

daira · 2025-01-29T19:38:58Z

zcash_client_backend/src/data_api/testing/pool.rs

-    assert_eq!(next_reserved[0], known_addrs[11]);
-
-    // Calling `reserve_next_n_ephemeral_addresses(account_id, 1)` will have advanced
-    // the start of the gap to index 12. This also tests the `index_range` parameter.
-    let newer_known_addrs = st
-        .wallet()
-        .get_known_ephemeral_addresses(
-            account_id,
-            Some(
-                NonHardenedChildIndex::from_index(5).unwrap()
-                    ..NonHardenedChildIndex::from_index(100).unwrap(),
-            ),
-        )
-        .unwrap();
-    assert_eq!(newer_known_addrs.len(), (GAP_LIMIT as usize) + 12 - 5);
-    assert!(newer_known_addrs.starts_with(&new_known_addrs[5..]));
-
-    // None of the five transactions created above (two from each proposal and the
-    // one built manually) have been mined yet. So, the range of address indices
-    // that are safe to reserve is still 0..20, and we have already reserved 12
-    // addresses, so trying to reserve another 9 should fail.
-    reservation_should_fail(&mut st, 9, 20);
-    reservation_should_succeed(&mut st, 8);
-    reservation_should_fail(&mut st, 1, 20);
-
-    // Now mine the transaction with the ephemeral output at index 1.
-    // We already reserved 20 addresses, so this should allow 2 more (..22).
-    // It does not matter that the transaction with ephemeral output at index 0
-    // remains unmined.
-    let (h, _) = st.generate_next_block_including(txids1.head);
-    st.scan_cached_blocks(h, 1);
-    reservation_should_succeed(&mut st, 2);
-    reservation_should_fail(&mut st, 1, 22);
-
-    // Mining the transaction with the ephemeral output at index 0 at this point
-    // should make no difference.
-    let (h, _) = st.generate_next_block_including(txids0.head);
-    st.scan_cached_blocks(h, 1);
-    reservation_should_fail(&mut st, 1, 22);
-
-    // Now mine the transaction with the ephemeral output at index 10.
-    let tx = build_result.transaction();
-    let tx_index = 1;
-    let (h, _) = st.generate_next_block_from_tx(tx_index, tx);
-    st.scan_cached_blocks(h, 1);
-
-    // The above `scan_cached_blocks` does not detect `tx` as interesting to the
-    // wallet. If a transaction is in the database with a null `mined_height`,
-    // as in this case, its `mined_height` will remain null unless either
-    // `put_tx_meta` or `set_transaction_status` is called on it. The former
-    // is normally called internally via `put_blocks` as a result of scanning,
-    // but not for the case of a fully transparent transaction. The latter is
-    // called by the wallet implementation in response to processing the
-    // `transaction_data_requests` queue.
-
-    // The reservation should fail because `tx` is not yet seen as mined.
-    reservation_should_fail(&mut st, 1, 22);
-
-    // Simulate the wallet processing the `transaction_data_requests` queue.
-    let tx_data_requests = st.wallet().transaction_data_requests().unwrap();
-    assert!(tx_data_requests.contains(&TransactionDataRequest::GetStatus(tx.txid())));
-
-    // Respond to the GetStatus request.
-    st.wallet_mut()
-        .set_transaction_status(tx.txid(), TransactionStatus::Mined(h))
-        .unwrap();
-
-    // We already reserved 22 addresses, so mining the transaction with the
-    // ephemeral output at index 10 should allow 9 more (..31).
-    reservation_should_succeed(&mut st, 9);
-    reservation_should_fail(&mut st, 1, 31);
-
-    let newest_known_addrs = st
-        .wallet()
-        .get_known_ephemeral_addresses(account_id, None)
-        .unwrap();
-    assert_eq!(newest_known_addrs.len(), (GAP_LIMIT as usize) + 31);
-    assert!(newest_known_addrs.starts_with(&known_addrs));
-    assert!(newest_known_addrs[5..].starts_with(&newer_known_addrs));


I'm a bit concerned about loss of coverage here. Granted that the old test was depending on more implementation behaviour than it really needed to, but it was also testing a lot of subtle details of the gap handling needed for correctness, iirc.

We should look at this together; it was my conclusion that some of what was being tested here was not just implementation-dependent, but it was verifying behavior that I considered to be a bug.

daira · 2025-01-29T19:49:49Z

zcash_client_sqlite/src/error.rs

+                "The proposal cannot be constructed until transactions with outputs to previously reserved ephemeral addresses have been mined. \
+                 The ephemeral address at index {bad_index} could not be safely reserved.",


Suggested change

"The proposal cannot be constructed until transactions with outputs to previously reserved ephemeral addresses have been mined. \

The ephemeral address at index {bad_index} could not be safely reserved.",

"The proposal cannot be constructed until transactions with outputs to previously reserved transparent addresses have been mined. \

The transparent address at index {bad_index} could not be safely reserved.",

I think this now does not only occur for ephemeral addresses. Ideally we would have the KeyScope as a parameter?

Also, why is it that we now don't know the account?

The caller provided the account index for proposal construction, so we don't need to return it (and we don't have the account UUID in the place that the error is generated, and it would be a hassle to look it up.)

daira · 2025-01-29T19:50:36Z

zcash_client_sqlite/src/error.rs

-            #[cfg(feature = "transparent-inputs")]
-            SqliteClientError::EphemeralAddressReuse(address_str, txid) => write!(f, "The ephemeral address {address_str} previously used in txid {txid} would be reused."),
+            SqliteClientError::AddressReuse(address_str, txids) => {
+                write!(f, "The address {address_str} previously used in txid(s) {:?} would be reused.", txids)


Suggested change

write!(f, "The address {address_str} previously used in txid(s) {:?} would be reused.", txids)

write!(f, "The transparent address {address_str} previously used in txid(s) {:?} would be reused.", txids)

daira · 2025-01-29T19:58:50Z

zcash_client_backend/src/sync.rs

@@ -135,7 +135,7 @@ where
            "Refreshing UTXOs for {:?} from height {}",
            account_id, start_height,
        );
-        refresh_utxos(params, client, db_data, account_id, start_height).await?;
+        refresh_utxos(params, client, db_data, account_id, start_height, false).await?;


Expand the comment to explain why it is correct to exclude ephemeral addresses here (if it is!)

This sync code is in the process of being refactored by @str4d for inclusion in zallet; I made the minimal change to preserve the existing behavior here. This call will actually happen in a separate process that handles the retrieval queue (and will likely not even be a singular call for an account) so the goal here is minimizing disruption.

daira · 2025-01-29T20:06:24Z

zcash_client_sqlite/src/error.rs

-    #[cfg(feature = "transparent-inputs")]
-    EphemeralAddressReuse(String, TxId),
+    /// The wallet attempted to create a transaction that would use of one of the wallet's
+    /// previously-used addresses, potentially creating a problem with on-chain transaction


Suggested change

/// previously-used addresses, potentially creating a problem with on-chain transaction

/// previously-used transparent addresses, potentially creating a problem with on-chain transaction

Also consider renaming this to TransparentAddressReuse.

I specifically generalized this to AddressReuse in order to make it possible to use this for unified addresses that contain a transparent receiver.

daira · 2025-01-29T20:10:11Z

zcash_client_sqlite/src/lib.rs

+pub struct GapLimits {
+    external: u32,
+    transparent_internal: u32,
+    ephemeral: u32,


As previously commented, I think that compatibility requires we keep the ephemeral gap limit at 20. I'm also concerned about the risk of inadvertently creating similar compatibility problems for external and transparent_internal addresses, since it is incorrect to reduce any of these limits for a given wallet.

str4d

Flushing some comments from an old review.

str4d · 2025-01-06T16:16:48Z

zcash_transparent/src/keys.rs

+    /// Adds the given delta to this index, returning a maximum possible value of
+    /// [`NonHardenedChildIndex::MAX`].
+    pub const fn saturating_add(&self, delta: u32) -> Self {
+        let idx = self.0 + delta;


This can overflow ((1 << 31) - 1 + u32::MAX will wrap around).

Suggested change

let idx = self.0 + delta;

let idx = self.0.saturating_add(delta);

str4d · 2025-01-06T17:39:04Z

zcash_client_sqlite/src/error.rs

@@ -119,16 +118,16 @@ pub enum SqliteClientError {
    NoteFilterInvalid(NoteFilter),

    /// The proposal cannot be constructed until transactions with previously reserved
-    /// ephemeral address outputs have been mined. The parameters are the account UUID and
-    /// the index that could not safely be reserved.
+    /// ephemeral address outputs have been mined. The error contains the index that could not


I don't believe that this only triggers on ephemeral addresses anymore:

Suggested change

/// ephemeral address outputs have been mined. The error contains the index that could not

/// address outputs have been mined. The error contains the index that could not

str4d · 2025-01-06T17:39:28Z

zcash_client_sqlite/src/error.rs

+                "The proposal cannot be constructed until transactions with outputs to previously reserved ephemeral addresses have been mined. \
+                 The ephemeral address at index {bad_index} could not be safely reserved.",


Suggested change

"The proposal cannot be constructed until transactions with outputs to previously reserved ephemeral addresses have been mined. \

The ephemeral address at index {bad_index} could not be safely reserved.",

"The proposal cannot be constructed until transactions with outputs to previously reserved addresses have been mined. \

The address at index {bad_index} could not be safely reserved.",

nuttycom marked this pull request as draft December 23, 2024 20:43

nuttycom force-pushed the feature/transparent_gap_limit_handling branch 16 times, most recently from 62c1394 to bd2df86 Compare December 28, 2024 04:13

nuttycom changed the title ~~WIP: Add transparent address gap limit handling & general address rotation functionality.~~ Add transparent address gap limit handling & general address rotation functionality. Dec 28, 2024

nuttycom marked this pull request as ready for review December 28, 2024 04:14

nuttycom force-pushed the feature/transparent_gap_limit_handling branch from bd2df86 to 4b99663 Compare December 28, 2024 04:27

nuttycom requested a review from str4d December 28, 2024 04:27

nuttycom mentioned this pull request Dec 30, 2024

Preparatory refactoring for zcash/librustzcash#1673 #1675

Merged

nuttycom force-pushed the feature/transparent_gap_limit_handling branch 2 times, most recently from 1386dd1 to 49230de Compare December 30, 2024 22:20

str4d added a commit that referenced this pull request Dec 30, 2024

Merge pull request #1675 from nuttycom/feature/transparent_gap_limit_…

5651d80

…handling-prep Preparatory refactoring for #1673

nuttycom force-pushed the feature/transparent_gap_limit_handling branch 2 times, most recently from b39c6c3 to 3ac0396 Compare December 30, 2024 22:33