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

feat: handle l1 rpc error #854

Open
wants to merge 8 commits into
base: l1sload
Choose a base branch
from
Open

Conversation

NazariiDenha
Copy link

@NazariiDenha NazariiDenha commented Jun 25, 2024

1. Purpose or design rationale of this PR

Handle Rpc Error as documented here https://hackmd.io/@haichen/l1sload#Errors

Do not treat L1RpcError as an execution error, but instead retry the request and not remove tx from TxPool

2. PR 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:

  • build: Changes that affect the build system or external dependencies (example scopes: yarn, eslint, typescript)
  • ci: Changes to our CI configuration files and scripts (example scopes: vercel, github, cypress)
  • docs: Documentation-only changes
  • feat: A new feature
  • fix: A bug fix
  • perf: A code change that improves performance
  • refactor: A code change that doesn't fix a bug, or add a feature, or improves performance
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
  • test: Adding missing tests or correcting existing tests

3. Deployment tag versioning

Has the version in params/version.go been updated?

  • This PR doesn't involve a new deployment, git tag, docker image tag, and it doesn't affect traces
  • Yes

4. Breaking change label

Does this PR have the breaking-change label?

  • This PR is not a breaking change
  • Yes

miner/worker.go Show resolved Hide resolved
miner/worker.go Show resolved Hide resolved
miner/worker.go Outdated Show resolved Hide resolved
miner/worker.go Show resolved Hide resolved
core/state_transition.go Show resolved Hide resolved
@NazariiDenha NazariiDenha force-pushed the l1sload-error-handle branch from 638193a to d3adddf Compare July 22, 2024 08:56
@NazariiDenha NazariiDenha force-pushed the l1sload-error-handle branch from 6be1f88 to 712fd3b Compare July 22, 2024 09:33
@NazariiDenha NazariiDenha force-pushed the l1sload-error-handle branch from 5d042e2 to edeb6d9 Compare July 22, 2024 11:06
@@ -1179,6 +1181,9 @@ func (c *l1sload) RequiredGas(input []byte) uint64 {
}

func (c *l1sload) Run(state StateDB, input []byte) ([]byte, error) {
log.Info("l1sload", "input", input)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.Info("l1sload", "input", input)
log.Debug("l1sload", "input", input)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional to set log.Info here? btw, can print block number here for easier searching of the specific tx containing l1sload in other tools like scrollscan.

}
// wait before retrying
time.Sleep(100 * time.Millisecond)
log.Warn("L1 client request error", "err", err)
Copy link
Member

@colinlyguo colinlyguo Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can add address, keys, block in the log for easier debugging. e.g. if one needs to replay the rpc failure by sending through json rpc.

for i := 0; i < l1ClientMaxRetries; i++ {
res, err := c.l1Client.StoragesAt(context.Background(), address, keys, block)
if err != nil {
innerErr = err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about printing the number of i-th tries, address, keys, block and error messages here? because during the middle of the for-loop, the error will be simply omitted, it's not worse recording them somehow at first.

res, err := c.l1Client.StoragesAt(context.Background(), address, keys, block)
if err != nil {
return nil, &ErrL1RPCError{err: err}
// if caller type is non-worker then we can retry request multiple times and return err, the tx will be reinserted in tx poll
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// if caller type is non-worker then we can retry request multiple times and return err, the tx will be reinserted in tx poll
// if caller type is worker then we can retry request multiple times and return err, the tx will be reinserted in tx poll

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and use if c.callerType == CallerTypeWorker first to align with this comment.

return res, nil
}
// wait before retrying
time.Sleep(100 * time.Millisecond)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do exponential backoff?

}
// wait before retrying
time.Sleep(100 * time.Millisecond)
log.Warn("L1 client request error", "err", err)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.Warn("L1 client request error", "err", err)
log.Warn("failed to perform L1Sload RPC call", "err", err)

} else {
var innerErr error
for i := 0; i < l1ClientMaxRetries; i++ {
res, err := c.l1Client.StoragesAt(context.Background(), address, keys, block)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this call have a built-in timeout mechanism? Or is it possible that it would hang indefinitely?

@@ -1168,6 +1174,12 @@ loop:
log.Trace("Skipping account with hight nonce", "sender", from, "nonce", tx.Nonce())
txs.Pop()

case errors.As(err, &errL1):
// Skip the current transaction failed on L1Sload precompile with L1RpcError without shifting in the next from the account, this tx will be left in txpool and retried in future block
log.Trace("Skipping transaction failed on L1Sload precompile with L1RpcError", "sender", from)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log error

case errors.As(err, &errL1):
// Skip the current transaction failed on L1Sload precompile with L1RpcError without shifting in the next from the account, this tx will be left in txpool and retried in future block
log.Trace("Skipping transaction failed on L1Sload precompile with L1RpcError", "sender", from)
atomic.AddInt32(&w.newTxs, int32(1))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants