Skip to content

Commit

Permalink
Merge pull request #1243 from nuttycom/allow_missing_receiving_key_scope
Browse files Browse the repository at this point in the history 
zcash_client_sqlite: Only consider notes spendable when both seed fingerprint & key scope are available.
  • Loading branch information
@str4d
str4d authored Mar 9, 2024
2 parents df93c1c + 5511bac commit e834c30
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 61 deletions.
2 changes: 1 addition & 1 deletion zcash_client_sqlite/src/wallet/init.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ mod tests {
memo BLOB,
spent INTEGER,
commitment_tree_position INTEGER,
recipient_key_scope INTEGER NOT NULL DEFAULT 0,
recipient_key_scope INTEGER,
FOREIGN KEY (tx) REFERENCES transactions(id_tx),
FOREIGN KEY (account_id) REFERENCES accounts(id),
FOREIGN KEY (spent) REFERENCES transactions(id_tx),
Expand Down
2 changes: 1 addition & 1 deletion zcash_client_sqlite/src/wallet/init/migrations/full_account_ids.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
memo BLOB,
spent INTEGER,
commitment_tree_position INTEGER,
recipient_key_scope INTEGER NOT NULL DEFAULT 0,
recipient_key_scope INTEGER,
FOREIGN KEY (tx) REFERENCES transactions(id_tx),
FOREIGN KEY (account_id) REFERENCES accounts(id),
FOREIGN KEY (spent) REFERENCES transactions(id_tx),
Expand Down
132 changes: 73 additions & 59 deletions zcash_client_sqlite/src/wallet/sapling.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ use std::rc::Rc;
use sapling::{self, Diversifier, Nullifier, Rseed};
use zcash_client_backend::{
data_api::NullifierQuery,
keys::UnifiedFullViewingKey,
wallet::{Note, ReceivedNote, WalletSaplingOutput},
DecryptedOutput, ShieldedProtocol, TransferType,
};
use zcash_keys::keys::UnifiedFullViewingKey;
use zcash_primitives::transaction::{components::amount::NonNegativeAmount, TxId};
use zcash_protocol::{
consensus::{self, BlockHeight},
Expand Down Expand Up @@ -96,7 +96,7 @@ impl ReceivedSaplingOutput for DecryptedOutput<sapling::Note, AccountId> {
fn to_spendable_note<P: consensus::Parameters>(
params: &P,
row: &Row,
) -> Result<ReceivedNote<ReceivedNoteId, Note>, SqliteClientError> {
) -> Result<Option<ReceivedNote<ReceivedNoteId, Note>>, SqliteClientError> {
let note_id = ReceivedNoteId(ShieldedProtocol::Sapling, row.get(0)?);
let txid = row.get::<_, [u8; 32]>(1).map(TxId::from_bytes)?;
let output_index = row.get(2)?;
Expand Down Expand Up @@ -136,37 +136,50 @@ fn to_spendable_note<P: consensus::Parameters>(
SqliteClientError::CorruptedData("Note commitment tree position invalid.".to_string())
})?);

let ufvk_str: String = row.get(7)?;
let ufvk = UnifiedFullViewingKey::decode(params, &ufvk_str)
.map_err(SqliteClientError::CorruptedData)?;

let scope_code: i64 = row.get(8)?;
let spending_key_scope = parse_scope(scope_code).ok_or_else(|| {
SqliteClientError::CorruptedData(format!("Invalid key scope code {}", scope_code))
})?;

let recipient = match spending_key_scope {
Scope::Internal => ufvk
.sapling()
.and_then(|dfvk| dfvk.diversified_change_address(diversifier)),
Scope::External => ufvk
.sapling()
.and_then(|dfvk| dfvk.diversified_address(diversifier)),
}
.ok_or_else(|| SqliteClientError::CorruptedData("Diversifier invalid.".to_owned()))?;

Ok(ReceivedNote::from_parts(
note_id,
txid,
output_index,
Note::Sapling(sapling::Note::from_parts(
recipient,
sapling::value::NoteValue::from_raw(note_value),
rseed,
)),
spending_key_scope,
note_commitment_tree_position,
))
let ufvk_str: Option<String> = row.get(7)?;
let scope_code: Option<i64> = row.get(8)?;

// If we don't have information about the recipient key scope or the ufvk we can't determine
// which spending key to use. This may be because the received note was associated with an
// imported viewing key, so we treat such notes as not spendable. Although this method is
// presently only called using the results of queries where both the ufvk and
// recipient_key_scope columns are checked to be non-null, this is method is written
// defensively to account for the fact that both of these are nullable columns in case it
// is used elsewhere in the future.
ufvk_str
.zip(scope_code)
.map(|(ufvk_str, scope_code)| {
let ufvk = UnifiedFullViewingKey::decode(params, &ufvk_str)
.map_err(SqliteClientError::CorruptedData)?;

let spending_key_scope = parse_scope(scope_code).ok_or_else(|| {
SqliteClientError::CorruptedData(format!("Invalid key scope code {}", scope_code))
})?;

let recipient = match spending_key_scope {
Scope::Internal => ufvk
.sapling()
.and_then(|dfvk| dfvk.diversified_change_address(diversifier)),
Scope::External => ufvk
.sapling()
.and_then(|dfvk| dfvk.diversified_address(diversifier)),
}
.ok_or_else(|| SqliteClientError::CorruptedData("Diversifier invalid.".to_owned()))?;

Ok(ReceivedNote::from_parts(
note_id,
txid,
output_index,
Note::Sapling(sapling::Note::from_parts(
recipient,
sapling::value::NoteValue::from_raw(note_value),
rseed,
)),
spending_key_scope,
note_commitment_tree_position,
))
})
.transpose()
}

// The `clippy::let_and_return` lint is explicitly allowed here because a bug in Clippy
Expand All @@ -179,29 +192,32 @@ pub(crate) fn get_spendable_sapling_note<P: consensus::Parameters>(
txid: &TxId,
index: u32,
) -> Result<Option<ReceivedNote<ReceivedNoteId, Note>>, SqliteClientError> {
let mut stmt_select_note = conn.prepare_cached(
"SELECT sapling_received_notes.id, txid, output_index, diversifier, value, rcm, commitment_tree_position,
let result = conn.query_row_and_then(
"SELECT sapling_received_notes.id, txid, output_index,
diversifier, value, rcm, commitment_tree_position,
accounts.ufvk, recipient_key_scope
FROM sapling_received_notes
INNER JOIN accounts on accounts.id = sapling_received_notes.account_id
INNER JOIN transactions ON transactions.id_tx = sapling_received_notes.tx
WHERE txid = :txid AND accounts.ufvk IS NOT NULL
WHERE txid = :txid
AND accounts.ufvk IS NOT NULL
AND recipient_key_scope IS NOT NULL
AND output_index = :output_index
AND spent IS NULL",
)?;

let result = stmt_select_note
.query_and_then(
named_params![
":txid": txid.as_ref(),
":output_index": index,
],
|r| to_spendable_note(params, r),
)?
.next()
.transpose();

result
named_params![
":txid": txid.as_ref(),
":output_index": index,
],
|row| to_spendable_note(params, row),
);

// `OptionalExtension` doesn't work here because the error type of `Result` is already
// `SqliteClientError`
match result {
Ok(r) => Ok(r),
Err(SqliteClientError::DbError(rusqlite::Error::QueryReturnedNoRows)) => Ok(None),
Err(e) => Err(e),
}
}

/// Utility method for determining whether we have any spendable notes
Expand Down Expand Up @@ -276,7 +292,9 @@ pub(crate) fn select_spendable_sapling_notes<P: consensus::Parameters>(
INNER JOIN accounts on accounts.id = sapling_received_notes.account_id
INNER JOIN transactions
ON transactions.id_tx = sapling_received_notes.tx
WHERE sapling_received_notes.account_id = :account AND ufvk IS NOT NULL
WHERE sapling_received_notes.account_id = :account
AND ufvk IS NOT NULL
AND recipient_key_scope IS NOT NULL
AND commitment_tree_position IS NOT NULL
AND spent IS NULL
AND transactions.block <= :anchor_height
Expand Down Expand Up @@ -313,7 +331,9 @@ pub(crate) fn select_spendable_sapling_notes<P: consensus::Parameters>(
|r| to_spendable_note(params, r),
)?;

notes.collect::<Result<_, _>>()
notes
.filter_map(|r| r.transpose())
.collect::<Result<_, _>>()
}

/// Retrieves the set of nullifiers for "potentially spendable" Sapling notes that the
Expand Down Expand Up @@ -420,12 +440,6 @@ pub(crate) fn put_received_note<T: ReceivedSaplingOutput>(
let to = output.note().recipient();
let diversifier = to.diversifier();

// FIXME: recipient key scope will always be available until IVK import is supported.
// Remove this expectation after #1175 merges.
let scope = output
.recipient_key_scope()
.expect("Key import is not yet supported.");

let sql_args = named_params![
":tx": &tx_ref,
":output_index": i64::try_from(output.index()).expect("output indices are representable as i64"),
Expand All @@ -438,7 +452,7 @@ pub(crate) fn put_received_note<T: ReceivedSaplingOutput>(
":is_change": output.is_change(),
":spent": spent_in,
":commitment_tree_position": output.note_commitment_tree_position().map(u64::from),
":recipient_key_scope": scope_code(scope),
":recipient_key_scope": output.recipient_key_scope().map(scope_code)
];

stmt_upsert_received_note
Expand Down

0 comments on commit e834c30

Please sign in to comment.