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

zcb::sync: Refresh UTXOs at the start of each scanning cycle #1536

Merged
merged 1 commit into from
Sep 12, 2024
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
2 changes: 2 additions & 0 deletions zcash_client_backend/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ and this library adheres to Rust's notion of
- The `Account` trait now uses an associated type for its `AccountId`
type instead of a type parameter. This change allows for the simplification
of some type signatures.
- `zcash_client_backend::sync::run`:
- Transparent outputs are now refreshed in addition to shielded notes.

### Fixed
- `zcash_client_backend::tor::grpc` now needs the `lightwalletd-tonic-tls-webpki-roots`
Expand Down
140 changes: 139 additions & 1 deletion zcash_client_backend/src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,16 @@
#[cfg(feature = "orchard")]
use orchard::tree::MerkleHashOrchard;

#[cfg(feature = "transparent-inputs")]
use {
crate::{encoding::AddressCodec, wallet::WalletTransparentOutput},
zcash_primitives::{
legacy::Script,
transaction::components::transparent::{OutPoint, TxOut},
},
zcash_protocol::{consensus::NetworkUpgrade, value::Zatoshis},
};

/// Scans the chain until the wallet is up-to-date.
pub async fn run<P, ChT, CaT, DbT>(
client: &mut CompactTxStreamerClient<ChT>,
Expand All @@ -62,11 +72,27 @@
<DbT as WalletRead>::Error: std::error::Error + Send + Sync + 'static,
<DbT as WalletCommitmentTrees>::Error: std::error::Error + Send + Sync + 'static,
{
#[cfg(feature = "transparent-inputs")]
let wallet_birthday = db_data

Check warning on line 76 in zcash_client_backend/src/sync.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/sync.rs#L76

Added line #L76 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

We should set Sapling activation (or maybe Sapling activation + 1) as the minimum height to use for block retrieval in scanning. IIRC there's currently a bug where lightwalletd won't return the frontiers prior to Sapling activation correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that already happens elsewhere, or if not then it should? Block retrieval is entirely dictated by the scan queue, so what matters is how the queue is filled; account creation (and the birthday heights provided therein) is what defines the earliest entries in the scan queue.

.get_wallet_birthday()
.map_err(Error::Wallet)?
.unwrap_or_else(|| params.activation_height(NetworkUpgrade::Sapling).unwrap());

Check warning on line 79 in zcash_client_backend/src/sync.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/sync.rs#L78-L79

Added lines #L78 - L79 were not covered by tests

// 1) Download note commitment tree data from lightwalletd
// 2) Pass the commitment tree data to the database.
update_subtree_roots(client, db_data).await?;

while running(client, params, db_cache, db_data, batch_size).await? {}
while running(
client,
params,
db_cache,
db_data,
batch_size,
#[cfg(feature = "transparent-inputs")]
wallet_birthday,

Check warning on line 92 in zcash_client_backend/src/sync.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/sync.rs#L85-L92

Added lines #L85 - L92 were not covered by tests
)
.await?

Check warning on line 94 in zcash_client_backend/src/sync.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/sync.rs#L94

Added line #L94 was not covered by tests
{}

Ok(())
}
Expand All @@ -77,6 +103,7 @@
db_cache: &CaT,
db_data: &mut DbT,
batch_size: u32,
#[cfg(feature = "transparent-inputs")] wallet_birthday: BlockHeight,
) -> Result<bool, Error<CaT::Error, <DbT as WalletRead>::Error, TrErr>>
where
P: Parameters + Send + 'static,
Expand All @@ -94,6 +121,23 @@
// 4) Notify the wallet of the updated chain tip.
update_chain_tip(client, db_data).await?;

// Refresh UTXOs for the accounts in the wallet. We do this before we perform
// any shielded scanning, to ensure that we discover any UTXOs between the old
// fully-scanned height and the current chain tip.
#[cfg(feature = "transparent-inputs")]
for account_id in db_data.get_account_ids().map_err(Error::Wallet)? {
let start_height = db_data

Check warning on line 129 in zcash_client_backend/src/sync.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/sync.rs#L128-L129

Added lines #L128 - L129 were not covered by tests
.block_fully_scanned()
.map_err(Error::Wallet)?
.map(|meta| meta.block_height())
.unwrap_or(wallet_birthday);
info!(
"Refreshing UTXOs for {:?} from height {}",
account_id, start_height,

Check warning on line 136 in zcash_client_backend/src/sync.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/sync.rs#L131-L136

Added lines #L131 - L136 were not covered by tests
);
refresh_utxos(params, client, db_data, account_id, start_height).await?;

Check warning on line 138 in zcash_client_backend/src/sync.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/sync.rs#L138

Added line #L138 was not covered by tests
}

// 5) Get the suggested scan ranges from the wallet database
let mut scan_ranges = db_data.suggest_scan_ranges().map_err(Error::Wallet)?;

Expand Down Expand Up @@ -422,6 +466,100 @@
}
}

/// Refreshes the given account's view of UTXOs that exist starting at the given height.
///
/// ## Note about UTXO tracking
///
/// (Extracted from [a comment in the Android SDK].)
///
/// We no longer clear UTXOs here, as `WalletDb::put_received_transparent_utxo` now uses
/// an upsert instead of an insert. This means that now-spent UTXOs would previously have
/// been deleted, but now are left in the database (like shielded notes).
///
/// Due to the fact that the `lightwalletd` query only returns _current_ UTXOs, we don't
/// learn about recently-spent UTXOs here, so the transparent balance does not get updated
/// here.
///
/// Instead, when a received shielded note is "enhanced" by downloading the full
/// transaction, we mark any UTXOs spent in that transaction as spent in the database.
/// This relies on two current properties:
/// - UTXOs are only ever spent in shielding transactions.
/// - At least one shielded note from each shielding transaction is always enhanced.
///
/// However, for greater reliability, we may want to alter the Data Access API to support
/// "inferring spentness" from what is _not_ returned as a UTXO, or alternatively fetch
/// TXOs from `lightwalletd` instead of just UTXOs.
///
/// [a comment in the Android SDK]: https://github.com/Electric-Coin-Company/zcash-android-wallet-sdk/blob/855204fc8ae4057fdac939f98df4aa38c8e662f1/sdk-lib/src/main/java/cash/z/ecc/android/sdk/block/processor/CompactBlockProcessor.kt#L979-L991
#[cfg(feature = "transparent-inputs")]
async fn refresh_utxos<P, ChT, DbT, CaErr, TrErr>(

Check warning on line 495 in zcash_client_backend/src/sync.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/sync.rs#L495

Added line #L495 was not covered by tests
params: &P,
client: &mut CompactTxStreamerClient<ChT>,
db_data: &mut DbT,
account_id: DbT::AccountId,
start_height: BlockHeight,
) -> Result<(), Error<CaErr, <DbT as WalletRead>::Error, TrErr>>
where
P: Parameters + Send + 'static,
ChT: GrpcService<BoxBody>,
ChT::Error: Into<StdError>,
ChT::ResponseBody: Body<Data = Bytes> + Send + 'static,
<ChT::ResponseBody as Body>::Error: Into<StdError> + Send,
DbT: WalletWrite,
DbT::Error: std::error::Error + Send + Sync + 'static,
{
let request = service::GetAddressUtxosArg {
addresses: db_data

Check warning on line 512 in zcash_client_backend/src/sync.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/sync.rs#L512

Added line #L512 was not covered by tests
.get_transparent_receivers(account_id)
.map_err(Error::Wallet)?
.into_keys()
.map(|addr| addr.encode(params))
.collect(),
start_height: start_height.into(),

Check warning on line 518 in zcash_client_backend/src/sync.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/sync.rs#L518

Added line #L518 was not covered by tests
max_entries: 0,
};

if request.addresses.is_empty() {
info!("{:?} has no transparent receivers", account_id);

Check warning on line 523 in zcash_client_backend/src/sync.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/sync.rs#L522-L523

Added lines #L522 - L523 were not covered by tests
} else {
client
.get_address_utxos_stream(request)
.await?

Check warning on line 527 in zcash_client_backend/src/sync.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/sync.rs#L525-L527

Added lines #L525 - L527 were not covered by tests
.into_inner()
.map_err(Error::Server)
.and_then(|reply| async move {
WalletTransparentOutput::from_parts(
OutPoint::new(
reply.txid[..]
.try_into()
.map_err(|_| Error::MisbehavingServer)?,
reply
.index
.try_into()
.map_err(|_| Error::MisbehavingServer)?,

Check warning on line 539 in zcash_client_backend/src/sync.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/sync.rs#L529-L539

Added lines #L529 - L539 were not covered by tests
),
TxOut {
value: Zatoshis::from_nonnegative_i64(reply.value_zat)
.map_err(|_| Error::MisbehavingServer)?,
script_pubkey: Script(reply.script),

Check warning on line 544 in zcash_client_backend/src/sync.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/sync.rs#L541-L544

Added lines #L541 - L544 were not covered by tests
},
Some(
BlockHeight::try_from(reply.height)
.map_err(|_| Error::MisbehavingServer)?,

Check warning on line 548 in zcash_client_backend/src/sync.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/sync.rs#L546-L548

Added lines #L546 - L548 were not covered by tests
),
)
.ok_or(Error::MisbehavingServer)

Check warning on line 551 in zcash_client_backend/src/sync.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/sync.rs#L551

Added line #L551 was not covered by tests
})
.try_for_each(|output| {
let res = db_data.put_received_transparent_utxo(&output).map(|_| ());
async move { res.map_err(Error::Wallet) }

Check warning on line 555 in zcash_client_backend/src/sync.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/sync.rs#L553-L555

Added lines #L553 - L555 were not covered by tests
})
.await?;

Check warning on line 557 in zcash_client_backend/src/sync.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/sync.rs#L557

Added line #L557 was not covered by tests
}

Ok(())

Check warning on line 560 in zcash_client_backend/src/sync.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/sync.rs#L560

Added line #L560 was not covered by tests
}

/// Errors that can occur while syncing.
#[derive(Debug)]
pub enum Error<CaErr, DbErr, TrErr> {
Expand Down
Loading