Skip to content
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

Retry failed transactions and adjust the cached nonce #473

Open
iFrostizz opened this issue Sep 4, 2024 · 3 comments
Open

Retry failed transactions and adjust the cached nonce #473

iFrostizz opened this issue Sep 4, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@iFrostizz
Copy link
Contributor

iFrostizz commented Sep 4, 2024

Context

if err := c.client.SendTransaction(context.Background(), signedTx); err != nil {
c.logger.Error(
"Failed to send transaction",
zap.Error(err),
)
return common.Hash{}, err
}
c.logger.Info(
"Sent transaction",
zap.String("txID", signedTx.Hash().String()),
zap.Uint64("nonce", c.currentNonce),
)
c.currentNonce++

If the transaction sending returns an error, or if the transaction is dropped, the transaction does not appear do be sent again. It would happen though as pointed by @geoff-vball when restarting the node and it would catch up those transactions.

Description
Rather than treating a dropped transaction as fatal, we could add sent transactions to a pending queue and retry them in this scenario. If they are accepted (i.e. they produce a receipt), then they can be safely removed from the pending queue.

Expected behavior
If the transaction failed (and return an error) because of a revert, mark the transaction as "executed" and don't try to execute it again.
If it was dropped, these should have been placed in a queue and retried later while reverting the nonce or fetch it.

@iFrostizz iFrostizz added the bug Something isn't working label Sep 4, 2024
@geoff-vball
Copy link
Contributor

Just a note: c.currentNonce is not incremented in the error case.

@iFrostizz
Copy link
Contributor Author

Just a note: c.currentNonce is not incremented in the error case.

Oops, phrased this incorrectly, thanks! So one case that may trigger this code path is if the transaction was sent correctly but was dropped

@cam-schultz
Copy link
Collaborator

This is not a bug. This case is intentionally treated as fatal. The block that contained the message on the sending chain will not be considered processed if the tx on the destination is dropped, and will be re-processed on the next startup.

That said, gracefully handling dropped transactions would improve overall throughput, and could make a sizable difference for certain applications where the rate of dropped transactions is high, and end-to-end latency is important. I'm going to convert this ticket to a feature request, and edit the fields of the issue description accordingly.

@cam-schultz cam-schultz added enhancement New feature or request and removed bug Something isn't working labels Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Backlog 🗄️
Development

No branches or pull requests

3 participants