zec
/
librustzcash
mirror of https://github.com/zcash/librustzcash.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Merge pull request #886 from nuttycom/bug/memo_row_absent 

Fix a number of bugs related to the creation of change memos and storage of empty memos.
This commit is contained in:
str4d 2023-08-07 15:49:30 +01:00 committed by GitHub
parent eade2acab9 522f1e4da0
commit cdb904a28c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 448 additions and 51 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
zcash_client_backend/src/data_api.rs
View File

@ -451,12 +451,14 @@ impl SentTransactionOutput {
pub fn value(&self) -> Amount {
self.value
}
/// Returns the memo that was attached to the output, if any.
/// Returns the memo that was attached to the output, if any. This will only be `None`
/// for transparent outputs.
pub fn memo(&self) -> Option<&MemoBytes> {
self.memo.as_ref()
}
/// Returns t decrypted note, if the sent output belongs to this wallet
/// Returns the account to which change (or wallet-internal value in the case of a shielding
/// transaction) was sent, along with the change note.
pub fn sapling_change_to(&self) -> Option<&(AccountId, sapling::Note)> {
self.sapling_change_to.as_ref()
}

32
zcash_client_backend/src/data_api/wallet.rs
View File

@ -545,30 +545,29 @@ where
for payment in proposal.transaction_request().payments() {
match &payment.recipient_address {
RecipientAddress::Unified(ua) => {
let memo = payment
.memo
.as_ref()
.map_or_else(MemoBytes::empty, |m| m.clone());
builder.add_sapling_output(
external_ovk,
*ua.sapling().expect("TODO: Add Orchard support to builder"),
payment.amount,
payment.memo.clone().unwrap_or_else(MemoBytes::empty),
memo.clone(),
)?;
sapling_output_meta.push((
Recipient::Unified(ua.clone(), PoolType::Shielded(ShieldedProtocol::Sapling)),
payment.amount,
payment.memo.clone(),
Some(memo),
));
}
RecipientAddress::Shielded(addr) => {
builder.add_sapling_output(
external_ovk,
*addr,
payment.amount,
payment.memo.clone().unwrap_or_else(MemoBytes::empty),
)?;
sapling_output_meta.push((
Recipient::Sapling(*addr),
payment.amount,
payment.memo.clone(),
));
let memo = payment
.memo
.as_ref()
.map_or_else(MemoBytes::empty, |m| m.clone());
builder.add_sapling_output(external_ovk, *addr, payment.amount, memo.clone())?;
sapling_output_meta.push((Recipient::Sapling(*addr), payment.amount, Some(memo)));
}
RecipientAddress::Transparent(to) => {
if payment.memo.is_some() {
@ -584,11 +583,14 @@ where
for change_value in proposal.balance().proposed_change() {
match change_value {
ChangeValue::Sapling(amount) => {
let memo = change_memo
.as_ref()
.map_or_else(MemoBytes::empty, |m| m.clone());
builder.add_sapling_output(
internal_ovk(),
dfvk.change_address().1,
*amount,
MemoBytes::empty(),
memo.clone(),
)?;
sapling_output_meta.push((
Recipient::InternalAccount(
@ -596,7 +598,7 @@ where
PoolType::Shielded(ShieldedProtocol::Sapling),
),
*amount,
change_memo.clone(),
Some(memo),
))
}
}

2
zcash_client_sqlite/src/lib.rs
View File

@ -240,7 +240,7 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters> WalletRead for W
}
fn get_transaction(&self, id_tx: i64) -> Result<Transaction, Self::Error> {
wallet::get_transaction(self.conn.borrow(), &self.params, id_tx)
wallet::get_transaction(self.conn.borrow(), &self.params, id_tx).map(|(_, tx)| tx)
}
fn get_sapling_nullifiers(

21
zcash_client_sqlite/src/wallet.rs
View File

@ -129,8 +129,14 @@ pub(crate) fn pool_code(pool_type: PoolType) -> i64 {
}
pub(crate) fn memo_repr(memo: Option<&MemoBytes>) -> Option<&[u8]> {
memo.filter(|m| *m != &MemoBytes::empty())
.map(|m| m.as_slice())
memo.map(|m| {
if m == &MemoBytes::empty() {
// we store the empty memo as a single 0xf6 byte
&[0xf6]
} else {
m.as_slice()
}
})
}
pub(crate) fn get_max_account_id(
@ -493,11 +499,16 @@ pub(crate) fn get_received_memo(
}
/// Looks up a transaction by its internal database identifier.
///
/// Returns the decoded transaction, along with the block height that was used in its decoding.
/// This is either the block height at which the transaction was mined, or the expiry height if the
/// wallet created the transaction but the transaction has not yet been mined from the perspective
/// of the wallet.
pub(crate) fn get_transaction<P: Parameters>(
conn: &rusqlite::Connection,
params: &P,
id_tx: i64,
) -> Result<Transaction, SqliteClientError> {
) -> Result<(BlockHeight, Transaction), SqliteClientError> {
let (tx_bytes, block_height, expiry_height): (
Vec<_>,
Option<BlockHeight>,
@ -530,6 +541,7 @@ pub(crate) fn get_transaction<P: Parameters>(
block_height.or_else(|| expiry_height.filter(|h| h > &BlockHeight::from(0)))
{
Transaction::read(&tx_bytes[..], BranchId::for_height(params, height))
.map(|t| (height, t))
.map_err(SqliteClientError::from)
} else {
let tx_data = Transaction::read(&tx_bytes[..], BranchId::Sprout)
@ -549,6 +561,7 @@ pub(crate) fn get_transaction<P: Parameters>(
tx_data.orchard_bundle().cloned(),
)
.freeze()
.map(|t| (expiry_height, t))
.map_err(SqliteClientError::from)
} else {
Err(SqliteClientError::CorruptedData(
@ -1262,7 +1275,7 @@ pub(crate) fn insert_sent_output<P: consensus::Parameters>(
":to_address": &to_address,
":to_account": &to_account,
":value": &i64::from(output.value()),
":memo": output.memo().filter(|m| *m != &MemoBytes::empty()).map(|m| m.as_slice()),
":memo": memo_repr(output.memo())
];
stmt_insert_sent_output.execute(sql_args)?;

10
zcash_client_sqlite/src/wallet/init.rs
View File

@ -574,8 +574,9 @@ mod tests {
ELSE 1
END AS received_count,
CASE
WHEN sapling_received_notes.memo IS NULL THEN 0
ELSE 1
WHEN (sapling_received_notes.memo IS NULL OR sapling_received_notes.memo = X'F6')
THEN 0
ELSE 1
END AS memo_present
FROM sapling_received_notes
UNION
@ -606,8 +607,9 @@ mod tests {
COUNT(DISTINCT sent_notes.id_note) as sent_notes,
SUM(
CASE
WHEN sent_notes.memo IS NULL THEN 0
ELSE 1
WHEN (sent_notes.memo IS NULL OR sent_notes.memo = X'F6')
THEN 0
ELSE 1
END
) AS memo_count
FROM sent_notes

30
zcash_client_sqlite/src/wallet/init/migrations.rs
View File

@ -4,6 +4,7 @@ mod addresses_table;
mod initial_setup;
mod nullifier_map;
mod received_notes_nullable_nf;
mod sapling_memo_consistency;
mod sent_notes_to_internal;
mod shardtree_support;
mod ufvk_support;
@ -22,19 +23,19 @@ pub(super) fn all_migrations<P: consensus::Parameters + 'static>(
) -> Vec<Box<dyn RusqliteMigration<Error = WalletMigrationError>>> {
// initial_setup
// / \
// utxos_table ufvk_support ----------
// \ \ \
// \ addresses_table sent_notes_to_internal
// \ / /
// add_utxo_account /
// \ /
// add_transaction_views
// /
// v_transactions_net
// /
// received_notes_nullable_nf
// / \
// shardtree_support nullifier_map
// utxos_table ufvk_support ---------
// \ | \
// \ addresses_table sent_notes_to_internal
// \ / /
// add_utxo_account /
// \ /
// add_transaction_views
// |
// v_transactions_net
// |
// received_notes_nullable_nf
// / | \
// shardtree_support nullifier_map sapling_memo_consistency
vec![
Box::new(initial_setup::Migration {}),
Box::new(utxos_table::Migration {}),
@ -54,5 +55,8 @@ pub(super) fn all_migrations<P: consensus::Parameters + 'static>(
Box::new(received_notes_nullable_nf::Migration),
Box::new(shardtree_support::Migration),
Box::new(nullifier_map::Migration),
Box::new(sapling_memo_consistency::Migration {
params: params.clone(),
}),
]
}

209
zcash_client_sqlite/src/wallet/init/migrations/sapling_memo_consistency.rs Normal file
View File

@ -0,0 +1,209 @@
//! This migration reads the wallet's raw transaction data and updates the `sent_notes` table to
//! ensure that memo entries are consistent with the decrypted transaction's outputs. The empty
//! memo is now consistently represented as a single `0xf6` byte.
use std::collections::{BTreeMap, HashMap, HashSet};
use rusqlite::named_params;
use schemer_rusqlite::RusqliteMigration;
use uuid::Uuid;
use zcash_client_backend::{decrypt_transaction, keys::UnifiedFullViewingKey};
use zcash_primitives::{consensus, zip32::AccountId};
use crate::{
error::SqliteClientError,
wallet::{get_transaction, init::WalletMigrationError, memo_repr},
};
use super::received_notes_nullable_nf;
pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0x7029b904_6557_4aa1_9da5_6904b65d2ba5);
pub(super) struct Migration<P> {
pub(super) params: P,
}
impl<P> schemer::Migration for Migration<P> {
fn id(&self) -> Uuid {
MIGRATION_ID
}
fn dependencies(&self) -> HashSet<Uuid> {
[received_notes_nullable_nf::MIGRATION_ID]
.into_iter()
.collect()
}
fn description(&self) -> &'static str {
"This migration reads the wallet's raw transaction data and updates the `sent_notes` table to
ensure that memo entries are consistent with the decrypted transaction's outputs. The empty
memo is now consistently represented as a single `0xf6` byte."
}
}
impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
type Error = WalletMigrationError;
fn up(&self, transaction: &rusqlite::Transaction) -> Result<(), Self::Error> {
let mut stmt_raw_tx = transaction.prepare(
"SELECT DISTINCT sent_notes.tx, accounts.account, accounts.ufvk
FROM sent_notes
JOIN accounts ON sent_notes.from_account = accounts.account
JOIN transactions ON transactions.id_tx = sent_notes.tx
WHERE transactions.raw IS NOT NULL",
)?;
let mut rows = stmt_raw_tx.query([])?;
let mut tx_sent_notes: BTreeMap<i64, HashMap<AccountId, UnifiedFullViewingKey>> =
BTreeMap::new();
while let Some(row) = rows.next()? {
let id_tx: i64 = row.get(0)?;
let account: u32 = row.get(1)?;
let ufvk_str: String = row.get(2)?;
let ufvk = UnifiedFullViewingKey::decode(&self.params, &ufvk_str).map_err(|e| {
WalletMigrationError::CorruptedData(format!(
"Could not decode unified full viewing key for account {}: {:?}",
account, e
))
})?;
tx_sent_notes
.entry(id_tx)
.or_default()
.insert(AccountId::from(account), ufvk);
}
let mut stmt_update_sent_memo = transaction.prepare(
"UPDATE sent_notes
SET memo = :memo
WHERE tx = :id_tx
AND output_index = :output_index",
)?;
for (id_tx, ufvks) in tx_sent_notes {
let (block_height, tx) =
get_transaction(transaction, &self.params, id_tx).map_err(|err| match err {
SqliteClientError::CorruptedData(msg) => {
WalletMigrationError::CorruptedData(msg)
}
SqliteClientError::DbError(err) => WalletMigrationError::DbError(err),
other => WalletMigrationError::CorruptedData(format!(
"An error was encountered decoding transaction data: {:?}",
other
)),
})?;
let decrypted_outputs = decrypt_transaction(&self.params, block_height, &tx, &ufvks);
for d_out in decrypted_outputs {
stmt_update_sent_memo.execute(named_params![
":id_tx": id_tx,
":output_index": d_out.index,
":memo": memo_repr(Some(&d_out.memo))
])?;
}
}
// Update the `v_transactions` view to avoid counting the empty memo as a memo
transaction.execute_batch(
"DROP VIEW v_transactions;
CREATE VIEW v_transactions AS
WITH
notes AS (
SELECT sapling_received_notes.account AS account_id,
sapling_received_notes.tx AS id_tx,
2 AS pool,
sapling_received_notes.value AS value,
CASE
WHEN sapling_received_notes.is_change THEN 1
ELSE 0
END AS is_change,
CASE
WHEN sapling_received_notes.is_change THEN 0
ELSE 1
END AS received_count,
CASE
WHEN (sapling_received_notes.memo IS NULL OR sapling_received_notes.memo = X'F6')
THEN 0
ELSE 1
END AS memo_present
FROM sapling_received_notes
UNION
SELECT utxos.received_by_account AS account_id,
transactions.id_tx AS id_tx,
0 AS pool,
utxos.value_zat AS value,
0 AS is_change,
1 AS received_count,
0 AS memo_present
FROM utxos
JOIN transactions
ON transactions.txid = utxos.prevout_txid
UNION
SELECT sapling_received_notes.account AS account_id,
sapling_received_notes.spent AS id_tx,
2 AS pool,
-sapling_received_notes.value AS value,
0 AS is_change,
0 AS received_count,
0 AS memo_present
FROM sapling_received_notes
WHERE sapling_received_notes.spent IS NOT NULL
),
sent_note_counts AS (
SELECT sent_notes.from_account AS account_id,
sent_notes.tx AS id_tx,
COUNT(DISTINCT sent_notes.id_note) as sent_notes,
SUM(
CASE
WHEN (sent_notes.memo IS NULL OR sent_notes.memo = X'F6')
THEN 0
ELSE 1
END
) AS memo_count
FROM sent_notes
LEFT JOIN sapling_received_notes
ON (sent_notes.tx, sent_notes.output_pool, sent_notes.output_index) =
(sapling_received_notes.tx, 2, sapling_received_notes.output_index)
WHERE sapling_received_notes.is_change IS NULL
OR sapling_received_notes.is_change = 0
GROUP BY account_id, id_tx
),
blocks_max_height AS (
SELECT MAX(blocks.height) as max_height FROM blocks
)
SELECT notes.account_id AS account_id,
transactions.id_tx AS id_tx,
transactions.block AS mined_height,
transactions.tx_index AS tx_index,
transactions.txid AS txid,
transactions.expiry_height AS expiry_height,
transactions.raw AS raw,
SUM(notes.value) AS account_balance_delta,
transactions.fee AS fee_paid,
SUM(notes.is_change) > 0 AS has_change,
MAX(COALESCE(sent_note_counts.sent_notes, 0)) AS sent_note_count,
SUM(notes.received_count) AS received_note_count,
SUM(notes.memo_present) + MAX(COALESCE(sent_note_counts.memo_count, 0)) AS memo_count,
blocks.time AS block_time,
(
blocks.height IS NULL
AND transactions.expiry_height <= blocks_max_height.max_height
) AS expired_unmined
FROM transactions
JOIN notes ON notes.id_tx = transactions.id_tx
JOIN blocks_max_height
LEFT JOIN blocks ON blocks.height = transactions.block
LEFT JOIN sent_note_counts
ON sent_note_counts.account_id = notes.account_id
AND sent_note_counts.id_tx = notes.id_tx
GROUP BY notes.account_id, transactions.id_tx",
)?;
Ok(())
}
fn down(&self, _: &rusqlite::Transaction) -> Result<(), Self::Error> {
panic!("Reversing this migration is not supported.");
}
}

189
zcash_client_sqlite/src/wallet/sapling.rs
View File

@ -371,7 +371,7 @@ pub(crate) fn put_received_note<T: ReceivedSaplingOutput>(
#[cfg(test)]
#[allow(deprecated)]
pub(crate) mod tests {
use std::num::NonZeroU32;
use std::{convert::Infallible, num::NonZeroU32};
use rusqlite::Connection;
use secrecy::Secret;
@ -383,8 +383,13 @@ pub(crate) mod tests {
block::BlockHash,
consensus::{BlockHeight, BranchId},
legacy::TransparentAddress,
memo::Memo,
sapling::{note_encryption::try_sapling_output_recovery, prover::TxProver},
transaction::{components::Amount, fees::zip317::FeeRule as Zip317FeeRule, Transaction},
transaction::{
components::Amount,
fees::{fixed::FeeRule as FixedFeeRule, zip317::FeeRule as Zip317FeeRule},
Transaction,
},
zip32::{sapling::ExtendedSpendingKey, Scope},
};
@ -394,13 +399,17 @@ pub(crate) mod tests {
self,
chain::scan_cached_blocks,
error::Error,
wallet::{create_spend_to_address, input_selection::GreedyInputSelector, spend},
wallet::{
create_proposed_transaction, create_spend_to_address,
input_selection::GreedyInputSelector, propose_transfer, spend,
},
WalletRead, WalletWrite,
},
fees::{zip317, DustOutputPolicy},
decrypt_transaction,
fees::{fixed, zip317, DustOutputPolicy},
keys::UnifiedSpendingKey,
wallet::OvkPolicy,
zip321::{Payment, TransactionRequest},
zip321::{self, Payment, TransactionRequest},
};
use crate::{
@ -413,21 +422,17 @@ pub(crate) mod tests {
get_balance, get_balance_at,
init::{init_blocks_table, init_wallet_db},
},
AccountId, BlockDb, WalletDb,
AccountId, BlockDb, NoteId, WalletDb,
};
#[cfg(feature = "transparent-inputs")]
use {
zcash_client_backend::{
data_api::wallet::shield_transparent_funds, fees::fixed,
wallet::WalletTransparentOutput,
data_api::wallet::shield_transparent_funds, wallet::WalletTransparentOutput,
},
zcash_primitives::{
memo::MemoBytes,
transaction::{
components::{amount::NonNegativeAmount, OutPoint, TxOut},
fees::fixed::FeeRule as FixedFeeRule,
},
transaction::components::{amount::NonNegativeAmount, OutPoint, TxOut},
},
};
@ -440,6 +445,166 @@ pub(crate) mod tests {
}
}
#[test]
fn send_proposed_transfer() {
let cache_file = NamedTempFile::new().unwrap();
let db_cache = BlockDb(Connection::open(cache_file.path()).unwrap());
init_cache_database(&db_cache).unwrap();
let data_file = NamedTempFile::new().unwrap();
let mut db_data = WalletDb::for_path(data_file.path(), tests::network()).unwrap();
init_wallet_db(&mut db_data, None).unwrap();
// Add an account to the wallet
let seed = Secret::new([0u8; 32].to_vec());
let (account, usk) = db_data.create_account(&seed).unwrap();
let dfvk = usk.sapling().to_diversifiable_full_viewing_key();
// Add funds to the wallet in a single note
let value = Amount::from_u64(60000).unwrap();
let (cb, _) = fake_compact_block(
sapling_activation_height(),
BlockHash([0; 32]),
&dfvk,
AddressType::DefaultExternal,
value,
0,
);
insert_into_cache(&db_cache, &cb);
scan_cached_blocks(
&tests::network(),
&db_cache,
&mut db_data,
sapling_activation_height(),
1,
)
.unwrap();
// Verified balance matches total balance
let (_, anchor_height) = db_data
.get_target_and_anchor_heights(NonZeroU32::new(1).unwrap())
.unwrap()
.unwrap();
assert_eq!(
get_balance(&db_data.conn, AccountId::from(0)).unwrap(),
value
);
assert_eq!(
get_balance_at(&db_data.conn, AccountId::from(0), anchor_height).unwrap(),
value
);
let to_extsk = ExtendedSpendingKey::master(&[]);
let to: RecipientAddress = to_extsk.default_address().1.into();
let request = zip321::TransactionRequest::new(vec![Payment {
recipient_address: to,
amount: Amount::from_u64(10000).unwrap(),
memo: None, // this should result in the creation of an empty memo
label: None,
message: None,
other_params: vec![],
}])
.unwrap();
let fee_rule = FixedFeeRule::standard();
let change_strategy = fixed::SingleOutputChangeStrategy::new(fee_rule);
let input_selector =
&GreedyInputSelector::new(change_strategy, DustOutputPolicy::default());
let proposal_result = propose_transfer::<_, _, _, Infallible>(
&mut db_data,
&tests::network(),
account,
input_selector,
request,
NonZeroU32::new(1).unwrap(),
);
assert_matches!(proposal_result, Ok(_));
let change_memo = "Test change memo".parse::<Memo>().unwrap();
let create_proposed_result = create_proposed_transaction::<_, _, Infallible, _>(
&mut db_data,
&tests::network(),
test_prover(),
&usk,
OvkPolicy::Sender,
proposal_result.unwrap(),
NonZeroU32::new(1).unwrap(),
Some(change_memo.clone().into()),
);
assert_matches!(create_proposed_result, Ok(_));
let sent_tx_id = create_proposed_result.unwrap();
// Verify that the sent transaction was stored and that we can decrypt the memos
let tx = db_data
.get_transaction(sent_tx_id)
.expect("Created transaction was stored.");
let ufvks = [(account, usk.to_unified_full_viewing_key())]
.into_iter()
.collect();
let decrypted_outputs = decrypt_transaction(
&tests::network(),
sapling_activation_height() + 1,
&tx,
&ufvks,
);
assert_eq!(decrypted_outputs.len(), 2);
let mut found_tx_change_memo = false;
let mut found_tx_empty_memo = false;
for output in decrypted_outputs {
if output.memo == change_memo.clone().into() {
found_tx_change_memo = true
}
if output.memo == Memo::Empty.into() {
found_tx_empty_memo = true
}
}
assert!(found_tx_change_memo);
assert!(found_tx_empty_memo);
// Verify that the stored sent notes match what we're expecting
let mut stmt_sent_notes = db_data
.conn
.prepare("SELECT id_note FROM sent_notes WHERE tx = ?")
.unwrap();
let sent_note_ids = stmt_sent_notes
.query(rusqlite::params![sent_tx_id])
.unwrap()
.mapped(|row| row.get::<_, i64>(0).map(NoteId::SentNoteId))
.collect::<Result<Vec<_>, _>>()
.unwrap();
assert_eq!(sent_note_ids.len(), 2);
// The sent memo should be the empty memo for the sent output, and the
// change output's memo should be as specified.
let mut found_sent_change_memo = false;
let mut found_sent_empty_memo = false;
for sent_note_id in sent_note_ids {
match db_data
.get_memo(sent_note_id)
.expect("Note id is valid")
.as_ref()
{
Some(m) if m == &change_memo => {
found_sent_change_memo = true;
}
Some(m) if m == &Memo::Empty => {
found_sent_empty_memo = true;
}
Some(other) => panic!("Unexpected memo value: {:?}", other),
None => panic!("Memo should not be stored as NULL"),
}
}
assert!(found_sent_change_memo);
assert!(found_sent_empty_memo);
// Check that querying for a nonexistent sent note returns an error
assert_matches!(db_data.get_memo(NoteId::SentNoteId(12345)), Err(_));
}
#[test]
fn create_to_address_fails_on_incorrect_usk() {
let data_file = NamedTempFile::new().unwrap();