Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add get_wallet_summary to WalletRead #914

Merged
merged 4 commits into from
Sep 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion zcash_client_backend/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,19 @@ and this library adheres to Rust's notion of
- `impl Eq for zcash_client_backend::zip321::{Payment, TransactionRequest}`
- `impl Debug` for `zcash_client_backend::{data_api::wallet::input_selection::Proposal, wallet::ReceivedSaplingNote}`
- `zcash_client_backend::data_api`:
- `AccountBalance`
- `AccountBirthday`
- `Balance`
- `BlockMetadata`
- `NoteId`
- `NullifierQuery` for use with `WalletRead::get_sapling_nullifiers`
- `Ratio`
- `ScannedBlock`
- `ShieldedProtocol`
- `WalletCommitmentTrees`
- `WalletSummary`
- `WalletRead::{chain_height, block_metadata, block_fully_scanned, suggest_scan_ranges,
get_wallet_birthday, get_account_birthday}`
get_wallet_birthday, get_account_birthday, get_wallet_summary}`
- `WalletWrite::{put_blocks, update_chain_tip}`
- `chain::CommitmentTreeRoot`
- `scanning` A new module containing types required for `suggest_scan_ranges`
Expand Down Expand Up @@ -113,6 +117,8 @@ and this library adheres to Rust's notion of
instead to obtain the wallet's view of the chain tip instead, or
`suggest_scan_ranges` to obtain information about blocks that need to be
scanned.
- `WalletRead::get_balance_at` has been removed. Use `WalletRead::get_wallet_summary`
instead.
- `WalletRead::{get_all_nullifiers, get_commitment_tree, get_witnesses}` have
been removed without replacement. The utility of these methods is now
subsumed by those available from the `WalletCommitmentTrees` trait.
Expand Down
188 changes: 169 additions & 19 deletions zcash_client_backend/src/data_api.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
//! Interfaces for wallet data persistence & low-level wallet utilities.

use std::fmt::Debug;
use std::io;
use std::num::NonZeroU32;
use std::{collections::HashMap, num::TryFromIntError};
use std::{
collections::{BTreeMap, HashMap},
fmt::Debug,
io,
num::{NonZeroU32, TryFromIntError},
};

use incrementalmerkletree::{frontier::Frontier, Retention};
use secrecy::SecretVec;
Expand All @@ -15,7 +17,10 @@ use zcash_primitives::{
memo::{Memo, MemoBytes},
sapling::{self, Node, NOTE_COMMITMENT_TREE_DEPTH},
transaction::{
components::{amount::Amount, OutPoint},
components::{
amount::{Amount, NonNegativeAmount},
OutPoint,
},
Transaction, TxId,
},
zip32::{AccountId, ExtendedFullViewingKey},
Expand Down Expand Up @@ -46,6 +51,155 @@ pub enum NullifierQuery {
All,
}

/// Balance information for a value within a single pool in an account.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct Balance {
/// The value in the account that may currently be spent; it is possible to compute witnesses
/// for all the notes that comprise this value, and all of this value is confirmed to the
/// required confirmation depth.
pub spendable_value: NonNegativeAmount,

/// The value in the account of shielded change notes that do not yet have sufficient
/// confirmations to be spendable.
pub change_pending_confirmation: NonNegativeAmount,

/// The value in the account of all remaining received notes that either do not have sufficient
/// confirmations to be spendable, or for which witnesses cannot yet be constructed without
/// additional scanning.
pub value_pending_spendability: NonNegativeAmount,
}

impl Balance {
/// The [`Balance`] value having zero values for all its fields.
pub const ZERO: Self = Self {
spendable_value: NonNegativeAmount::ZERO,
change_pending_confirmation: NonNegativeAmount::ZERO,
value_pending_spendability: NonNegativeAmount::ZERO,
};

/// Returns the total value of funds represented by this [`Balance`].
pub fn total(&self) -> NonNegativeAmount {
(self.spendable_value + self.change_pending_confirmation + self.value_pending_spendability)
.expect("Balance cannot overflow MAX_MONEY")
}
}

/// Balance information for a single account. The sum of this struct's fields is the total balance
/// of the wallet.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct AccountBalance {
/// The value of unspent Sapling outputs belonging to the account.
pub sapling_balance: Balance,

/// The value of all unspent transparent outputs belonging to the account, irrespective of
/// confirmation depth.
///
/// Unshielded balances are not subject to confirmation-depth constraints, because the only
/// possible operation on a transparent balance is to shield it, it is possible to create a
/// zero-conf transaction to perform that shielding, and the resulting shielded notes will be
/// subject to normal confirmation rules.
Comment on lines +97 to +100
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defining this to include all zero-conf funds means that an adversary will be able to arbitrarily inflate it above the realistic balance, as soon as we implement detection of zero-conf funds.

If I send you funds in a bunch of transactions that each have 50 unpaid actions and expire at the next block, I can be pretty confident that at most one (probably zero) of them will be mined. So, using this value as defined for anything opens the wallet up to the transparent equivalent of a Faerie Gold attack (and to various social engineering attacks, such as refund scams, if it is ever shown to a user). This is also a potential DoS attack on auto-shielding.

I agree that we haven't yet implemented the things that would enable these attacks, but still, it should not be defined that way.

Suggested change
/// Unshielded balances are not subject to confirmation-depth constraints, because the only
/// possible operation on a transparent balance is to shield it, it is possible to create a
/// zero-conf transaction to perform that shielding, and the resulting shielded notes will be
/// subject to normal confirmation rules.
/// Unshielded balance, with a minimum confirmation depth of 1.
///
/// The only possible operation on a transparent balance is to shield it, and the resulting
/// shielded notes will be subject to normal confirmation rules. Nevertheless, the unshielded
/// balance should not be over-reported, and including zero-conf UTXOs without restriction
/// would make it too easy for an adversary to cause such over-reporting. This policy might
/// be refined in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zero-conf balances have essentially become the standard among zcash (and Bitcoin and Ethereum) wallets, and the functionality to provide zero-conf balance is highly in demand. I don't know how we get around that; users in general would prefer to immediately see incoming funds as being recognized, at the cost of having to be diligent about waiting for important transactions to be mined.

Copy link
Contributor

@daira daira Sep 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We spent a huge amount of time, effort, and protocol complexity preventing Faerie Gold attacks. I don't see why we would just throw up our hands and accept a transparent attack with similar impact because not caring about it is "the standard".

What do Bitcoin and Ethereum wallets do these days about minimum-relay-fee transactions that definitely won't get mined? Are you really saying they just show them as balance anyway?

pub unshielded: NonNegativeAmount,
nuttycom marked this conversation as resolved.
Show resolved Hide resolved
}

impl AccountBalance {
/// Returns the total value of funds belonging to the account.
pub fn total(&self) -> NonNegativeAmount {
(self.sapling_balance.total() + self.unshielded)
.expect("Account balance cannot overflow MAX_MONEY")
}
}

/// A polymorphic ratio type, usually used for rational numbers.
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub struct Ratio<T> {
numerator: T,
denominator: T,
}

impl<T> Ratio<T> {
/// Constructs a new Ratio from a numerator and a denominator.
pub fn new(numerator: T, denominator: T) -> Self {
daira marked this conversation as resolved.
Show resolved Hide resolved
Self {
numerator,
denominator,
}
}

/// Returns the numerator of the ratio.
pub fn numerator(&self) -> &T {
&self.numerator
}

/// Returns the denominator of the ratio.
pub fn denominator(&self) -> &T {
&self.denominator
}
}

/// A type representing the potentially-spendable value of unspent outputs in the wallet.
///
/// The balances reported using this data structure may overestimate the total spendable value of
/// the wallet, in the case that the spend of a previously received shielded note has not yet been
/// detected by the process of scanning the chain. The balances reported using this data structure
/// can only be certain to be unspent in the case that [`Self::is_synced`] is true, and even in
/// this circumstance it is possible that a newly created transaction could conflict with a
/// not-yet-mined transaction in the mempool.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct WalletSummary {
account_balances: BTreeMap<AccountId, AccountBalance>,
chain_tip_height: BlockHeight,
fully_scanned_height: BlockHeight,
scan_progress: Option<Ratio<u64>>,
}

impl WalletSummary {
/// Constructs a new [`WalletSummary`] from its constituent parts.
pub fn new(
account_balances: BTreeMap<AccountId, AccountBalance>,
chain_tip_height: BlockHeight,
fully_scanned_height: BlockHeight,
scan_progress: Option<Ratio<u64>>,
) -> Self {
Self {
account_balances,
chain_tip_height,
fully_scanned_height,
scan_progress,
}
}

/// Returns the balances of accounts in the wallet, keyed by account ID.
pub fn account_balances(&self) -> &BTreeMap<AccountId, AccountBalance> {
nuttycom marked this conversation as resolved.
Show resolved Hide resolved
&self.account_balances
}

/// Returns the height of the current chain tip.
pub fn chain_tip_height(&self) -> BlockHeight {
self.chain_tip_height
}

/// Returns the height below which all blocks have been scanned by the wallet, ignoring blocks
/// below the wallet birthday.
pub fn fully_scanned_height(&self) -> BlockHeight {
self.fully_scanned_height
}

/// Returns the progress of scanning shielded outputs, in terms of the ratio between notes
/// scanned and the total number of notes added to the chain since the wallet birthday.
///
/// This ratio should only be used to compute progress percentages, and the numerator and
/// denominator should not be treated as authoritative note counts. Returns `None` if the
/// wallet is unable to determine the size of the note commitment tree.
pub fn scan_progress(&self) -> Option<Ratio<u64>> {
self.scan_progress
}

/// Returns whether or not wallet scanning is complete.
pub fn is_synced(&self) -> bool {
self.chain_tip_height == self.fully_scanned_height
}
}

/// Read-only operations required for light wallet functions.
///
/// This trait defines the read-only portion of the storage interface atop which
Expand Down Expand Up @@ -157,15 +311,12 @@ pub trait WalletRead {
extfvk: &ExtendedFullViewingKey,
) -> Result<bool, Self::Error>;

/// Returns the wallet balance for an account as of the specified block height.
///
/// This may be used to obtain a balance that ignores notes that have been received so recently
/// that they are not yet deemed spendable.
fn get_balance_at(
/// Returns the wallet balances and sync status for an account given the specified minimum
/// number of confirmations, or `Ok(None)` if the wallet has no balance data available.
fn get_wallet_summary(
&self,
account: AccountId,
anchor_height: BlockHeight,
) -> Result<Amount, Self::Error>;
min_confirmations: u32,
) -> Result<Option<WalletSummary>, Self::Error>;

/// Returns the memo for a note.
///
Expand Down Expand Up @@ -747,7 +898,7 @@ pub mod testing {
use super::{
chain::CommitmentTreeRoot, scanning::ScanRange, AccountBirthday, BlockMetadata,
DecryptedTransaction, NoteId, NullifierQuery, ScannedBlock, SentTransaction,
WalletCommitmentTrees, WalletRead, WalletWrite, SAPLING_SHARD_HEIGHT,
WalletCommitmentTrees, WalletRead, WalletSummary, WalletWrite, SAPLING_SHARD_HEIGHT,
};

pub struct MockWalletDb {
Expand Down Expand Up @@ -853,12 +1004,11 @@ pub mod testing {
Ok(false)
}

fn get_balance_at(
fn get_wallet_summary(
&self,
_account: AccountId,
_anchor_height: BlockHeight,
) -> Result<Amount, Self::Error> {
Ok(Amount::zero())
_min_confirmations: u32,
) -> Result<Option<WalletSummary>, Self::Error> {
Ok(None)
}

fn get_memo(&self, _id_note: NoteId) -> Result<Option<Memo>, Self::Error> {
Expand Down
Loading
Loading