-
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 6 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 |
|---|---|---|
|
|
@@ -226,7 +226,32 @@ where | |
| let submission_height = | ||
| self.submit_proven_transaction(proven_transaction, &tx_result).await?; | ||
|
|
||
| self.apply_transaction(&tx_result, submission_height).await?; | ||
| // The transaction has been accepted by the node; the local store update | ||
| // is a separate step that can fail independently. Build the update once | ||
| // and retry the write once before surfacing a distinct error that | ||
| // carries the pending update for caller-driven recovery. | ||
| // | ||
| // The update is boxed so it does not inflate the enclosing future | ||
| // across await points (triggers clippy::large_futures). | ||
| let tx_update = | ||
| Box::new(self.get_transaction_store_update(&tx_result, submission_height).await?); | ||
|
|
||
| if let Err(first_err) = self.apply_transaction_update((*tx_update).clone()).await { | ||
| info!("apply_transaction_update failed once; retrying to cover transient errors"); | ||
| if let Err(second_err) = self.apply_transaction_update((*tx_update).clone()).await { | ||
| info!( | ||
| "apply_transaction_update failed twice for submitted tx {tx_id}; \ | ||
| returning ApplyTransactionAfterSubmitFailed with the pending update \ | ||
| attached. First error: {first_err}" | ||
| ); | ||
| return Err(ClientError::ApplyTransactionAfterSubmitFailed { | ||
| tx_id, | ||
| submission_height, | ||
| pending_update: tx_update, | ||
| source: Box::new(second_err), | ||
| }); | ||
| } | ||
|
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 I still disagree with trying to apply the update again. You mention:
Retrying once does not help with any of these, does it? I also don't think these are precisely the right problems to solve: Why do we need to solve for a problem related to the wallet's stress suite which decides to do a reload? There isn't much you can do if the process exits at any specific point (power goes out, user closes tab, OS evicts process). It feels a bit like cargo culting. Lastly, does retrying immediately after having failed a write make sense? Or should there be a small delay, etc? Because we'd need to make these general assumptions I'd rather just return the store update and let the user handle it. A wallet or app implementation should already have the tools to do something that makes sense for their environment, even without returning the new error variant (although I think this one is an improvement). I think a better approach could be to do 2 separate DB writes: one for a "pending insert" and then a "commit" one, but this probably requires a larger refactor
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.
|
||
| } | ||
|
|
||
| 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.
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.