-
Notifications
You must be signed in to change notification settings - Fork 111
feat(client): distinct ApplyTransactionAfterSubmitFailed error + one retry #2059
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
Changes from 2 commits
78fd27e
b89046a
5961377
8c30fab
868eca0
3ec08c7
87b2973
1f1862f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,11 @@ | ||
| use alloc::boxed::Box; | ||
| use alloc::string::{String, ToString}; | ||
| use alloc::vec::Vec; | ||
| use core::fmt; | ||
|
|
||
| use miden_protocol::Word; | ||
| use miden_protocol::account::AccountId; | ||
| use miden_protocol::block::BlockNumber; | ||
| use miden_protocol::crypto::merkle::MerkleError; | ||
| pub use miden_protocol::errors::{AccountError, AccountIdError, AssetError, NetworkIdError}; | ||
| use miden_protocol::errors::{ | ||
|
|
@@ -13,6 +15,7 @@ use miden_protocol::errors::{ | |
| TransactionScriptError, | ||
| }; | ||
| use miden_protocol::note::{NoteId, NoteTag}; | ||
| use miden_protocol::transaction::TransactionId; | ||
| use miden_standards::account::interface::AccountInterfaceError; | ||
| // RE-EXPORTS | ||
| // ================================================================================================ | ||
|
|
@@ -165,6 +168,18 @@ pub enum ClientError { | |
| #[source] | ||
| source: RpcError, | ||
| }, | ||
| #[error( | ||
| "transaction {tx_id} was submitted to the network at block {submission_height} but \ | ||
| apply_transaction failed when writing local state. The on-chain effect is durable; the \ | ||
| next successful sync will reconcile note states via ConsumedExternal. Do NOT retry the \ | ||
| same transaction." | ||
| )] | ||
|
Comment on lines
+169
to
+177
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should avoid giving out too much internal details, but also they should be more correct: really, the nullifiers are not the only problem here. In fact, there may be no nullifiers involved in the transaction at all. Rather, we should say that if the original transaction was accepted by the mempool and has not expired (and/or was finalized in a block), a duplicate submission would fail because the state of the account (and/or the network's state) was already mutated. |
||
| ApplyTransactionAfterSubmitFailed { | ||
| tx_id: TransactionId, | ||
| submission_height: BlockNumber, | ||
|
WiktorStarczewski marked this conversation as resolved.
Outdated
|
||
| #[source] | ||
| source: Box<ClientError>, | ||
| }, | ||
| } | ||
|
|
||
| // CONVERSIONS | ||
|
|
@@ -228,6 +243,19 @@ impl From<&ClientError> for Option<ErrorHint> { | |
| or provide the seed when importing.".to_string(), | ||
| docs_url: Some(TROUBLESHOOTING_DOC), | ||
| }), | ||
| ClientError::ApplyTransactionAfterSubmitFailed { tx_id, submission_height, .. } => { | ||
| Some(ErrorHint { | ||
| message: format!( | ||
| "Transaction {tx_id} was submitted to the network at block \ | ||
| {submission_height} but the local apply step (which writes the new \ | ||
| note states and account commitment to the store) failed. The on-chain \ | ||
| effect is permanent — do NOT resubmit. Run `sync` to reconcile local \ | ||
| state; input notes consumed by this transaction will be detected via \ | ||
| their nullifiers and transitioned to ConsumedExternal automatically." | ||
| ), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above: this is not fully correct
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 8c30fab — the hint message no longer promises sync reconciliation or "on-chain permanence." It now:
This avoids both the finality overstatement and the private-account issue you flagged. |
||
| docs_url: Some(TROUBLESHOOTING_DOC), | ||
| }) | ||
| }, | ||
| _ => None, | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -226,7 +226,34 @@ where | |
| let submission_height = | ||
| self.submit_proven_transaction(proven_transaction, &tx_result).await?; | ||
|
|
||
| self.apply_transaction(&tx_result, submission_height).await?; | ||
| // From this point on, the transaction is live on the network: the | ||
| // sender's account state has changed and the input note | ||
| // nullifiers will be recorded in the nullifier SMT. If | ||
| // apply_transaction fails (e.g. transient IDB write error), the | ||
| // local state disagrees with chain until the next sync | ||
| // reconciles it. | ||
| // | ||
| // Try apply once more before surfacing a distinct error that | ||
| // tells the caller "the tx landed on chain, don't retry it." | ||
| // A small subset of apply failures (IDB quota spike, connection | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, IndexedDB. Per-origin quotas in browsers are split into a soft and hard limit:
The writes we've seen fail weren't about hitting the quota — they were transient failures where the next write in the same session succeeded (service worker killed mid-transaction, the OS paging out IDB under pressure, the wallet's stress suite doing a page reload mid-commit, etc.). Hence the single in-memory retry before we surface the error: cheap, resolves the transient class, and doesn't paper over genuinely full quotas. Configuring the quota isn't really a lever we have on the web (it's browser policy, not our storage layer). Applying retry at the store level is reasonable for idempotent single-row writes, but the apply step is multi-table (accounts + notes + tags + future notes + tx record) and the right retry boundary is the whole |
||
| // reset on the store worker) clear immediately. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not fully correct: the sync may not reconcile state if the account is private, so we should avoid suggesting this. Additionally, the transaction might have reached the network at this point but this tells us nothing about whether the new state of notes and accounts will actually get finalized, so I wouldn't directly suggest that either.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also nit (feel free to disregard): the comment is a bit verbose. I'd go with something like // The transaction is already on-chain at this point. If
// `apply_transaction` fails, local state may diverge from the network's state.
//
// Retry once before returning an error that makes clear the transaction
// was submitted, since some failures may be transient.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 8c30fab — same response as on the hint thread below. The comment and hint text now only claim "the node accepted it into the mempool" rather than "it's on-chain and will reconcile." For private accounts specifically, sync cannot restore state from the network at all, so the pending
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 8c30fab — shortened the comment to 4 lines, in the spirit of your version but also updated to match the new flow (build the update once, retry the write, attach the pending update on failure). |
||
| if let Err(first_err) = self.apply_transaction(&tx_result, submission_height).await { | ||
| info!("apply_transaction failed once; retrying to cover transient errors"); | ||
| if let Err(second_err) = | ||
| self.apply_transaction(&tx_result, submission_height).await | ||
| { | ||
| info!( | ||
| "apply_transaction failed twice for submitted tx {tx_id}; surfacing \ | ||
| ApplyTransactionAfterSubmitFailed so the caller can rely on the next \ | ||
| sync to reconcile local state. First error: {first_err}" | ||
| ); | ||
| return Err(ClientError::ApplyTransactionAfterSubmitFailed { | ||
| tx_id, | ||
| submission_height, | ||
| source: Box::new(second_err), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| Ok(tx_id) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, the on-chain effect may indeed not be durable, and also the sync may reconcile notes but will make private accounts corrupt because their state cannot be retrieved from the network
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right on both points. Pushed 8c30fab which:
Instead the variant now attaches the pending
TransactionStoreUpdate, so the caller has a concrete recovery path independent of account privacy: persist it, re-apply later viaapply_transaction_update. That's the middle ground you suggested in the top-level review.