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

Retry remote contract calls for unsupported block number errors #2563

Merged
merged 7 commits into from
Aug 10, 2022

Conversation

yondonfu
Copy link
Member

@yondonfu yondonfu commented Aug 9, 2022

What does this pull request do? Explain your changes. (required)

We've occasionally observed an unsupported block number error for RPC requests which can cause problems for node operation (see #2560).

According to an Offchain Labs team member, an Arb node can return this error if the node is not synced. This situation could occur if:

  • A single Arb node has fallen behind the tip of the chain
  • If requests are load balanced across a set of nodes and request 1 fetches block number N from node 1, but request 2 that references block number N is routed to node 2 which has only seen block number N - 1

The unsupported block number error appears to originate from the getSnapshot() and getSnapshotForNumberForHash() functions in offchainlabs/arbitrum/packages/arb-rpc-node/web3 which are called by many of the functions implementing the ETH JSON-RPC API [1].

From reviewing Discord conversations, it appears that this error is more likely to happen when sending concurrent RPC requests.

The solution in this PR is to add the unsupported block number error to the list of errors that will be retried when sending requests for remote contract calls. Note: This change only applies to contract calls and not other RPC requests which could be affected by this error as well. Addressing this issue with all RPC requests requires additional code changes, but since this error is primarily affecting contract calls right now, I've opted to just address this for contract calls.

Specific updates (required)

See commit history.

How did you test each of these updates (required)

  • Added unit tests for helper functions
  • Rolled this change to a broadcaster and observed retries for unsupported block number errors

Does this pull request close any open issues?

Addresses the root cause of #2560 although it could still make sense to update the behavior of the node following the suggestion in the issue.

Checklist:

@codecov
Copy link

codecov bot commented Aug 9, 2022

Codecov Report

Merging #2563 (fa007c8) into master (f6948b8) will increase coverage by 0.09914%.
The diff coverage is 100.00000%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #2563         +/-   ##
===================================================
+ Coverage   54.71489%   54.81403%   +0.09914%     
===================================================
  Files             95          95                 
  Lines          19852       19869         +17     
===================================================
+ Hits           10862       10891         +29     
+ Misses          8385        8373         -12     
  Partials         605         605                 
Impacted Files Coverage Δ
eth/backend.go 50.35461% <100.00000%> (+16.48364%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6948b8...fa007c8. Read the comment docs.

Impacted Files Coverage Δ
eth/backend.go 50.35461% <100.00000%> (+16.48364%) ⬆️

@yondonfu yondonfu marked this pull request as ready for review August 9, 2022 17:07
Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

Added one comment. Other than that LGTM 👍

eth/backend.go Outdated Show resolved Hide resolved
@yondonfu yondonfu force-pushed the yf/retry-unsupported-block-num branch from a444bcc to fa007c8 Compare August 10, 2022 15:44
@yondonfu
Copy link
Member Author

Rebased to re-order commits.

@yondonfu yondonfu merged commit 3687c3e into master Aug 10, 2022
@yondonfu yondonfu deleted the yf/retry-unsupported-block-num branch August 10, 2022 16:18
Copy link
Contributor

@thomshutt thomshutt left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

3 participants