refactor(rust-client): make NTL and chain syncing concurrent - #2453
refactor(rust-client): make NTL and chain syncing concurrent#2453ricomateo wants to merge 39 commits into
Conversation
ricomateo
changed the title
…em in the apply phase
…e by-details import
…_transaction_updates
…d of refetching them
ricomateo
changed the title
| /// | ||
| /// Covers the two node calls that depend on nothing but the sync input: | ||
| /// 1. Fetch sync data from the node (MMR delta, note inclusions, transactions). | ||
| /// 2. Update account states (fetch updated public accounts, flag mismatched private ones). |
There was a problem hiding this comment.
This now sounds like something that should be done in the apply phase
There was a problem hiding this comment.
Moved the apply_superseded_account_state call to the apply phase here 90ce893.
The derive_account_commitments step cannot be moved since it is required by the account_state_sync call, which performs RPC calls to the node.
| assert!( | ||
| note_blocks_awaiting_screening.is_empty(), | ||
| "note blocks must be screened before the update is built" | ||
| ); |
There was a problem hiding this comment.
This should be a check, not an assertion.
There was a problem hiding this comment.
Replaced the assertion with an error here d24f669
| /// A note delivered over the NTL whose nullifier is already on chain must be stored as consumed. | ||
| /// | ||
| /// Probe for 0xMiden/rust-sdk#2422. | ||
| #[tokio::test] | ||
| async fn ntl_note_already_spent_below_the_checkpoint_is_not_left_committed() { |
|
Looks good! Two small comments, not necessarily for this PR:
Let me know what you think! :) |
igamigo
left a comment
igamigo
left a comment
There was a problem hiding this comment.
Sorry I took long to review this. I think the concurrency win is nice and looks great, but I think the current shape adds a bunch of complexity (and a little bit in the wrong place) and is not very easy to read/understand.
IIUC, to run the transport's node calls concurrently, the PR splits import_note_records_by_details in two, names both halves around the transport use case, and then has the transport module reimplement the first half of import_notes. Ideally "import" and "NTL" should not need to know about each other, since we also use import more generally (to import note files).
I wonder if for simplicity we could start with a smaller scope and then eventually modify it more, maybe through more general refactors (#2118 which I want to tackle soon). Like you did, we could fetch both updates simultaneously, but only the network part of the transport side. The transport fetches its pages, sync_chain runs as it does today, and once both are done the delivered notes go through the regular import_notes path as NoteFile::ExpectedNotes (wihtout modifications). The import already does the checks (including lookback), stores the headers and writes the records, so sync_chain/StateSync don't need to change much either.
There is a cost to this and it's that it's by nature much less parallel, lookback and nullifier check run after the join (3 sequential RPCs calls?), but I think it's a step in the right direction and could be simpler to reason about. From this PR we could keep the fact that we could reuse the lookback response's header and MMR path instead of fetching a header per block (inside import_note_records_by_details, since it isn't transport-specific), and join! rather than try_join! so an NTL error doesn't cancel the chain sync mid-way.
|
|
||
| /// A note to import: the stored record it updates when there is one, its details, the block from | ||
| /// which to look for its commitment, and the tag to track it under. | ||
| pub(crate) type NoteImportRequest = (Option<InputNoteRecord>, NoteDetails, BlockNumber, NoteTag); |
There was a problem hiding this comment.
nit: This is only an import by details, so maybe we could name the type something like NoteImportByDetailsRequest (although this name is not great at all)
| /// node has not reported as committed keep (or get) their expected record; the rest become | ||
| /// `Committed`, since the response carries the block header that verifies their inclusion, | ||
| /// and are returned only if the new information changed them. | ||
| pub(crate) async fn fetch_transport_notes_onchain_state( |
There was a problem hiding this comment.
I think ideally this module shouldn't need to know about transport. AFAICT import_note_records_by_details was split only so the transport could run the lookback during the concurrent phase. If we're okay with doing the import sequentially for now, this file can stay as it is on next (I think). I also think this does not mess with the screening PR, but if it does please let me know.
| let mut previous_by_commitment: BTreeMap<NoteDetailsCommitment, InputNoteRecord> = self | ||
| .get_input_notes(NoteFilter::DetailsCommitments( | ||
| requests_by_commitment.keys().copied().collect(), | ||
| )) | ||
| .await? | ||
| .into_iter() | ||
| .filter_map(|commitment| id_by_commitment.get(&commitment).copied()) | ||
| .map(|note| (note.details_commitment(), note)) | ||
| .collect(); | ||
|
|
||
| Ok((imported_ids, rcursor)) | ||
| let mut requests = Vec::with_capacity(requests_by_commitment.len()); | ||
| for (commitment, (details, after_block_num, tag)) in requests_by_commitment { | ||
| let previous_note = previous_by_commitment.remove(&commitment); | ||
| ensure_not_processing(previous_note.as_ref())?; |
There was a problem hiding this comment.
If we hand raw notes to import_notes I think this becomes redundant
5 participants
Refactors
Client::sync_stateso the network fetching phase of the NTL and chain sync run concurrently.This reduces the syncing time by ~25%, according to some benchmarks performed against devnet, syncing the client with 6 imported accounts.
In order to make the NTL and chain fetching concurrent, both syncs are split into a fetch phase (which makes the network requests to the RPC and NTL) and an apply phase (which applies the sync updates to the store).
Client::sync_statethen runs the two fetch phases undertry_join!and applies the changes to the store afterwards, instead of running a full note transport sync before the chain sync.It also adds a check for the nullifiers of the NTL delivered notes (closes #2422).
Closes #2361