feat(relayer): adapt to CodecV7 for EuclidV2 #1583
Conversation
…submit multiple batches in a single transaction
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…ntextID logic to support multiple batches by concatenating batch hashes
| return | ||
| } | ||
|
|
||
| dbParentBatch, err = r.batchOrm.GetBatchByIndex(r.ctx, dbBatch.Index-1) |
There was a problem hiding this comment.
Most of these batches are already in memory (dbBatches).
There was a problem hiding this comment.
changed to use dbBatches except for i==0 in 5a479c3
|
|
||
| r.metrics.rollupL2RelayerCommitBlockHeight.Set(float64(maxBlockHeight)) | ||
| r.metrics.rollupL2RelayerCommitThroughput.Add(float64(totalGasUsed)) | ||
| r.metrics.rollupL2RelayerProcessPendingBatchSuccessTotal.Add(float64(len(batchesToSubmit))) |
There was a problem hiding this comment.
Might also want to monitor the time series of len(batchesToSubmit) (to see average submission).
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## omerfirmak/euclid-prover #1583 +/- ##
============================================================
- Coverage 42.15% 41.74% -0.41%
============================================================
Files 222 222
Lines 17733 17955 +222
============================================================
+ Hits 7475 7495 +20
- Misses 9550 9735 +185
- Partials 708 725 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jonastheis
force-pushed
the
feat/use-codec-v6
branch
3 times, most recently
from
f277483 to
e324f2a
Compare
jonastheis
force-pushed
the
feat/use-codec-v6
branch
from
e324f2a to
f4e17bc
Compare
| @@ -378,11 +378,194 @@ func (r *Layer2Relayer) ProcessGasPriceOracle() { | |||
| // ProcessPendingBatches processes the pending batches by sending commitBatch transactions to layer 1. | |||
| func (r *Layer2Relayer) ProcessPendingBatches() { | |||
| // get pending batches from database in ascending order by their index. | |||
| dbBatches, err := r.batchOrm.GetFailedAndPendingBatches(r.ctx, 5) | |||
| dbBatches, err := r.batchOrm.GetFailedAndPendingBatches(r.ctx, max(5, r.cfg.SenderConfig.BatchSubmission.MaxBatches)) | |||
There was a problem hiding this comment.
Should we remove 5, and use config value only?
There was a problem hiding this comment.
done in fbc14ac. also added some sanity checks on startup to make sure it is always set to at least 1
jonastheis
force-pushed
the
feat/use-codec-v6
branch
3 times, most recently
from
a9908cf to
be7dfa7
Compare
jonastheis
force-pushed
the
feat/use-codec-v6
branch
from
be7dfa7 to
69a80d4
Compare
| } | ||
| } | ||
|
|
||
| if dbBatch.Index == 0 { |
There was a problem hiding this comment.
when we build a new network (eg: devnet by CodecV7), I think this will make the ProcessPendingBatches doesn't work
There was a problem hiding this comment.
it's the same check that is applied for < CodecV7. So I assume it would be fine.
Also my understanding is that batch 0 is committed in
.| } | ||
| } | ||
|
|
||
| if !forceSubmit && len(batchesToSubmit) < r.cfg.SenderConfig.BatchSubmission.MinBatches { |
There was a problem hiding this comment.
Why here is MinBatches, I think Minbatches doesn't participate in the sanity check
There was a problem hiding this comment.
it's checking whether we have enough batches to be committed. we only submit batches if we have a timeout or if we have enough batches (MinBatches) to submit. added a comment in the code as well.
| case encoding.CodecV7: | ||
| calldata, blobs, maxBlockHeight, totalGasUsed, err = r.constructCommitBatchPayloadCodecV7(batchesToSubmit) | ||
| if err != nil { | ||
| log.Error("failed to construct commitBatchWithBlobProof payload for V7", "codecVersion", codecVersion, "start index", firstBatch.Index, "end index", lastBatch.Index, "err", err) | ||
| return | ||
| } | ||
| default: | ||
| log.Error("unsupported codec version in ProcessPendingBatches", "codecVersion", codecVersion, "start index", firstBatch, "end index", lastBatch.Index) | ||
| return | ||
| } |
There was a problem hiding this comment.
I think don't need this check. the above checks have ensured here is the CodecV7
There was a problem hiding this comment.
above we check that the codec is not smaller than CodecV7 but it doesn't check the exact version. that's why I would like to keep it
…nd renaming to prev and post L1MessageQueueHash
jonastheis
changed the base branch from
omerfirmak/mpt
to
omerfirmak/euclid-prover
…o feat/use-codec-v6
omerfirmak
force-pushed
the
omerfirmak/euclid-prover
branch
2 times, most recently
from
ed93b96 to
e27ab5a
Compare
Purpose or design rationale of this PR
This PR implements changes according to
CodecV7forEuclidV2. Depends on scroll-tech/da-codec#33CodecV7minandmaxbatches in combination withtimeoutPR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?