From 56aa348a41f8eff4427e89d9e1826f0781911949 Mon Sep 17 00:00:00 2001 From: Daira-Emma Hopwood Date: Thu, 4 Jul 2024 23:00:15 +0100 Subject: [PATCH] Extend the `send_multi_step_proposed_transfer` test to check the behaviour when another wallet creates a transaction with an output to one of our ephemeral addresses, and repair the implementation to pass this test. Signed-off-by: Daira-Emma Hopwood --- zcash_client_sqlite/src/lib.rs | 53 ++--- zcash_client_sqlite/src/testing/pool.rs | 216 +++++++++++++++++- zcash_client_sqlite/src/wallet/db.rs | 29 +-- .../init/migrations/ephemeral_addresses.rs | 9 +- .../src/wallet/transparent/ephemeral.rs | 75 +++--- 5 files changed, 293 insertions(+), 89 deletions(-) diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index d6ad1acca..028124f2c 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -1099,6 +1099,8 @@ impl WalletWrite for WalletDb self.transactionally(|wdb| { let tx_ref = wallet::put_tx_data(wdb.conn.0, d_tx.tx(), None, None)?; let funding_accounts = wallet::get_funding_accounts(wdb.conn.0, d_tx.tx())?; + + // TODO(#1305): Correctly track accounts that fund each transaction output. let funding_account = funding_accounts.iter().next().copied(); if funding_accounts.len() > 1 { warn!( @@ -1287,38 +1289,27 @@ impl WalletWrite for WalletDb wallet::transparent::mark_transparent_utxo_spent(wdb.conn.0, tx_ref, &txin.prevout)?; } - // If we have some transparent outputs: - if d_tx - .tx() - .transparent_bundle() - .iter() - .any(|b| !b.vout.is_empty()) - { - // If the transaction contains spends from our wallet, we will store z->t - // transactions we observe in the same way they would be stored by - // create_spend_to_address. - let funding_accounts = wallet::get_funding_accounts(wdb.conn.0, d_tx.tx())?; - let funding_account = funding_accounts.iter().next().copied(); - if let Some(account_id) = funding_account { - if funding_accounts.len() > 1 { - warn!( - "More than one wallet account detected as funding transaction {:?}, selecting {:?}", - d_tx.tx().txid(), - account_id - ) - } - - for (output_index, txout) in d_tx - .tx() - .transparent_bundle() - .iter() - .flat_map(|b| b.vout.iter()) - .enumerate() - { - if let Some(address) = txout.recipient_address() { - #[cfg(feature = "transparent-inputs")] - wallet::transparent::ephemeral::mark_ephemeral_address_as_mined(wdb, &address, tx_ref)?; + // This `if` is just an optimization for cases where we would do nothing in the loop. + if funding_account.is_some() || cfg!(feature = "transparent-inputs") { + for (output_index, txout) in d_tx + .tx() + .transparent_bundle() + .iter() + .flat_map(|b| b.vout.iter()) + .enumerate() + { + if let Some(address) = txout.recipient_address() { + // The transaction is not necessarily mined yet, but we want to record + // that an output to the address was seen in this tx anyway. This will + // advance the gap regardless of whether it is mined, but an output in + // an unmined transaction won't advance the range of safe indices. + #[cfg(feature = "transparent-inputs")] + wallet::transparent::ephemeral::mark_ephemeral_address_as_seen(wdb, &address, tx_ref)?; + // If a transaction we observe contains spends from our wallet, we will + // store its transparent outputs in the same way they would be stored by + // create_spend_to_address. + if let Some(account_id) = funding_account { let receiver = Receiver::Transparent(address); #[cfg(feature = "transparent-inputs")] diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index c48102f73..5e64a06eb 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -302,9 +302,19 @@ pub(crate) fn send_single_step_proposed_transfer() { #[cfg(feature = "transparent-inputs")] pub(crate) fn send_multi_step_proposed_transfer() { - use std::str::FromStr; + use std::{collections::HashSet, str::FromStr}; - use zcash_client_backend::fees::ChangeValue; + use rand_core::OsRng; + use zcash_client_backend::{ + fees::ChangeValue, + wallet::{TransparentAddressMetadata, WalletTx}, + }; + use zcash_primitives::{ + legacy::keys::{NonHardenedChildIndex, TransparentKeyScope}, + transaction::builder::{BuildConfig, Builder}, + }; + + use crate::wallet::{sapling::tests::test_prover, GAP_LIMIT}; let mut st = TestBuilder::new() .with_block_cache() @@ -312,6 +322,8 @@ pub(crate) fn send_multi_step_proposed_transfer() { .build(); let account = st.test_account().cloned().unwrap(); + let account_id = account.account_id(); + let (default_addr, default_index) = account.usk().default_transparent_address(); let dfvk = T::test_account_fvk(&st); let add_funds = |st: &mut TestState<_>, value| { @@ -325,7 +337,8 @@ pub(crate) fn send_multi_step_proposed_transfer() { .block_height(), h ); - assert_eq!(st.get_spendable_balance(account.account_id(), 1), value); + assert_eq!(st.get_spendable_balance(account_id, 1), value); + h }; let value = NonNegativeAmount::const_from_u64(100000); @@ -344,7 +357,7 @@ pub(crate) fn send_multi_step_proposed_transfer() { // Generate a ZIP 320 proposal, sending to the wallet's default transparent address // expressed as a TEX address. - let tex_addr = match account.usk().default_transparent_address().0 { + let tex_addr = match default_addr { TransparentAddress::PublicKeyHash(data) => Address::Tex(data), _ => unreachable!(), }; @@ -354,7 +367,7 @@ pub(crate) fn send_multi_step_proposed_transfer() { // serialization of the proposal. let proposal = st .propose_standard_transfer::( - account.account_id(), + account_id, StandardFeeRule::Zip317, NonZeroU32::new(1).unwrap(), &tex_addr, @@ -439,15 +452,15 @@ pub(crate) fn send_multi_step_proposed_transfer() { (sent_v, sent_to_addr, None, None) if sent_v == u64::try_from(transfer_amount).unwrap() && sent_to_addr == Some(tex_addr.encode(&st.wallet().params))); - (ephemeral_addr.unwrap(), txids.head) + (ephemeral_addr.unwrap(), txids) }; // Each transfer should use a different ephemeral address. - let (ephemeral0, _) = run_test(&mut st, 0); - let (ephemeral1, _) = run_test(&mut st, 1); + let (ephemeral0, txids0) = run_test(&mut st, 0); + let (ephemeral1, txids1) = run_test(&mut st, 1); assert_ne!(ephemeral0, ephemeral1); - add_funds(&mut st, value); + let height = add_funds(&mut st, value); let ephemeral_taddr = Address::decode(&st.wallet().params, &ephemeral0).expect("valid address"); assert_matches!( @@ -458,7 +471,7 @@ pub(crate) fn send_multi_step_proposed_transfer() { // Attempting to pay to an ephemeral address should cause an error. let proposal = st .propose_standard_transfer::( - account.account_id(), + account_id, StandardFeeRule::Zip317, NonZeroU32::new(1).unwrap(), &ephemeral_taddr, @@ -477,6 +490,184 @@ pub(crate) fn send_multi_step_proposed_transfer() { assert_matches!( &create_proposed_result, Err(Error::PaysEphemeralTransparentAddress(address_str)) if address_str == &ephemeral0); + + // Simulate another wallet sending to an ephemeral address with an index + // within the current gap limit. The `PaysEphemeralTransparentAddress` error + // prevents us from doing so straightforwardly, so we'll do it by building + // a transaction and calling `store_decrypted_tx` with it. + let known_addrs = st + .wallet() + .get_known_ephemeral_addresses(account_id, None) + .unwrap(); + assert_eq!(known_addrs.len(), (GAP_LIMIT as usize) + 2); + + // Check that the addresses are all distinct. + let known_set: HashSet<_> = known_addrs.iter().map(|(addr, _)| addr).collect(); + assert_eq!(known_set.len(), known_addrs.len()); + // Check that the metadata is as expected. + for (i, (_, meta)) in known_addrs.iter().enumerate() { + assert_eq!( + meta, + &TransparentAddressMetadata::new( + TransparentKeyScope::EPHEMERAL, + NonHardenedChildIndex::from_index(i.try_into().unwrap()).unwrap() + ) + ); + } + + let mut builder = Builder::new( + st.wallet().params, + height + 1, + BuildConfig::Standard { + sapling_anchor: None, + orchard_anchor: None, + }, + ); + let (colliding_addr, _) = &known_addrs[10]; + assert_matches!( + builder.add_transparent_output(colliding_addr, (value - zip317::MINIMUM_FEE).unwrap()), + Ok(_) + ); + let sk = account + .usk() + .transparent() + .derive_secret_key(Scope::External.into(), default_index) + .unwrap(); + let outpoint = OutPoint::fake(); + let txout = TxOut { + script_pubkey: default_addr.script(), + value, + }; + assert_matches!(builder.add_transparent_input(sk, outpoint, txout), Ok(_)); + let test_prover = test_prover(); + let build_result = builder + .build( + OsRng, + &test_prover, + &test_prover, + &zip317::FeeRule::standard(), + ) + .unwrap(); + let decrypted_tx = DecryptedTransaction::::new( + build_result.transaction(), + vec![], + #[cfg(feature = "orchard")] + vec![], + ); + st.wallet_mut().store_decrypted_tx(decrypted_tx).unwrap(); + + // That should have advanced the start of the gap to index 11. + let new_known_addrs = st + .wallet() + .get_known_ephemeral_addresses(account_id, None) + .unwrap(); + assert_eq!(new_known_addrs.len(), (GAP_LIMIT as usize) + 11); + assert!(new_known_addrs.starts_with(&known_addrs)); + + let reservation_should_succeed = |st: &mut TestState<_>, n| { + let reserved = st + .wallet_mut() + .reserve_next_n_ephemeral_addresses(account_id, n) + .unwrap(); + assert_eq!(reserved.len(), n); + reserved + }; + let reservation_should_fail = |st: &mut TestState<_>, n, expected_bad_index| { + assert_matches!(st + .wallet_mut() + .reserve_next_n_ephemeral_addresses(account_id, n), + Err(SqliteClientError::ReachedGapLimit(acct, bad_index)) + if acct == account_id && bad_index == expected_bad_index); + }; + + let next_reserved = reservation_should_succeed(&mut st, 1); + 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(5..100)) + .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 rest of this test would currently fail without the explicit call to + // `put_tx_meta` below. Ideally the above `scan_cached_blocks` would be + // sufficient, but it does not detect the transaction 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 `put_tx_meta` + // is called on it. Normally `put_tx_meta` would be called via `put_blocks` + // as a result of scanning, but that won't happen for any fully transparent + // transaction, and currently it also will not happen for a partially shielded + // transaction unless it is interesting to the wallet for another reason. + // Therefore we will not currently detect either collisions with uses of + // ephemeral outputs by other wallets, or refunds of funds sent to TEX + // addresses. (#1354, #1379) + + // Check that what we say in the above paragraph remains true, so that we + // don't accidentally fix it without updating this test. + reservation_should_fail(&mut st, 1, 22); + + // For now, we demonstrate that this problem is the only obstacle to the rest + // of the ZIP 320 code doing the right thing, by manually calling `put_tx_meta`: + crate::wallet::put_tx_meta( + &st.wallet_mut().conn, + &WalletTx::new( + tx.txid(), + tx_index, + vec![], + vec![], + #[cfg(feature = "orchard")] + vec![], + #[cfg(feature = "orchard")] + vec![], + ), + 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)); } #[cfg(feature = "transparent-inputs")] @@ -490,6 +681,7 @@ pub(crate) fn proposal_fails_if_not_all_ephemeral_outputs_consumed, value| { @@ -503,7 +695,7 @@ pub(crate) fn proposal_fails_if_not_all_ephemeral_outputs_consumed( - account.account_id(), + account_id, StandardFeeRule::Zip317, NonZeroU32::new(1).unwrap(), &tex_addr, diff --git a/zcash_client_sqlite/src/wallet/db.rs b/zcash_client_sqlite/src/wallet/db.rs index f4e9203c6..64b45acc0 100644 --- a/zcash_client_sqlite/src/wallet/db.rs +++ b/zcash_client_sqlite/src/wallet/db.rs @@ -94,38 +94,41 @@ CREATE INDEX "addresses_accounts" ON "addresses" ( /// - `address` contains the string (Base58Check) encoding of a transparent P2PKH address. /// - `used_in_tx` indicates that the address has been used by this wallet in a transaction (which /// has not necessarily been mined yet). This should only be set once, when the txid is known. -/// - `mined_in_tx` is non-null iff the address has been observed in a mined transaction (which may -/// have been sent by this wallet or another one using the same seed, or by a TEX address recipient -/// sending back the funds). This is used to advance the "gap limit", as well as to heuristically -/// reduce the chance of address reuse collisions with another wallet using the same seed. -/// -/// Note that the fact that `used_in_tx` and `mined_in_tx` reference specific transactions is primarily -/// a debugging aid (although the latter allows us to account for whether the referenced transaction -/// is unmined). We only really care which addresses have been used, and whether we can allocate a -/// new address within the gap limit. +/// - `seen_in_tx` is non-null iff an output to the address has been seed in a transaction observed +/// on the network and passed to `store_decrypted_tx`. The transaction may have been sent by this +// wallet or another one using the same seed, or by a TEX address recipient sending back the +/// funds. This is used to advance the "gap", as well as to heuristically reduce the chance of +/// address reuse collisions with another wallet using the same seed. /// /// It is an external invariant that within each account: /// - the address indices are contiguous and start from 0; -/// - the last `GAP_LIMIT` addresses have `used_in_tx` and `mined_in_tx` both NULL. +/// - the last `GAP_LIMIT` addresses have `used_in_tx` and `seen_in_tx` both NULL. /// /// All but the last `GAP_LIMIT` addresses are defined to be "reserved" addresses. Since the next /// index to reserve is determined by dead reckoning from the last stored address, we use dummy /// entries after the maximum valid index in order to allow the last `GAP_LIMIT` addresses at the /// end of the index range to be used. +/// +/// Note that the fact that `used_in_tx` references a specific transaction is just a debugging aid. +/// The same is mostly true of `seen_in_tx`, but we also take into account whether the referenced +/// transaction is unmined in order to determine the last index that is safe to reserve. pub(super) const TABLE_EPHEMERAL_ADDRESSES: &str = r#" CREATE TABLE ephemeral_addresses ( account_id INTEGER NOT NULL, address_index INTEGER NOT NULL, address TEXT, used_in_tx INTEGER, - mined_in_tx INTEGER, + seen_in_tx INTEGER, FOREIGN KEY (account_id) REFERENCES accounts(id), FOREIGN KEY (used_in_tx) REFERENCES transactions(id_tx), - FOREIGN KEY (mined_in_tx) REFERENCES transactions(id_tx), + FOREIGN KEY (seen_in_tx) REFERENCES transactions(id_tx), PRIMARY KEY (account_id, address_index), + CONSTRAINT used_implies_seen CHECK ( + used_in_tx IS NULL OR seen_in_tx IS NOT NULL + ), CONSTRAINT index_range_and_address_nullity CHECK ( (address_index BETWEEN 0 AND 0x7FFFFFFF AND address IS NOT NULL) OR - (address_index BETWEEN 0x80000000 AND 0x7FFFFFFF + 20 AND address IS NULL AND used_in_tx IS NULL AND mined_in_tx IS NULL) + (address_index BETWEEN 0x80000000 AND 0x7FFFFFFF + 20 AND address IS NULL AND used_in_tx IS NULL AND seen_in_tx IS NULL) ) ) WITHOUT ROWID"#; // Hexadecimal integer literals were added in SQLite version 3.8.6 (2014-08-15). diff --git a/zcash_client_sqlite/src/wallet/init/migrations/ephemeral_addresses.rs b/zcash_client_sqlite/src/wallet/init/migrations/ephemeral_addresses.rs index 25f9c2628..7f68792b3 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/ephemeral_addresses.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/ephemeral_addresses.rs @@ -47,14 +47,17 @@ impl RusqliteMigration for Migration

{ address_index INTEGER NOT NULL, address TEXT, used_in_tx INTEGER, - mined_in_tx INTEGER, + seen_in_tx INTEGER, FOREIGN KEY (account_id) REFERENCES accounts(id), FOREIGN KEY (used_in_tx) REFERENCES transactions(id_tx), - FOREIGN KEY (mined_in_tx) REFERENCES transactions(id_tx), + FOREIGN KEY (seen_in_tx) REFERENCES transactions(id_tx), PRIMARY KEY (account_id, address_index), + CONSTRAINT used_implies_seen CHECK ( + used_in_tx IS NULL OR seen_in_tx IS NOT NULL + ), CONSTRAINT index_range_and_address_nullity CHECK ( (address_index BETWEEN 0 AND 0x7FFFFFFF AND address IS NOT NULL) OR - (address_index BETWEEN 0x80000000 AND 0x7FFFFFFF + 20 AND address IS NULL AND used_in_tx IS NULL AND mined_in_tx IS NULL) + (address_index BETWEEN 0x80000000 AND 0x7FFFFFFF + 20 AND address IS NULL AND used_in_tx IS NULL AND seen_in_tx IS NULL) ) ) WITHOUT ROWID; CREATE INDEX ephemeral_addresses_address ON ephemeral_addresses ( diff --git a/zcash_client_sqlite/src/wallet/transparent/ephemeral.rs b/zcash_client_sqlite/src/wallet/transparent/ephemeral.rs index 08b95f98f..a70016885 100644 --- a/zcash_client_sqlite/src/wallet/transparent/ephemeral.rs +++ b/zcash_client_sqlite/src/wallet/transparent/ephemeral.rs @@ -70,7 +70,7 @@ pub(crate) fn first_unsafe_index( account_id: AccountId, ) -> Result { // The inner join with `transactions` excludes addresses for which - // `mined_in_tx` is NULL. The query also excludes addresses observed + // `seen_in_tx` is NULL. The query also excludes addresses observed // to have been mined in a transaction that we currently see as unmined. // This is conservative in terms of avoiding violation of the gap // invariant: it can only cause us to get to the end of the gap sooner. @@ -80,7 +80,7 @@ pub(crate) fn first_unsafe_index( let first_unmined_index: u32 = match conn .query_row( "SELECT address_index FROM ephemeral_addresses - JOIN transactions t ON t.id_tx = mined_in_tx + JOIN transactions t ON t.id_tx = seen_in_tx WHERE account_id = :account_id AND t.mined_height IS NOT NULL ORDER BY address_index DESC LIMIT 1", @@ -164,12 +164,11 @@ pub(crate) fn get_known_ephemeral_addresses( Ok(result) } -/// If this is an ephemeral address in any account, return its account id. +/// If this is a known ephemeral address in any account, return its account id. pub(crate) fn find_account_for_ephemeral_address_str( conn: &rusqlite::Connection, address_str: &str, ) -> Result, SqliteClientError> { - // Search ephemeral addresses that have already been reserved. Ok(conn .query_row( "SELECT account_id FROM ephemeral_addresses WHERE address = :address", @@ -179,7 +178,7 @@ pub(crate) fn find_account_for_ephemeral_address_str( .optional()?) } -#[cfg(feature = "transparent-inputs")] +/// If this is a known ephemeral address in the given account, return its index. pub(crate) fn find_index_for_ephemeral_address_str( conn: &rusqlite::Connection, account_id: AccountId, @@ -259,7 +258,7 @@ pub(crate) fn init_account( /// /// # Panics /// -/// Panics if `next_to_reserve > (1 << 31)`. +/// Panics if the precondition `next_to_reserve <= (1 << 31)` does not hold. fn reserve_until( conn: &rusqlite::Transaction, params: &P, @@ -276,7 +275,7 @@ fn reserve_until( let ephemeral_ivk = get_ephemeral_ivk(conn, params, account_id)?; - // used_in_tx and mined_in_tx are initially NULL + // used_in_tx and seen_in_tx are initially NULL let mut stmt_insert_ephemeral_address = conn.prepare_cached( "INSERT INTO ephemeral_addresses (account_id, address_index, address) VALUES (:account_id, :address_index, :address)", @@ -314,18 +313,18 @@ fn ephemeral_address_reuse_check( // using a given seed, because such a wallet will not reuse an address that // it ever reserved. // - // `COALESCE(used_in_tx, mined_in_tx)` can only differ from `used_in_tx` + // `COALESCE(used_in_tx, seen_in_tx)` can only differ from `used_in_tx` // if the address was reserved, an error occurred in transaction creation - // before calling `mark_ephemeral_address_as_used`, and then we observed - // the address to have been used in a mined transaction (presumably by - // another wallet instance, or due to a bug) anyway. + // before calling `mark_ephemeral_address_as_used`, and then we saw the + // address in another transaction (presumably created by another wallet + // instance, or as a result of a bug) anyway. let res = wdb .conn .0 .query_row( "SELECT t.txid FROM ephemeral_addresses LEFT OUTER JOIN transactions t - ON t.id_tx = COALESCE(used_in_tx, mined_in_tx) + ON t.id_tx = COALESCE(used_in_tx, seen_in_tx) WHERE address = :address", named_params![":address": address_str], |row| row.get::<_, Option>>(0), @@ -362,10 +361,28 @@ pub(crate) fn mark_ephemeral_address_as_used( let address_str = ephemeral_address.encode(&wdb.params); ephemeral_address_reuse_check(wdb, &address_str)?; - wdb.conn.0.execute( - "UPDATE ephemeral_addresses SET used_in_tx = :used_in_tx WHERE address = :address", - named_params![":used_in_tx": &tx_ref, ":address": address_str], - )?; + // We update both `used_in_tx` and `seen_in_tx` here, because a used address has + // necessarily been seen in a transaction. We will not treat this as extending the + // range of addresses that are safe to reserve unless and until the transaction is + // observed as mined. + let update_result = wdb + .conn + .0 + .query_row( + "UPDATE ephemeral_addresses + SET used_in_tx = :tx_ref, seen_in_tx = :tx_ref + WHERE address = :address + RETURNING account_id, address_index", + named_params![":tx_ref": &tx_ref, ":address": address_str], + |row| Ok((AccountId(row.get::<_, u32>(0)?), row.get::<_, u32>(1)?)), + ) + .optional()?; + + // Maintain the invariant that the last `GAP_LIMIT` addresses are unused and unseen. + if let Some((account_id, address_index)) = update_result { + let next_to_reserve = address_index.checked_add(1).expect("ensured by constraint"); + reserve_until(wdb.conn.0, &wdb.params, account_id, next_to_reserve)?; + } Ok(()) } @@ -374,7 +391,7 @@ pub(crate) fn mark_ephemeral_address_as_used( /// /// `tx_ref` must be a valid transaction reference. This call has no effect if /// `address` is not one of our ephemeral addresses. -pub(crate) fn mark_ephemeral_address_as_mined( +pub(crate) fn mark_ephemeral_address_as_seen( wdb: &mut WalletDb, P>, address: &TransparentAddress, tx_ref: i64, @@ -382,7 +399,7 @@ pub(crate) fn mark_ephemeral_address_as_mined( let address_str = address.encode(&wdb.params); // Figure out which transaction was mined earlier: `tx_ref`, or any existing - // tx referenced by `mined_in_tx` for the given address. Prefer the existing + // tx referenced by `seen_in_tx` for the given address. Prefer the existing // reference in case of a tie or if both transactions are unmined. // This slightly reduces the chance of unnecessarily reaching the gap limit // too early in some corner cases (because the earlier transaction is less @@ -392,34 +409,32 @@ pub(crate) fn mark_ephemeral_address_as_mined( let earlier_ref = wdb.conn.0.query_row( "SELECT id_tx FROM transactions LEFT OUTER JOIN ephemeral_addresses e - ON id_tx = e.mined_in_tx + ON id_tx = e.seen_in_tx WHERE id_tx = :tx_ref OR e.address = :address ORDER BY mined_height ASC NULLS LAST, tx_index ASC NULLS LAST, - e.mined_in_tx ASC NULLS LAST + e.seen_in_tx ASC NULLS LAST LIMIT 1", named_params![":tx_ref": &tx_ref, ":address": address_str], |row| row.get::<_, i64>(0), )?; - let mined_ephemeral = wdb + let update_result = wdb .conn .0 .query_row( "UPDATE ephemeral_addresses - SET mined_in_tx = :mined_in_tx - WHERE address = :address - RETURNING (account_id, address_index)", - named_params![":mined_in_tx": &earlier_ref, ":address": address_str], + SET seen_in_tx = :seen_in_tx + WHERE address = :address + RETURNING account_id, address_index", + named_params![":seen_in_tx": &earlier_ref, ":address": address_str], |row| Ok((AccountId(row.get::<_, u32>(0)?), row.get::<_, u32>(1)?)), ) .optional()?; - // If this is a known ephemeral address for an account in this wallet, we might need - // to extend the indices stored for that account to maintain the invariant that the - // last `GAP_LIMIT` addresses are unused and unmined. - if let Some((account_id, address_index)) = mined_ephemeral { - let next_to_reserve = min(1 << 31, address_index.saturating_add(1)); + // Maintain the invariant that the last `GAP_LIMIT` addresses are unused and unseen. + if let Some((account_id, address_index)) = update_result { + let next_to_reserve = address_index.checked_add(1).expect("ensured by constraint"); reserve_until(wdb.conn.0, &wdb.params, account_id, next_to_reserve)?; } Ok(())