Scan cleanup #1328
Scan cleanup #1328
Conversation
|
I am seeing CI failures, and I clicked update branch. |
|
i am going to review this set of PRs as a whole starting from here |
|
|
||
| // create channel for sending fetch requests and launch fetcher task | ||
| let (fetch_request_sender, fetch_request_receiver) = unbounded_channel(); | ||
| let (fetch_request_sender, fetch_request_receiver) = mpsc::unbounded_channel(); |
There was a problem hiding this comment.
because, only because i have semantic saturation on the word 'fetch', this bit could use commentary explaining how the channels interface
There was a problem hiding this comment.
ok thanks i will try to add clarity in the following PR. this channel can essentially be abstracted as how the sync engine connects to the server
| if let Some(worker) = self.workers.iter().find(|worker| !worker.is_scanning()) { | ||
| worker.add_scan_task(scan_task); | ||
| } else { | ||
| panic!("no idle workers!") |
There was a problem hiding this comment.
to be replaced with err handling
There was a problem hiding this comment.
yes! all panics and asserts and unwraps will definately be replaced after PoC
| Err(TryRecvError::Empty) => (), | ||
| Err(TryRecvError::Disconnected) => break, | ||
| } | ||
| } |
There was a problem hiding this comment.
at this point IIUC, either the connection broke or all ranges are scanned.
are we now making assumptions about how many tuples are left in the receiver? couldnt it still be more than one? or shouldnt it be 0?
There was a problem hiding this comment.
yes thats right. if all senders are closed, try_recv will still return Ok until the channel is empty and then it will hit the Err(Disconnected) and break the loop. although this will not happen the way the code is currently written as in the happy path the break on line 83 will be hit when all ranges are scanned (except the ones currently scanning). in that case we break and the while loop will process that remaining results being scanned. this break on line 95 will still be useful if we decide to close the workers with the scan_results_senders but if its just an error case then we will change this to return an error
| update_wallet_data(wallet, scan_results).unwrap(); | ||
| // TODO: link nullifiers and scan linked transactions | ||
| remove_irrelevant_data(wallet, &scan_range).unwrap(); | ||
| mark_scanned(scan_range, wallet.get_sync_state_mut().unwrap()).unwrap(); | ||
| } |
There was a problem hiding this comment.
yes if we keep this repeated part for shutdown we can DRY, i havent yet because i dont know if this architecture will stay and I also dont know what this block really is yet
| scan_ranges.splice(index..=index, vec![lower_range.clone(), higher_range]); | ||
|
|
||
| // TODO: set selected scan range to `ignored` so the same range is not selected twice when multiple tasks call this fn | ||
| let lower_range_ignored = |
There was a problem hiding this comment.
okay, i am understanding ScanPriority::Ignored here means [currently in scan]
There was a problem hiding this comment.
thats right you are following this really well, so LRZ probably had a slightly different vision for scan ranges but in the way we are using them ignored is not used and scanning does not exist. for now, ignored means scanning until we maybe PR into LRZ or create our own copy
| fn mark_scanned(scan_range: ScanRange, sync_state: &mut SyncState) -> Result<(), ()> { | ||
| let scan_ranges = sync_state.scan_ranges_mut(); | ||
|
|
||
| if let Some((index, range)) = scan_ranges |
There was a problem hiding this comment.
there has gotta be a better way to do this! eventually
not just because of mark. but because == comparison on the range seems weird, and because rebuilding it instead of changing its priority seems weird, and because panicking is not great, and because what if the ranges overlappp?
There was a problem hiding this comment.
i am open to suggestions about the comparison but i dont see an issue, it is essentially just two numbers that we must make sure are identical.
i agree rebuilding to change priority could be improved, again we will have to open a PR into LRZ or copy it into our lib.
in terms of overlapping. only one scan range is ever created, from birthday to chain height. then this scan range is only split so no overlapping or gaps are possible, as long as we unit test the split and splicing functions properly. the only exception is when ranges are rebuilt to change its priority but the ranges are copied so we can assume this will never be bugged and should also be tested eventually
| pub(crate) shard_tree_data: ShardTreeData, | ||
| } | ||
|
|
||
| pub(crate) struct ScanResults { |
There was a problem hiding this comment.
this could use some comments explaning the methodology and significance of these ScanResults
There was a problem hiding this comment.
sure, when the scan code is finished i will make sure the comments are thorough. i dont want to waste too much time yet as its still ion flux so may change
ontop of #1327