This repository has been archived by the owner on Sep 23, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 13
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
goran-ethernal
force-pushed
the
CDK-142-nonce-hole-created
branch
from
March 28, 2024 12:21
2e3b47c
to
9e1521c
Compare
rachit77
approved these changes
Apr 3, 2024
vcastellm
reviewed
Apr 4, 2024
Quality Gate passedIssues Measures |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR copies
ethTxManager
implementation fromzkevm-node
toagglayer
and implements custom logic theagglayer
needs.Since
zkevm
team does not want changes done to the originalethTxManager
, the goal of this PR is to copy all the logic from it (including the fix from PR: 0xPolygonHermez/zkevm-node#3338 regarding the nonce hole issue), and add an extra feature of dropping monitored transactions that fail for a configured number of times. It also removed all the functions thatagglayer
does not use fromethTxManager
, likeReorg
.We also needed to copy
pgstorage.go
since we can not use the one fromzkevm-node
since it uses the non-exposedmonitoredTx
struct.Issue that we faced
During testing of the agglayer on testnet, an issue popped up where a transaction was repeated indefinitely even though for some reason, it constantly failed to be executed or it failed to estimate gas. This caused unnecessary traffic, load and logs, since that transaction was never going to be executed successfully, since it failed, 100s of times.
Fix
This PR introduces a new configuration parameter to the
EthTxManager
configuration onagglayer
calledMaxRetries
. It represents a maximum number a transaction will be resent to the L1, or a maximum number of times it will try to estimate its gas until it drops it (updates the monitored transaction with theMonitoredTxStatusFailed
status in theagglayer-db
), meaning, it will not try to resend it again.In order to know how many times a transaction was tried to be resent or estimated, a new column to the
state.monitored_txs
table needed to be added, callednum_retries
. That is why, a new migration file was added0002.sql
which alters given table, by adding the new column to it.Tests
This PR copied the original
ethTxManager
tests, as well as tests for its postgress storage. Tests added:monitoredtx_test.go
- this is the same as in originalethTxManager
, no need to review it.pgstorage_test.go
- this is the same as in originalethTxManager
, no need to review it.txmanager_test.go
- this is the same as in originalethTxManager
. Only need to review testTestTxRetry_MaxRetriesReached
which tests the new logic.How to review this PR
In order not to waste time reviewing the copy/pasted code this is what should be considered for reviewing:
main.go
config.go
default.go
0002.sql
agglayer.toml
executor_test.go
executor.go
rpc_test.go
pgstorage.go
- only changes regarding thenum_retries
column.txmanager.go
- parts of code wheremtx.NumRetries
andcfg.MaxRetries
are mentioned. Other parts of code were left the same.txmanager_test.go
- only testTestTxRetry_MaxRetriesReached
, other tests are just moved from originalethTxManager
.-
types/interfaces.go
Make file