Fix waitsync operation and performance #2008
Conversation
…ng on fragile substring checks
|
Read, tested, looks good. But needs one more pair of eyes on it. |
zancas
left a comment
zancas
left a comment
There was a problem hiding this comment.
Please don't use unnecessarily derived representations when the source quantity is directly observable.
zingo-cli/src/lib.rs
Outdated
| Ok(json_val) => { | ||
| // Extract both percentage fields as f64. | ||
| let blocks_pct_opt = json_val | ||
| .get("percentage_total_blocks_scanned") |
There was a problem hiding this comment.
Why use a percentage?
That's just a lossee re-representation of an underyling state, is it not?
Doesn't the percentage shift as a function of the denominator?
If it does not then isn't it just some fixed value... if it DOES... then isn't it reporting the same percentage for multiple different degrees of completion?!
Please don't use derived statistics when the actual observable underlying quantity is available.
If what we mean is 10 blocks from the tip, or 100 blocks unfinalized.. or whatever then lets use that!
zingo-cli/src/lib.rs
Outdated
| // Extract both percentage fields as f64. | ||
| let blocks_pct_opt = json_val | ||
| .get("percentage_total_blocks_scanned") | ||
| .and_then(|v| v.as_f64()); |
There was a problem hiding this comment.
Using the directly countable observables.. whatever they are will permit us to track values as e.g. u32.
The complexity introduced by floating point operations doesn't seem to paid for anywhere in this design.
|
I agree, thanks for feedback. |
|
any chance to review this again and possibly accept it? Thanks |
|
@zancas @fluidvanadium |
Oscar-Pepper
left a comment
Oscar-Pepper
left a comment
There was a problem hiding this comment.
hi @Tomas-M thanks for your contribution, im not sure i understand your problem and i suspect we can solve it with a different approach. i have left some comments detailing why I dont think we should be adding fields to SyncStatus and also zingo-cli needs to be calling sync poll as this is how it will know sync is complete or be able to return the exact error from the sync handle
| // Poll sync task status | ||
| // Request machine-readable sync status. | ||
| command_transmitter | ||
| .send(("sync".to_string(), vec!["poll".to_string()])) |
There was a problem hiding this comment.
sync poll cannot be replaced by sync status. sync poll is polling the sync handle and it the only way to return the actual errors from the sync task.
| pub scan_ranges: Vec<ScanRange>, | ||
| pub sync_start_height: BlockHeight, | ||
| pub total_blocks: u32, | ||
| pub total_outputs: u32, |
| let sync_complete = value.total_blocks_scanned >= value.total_blocks | ||
| && (value.total_sapling_outputs_scanned + value.total_orchard_outputs_scanned) | ||
| >= value.total_outputs; | ||
|
|
There was a problem hiding this comment.
this sync completeness is equivalent to percentage_total_outputs_scanned being 100%. if there is an issue with using percentage_total_outputs_scanned (?) then this should be fixed instead of duplicating the logic into the json conversion. due to this it is unnecessary for the sync engine to maintain the two new fields added to SyncStatus, these values can actually be derived from the other data if necessary so im not sure this is the right approach
There was a problem hiding this comment.
oh sorry, it is actually equivalent to the sync poll returning a sync complete. is there an issue with sync poll you are experiencing? if you could write up a detailed issue we can discuss the solution if thats ok
|
@Tomas-M BTW sorry for the delay, I should be able to have more bandwidth for zingolib now |
|
Thank you for comments. I initially intended to check if the sync status is 100 (meaning 100%), but since this number is a float, a simple check like if (status==100) wont necessarily work since the number can be in fact 99.999999999999434 or something like that. For that reason, @zancas suggested to take different approach, and the one I proposed was easiest for me to do, with my limited knowledge and understanding of the code. If you propose a better solution, then please feel free to close this PR and do it in a better way, which is unfortunately outside of my abilities :) Thank you for consideration. |
Ah sorry to send you round in circles on this. So the sure way to check if sync is complete is to use the |
Signed-off-by: Tomas Matejicek <[email protected]>
4 participants
The waitsync loop relied on brittle string matching against the human-readable poll output. When the wallet was already synced, poll did not necessarily return the exact expected strings, causing the loop to wait indefinitely. This patch replaces this with a check that queries sync status in JSON form and reads the numeric
percentage_total_blocks_scannedandpercentage_total_outputs_scannedfields. Sync completion is determined reliably by numeric threshold.