refactor(ntx-builder): simplify coordinator-actor messaging with Notify#1699
refactor(ntx-builder): simplify coordinator-actor messaging with Notify#1699SantiagoPittella wants to merge 7 commits intonextfrom
Conversation
SantiagoPittella
added
the
no changelog
crates/ntx-builder/src/actor/mod.rs
Outdated
| let resolved = self.db | ||
| .is_transaction_resolved(account_id, awaited_id) | ||
| .await | ||
| .expect("should be able to check tx status"); |
There was a problem hiding this comment.
Do we really want to stop everything if a single actor fails on this basis?
There was a problem hiding this comment.
If an actor panics, the coordinator catches the error in coordinator.next() and logs it, but the system keeps running without the actor. Though know that you mentioned this, probably the best would be to return an specific error for this Db fail case.
crates/ntx-builder/src/actor/mod.rs
Outdated
| /// before the failure counts are updated in the database. | ||
| async fn mark_notes_failed(&self, nullifiers: &[Nullifier], block_num: BlockNumber) { | ||
| let (ack_tx, ack_rx) = tokio::sync::oneshot::channel(); | ||
| let _ = self |
There was a problem hiding this comment.
_ is type Result<(), SendError<ActorRequest>>. Are we sure we don't want to be handling that error here?
There was a problem hiding this comment.
Same with let _ = ack_rx.await; below
There was a problem hiding this comment.
Replaced them with .is_err checks
sergerad
left a comment
sergerad
left a comment
There was a problem hiding this comment.
LGTM just a few questions on errors we don't handle
3 participants
First task of #1694
mpsc<Arc<MempoolEvent>>channels withArc<Notify>. The DB is already the source of truth (since chore(ntx): replace in memory with sqlite database #1662), so actors only need a "re-check your state" signal and not the event payload. This removes allSendErrorhandling, failed-actor cleanup, and thesend()helper from the coordinator.TransactionInflightmode, actors now query the DB (is_transaction_resolved) to check if their awaited transaction was committed/reverted, sinceNotifydoesn't carry payload.ActorRequestenum over onempscchannel.NotesFailedcarries a oneshot ack to prevent a race condition where the actor could re-select notes before failure counts are persisted (context).CacheNoteScriptremains fire-and-forget.