Skip to content

go/proxyd: rate limiting and WS support#1679

Merged
tynes merged 1 commit intoethereum-optimism:developfrom
mslipper:feat/proxyd-ws
Nov 4, 2021
Merged

go/proxyd: rate limiting and WS support#1679
tynes merged 1 commit intoethereum-optimism:developfrom
mslipper:feat/proxyd-ws

Conversation

@mslipper
Copy link
Collaborator

@mslipper mslipper commented Nov 3, 2021

Fixes ENG-1612
Fixes ENG-1626

@changeset-bot
Copy link

changeset-bot bot commented Nov 3, 2021

🦋 Changeset detected

Latest commit: 6c50098

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@eth-optimism/proxyd Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codecov-commenter
Copy link

codecov-commenter commented Nov 3, 2021

Codecov Report

Merging #1679 (eff7dff) into develop (e70e762) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1679   +/-   ##
========================================
  Coverage    76.52%   76.52%           
========================================
  Files           82       82           
  Lines         3041     3041           
  Branches       466      466           
========================================
  Hits          2327     2327           
  Misses         714      714           
Flag Coverage Δ
batch-submitter 61.74% <ø> (ø)
contracts 86.05% <ø> (ø)
core-utils 63.27% <ø> (ø)
data-transport-layer 37.86% <ø> (ø)
message-relayer 83.48% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 e70e762...eff7dff. Read the comment docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

The iteration order for the backends is non deterministic, so is it possible for a single user to send 2 requests and have them go to different backends? Same with a user who is both using websockets and JSON RPC from the same app - an example would be subscribing to the tip and then querying for transaction data on each new tip

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Backend iteration order is deterministic. The backend would have to be marked offline for the case you are describing to happen. Essentially:

  • User is connected via WS. Backend goes down, but stays connected via WS.
  • User makes request via RPC. Request gets routed to the next online backend.
  • Alternatively, a user makes two requests and the backend goes down in between requests 1 and 2.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to handle the ErrMethodNotWhitelisted case as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does - see the implementation of WSProxier. We can't capture that error here because the WS connection happens before we start receiving messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is not closing this channel a memory leak?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope! We actually don't want to close this channel. It's got two buffer slots so that closing either the client of backend end of the connection will close the other. If we close it, it'll cause the application to panic when the side that didn't close shuts down.

@tynes
Copy link
Contributor

tynes commented Nov 3, 2021

Generally looks good, left some comments. Do you mind adding a changeset?

@tynes tynes changed the title go/proxyd: ENG-1612 ENG-1626 rate limiting and WS support go/proxyd: rate limiting and WS support Nov 3, 2021
@tynes
Copy link
Contributor

tynes commented Nov 3, 2021

I modified the name of the PR and moved the linear issue numbers to the description. Linear will link the PR to the issue if you put Fixes <isssue number> in the description

Copy link
Contributor

@tynes tynes left a comment

Choose a reason for hiding this comment

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

Looks good to me, will merge after a squash + nice commit message

@tynes
Copy link
Contributor

tynes commented Nov 4, 2021

#1692 adds the http header User-Agent so this should be able to have insight on where the requests are coming from now when it is used as the internal proxy

@tynes tynes merged commit 8f39b07 into ethereum-optimism:develop Nov 4, 2021
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