Skip to content

Commit

Permalink
Address comments from code review.
Browse files Browse the repository at this point in the history
  • Loading branch information
nuttycom committed Jul 3, 2023
1 parent c363e71 commit e6676c6
Show file tree
Hide file tree
Showing 12 changed files with 76 additions and 91 deletions.
5 changes: 2 additions & 3 deletions zcash_client_backend/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ and this library adheres to Rust's notion of
respective `min_confirmations` arguments as `NonZeroU32`
- `data_api::wallet::input_selection::InputSelector::{propose_transaction, propose_shielding}`
now take their respective `min_confirmations` arguments as `NonZeroU32`
- A new `Sync` variant has been added to `data_api::chain::error::Error`.
- A new `Scan` variant has been added to `data_api::chain::error::Error`.
- A new `SyncRequired` variant has been added to `data_api::wallet::input_selection::InputSelectorError`.
- `zcash_client_backend::wallet`:
- `SpendableNote` has been renamed to `ReceivedSaplingNote`.
Expand All @@ -61,8 +61,7 @@ and this library adheres to Rust's notion of
- Arguments to `zcash_client_backend::scanning::scan_block` have changed. This
method now takes an optional `BlockMetadata` argument instead of a base commitment
tree and incremental witnesses for each previously-known note. In addition, the
return type has now been updated to return a `Result<ScannedBlock, ScanError>`
in the case of scan failure.
return type has now been updated to return a `Result<ScannedBlock, ScanError>`.


### Removed
Expand Down
4 changes: 4 additions & 0 deletions zcash_client_backend/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ fn build() -> io::Result<()> {
// Build the gRPC types and client.
tonic_build::configure()
.build_server(false)
.extern_path(
".cash.z.wallet.sdk.rpc.ChainMetadata",
"crate::proto::compact_formats::ChainMetadata",
)
.extern_path(
".cash.z.wallet.sdk.rpc.CompactBlock",
"crate::proto::compact_formats::CompactBlock",
Expand Down
4 changes: 1 addition & 3 deletions zcash_client_backend/proto/compact_formats.proto
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ option swift_prefix = "";
// Remember that proto3 fields are all optional. A field that is not present will be set to its zero value.
// bytes fields of hashes are in canonical little-endian format.

// BlockMetadata represents information about a block that may not be
// represented directly in the block data, but is instead derived from chain
// data or other external sources.
// ChainMetadata represents information about the state of the chain as of a given block.
message ChainMetadata {
uint32 saplingCommitmentTreeSize = 1; // the size of the Sapling note commitment tree as of the end of this block
uint32 orchardCommitmentTreeSize = 2; // the size of the Orchard note commitment tree as of the end of this block
Expand Down
2 changes: 1 addition & 1 deletion zcash_client_backend/src/data_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ impl<Nf> ScannedBlock<Nf> {
&self.sapling_commitments
}

pub fn take_sapling_commitments(self) -> Vec<(sapling::Node, Retention<BlockHeight>)> {
pub fn into_sapling_commitments(self) -> Vec<(sapling::Node, Retention<BlockHeight>)> {
self.sapling_commitments
}
}
Expand Down
9 changes: 5 additions & 4 deletions zcash_client_backend/src/data_api/chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,11 @@ pub trait BlockSource {
/// Scans at most `limit` new blocks added to the block source for any transactions received by the
/// tracked accounts.
///
/// If the `from_height` argument is not `None`, then the block source will begin requesting blocks
/// from the provided block source at the specified height; if `from_height` is `None then this
/// will begin scanning at first block after the position to which the wallet has previously
/// fully scanned the chain, thereby beginning or continuing a linear scan over all blocks.
/// If the `from_height` argument is not `None`, then this method block source will begin
/// requesting blocks from the provided block source at the specified height; if `from_height` is
/// `None then this will begin scanning at first block after the position to which the wallet has
/// previously fully scanned the chain, thereby beginning or continuing a linear scan over all
/// blocks.
///
/// This function will return without error after scanning at most `limit` new blocks, to enable
/// the caller to update their UI with scanning progress. Repeatedly calling this function with
Expand Down
4 changes: 1 addition & 3 deletions zcash_client_backend/src/proto/compact_formats.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 0 additions & 13 deletions zcash_client_backend/src/proto/service.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions zcash_client_backend/src/scanning.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ pub(crate) fn scan_block_with_runner<
at_height: cur_height,
})?;

let block_tx_count = block.vtx.len();
let compact_block_tx_count = block.vtx.len();
for (tx_idx, tx) in block.vtx.into_iter().enumerate() {
let txid = tx.txid();

Expand Down Expand Up @@ -392,7 +392,8 @@ pub(crate) fn scan_block_with_runner<
{
// Collect block note commitments
let node = sapling::Node::from_cmu(&output.cmu);
let is_checkpoint = output_idx + 1 == decoded.len() && tx_idx + 1 == block_tx_count;
let is_checkpoint =
output_idx + 1 == decoded.len() && tx_idx + 1 == compact_block_tx_count;
let retention = match (dec_output.is_some(), is_checkpoint) {
(is_marked, true) => Retention::Checkpoint {
id: cur_height,
Expand Down
4 changes: 2 additions & 2 deletions zcash_client_sqlite/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use shardtree::ShardTreeError;
use zcash_client_backend::encoding::{Bech32DecodeError, TransparentCodecError};
use zcash_primitives::{consensus::BlockHeight, zip32::AccountId};

use crate::PRUNING_HEIGHT;
use crate::PRUNING_DEPTH;

#[cfg(feature = "transparent-inputs")]
use zcash_primitives::legacy::TransparentAddress;
Expand Down Expand Up @@ -108,7 +108,7 @@ impl fmt::Display for SqliteClientError {
SqliteClientError::InvalidNoteId =>
write!(f, "The note ID associated with an inserted witness must correspond to a received note."),
SqliteClientError::RequestedRewindInvalid(h, r) =>
write!(f, "A rewind must be either of less than {} blocks, or at least back to block {} for your wallet; the requested height was {}.", PRUNING_HEIGHT, h, r),
write!(f, "A rewind must be either of less than {} blocks, or at least back to block {} for your wallet; the requested height was {}.", PRUNING_DEPTH, h, r),
SqliteClientError::Bech32DecodeError(e) => write!(f, "{}", e),
#[cfg(feature = "transparent-inputs")]
SqliteClientError::HdwalletError(e) => write!(f, "{:?}", e),
Expand Down
8 changes: 4 additions & 4 deletions zcash_client_sqlite/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ pub mod wallet;
/// The maximum number of blocks the wallet is allowed to rewind. This is
/// consistent with the bound in zcashd, and allows block data deeper than
/// this delta from the chain tip to be pruned.
pub(crate) const PRUNING_HEIGHT: u32 = 100;
pub(crate) const PRUNING_DEPTH: u32 = 100;

pub(crate) const SAPLING_TABLES_PREFIX: &str = "sapling";

Expand Down Expand Up @@ -428,7 +428,7 @@ impl<P: consensus::Parameters> WalletWrite for WalletDb<rusqlite::Connection, P>
let block_height = block.height();
let sapling_tree_size = block.metadata().sapling_tree_size();
let sapling_commitments_len = block.sapling_commitments().len();
let mut sapling_commitments = block.take_sapling_commitments().into_iter();
let mut sapling_commitments = block.into_sapling_commitments().into_iter();
wdb.with_sapling_tree_mut::<_, _, SqliteClientError>(move |sapling_tree| {
let start_position = Position::from(u64::from(sapling_tree_size))
- u64::try_from(sapling_commitments_len).unwrap();
Expand Down Expand Up @@ -640,7 +640,7 @@ impl<P: consensus::Parameters> WalletCommitmentTrees for WalletDb<rusqlite::Conn
let shard_store = SqliteShardStore::from_connection(&tx, SAPLING_TABLES_PREFIX)
.map_err(|e| ShardTreeError::Storage(Either::Right(e)))?;
let result = {
let mut shardtree = ShardTree::new(shard_store, PRUNING_HEIGHT.try_into().unwrap());
let mut shardtree = ShardTree::new(shard_store, PRUNING_DEPTH.try_into().unwrap());
callback(&mut shardtree)?
};
tx.commit()
Expand Down Expand Up @@ -668,7 +668,7 @@ impl<'conn, P: consensus::Parameters> WalletCommitmentTrees for WalletDb<SqlTran
let mut shardtree = ShardTree::new(
SqliteShardStore::from_connection(self.conn.0, SAPLING_TABLES_PREFIX)
.map_err(|e| ShardTreeError::Storage(Either::Right(e)))?,
PRUNING_HEIGHT.try_into().unwrap(),
PRUNING_DEPTH.try_into().unwrap(),
);
let result = callback(&mut shardtree)?;

Expand Down
105 changes: 51 additions & 54 deletions zcash_client_sqlite/src/wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ use zcash_client_backend::{
};

use crate::{
error::SqliteClientError, SqlTransaction, WalletCommitmentTrees, WalletDb, PRUNING_HEIGHT,
error::SqliteClientError, SqlTransaction, WalletCommitmentTrees, WalletDb, PRUNING_DEPTH,
};

#[cfg(feature = "transparent-inputs")]
Expand Down Expand Up @@ -543,94 +543,91 @@ pub(crate) fn block_height_extrema(

fn parse_block_metadata(
row: (BlockHeight, Vec<u8>, Option<u32>, Vec<u8>),
) -> Option<Result<BlockMetadata, SqliteClientError>> {
) -> Result<BlockMetadata, SqliteClientError> {
let (block_height, hash_data, sapling_tree_size_opt, sapling_tree) = row;
let sapling_tree_size = sapling_tree_size_opt.map(Ok).or_else(|| {
let sapling_tree_size = sapling_tree_size_opt.map_or_else(|| {
if sapling_tree == BLOCK_SAPLING_FRONTIER_ABSENT {
None
Err(SqliteClientError::CorruptedData("One of either the Sapling tree size or the legacy Sapling commitment tree must be present.".to_owned()))
} else {
// parse the legacy commitment tree data
read_commitment_tree::<
zcash_primitives::sapling::Node,
_,
{ zcash_primitives::sapling::NOTE_COMMITMENT_TREE_DEPTH },
>(Cursor::new(sapling_tree))
.map(|tree| Some(tree.size().try_into().unwrap()))
.map(|tree| tree.size().try_into().unwrap())
.map_err(SqliteClientError::from)
.transpose()
}
})?;
}, Ok)?;

let block_hash = BlockHash::try_from_slice(&hash_data).ok_or_else(|| {
SqliteClientError::from(io::Error::new(
io::ErrorKind::InvalidData,
format!("Invalid block hash length: {}", hash_data.len()),
))
});
})?;

Some(sapling_tree_size.and_then(|sapling_tree_size| {
block_hash.map(|block_hash| {
BlockMetadata::from_parts(block_height, block_hash, sapling_tree_size)
})
}))
Ok(BlockMetadata::from_parts(
block_height,
block_hash,
sapling_tree_size,
))
}

pub(crate) fn block_metadata(
conn: &rusqlite::Connection,
block_height: BlockHeight,
) -> Result<Option<BlockMetadata>, SqliteClientError> {
let res_opt = conn
.query_row(
"SELECT height, hash, sapling_commitment_tree_size, sapling_tree
conn.query_row(
"SELECT height, hash, sapling_commitment_tree_size, sapling_tree
FROM blocks
WHERE height = :block_height",
named_params![":block_height": u32::from(block_height)],
|row| {
let height: u32 = row.get(0)?;
let block_hash: Vec<u8> = row.get(1)?;
let sapling_tree_size: Option<u32> = row.get(2)?;
let sapling_tree: Vec<u8> = row.get(3)?;
Ok((
BlockHeight::from(height),
block_hash,
sapling_tree_size,
sapling_tree,
))
},
)
.optional()?;

res_opt.and_then(parse_block_metadata).transpose()
named_params![":block_height": u32::from(block_height)],
|row| {
let height: u32 = row.get(0)?;
let block_hash: Vec<u8> = row.get(1)?;
let sapling_tree_size: Option<u32> = row.get(2)?;
let sapling_tree: Vec<u8> = row.get(3)?;
Ok((
BlockHeight::from(height),
block_hash,
sapling_tree_size,
sapling_tree,
))
},
)
.optional()
.map_err(SqliteClientError::from)
.and_then(|meta_row| meta_row.map(parse_block_metadata).transpose())
}

pub(crate) fn block_fully_scanned(
conn: &rusqlite::Connection,
) -> Result<Option<BlockMetadata>, SqliteClientError> {
// FIXME: this will need to be rewritten once out-of-order scan range suggestion
// is implemented.
let res_opt = conn
.query_row(
"SELECT height, hash, sapling_commitment_tree_size, sapling_tree
conn.query_row(
"SELECT height, hash, sapling_commitment_tree_size, sapling_tree
FROM blocks
ORDER BY height DESC
LIMIT 1",
[],
|row| {
let height: u32 = row.get(0)?;
let block_hash: Vec<u8> = row.get(1)?;
let sapling_tree_size: Option<u32> = row.get(2)?;
let sapling_tree: Vec<u8> = row.get(3)?;
Ok((
BlockHeight::from(height),
block_hash,
sapling_tree_size,
sapling_tree,
))
},
)
.optional()?;

res_opt.and_then(parse_block_metadata).transpose()
[],
|row| {
let height: u32 = row.get(0)?;
let block_hash: Vec<u8> = row.get(1)?;
let sapling_tree_size: Option<u32> = row.get(2)?;
let sapling_tree: Vec<u8> = row.get(3)?;
Ok((
BlockHeight::from(height),
block_hash,
sapling_tree_size,
sapling_tree,
))
},
)
.optional()
.map_err(SqliteClientError::from)
.and_then(|meta_row| meta_row.map(parse_block_metadata).transpose())
}

/// Returns the block height at which the specified transaction was mined,
Expand Down Expand Up @@ -704,7 +701,7 @@ pub(crate) fn truncate_to_height<P: consensus::Parameters>(
.map(|opt| opt.map_or_else(|| sapling_activation_height - 1, BlockHeight::from))
})?;

if block_height < last_scanned_height - PRUNING_HEIGHT {
if block_height < last_scanned_height - PRUNING_DEPTH {
if let Some(h) = get_min_unspent_height(conn)? {
if block_height > h {
return Err(SqliteClientError::RequestedRewindInvalid(h, block_height));
Expand Down
4 changes: 2 additions & 2 deletions zcash_client_sqlite/src/wallet/init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use zcash_primitives::{

use zcash_client_backend::{data_api::SAPLING_SHARD_HEIGHT, keys::UnifiedFullViewingKey};

use crate::{error::SqliteClientError, wallet, WalletDb, SAPLING_TABLES_PREFIX};
use crate::{error::SqliteClientError, wallet, WalletDb, PRUNING_DEPTH, SAPLING_TABLES_PREFIX};

use super::commitment_tree::SqliteShardStore;

Expand Down Expand Up @@ -327,7 +327,7 @@ pub fn init_blocks_table<P: consensus::Parameters>(
_,
{ sapling::NOTE_COMMITMENT_TREE_DEPTH },
SAPLING_SHARD_HEIGHT,
> = ShardTree::new(shard_store, 100);
> = ShardTree::new(shard_store, PRUNING_DEPTH.try_into().unwrap());
shard_tree.insert_frontier_nodes(
nonempty_frontier.clone(),
Retention::Checkpoint {
Expand Down

0 comments on commit e6676c6

Please sign in to comment.