Merge pull request #1011 from nuttycom/wallet/note_identifiers

Make `ReceivedSaplingNote` internals private, and use (TxId, output_index) for note identifiers.

zcash_client_backend/CHANGELOG.md

@@ -7,9 +7,21 @@ and this library adheres to Rust's notion of
 
 ## [Unreleased]
 
+### Added
+
+- Added methods to `zcash_client_backend::wallet::ReceivedSaplingNote`:
+  `{from_parts, txid, output_index, diversifier, rseed, note_commitment_tree_position}`.
+
 ### Changed
 - `zcash_client_backend::data_api::chain::scan_cached_blocks` now returns
   a `ScanSummary` containing metadata about the scanned blocks on success.
+- The fields of `zcash_client_backend::wallet::ReceivedSaplingNote` are now
+  private. Use `ReceivedSaplingNote::from_parts` for construction instead.
+  Accessor methods are provided for each previously-public field.
+- `zcash_client_backend::data_api` changes:
+  - The `NoteMismatch` variant of `data_api::error::Error` now wraps a
+    `data_api::NoteId` instead of a backend-specific note identifier. The
+    related `NoteRef` type parameter has been removed from `data_api::error::Error`.
 
 ## [0.10.0] - 2023-09-25
 

zcash_client_backend/src/data_api/error.rs

@@ -20,9 +20,11 @@ use crate::data_api::wallet::input_selection::InputSelectorError;
 #[cfg(feature = "transparent-inputs")]
 use zcash_primitives::{legacy::TransparentAddress, zip32::DiversifierIndex};
 
+use super::NoteId;
+
 /// Errors that can occur as a consequence of wallet operations.
 #[derive(Debug)]
-pub enum Error<DataSourceError, CommitmentTreeError, SelectionError, FeeError, NoteRef> {
+pub enum Error<DataSourceError, CommitmentTreeError, SelectionError, FeeError> {
     /// An error occurred retrieving data from the underlying data source
     DataSource(DataSourceError),
 
@@ -56,7 +58,7 @@ pub enum Error<DataSourceError, CommitmentTreeError, SelectionError, FeeError, N
 
     /// A note being spent does not correspond to either the internal or external
     /// full viewing key for an account.
-    NoteMismatch(NoteRef),
+    NoteMismatch(NoteId),
 
     #[cfg(feature = "transparent-inputs")]
     AddressNotRecognized(TransparentAddress),
@@ -65,13 +67,12 @@ pub enum Error<DataSourceError, CommitmentTreeError, SelectionError, FeeError, N
     ChildIndexOutOfRange(DiversifierIndex),
 }
 
-impl<DE, CE, SE, FE, N> fmt::Display for Error<DE, CE, SE, FE, N>
+impl<DE, CE, SE, FE> fmt::Display for Error<DE, CE, SE, FE>
 where
     DE: fmt::Display,
     CE: fmt::Display,
     SE: fmt::Display,
     FE: fmt::Display,
-    N: fmt::Display,
 {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match &self {
@@ -111,7 +112,7 @@ where
             Error::ScanRequired => write!(f, "Must scan blocks first"),
             Error::Builder(e) => write!(f, "An error occurred building the transaction: {}", e),
             Error::MemoForbidden => write!(f, "It is not possible to send a memo to a transparent address."),
-            Error::NoteMismatch(n) => write!(f, "A note being spent ({}) does not correspond to either the internal or external full viewing key for the provided spending key.", n),
+            Error::NoteMismatch(n) => write!(f, "A note being spent ({:?}) does not correspond to either the internal or external full viewing key for the provided spending key.", n),
 
             #[cfg(feature = "transparent-inputs")]
             Error::AddressNotRecognized(_) => {
@@ -129,13 +130,12 @@ where
     }
 }
 
-impl<DE, CE, SE, FE, N> error::Error for Error<DE, CE, SE, FE, N>
+impl<DE, CE, SE, FE> error::Error for Error<DE, CE, SE, FE>
 where
     DE: Debug + Display + error::Error + 'static,
     CE: Debug + Display + error::Error + 'static,
     SE: Debug + Display + error::Error + 'static,
     FE: Debug + Display + 'static,
-    N: Debug + Display,
 {
     fn source(&self) -> Option<&(dyn error::Error + 'static)> {
         match &self {
@@ -148,19 +148,19 @@ where
     }
 }
 
-impl<DE, CE, SE, FE, N> From<builder::Error<FE>> for Error<DE, CE, SE, FE, N> {
+impl<DE, CE, SE, FE> From<builder::Error<FE>> for Error<DE, CE, SE, FE> {
     fn from(e: builder::Error<FE>) -> Self {
         Error::Builder(e)
     }
 }
 
-impl<DE, CE, SE, FE, N> From<BalanceError> for Error<DE, CE, SE, FE, N> {
+impl<DE, CE, SE, FE> From<BalanceError> for Error<DE, CE, SE, FE> {
     fn from(e: BalanceError) -> Self {
         Error::BalanceError(e)
     }
 }
 
-impl<DE, CE, SE, FE, N> From<InputSelectorError<DE, SE>> for Error<DE, CE, SE, FE, N> {
+impl<DE, CE, SE, FE> From<InputSelectorError<DE, SE>> for Error<DE, CE, SE, FE> {
     fn from(e: InputSelectorError<DE, SE>) -> Self {
         match e {
             InputSelectorError::DataSource(e) => Error::DataSource(e),
@@ -177,19 +177,19 @@ impl<DE, CE, SE, FE, N> From<InputSelectorError<DE, SE>> for Error<DE, CE, SE, F
     }
 }
 
-impl<DE, CE, SE, FE, N> From<sapling::builder::Error> for Error<DE, CE, SE, FE, N> {
+impl<DE, CE, SE, FE> From<sapling::builder::Error> for Error<DE, CE, SE, FE> {
     fn from(e: sapling::builder::Error) -> Self {
         Error::Builder(builder::Error::SaplingBuild(e))
     }
 }
 
-impl<DE, CE, SE, FE, N> From<transparent::builder::Error> for Error<DE, CE, SE, FE, N> {
+impl<DE, CE, SE, FE> From<transparent::builder::Error> for Error<DE, CE, SE, FE> {
     fn from(e: transparent::builder::Error) -> Self {
         Error::Builder(builder::Error::TransparentBuild(e))
     }
 }
 
-impl<DE, CE, SE, FE, N> From<ShardTreeError<CE>> for Error<DE, CE, SE, FE, N> {
+impl<DE, CE, SE, FE> From<ShardTreeError<CE>> for Error<DE, CE, SE, FE> {
     fn from(e: ShardTreeError<CE>) -> Self {
         Error::CommitmentTree(e)
     }

zcash_client_backend/src/data_api/wallet.rs

@@ -37,7 +37,7 @@ use crate::{
 pub mod input_selection;
 use input_selection::{GreedyInputSelector, GreedyInputSelectorError, InputSelector};
 
-use super::ShieldedProtocol;
+use super::{NoteId, ShieldedProtocol};
 
 #[cfg(feature = "transparent-inputs")]
 use {
@@ -206,7 +206,6 @@ pub fn create_spend_to_address<DbT, ParamsT>(
         <DbT as WalletCommitmentTrees>::Error,
         GreedyInputSelectorError<BalanceError, DbT::NoteRef>,
         Infallible,
-        DbT::NoteRef,
     >,
 >
 where
@@ -309,7 +308,6 @@ pub fn spend<DbT, ParamsT, InputsT>(
         <DbT as WalletCommitmentTrees>::Error,
         InputsT::Error,
         <InputsT::FeeRule as FeeRule>::Error,
-        DbT::NoteRef,
     >,
 >
 where
@@ -358,13 +356,7 @@ pub fn propose_transfer<DbT, ParamsT, InputsT, CommitmentTreeErrT>(
     min_confirmations: NonZeroU32,
 ) -> Result<
     Proposal<InputsT::FeeRule, DbT::NoteRef>,
-    Error<
+    Error<DbT::Error, CommitmentTreeErrT, InputsT::Error, <InputsT::FeeRule as FeeRule>::Error>,
-        DbT::Error,
-        CommitmentTreeErrT,
-        InputsT::Error,
-        <InputsT::FeeRule as FeeRule>::Error,
-        DbT::NoteRef,
-    >,
 >
 where
     DbT: WalletWrite,
@@ -395,13 +387,7 @@ pub fn propose_shielding<DbT, ParamsT, InputsT, CommitmentTreeErrT>(
     min_confirmations: NonZeroU32,
 ) -> Result<
     Proposal<InputsT::FeeRule, DbT::NoteRef>,
-    Error<
+    Error<DbT::Error, CommitmentTreeErrT, InputsT::Error, <InputsT::FeeRule as FeeRule>::Error>,
-        DbT::Error,
-        CommitmentTreeErrT,
-        InputsT::Error,
-        <InputsT::FeeRule as FeeRule>::Error,
-        DbT::NoteRef,
-    >,
 >
 where
     ParamsT: consensus::Parameters,
@@ -443,7 +429,6 @@ pub fn create_proposed_transaction<DbT, ParamsT, InputsErrT, FeeRuleT>(
         <DbT as WalletCommitmentTrees>::Error,
         InputsErrT,
         FeeRuleT::Error,
-        DbT::NoteRef,
     >,
 >
 where
@@ -488,8 +473,7 @@ where
     let mut builder = Builder::new(params.clone(), proposal.min_target_height(), None);
 
     let checkpoint_depth = wallet_db.get_checkpoint_depth(min_confirmations)?;
-
+    wallet_db.with_sapling_tree_mut::<_, _, Error<_, _, _, _>>(|sapling_tree| {
-    wallet_db.with_sapling_tree_mut::<_, _, Error<_, _, _, _, _>>(|sapling_tree| {
         for selected in proposal.sapling_inputs() {
             let (note, key, merkle_path) = select_key_for_note(
                 sapling_tree,
@@ -498,9 +482,15 @@ where
                 &dfvk,
                 checkpoint_depth,
             )?
-            .ok_or(Error::NoteMismatch(selected.note_id))?;
+            .ok_or_else(|| {
+                Error::NoteMismatch(NoteId {
+                    txid: *selected.txid(),
+                    protocol: ShieldedProtocol::Sapling,
+                    output_index: selected.output_index(),
+                })
+            })?;
 
-            builder.add_sapling_spend(key, selected.diversifier, note, merkle_path)?;
+            builder.add_sapling_spend(key, selected.diversifier(), note, merkle_path)?;
         }
         Ok(())
     })?;
@@ -725,7 +715,6 @@ pub fn shield_transparent_funds<DbT, ParamsT, InputsT>(
         <DbT as WalletCommitmentTrees>::Error,
         InputsT::Error,
         <InputsT::FeeRule as FeeRule>::Error,
-        DbT::NoteRef,
     >,
 >
 where
@@ -774,15 +763,15 @@ fn select_key_for_note<N, S: ShardStore<H = Node, CheckpointId = BlockHeight>>(
     // corresponding to the unified spending key, checking against the witness we are using
     // to spend the note that we've used the correct key.
     let external_note = dfvk
-        .diversified_address(selected.diversifier)
+        .diversified_address(selected.diversifier())
-        .map(|addr| addr.create_note(selected.note_value.into(), selected.rseed));
+        .map(|addr| addr.create_note(selected.value().into(), selected.rseed()));
     let internal_note = dfvk
-        .diversified_change_address(selected.diversifier)
+        .diversified_change_address(selected.diversifier())
-        .map(|addr| addr.create_note(selected.note_value.into(), selected.rseed));
+        .map(|addr| addr.create_note(selected.value().into(), selected.rseed()));
 
     let expected_root = commitment_tree.root_at_checkpoint(checkpoint_depth)?;
     let merkle_path = commitment_tree
-        .witness_caching(selected.note_commitment_tree_position, checkpoint_depth)?;
+        .witness_caching(selected.note_commitment_tree_position(), checkpoint_depth)?;
 
     Ok(external_note
         .filter(|n| expected_root == merkle_path.root(Node::from_cmu(&n.cmu())))

zcash_client_backend/src/data_api/wallet/input_selection.rs

@@ -419,7 +419,7 @@ where
 
             let new_available = sapling_inputs
                 .iter()
-                .map(|n| n.note_value)
+                .map(|n| n.value())
                 .sum::<Option<Amount>>()
                 .ok_or(BalanceError::Overflow)?;
 

zcash_client_backend/src/wallet.rs

@@ -177,11 +177,58 @@ impl<N> WalletSaplingOutput<N> {
 /// with sufficient information for use in note selection.
 #[derive(Debug)]
 pub struct ReceivedSaplingNote<NoteRef> {
-    pub note_id: NoteRef,
+    note_id: NoteRef,
-    pub diversifier: sapling::Diversifier,
+    txid: TxId,
-    pub note_value: Amount,
+    output_index: u16,
-    pub rseed: sapling::Rseed,
+    diversifier: sapling::Diversifier,
-    pub note_commitment_tree_position: Position,
+    note_value: Amount,
+    rseed: sapling::Rseed,
+    note_commitment_tree_position: Position,
+}
+
+impl<NoteRef> ReceivedSaplingNote<NoteRef> {
+    pub fn from_parts(
+        note_id: NoteRef,
+        txid: TxId,
+        output_index: u16,
+        diversifier: sapling::Diversifier,
+        note_value: Amount,
+        rseed: sapling::Rseed,
+        note_commitment_tree_position: Position,
+    ) -> Self {
+        ReceivedSaplingNote {
+            note_id,
+            txid,
+            output_index,
+            diversifier,
+            note_value,
+            rseed,
+            note_commitment_tree_position,
+        }
+    }
+
+    pub fn internal_note_id(&self) -> &NoteRef {
+        &self.note_id
+    }
+
+    pub fn txid(&self) -> &TxId {
+        &self.txid
+    }
+    pub fn output_index(&self) -> u16 {
+        self.output_index
+    }
+    pub fn diversifier(&self) -> sapling::Diversifier {
+        self.diversifier
+    }
+    pub fn value(&self) -> Amount {
+        self.note_value
+    }
+    pub fn rseed(&self) -> sapling::Rseed {
+        self.rseed
+    }
+    pub fn note_commitment_tree_position(&self) -> Position {
+        self.note_commitment_tree_position
+    }
 }
 
 impl<NoteRef> sapling_fees::InputView<NoteRef> for ReceivedSaplingNote<NoteRef> {

zcash_client_sqlite/src/testing.rs

@@ -445,7 +445,6 @@ impl<Cache> TestState<Cache> {
             commitment_tree::Error,
             GreedyInputSelectorError<BalanceError, ReceivedNoteId>,
             Infallible,
-            ReceivedNoteId,
         >,
     > {
         let params = self.network();
@@ -478,7 +477,6 @@ impl<Cache> TestState<Cache> {
             commitment_tree::Error,
             InputsT::Error,
             <InputsT::FeeRule as FeeRule>::Error,
-            ReceivedNoteId,
         >,
     >
     where
@@ -512,7 +510,6 @@ impl<Cache> TestState<Cache> {
             Infallible,
             InputsT::Error,
             <InputsT::FeeRule as FeeRule>::Error,
-            ReceivedNoteId,
         >,
     >
     where
@@ -546,7 +543,6 @@ impl<Cache> TestState<Cache> {
             Infallible,
             InputsT::Error,
             <InputsT::FeeRule as FeeRule>::Error,
-            ReceivedNoteId,
         >,
     >
     where
@@ -578,7 +574,6 @@ impl<Cache> TestState<Cache> {
             commitment_tree::Error,
             Infallible,
             FeeRuleT::Error,
-            ReceivedNoteId,
         >,
     >
     where
@@ -615,7 +610,6 @@ impl<Cache> TestState<Cache> {
             commitment_tree::Error,
             InputsT::Error,
             <InputsT::FeeRule as FeeRule>::Error,
-            ReceivedNoteId,
         >,
     >
     where

zcash_client_sqlite/src/wallet/sapling.rs

@@ -9,7 +9,7 @@ use zcash_primitives::{
     consensus::BlockHeight,
     memo::MemoBytes,
     sapling::{self, Diversifier, Note, Nullifier, Rseed},
-    transaction::components::Amount,
+    transaction::{components::Amount, TxId},
     zip32::AccountId,
 };
 
@@ -83,8 +83,10 @@ impl ReceivedSaplingOutput for DecryptedOutput<Note> {
 
 fn to_spendable_note(row: &Row) -> Result<ReceivedSaplingNote<ReceivedNoteId>, SqliteClientError> {
     let note_id = ReceivedNoteId(row.get(0)?);
+    let txid = row.get::<_, [u8; 32]>(1).map(TxId::from_bytes)?;
+    let output_index = row.get(2)?;
     let diversifier = {
-        let d: Vec<_> = row.get(1)?;
+        let d: Vec<_> = row.get(3)?;
         if d.len() != 11 {
             return Err(SqliteClientError::CorruptedData(
                 "Invalid diversifier length".to_string(),
@@ -95,10 +97,10 @@ fn to_spendable_note(row: &Row) -> Result<ReceivedSaplingNote<ReceivedNoteId>, S
         Diversifier(tmp)
     };
 
-    let note_value = Amount::from_i64(row.get(2)?).unwrap();
+    let note_value = Amount::from_i64(row.get(4)?).unwrap();
 
     let rseed = {
-        let rcm_bytes: Vec<_> = row.get(3)?;
+        let rcm_bytes: Vec<_> = row.get(5)?;
 
         // We store rcm directly in the data DB, regardless of whether the note
         // used a v1 or v2 note plaintext, so for the purposes of spending let's
@@ -113,17 +115,19 @@ fn to_spendable_note(row: &Row) -> Result<ReceivedSaplingNote<ReceivedNoteId>, S
     };
 
     let note_commitment_tree_position =
-        Position::from(u64::try_from(row.get::<_, i64>(4)?).map_err(|_| {
+        Position::from(u64::try_from(row.get::<_, i64>(6)?).map_err(|_| {
             SqliteClientError::CorruptedData("Note commitment tree position invalid.".to_string())
         })?);
 
-    Ok(ReceivedSaplingNote {
+    Ok(ReceivedSaplingNote::from_parts(
         note_id,
+        txid,
+        output_index,
         diversifier,
         note_value,
         rseed,
         note_commitment_tree_position,
-    })
+    ))
 }
 
 /// Utility method for determining whether we have any spendable notes
@@ -170,7 +174,7 @@ pub(crate) fn get_spendable_sapling_notes(
     }
 
     let mut stmt_select_notes = conn.prepare_cached(
-        "SELECT id_note, diversifier, value, rcm, commitment_tree_position
+        "SELECT id_note, txid, output_index, diversifier, value, rcm, commitment_tree_position
          FROM sapling_received_notes
          INNER JOIN transactions ON transactions.id_tx = sapling_received_notes.tx
          WHERE account = :account
@@ -244,7 +248,7 @@ pub(crate) fn select_spendable_sapling_notes(
     // 4) Match the selected notes against the witnesses at the desired height.
     let mut stmt_select_notes = conn.prepare_cached(
         "WITH eligible AS (
-             SELECT id_note, diversifier, value, rcm, commitment_tree_position,
+             SELECT id_note, txid, output_index, diversifier, value, rcm, commitment_tree_position,
                  SUM(value)
                     OVER (PARTITION BY account, spent ORDER BY id_note) AS so_far
              FROM sapling_received_notes
@@ -266,10 +270,10 @@ pub(crate) fn select_spendable_sapling_notes(
                 AND unscanned.block_range_end > :wallet_birthday
              )
          )
-         SELECT id_note, diversifier, value, rcm, commitment_tree_position
+         SELECT id_note, txid, output_index, diversifier, value, rcm, commitment_tree_position
          FROM eligible WHERE so_far < :target_value
          UNION
-         SELECT id_note, diversifier, value, rcm, commitment_tree_position
+         SELECT id_note, txid, output_index, diversifier, value, rcm, commitment_tree_position
          FROM (SELECT * from eligible WHERE so_far >= :target_value LIMIT 1)",
     )?;
 
@@ -982,7 +986,6 @@ pub(crate) mod tests {
                 commitment_tree::Error,
                 GreedyInputSelectorError<BalanceError, ReceivedNoteId>,
                 Infallible,
-                ReceivedNoteId,
             >,
         > {
             let txid = st.create_spend_to_address(

zcash_primitives/src/transaction/mod.rs

@@ -92,6 +92,12 @@ impl AsRef<[u8; 32]> for TxId {
     }
 }
 
+impl From<TxId> for [u8; 32] {
+    fn from(value: TxId) -> Self {
+        value.0
+    }
+}
+
 impl TxId {
     pub fn from_bytes(bytes: [u8; 32]) -> Self {
         TxId(bytes)