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

fix: split state validation status into kernel and rproof updates. #3096

Merged
merged 11 commits into from
Nov 17, 2019
Merged
19 changes: 11 additions & 8 deletions api/src/handlers/server_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,16 +113,19 @@ fn sync_status_to_api(sync_status: SyncStatus) -> (String, Option<serde_json::Va
"txhashset_download".to_string(),
Some(json!({ "downloaded_size": downloaded_size, "total_size": total_size })),
),
SyncStatus::TxHashsetValidation {
kernels,
kernel_total,
SyncStatus::TxHashsetRangeProofsValidation {
rproofs,
rproof_total,
rproofs_total,
} => (
"txhashset_rangeproof_validation".to_string(),
JosephGoulden marked this conversation as resolved.
Show resolved Hide resolved
Some(json!({ "rproofs": rproofs, "rproofs_total": rproofs_total })),
),
SyncStatus::TxHashsetKernelsValidation {
kernels,
kernels_total,
} => (
"txhashset_validation".to_string(),
Some(
json!({ "kernels": kernels, "kernel_total": kernel_total ,"rproofs": rproofs, "rproof_total": rproof_total }),
),
"txhashset_kernels_validation".to_string(),
Some(json!({ "kernels": kernels, "kernels_total": kernels_total })),
),
SyncStatus::BodySync {
current_height,
Expand Down
7 changes: 4 additions & 3 deletions chain/src/txhashset/txhashset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1241,7 +1241,7 @@ impl<'a> Extension<'a> {
TxKernel::batch_sig_verify(&tx_kernels)?;
kern_count += tx_kernels.len() as u64;
tx_kernels.clear();
status.on_validation(kern_count, total_kernels, 0, 0);
status.on_validation_kernels(kern_count, total_kernels);
debug!(
"txhashset: verify_kernel_signatures: verified {} signatures",
kern_count,
Expand All @@ -1266,7 +1266,8 @@ impl<'a> Extension<'a> {
let mut proofs: Vec<RangeProof> = Vec::with_capacity(1_000);

let mut proof_count = 0;
let total_rproofs = pmmr::n_leaves(self.output_pmmr.unpruned_size());
let total_rproofs = self.output_pmmr.n_leafs();

for pos in self.output_pmmr.leaf_pos_iter() {
let output = self.output_pmmr.get_data(pos);
let proof = self.rproof_pmmr.get_data(pos);
Expand Down Expand Up @@ -1295,7 +1296,7 @@ impl<'a> Extension<'a> {
}

if proof_count % 1_000 == 0 {
status.on_validation(0, 0, proof_count, total_rproofs);
status.on_validation_rproofs(proof_count, total_rproofs);
}
}

Expand Down
66 changes: 23 additions & 43 deletions chain/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,13 @@ pub enum SyncStatus {
/// Setting up before validation
TxHashsetSetup,
/// Validating the full state
TxHashsetValidation {
TxHashsetKernelsValidation {
kernels: u64,
kernel_total: u64,
kernels_total: u64,
},
TxHashsetRangeProofsValidation {
rproofs: u64,
rproof_total: u64,
rproofs_total: u64,
},
/// Finalizing the new state
TxHashsetSave,
Expand Down Expand Up @@ -155,43 +157,18 @@ impl TxHashsetWriteStatus for SyncState {
self.update(SyncStatus::TxHashsetSetup);
}

fn on_validation(&self, vkernels: u64, vkernel_total: u64, vrproofs: u64, vrproof_total: u64) {
let mut status = self.current.write();
match *status {
SyncStatus::TxHashsetValidation {
kernels,
kernel_total,
rproofs,
rproof_total,
} => {
let ks = if vkernels > 0 { vkernels } else { kernels };
let kt = if vkernel_total > 0 {
vkernel_total
} else {
kernel_total
};
let rps = if vrproofs > 0 { vrproofs } else { rproofs };
let rpt = if vrproof_total > 0 {
vrproof_total
} else {
rproof_total
};
*status = SyncStatus::TxHashsetValidation {
kernels: ks,
kernel_total: kt,
rproofs: rps,
rproof_total: rpt,
};
}
_ => {
*status = SyncStatus::TxHashsetValidation {
kernels: 0,
kernel_total: 0,
rproofs: 0,
rproof_total: 0,
}
}
}
fn on_validation_kernels(&self, kernels: u64, kernels_total: u64) {
*self.current.write() = SyncStatus::TxHashsetKernelsValidation {
kernels,
kernels_total,
};
}

fn on_validation_rproofs(&self, rproofs: u64, rproofs_total: u64) {
*self.current.write() = SyncStatus::TxHashsetRangeProofsValidation {
rproofs,
rproofs_total,
};
}

fn on_save(&self) {
Expand Down Expand Up @@ -315,8 +292,10 @@ pub trait ChainAdapter {
pub trait TxHashsetWriteStatus {
/// First setup of the txhashset
fn on_setup(&self);
/// Starting validation
fn on_validation(&self, kernels: u64, kernel_total: u64, rproofs: u64, rproof_total: u64);
/// Starting kernel validation
fn on_validation_kernels(&self, kernels: u64, kernel_total: u64);
/// Starting rproof validation
fn on_validation_rproofs(&self, rproofs: u64, rproof_total: u64);
/// Starting to save the txhashset and related data
fn on_save(&self);
/// Done writing a new txhashset
Expand All @@ -328,7 +307,8 @@ pub struct NoStatus;

impl TxHashsetWriteStatus for NoStatus {
fn on_setup(&self) {}
fn on_validation(&self, _ks: u64, _kts: u64, _rs: u64, _rt: u64) {}
fn on_validation_kernels(&self, _ks: u64, _kts: u64) {}
fn on_validation_rproofs(&self, _rs: u64, _rt: u64) {}
fn on_save(&self) {}
fn on_done(&self) {}
}
Expand Down
3 changes: 3 additions & 0 deletions core/src/core/pmmr/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ pub trait Backend<T: PMMRable> {
/// Iterator over current (unpruned, unremoved) leaf positions.
fn leaf_pos_iter(&self) -> Box<dyn Iterator<Item = u64> + '_>;

/// Number of leaves
fn n_leafs(&self) -> u64;
Copy link
Member

Choose a reason for hiding this comment

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

We should think about a better name for this. Its not the number of leaves. Its the number of "not removed" leaf positions but that's a pretty awkward name.


/// Remove Hash by insertion position. An index is also provided so the
/// underlying backend can implement some rollback of positions up to a
/// given index (practically the index is the height of a block that
Expand Down
5 changes: 5 additions & 0 deletions core/src/core/pmmr/pmmr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,11 @@ where
self.backend.leaf_pos_iter()
}

/// Number of leafs in the MMR
pub fn n_leafs(&self) -> u64 {
self.backend.n_leafs()
}

/// Returns a vec of the peaks of this MMR.
pub fn peaks(&self) -> Vec<Hash> {
let peaks_pos = peaks(self.last_pos);
Expand Down
4 changes: 4 additions & 0 deletions core/tests/vec_backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,10 @@ impl<T: PMMRable> Backend<T> for VecBackend<T> {
unimplemented!()
}

fn n_leafs(&self) -> u64 {
unimplemented!()
}

fn remove(&mut self, position: u64) -> Result<(), String> {
self.remove_list.push(position);
Ok(())
Expand Down
3 changes: 2 additions & 1 deletion servers/src/grin/sync/syncer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,8 @@ impl SyncRunner {
match self.sync_state.status() {
SyncStatus::TxHashsetDownload { .. }
| SyncStatus::TxHashsetSetup
| SyncStatus::TxHashsetValidation { .. }
Copy link
Member

Choose a reason for hiding this comment

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

@quentinlesceller What implications are there for changing the set of sync states here? Is this something we can safely do or should we maintain the existing set of states?

Copy link
Contributor Author

@JosephGoulden JosephGoulden Oct 30, 2019

Choose a reason for hiding this comment

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

could add the TxHashsetValidation status back to maintain compatibility for any consumers? Was thinking we could remove it in a major release.. 3.0.0

Copy link
Member

Choose a reason for hiding this comment

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

If think it's pretty safe to move forward and add this two new state. That could potentially break wallet using the status if they are doing strong type check but I presume it's okay for 3.0.0.

| SyncStatus::TxHashsetRangeProofsValidation { .. }
| SyncStatus::TxHashsetKernelsValidation { .. }
| SyncStatus::TxHashsetSave
| SyncStatus::TxHashsetDone => check_state_sync = true,
_ => {
Expand Down
Loading