-
Notifications
You must be signed in to change notification settings - Fork 684
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
fix: fixing forced batches error handling and normal batches transaction level error handling. #2100
Conversation
9265c2b
to
f69f520
Compare
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.
What about the list of errors that increase the nonce??
@@ -399,7 +398,8 @@ func (f *finalizer) processTransaction(ctx context.Context, tx *TxTracker) error | |||
// handleTxProcessResp handles the response of transaction processing. | |||
func (f *finalizer) handleTxProcessResp(ctx context.Context, tx *TxTracker, result *state.ProcessBatchResponse, oldStateRoot common.Hash) error { | |||
// Handle Transaction Error | |||
if result.Responses[0].RomError != nil && !errors.Is(result.Responses[0].RomError, runtime.ErrExecutionReverted) { | |||
if !result.IsBatchProcessed { | |||
// If intrinsic error or OOC error, we skip adding the transaction to the batch | |||
f.handleTransactionError(ctx, result, tx) | |||
return result.Responses[0].RomError |
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.
Why returning thefirst response?
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.
Here we process 1 by 1 we always can have only 1 response
sequencer/finalizer.go
Outdated
if err != nil { | ||
return lastBatchNumberInState, stateRoot | ||
} | ||
_ = f.handleForcedTxsProcessResp(request, response, stateRoot) |
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.
why do we ignore the error?
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.
because nothing happens even if we have error I can just remove returning it
sequencer/finalizer.go
Outdated
romErr := executor.RomErrorCode(txResp.RomError) | ||
if txResp.RomError != nil { |
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.
romErr := executor.RomErrorCode(txResp.RomError) | |
if txResp.RomError != nil { | |
if txResp.RomError != nil { | |
romErr := executor.RomErrorCode(txResp.RomError) | |
sequencer/finalizer.go
Outdated
return txResp.RomError | ||
} else if executor.IsIntrinsicError(romErr) { | ||
// If we have an intrinsic error, we should continue processing the batch, but skip the transaction | ||
if txResp.RomError != nil { |
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 this is redundant
sequencer/finalizer.go
Outdated
log.Warnf("handleForcedBatchProcessResp: ROM out of counters error: %s", txResp.RomError) | ||
romErr := executor.RomErrorCode(txResp.RomError) | ||
if txResp.RomError != nil { | ||
if executor.IsROMOutOfCountersError(romErr) { |
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.
if the batch has an out of counters, no transaction should be added.
I thinks that here, if there is the error in the 4th transaction for example, the first 3 ones will be stored and they should not
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.
What about the list of errors that increase the nonce??
Those will be added. We don't skip them.
f69f520
to
f02c4b2
Compare
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.
😸
…ion level error handling. Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>
f02c4b2
to
96a6a5a
Compare
Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>
…hub.com/0xPolygonHermez/zkevm-node into feature/fix-forced-batches-err-handling
Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>
Closes #2099.
What does this PR do?
Fixing:
error handling
forForced Batches
.transaction level error handling
forNormal Batches
.ErrIntrinsicInvalidBatchGasLimit
inRomErrorCode
function of the stateReviewers
Main reviewers:
Codeowner reviewers: