feat: handle l1 rpc error #854

NazariiDenha · 2024-06-25T11:57:47Z

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

core/state_transition.go

…load-error-handle

…d test

colinlyguo · 2024-07-25T09:23:49Z

core/vm/contracts.go

@@ -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)


Suggested change

log.Info("l1sload", "input", input)

log.Debug("l1sload", "input", input)

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.

colinlyguo · 2024-07-25T09:42:32Z

core/vm/contracts.go

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


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.

colinlyguo · 2024-07-25T09:43:50Z

core/vm/contracts.go

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


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.

colinlyguo · 2024-07-25T09:46:32Z

core/vm/contracts.go

-	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


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

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

Thegaram · 2024-07-25T12:49:46Z

core/vm/contracts.go

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


Should we do exponential backoff?

Thegaram · 2024-07-25T12:50:41Z

core/vm/contracts.go

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


Suggested change

log.Warn("L1 client request error", "err", err)

log.Warn("failed to perform L1Sload RPC call", "err", err)

Thegaram · 2024-07-25T12:51:37Z

core/vm/contracts.go

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


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

Thegaram · 2024-07-25T12:54:18Z

miner/worker.go

@@ -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)


Thegaram · 2024-07-25T12:54:32Z

miner/worker.go

+		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))


Why is this necessary?

handle l1 rpc error

a306ca2

Thegaram reviewed Jun 25, 2024

View reviewed changes

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 added 2 commits July 15, 2024 10:46

Merge branch 'l1sload' of github.com:scroll-tech/go-ethereum into l1s…

aac7d47

…load-error-handle

add flag to precompile to determine whether it called form worker, ad…

d3adddf

…d test

NazariiDenha force-pushed the l1sload-error-handle branch from 638193a to d3adddf Compare July 22, 2024 08:56

NazariiDenha added 2 commits July 22, 2024 11:20

comment test

8f10e58

trigger worker test

712fd3b

NazariiDenha force-pushed the l1sload-error-handle branch from 6be1f88 to 712fd3b Compare July 22, 2024 09:33

NazariiDenha added 3 commits July 22, 2024 11:39

test

11ad398

fix comment

c28fe4b

handle err in tracing

edeb6d9

NazariiDenha force-pushed the l1sload-error-handle branch from 5d042e2 to edeb6d9 Compare July 22, 2024 11:06

colinlyguo reviewed Jul 25, 2024

View reviewed changes

Thegaram reviewed Jul 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: handle l1 rpc error #854

feat: handle l1 rpc error #854

NazariiDenha commented Jun 25, 2024 •

edited by Thegaram

Loading

colinlyguo Jul 25, 2024

colinlyguo Jul 25, 2024

colinlyguo Jul 25, 2024 •

edited

Loading

colinlyguo Jul 25, 2024

colinlyguo Jul 25, 2024

colinlyguo Jul 25, 2024

Thegaram Jul 25, 2024

Thegaram Jul 25, 2024

Thegaram Jul 25, 2024

Thegaram Jul 25, 2024

Thegaram Jul 25, 2024

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

	// 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

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

feat: handle l1 rpc error #854

Are you sure you want to change the base?

feat: handle l1 rpc error #854

Conversation

NazariiDenha commented Jun 25, 2024 • edited by Thegaram Loading

1. Purpose or design rationale of this PR

2. PR title

3. Deployment tag versioning

4. Breaking change label

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinlyguo Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NazariiDenha commented Jun 25, 2024 •

edited by Thegaram

Loading

colinlyguo Jul 25, 2024 •

edited

Loading