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

metrics: HTTP status codes and inconsistent responses #126

Merged
merged 16 commits into from
Jan 18, 2024
Merged

Conversation

rvanasa
Copy link
Collaborator

@rvanasa rvanasa commented Jan 10, 2024

Adds the following metrics:

  • responses now records the HTTP status codes for JSON-RPC responses.
  • inconsistent_responses counts the number of MultiRpcResult::Inconsistent(..) results for each combination of RPC method and hostname.

Another notable change (not represented in the diff of this PR) is that I adjusted the implementation of MultiCallResult::all_ok() in the forked ckETH codebase to return all results in the case of an error. Otherwise, an RPC service could bypass the agreement / consistency logic by returning an error for an otherwise successful RPC request.

@rvanasa rvanasa changed the title metrics: rate limit status codes and inconsistent responses metrics: HTTP status codes and inconsistent responses Jan 11, 2024
@rvanasa rvanasa marked this pull request as ready for review January 12, 2024 20:45
@THLO
Copy link
Contributor

THLO commented Jan 18, 2024

an RPC service could bypass consensus

What do you mean by that? Every response to an HTTPS outcall goes through consensus, so there is no way to "bypass consensus".

@rvanasa
Copy link
Collaborator Author

rvanasa commented Jan 18, 2024

What do you mean by that?

Oh right, sorry for the ambiguity; I was using "consensus" to refer to the agreement logic between RPC providers.

In ckETH, if two responses are successful while one returns an error, this is considered a "consistent error" (rather than three "inconsistent results"). We need to change this for EVM RPC because a compromised third-party RPC service could bypass the agreement logic by responding with an error for an otherwise successful request.

@rvanasa rvanasa merged commit 0b09134 into main Jan 18, 2024
3 checks passed
@rvanasa rvanasa deleted the rate-limit-metrics branch January 18, 2024 19:29
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.

2 participants