Multicall rpc error check #64

cody-wang-cb · 2024-10-17T07:02:09Z

Description

Add bool to have an option that for multiple success responses from multicall, choose the one that doesn't have rpc error.

Tests

Added a unit test

Additional context

We observed that there are cases when different execution clients gave different responses, one with rpc success and one with rpc error, if the one with a rpc error returned first it'd return that. However in such case the rpc success one should be returned back to the user

Metadata

Fixes #[Link to Issue]

cody-wang-cb · 2024-10-17T15:33:30Z

@jelias2 @danyalprout PTAL, will add unit tests if this approach makes sense

danyalprout · 2024-10-17T15:43:59Z

proxyd/backend.go

@@ -896,9 +897,11 @@ func (bg *BackendGroup) MulticallRequest(backend *Backend, rpcReqs []*RPCReq, wg
 	}
 }

-func (bg *BackendGroup) ProcessMulticallResponses(ch chan *multicallTuple, ctx context.Context) *BackendGroupRPCResponse {
-	var finalResp *BackendGroupRPCResponse
+func (bg *BackendGroup) ProcessMulticallResponses(ch chan *multicallTuple, ctx context.Context, multicallRPCErrorCheck bool) *BackendGroupRPCResponse {


Isn't multicallRPCErrorCheck a property on bg? Probably don't need to pass it through

yup make sense

cody-wang-cb · 2024-10-17T20:51:28Z

proxyd/backend.go

+			// check if any of the response is an RPC error
+			var rpcError = false
+			for _, rpcRes := range resp.RPCRes {
+				// check if the ID is already in the success response


I think ID should be unique given it's part of the handleBatchRPC flow. It's either the incoming message is a batch request (which should have unique IDs otherwise they wouldn't know how to process the response), or a single request.
@jelias2 would be good if you could help to confirm.
cc: @danyalprout

Theres one key thing is that currently we don't allow batch requests in multicall at this point, so all IDs will be most likely 1

if bg.GetRoutingStrategy() == MulticallRoutingStrategy && isValidMulticallTx(rpcReqs) && !isBatch { backendResp := bg.ExecuteMulticall(ctx, rpcReqs) return backendResp.RPCRes, backendResp.ServedBy, backendResp.error }

cody-wang-cb · 2024-10-17T20:51:37Z

proxyd/backend.go

@@ -896,9 +897,11 @@ func (bg *BackendGroup) MulticallRequest(backend *Backend, rpcReqs []*RPCReq, wg
 	}
 }

-func (bg *BackendGroup) ProcessMulticallResponses(ch chan *multicallTuple, ctx context.Context) *BackendGroupRPCResponse {
-	var finalResp *BackendGroupRPCResponse
+func (bg *BackendGroup) ProcessMulticallResponses(ch chan *multicallTuple, ctx context.Context, multicallRPCErrorCheck bool) *BackendGroupRPCResponse {


yup make sense

jelias2 · 2024-10-17T21:24:56Z

proxyd/backend.go

+				// add rpc error responses to success responses
+				for id, rpcRes := range rpcResToCheck {
+					if _, ok := successResp[id]; !ok {
+						successResp[id] = rpcRes
+					}
+				}


Whats the goal here? Is it to add errored responses to the successful responses map that match the same ID?

If all the requests have a similar ID of 1, it may not work as intended

Oh I see isValidMulticallTx() now.
I think that should work then if it's just 1 request each time, it would just only go through the loop once per check.

jelias2

Just curious on the desired outcome/behvaior of this feature,

Given a request unique ID, does it intend to return the a success if it exists, otherwise it will return the error response.

If there is an error and a success for the same ID, will it overwrite the error with the success?

jelias2 · 2024-10-17T21:47:59Z

proxyd/backend.go

@@ -896,9 +897,11 @@ func (bg *BackendGroup) MulticallRequest(backend *Backend, rpcReqs []*RPCReq, wg
 	}
 }

-func (bg *BackendGroup) ProcessMulticallResponses(ch chan *multicallTuple, ctx context.Context) *BackendGroupRPCResponse {
-	var finalResp *BackendGroupRPCResponse
+func (bg *BackendGroup) ProcessMulticallResponses(ch chan *multicallTuple, ctx context.Context, multicallRPCErrorCheck bool) *BackendGroupRPCResponse {


Thoughts on making ProcessMulticallResponses and ProcessMulticallResponsesWithErrors two different functions?

cody-wang-cb · 2024-10-17T22:07:51Z

@jelias2 sorry I should've made the PR description more clear.

The motivation is that because we multicall sendRawTransaction to 2 different mempool nodes (geth + reth), we noticed sometimes one node would return an RPC error while the other return success with no error.

However at proxyd level, an RPC error is not an actual error since it's still 200. This gives the end user a bad experience as we've gotten reports saying that they received an RPC error but the TX actually went through.

Let me know if the motivation makes sense

jelias2 · 2024-10-18T15:51:00Z

So with this new design, what do you wish to return to the user if there is one success and one failure for the same request?

If its all failures or all success, just return the appropriate success/failure

Overall no qualms about the implementation, I would lean placing the processErrorMulticall in a separate function and clearly document the what the behavior is for the same request returned with different results

cody-wang-cb · 2024-10-22T14:28:24Z

So with this new design, what do you wish to return to the user if there is one success and one failure for the same request?

If its all failures or all success, just return the appropriate success/failure

Overall no qualms about the implementation, I would lean placing the processErrorMulticall in a separate function and clearly document the what the behavior is for the same request returned with different results

if one success and one failure it'd return the success.
Sounds good let me update it as a separate function

cody-wang-cb · 2024-11-05T09:58:58Z

proxyd/backend.go

@@ -931,6 +932,13 @@ func (bg *BackendGroup) ProcessMulticallResponses(ch chan *multicallTuple, ctx c
 			finalResp = resp
 			continue
 		}
+
+		// Assuming multicall doesn't support batch
+		if bg.multicallRPCErrorCheck && resp.RPCRes[0].IsError() {


@jelias2 is this ok? Assuming multicall doesn't support batch

Correct multi-call assumes single rpc request, no batches allowed

jelias2 · 2024-11-14T04:10:11Z

proxyd/v4.10.0 includes the new changes

multicall rpc error check

76a8bdc

cody-wang-cb requested a review from a team as a code owner October 17, 2024 07:02

cody-wang-cb requested a review from Inphi October 17, 2024 07:02

cody-wang-cb marked this pull request as draft October 17, 2024 07:02

danyalprout reviewed Oct 17, 2024

View reviewed changes

cody-wang-cb commented Oct 17, 2024

View reviewed changes

jelias2 reviewed Oct 17, 2024

View reviewed changes

cody-wang-cb added 2 commits October 28, 2024 20:38

wip

92b4054

add test and fix logic

f1bb860

cody-wang-cb commented Nov 5, 2024

View reviewed changes

cody-wang-cb requested a review from jelias2 November 5, 2024 22:05

merge from master

8cca894

cody-wang-cb marked this pull request as ready for review November 9, 2024 23:35

jelias2 approved these changes Nov 14, 2024

View reviewed changes

jelias2 merged commit 1b0bb87 into ethereum-optimism:main Nov 14, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multicall rpc error check #64

Multicall rpc error check #64

cody-wang-cb commented Oct 17, 2024 •

edited

Loading

cody-wang-cb commented Oct 17, 2024 •

edited

Loading

danyalprout Oct 17, 2024

cody-wang-cb Oct 17, 2024

cody-wang-cb Oct 17, 2024

jelias2 Oct 17, 2024

cody-wang-cb Oct 17, 2024

jelias2 Oct 17, 2024

cody-wang-cb Oct 17, 2024

jelias2 left a comment

jelias2 Oct 17, 2024

cody-wang-cb commented Oct 17, 2024

jelias2 commented Oct 18, 2024 •

edited

Loading

cody-wang-cb commented Oct 22, 2024

cody-wang-cb Nov 5, 2024

jelias2 Nov 14, 2024

jelias2 commented Nov 14, 2024 •

edited

Loading

Multicall rpc error check #64

Multicall rpc error check #64

Conversation

cody-wang-cb commented Oct 17, 2024 • edited Loading

cody-wang-cb commented Oct 17, 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

jelias2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cody-wang-cb commented Oct 17, 2024

jelias2 commented Oct 18, 2024 • edited Loading

cody-wang-cb commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelias2 commented Nov 14, 2024 • edited Loading

cody-wang-cb commented Oct 17, 2024 •

edited

Loading

cody-wang-cb commented Oct 17, 2024 •

edited

Loading

jelias2 commented Oct 18, 2024 •

edited

Loading

jelias2 commented Nov 14, 2024 •

edited

Loading