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

Add option for websocket message size limit #2266

Merged
merged 11 commits into from
May 21, 2024
Merged

Conversation

Tristan-Wilson
Copy link
Member

@Tristan-Wilson Tristan-Wilson commented Apr 30, 2024

closes NIT-2449

This plumbs through the websocket message size limit option for all rpc clients.

Testing done

Checked that this option is present for all clients and that the default is correctly set.

$ make target/bin/nitro
go build  -o target/bin/nitro "/home/tristan/offchain/nitro/cmd/nitro"
$ ./target/bin/nitro --help 2>&1 | grep websocket-message
      --node.block-validator.execution-server-config.websocket-message-size-limit int                          websocket message size limit used by the RPC client. 0 means no limit (default 33554432)
      --node.block-validator.validation-server.websocket-message-size-limit int                                websocket message size limit used by the RPC client. 0 means no limit (default 33554432)
      --parent-chain.connection.websocket-message-size-limit int                                               websocket message size limit used by the RPC client. 0 means no limit (default 33554432)

This plumbs through the websocket message size limit option for all
rpc clients.
@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Apr 30, 2024
@@ -56,6 +58,8 @@ var DefaultClientConfig = ClientConfig{
Retries: 3,
RetryErrors: "websocket: close.*|dial tcp .*|.*i/o timeout|.*connection reset by peer|.*connection refused",
ArgLogLimit: 2048,
// Use geth's unexported wsDefaultReadLimit from rpc/websocket.go
WebsocketMessageSizeLimit: 32 * 1024 * 1024,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's increase this to 256 MiB (for the other websocket clients as well). All of these websocket clients are expecting the server to be honest.

@@ -256,9 +260,9 @@ func (c *RpcClient) Start(ctx_in context.Context) error {
var err error
var client *rpc.Client
if jwt == nil {
client, err = rpc.DialContext(ctx, url)
client, err = rpc.DialOptions(ctx, url, rpc.WithWebsocketMessageSizeLimit(c.config().WebsocketMessageSizeLimit))
Copy link
Contributor

Choose a reason for hiding this comment

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

let's test here if WebsocketMessageSizeLimit == 0 (it can happen e.g. for validation-server-configs). If it's 0 either use default or don't apply rpc.WithWebsocketMessageSizeLimit

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@tsahee tsahee left a comment

Choose a reason for hiding this comment

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

LGTM

@Tristan-Wilson Tristan-Wilson merged commit 9375a4b into master May 21, 2024
10 checks passed
@Tristan-Wilson Tristan-Wilson deleted the wsMessageSizeLimit branch May 21, 2024 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants