rpc: add limit for batch request and response size #26681

mmsqe · 2023-02-14T01:00:43Z

This PR adds server-side limits for JSON-RPC batch requests. Before this change, batches
were limited only by processing time. The server would pick calls from the batch and
answer them until the response timeout occurred, then stop processing the remaining batch
items.

Here, we are adding two additional limits which can be configured:

the 'item limit': batches can have at most N items
the 'response size limit': batches can contain at most X response bytes

These limits are optional in package rpc. In Geth, we set a default limit of 1000 items
and 25MB response size.

When a batch goes over the limit, an error response is returned to the client. However,
doing this correctly isn't always possible. In JSON-RPC, only method calls with a valid
id can be responded to. Since batches may also contain non-call messages or
notifications, the best effort thing we can do to report an error with the batch itself is
reporting the limit violation as an error for the first method call in the batch. If a batch is
too large, but contains only notifications and responses, the error will be reported with
a null id.

The RPC client was also changed so it can deal with errors resulting from too large
batches. An older client connected to the server code in this PR could get stuck
until the request timeout occurred when the batch is too large. Upgrading to a version
of the RPC client containing this change is strongly recommended to avoid timeout issues.

For some weird reason, when writing the original client implementation, @fjl worked off of
the assumption that responses could be distributed across batches arbitrarily. So for a
batch request containing requests [A B C], the server could respond with [A B C] but
also with [A B] [C] or even [A] [B] [C] and it wouldn't make a difference to the
client.

So in the implementation of BatchCallContext, the client waited for all requests in the
batch individually. If the server didn't respond to some of the requests in the batch, the
client would eventually just time out (if a context was used).

With the addition of batch limits into the server, we anticipate that people will hit this
kind of error way more often. To handle this properly, the client now waits for a single
response batch and expects it to contain all responses to the requests.

fjl

This looks very good!

I think it would be better to make these limits configurable somehow. My suggestion for that would be a method like SetBatchLimits(length int, size int) on Server and Client.

rpc/errors.go

mmsqe · 2023-02-14T13:18:52Z

This looks very good!

I think it would be better to make these limits configurable somehow. My suggestion for that would be a method like SetBatchLimits(length int, size int) on Server and Client.

Since the limit make more sense to add in server side, I only move set batch limit related change in Server and keep Client unchanged.

fjl · 2023-02-14T13:22:17Z

The client can also accept requests from the server (and the server is implemented using Client) :)

mmsqe · 2023-02-15T15:04:25Z

The client can also accept requests from the server (and the server is implemented using Client) :)

May I ask how to add config to Client, which doesn't have access to this config right? And newClient would have breaking change by adding new params.

holiman · 2023-02-16T09:52:51Z

cmd/utils/flags.go

@@ -786,6 +786,18 @@ var (
 		Usage:    "Allow for unprotected (non EIP155 signed) transactions to be submitted via RPC",
 		Category: flags.APICategory,
 	}
+	BatchRequestLimit = &cli.IntFlag{
+		Name:     "batch.request-limit",


IMO these flags should be somwhere in the rpc. namespace

This changes how we handle batch responses in order to be able to react to situations where the server does not provide a response for every request batch element. For some weird reason, when writing the original client implementation, I worked off of the assumption that responses could be distributed across batches arbitrarily. So for a batch request containing requests [A, B, C], the server could respond with [A B C] but also with [A B] [C] or even [A] [B] [C] and it wouldn't make a difference to the client. So in the implementation of BatchCallContext, the client waited for all requests in the batch individually. If the server didn't respond to all requests in the batch, the client would eventually just time out. With the addition of batch limits into the server, I anticipate that people will hit this kind of error way more often. To handle this properly, the client now waits for a single response batch and expects it to contain all responses to the requests.

fjl · 2023-06-08T11:25:53Z

I was finally able to fix this up properly. With the latest changes, the client will now properly handle wrong size batch responses and return early without relying on the timeout.

In JSON-RPC, only method calls with a non-null "id" can be responded to. Since batches can contain non-call messages or notifications, the best effort thing we can do to report an error with the batch itself is the first method call message.

fjl · 2023-06-08T13:05:25Z

@holiman PTAL

I think adding these limits might break people's setups, so best to avoid configuring them by default in package rpc.

holiman

Still LGTM

@fjl

) This PR adds server-side limits for JSON-RPC batch requests. Before this change, batches were limited only by processing time. The server would pick calls from the batch and answer them until the response timeout occurred, then stop processing the remaining batch items. Here, we are adding two additional limits which can be configured: - the 'item limit': batches can have at most N items - the 'response size limit': batches can contain at most X response bytes These limits are optional in package rpc. In Geth, we set a default limit of 1000 items and 25MB response size. When a batch goes over the limit, an error response is returned to the client. However, doing this correctly isn't always possible. In JSON-RPC, only method calls with a valid `id` can be responded to. Since batches may also contain non-call messages or notifications, the best effort thing we can do to report an error with the batch itself is reporting the limit violation as an error for the first method call in the batch. If a batch is too large, but contains only notifications and responses, the error will be reported with a null `id`. The RPC client was also changed so it can deal with errors resulting from too large batches. An older client connected to the server code in this PR could get stuck until the request timeout occurred when the batch is too large. **Upgrading to a version of the RPC client containing this change is strongly recommended to avoid timeout issues.** For some weird reason, when writing the original client implementation, @fjl worked off of the assumption that responses could be distributed across batches arbitrarily. So for a batch request containing requests `[A B C]`, the server could respond with `[A B C]` but also with `[A B] [C]` or even `[A] [B] [C]` and it wouldn't make a difference to the client. So in the implementation of BatchCallContext, the client waited for all requests in the batch individually. If the server didn't respond to some of the requests in the batch, the client would eventually just time out (if a context was used). With the addition of batch limits into the server, we anticipate that people will hit this kind of error way more often. To handle this properly, the client now waits for a single response batch and expects it to contain all responses to the requests. --------- Co-authored-by: Felix Lange <fjl@twurst.com> Co-authored-by: Martin Holst Swende <martin@swende.se>

@fjl

) This PR adds server-side limits for JSON-RPC batch requests. Before this change, batches were limited only by processing time. The server would pick calls from the batch and answer them until the response timeout occurred, then stop processing the remaining batch items. Here, we are adding two additional limits which can be configured: - the 'item limit': batches can have at most N items - the 'response size limit': batches can contain at most X response bytes These limits are optional in package rpc. In Geth, we set a default limit of 1000 items and 25MB response size. When a batch goes over the limit, an error response is returned to the client. However, doing this correctly isn't always possible. In JSON-RPC, only method calls with a valid `id` can be responded to. Since batches may also contain non-call messages or notifications, the best effort thing we can do to report an error with the batch itself is reporting the limit violation as an error for the first method call in the batch. If a batch is too large, but contains only notifications and responses, the error will be reported with a null `id`. The RPC client was also changed so it can deal with errors resulting from too large batches. An older client connected to the server code in this PR could get stuck until the request timeout occurred when the batch is too large. **Upgrading to a version of the RPC client containing this change is strongly recommended to avoid timeout issues.** For some weird reason, when writing the original client implementation, @fjl worked off of the assumption that responses could be distributed across batches arbitrarily. So for a batch request containing requests `[A B C]`, the server could respond with `[A B C]` but also with `[A B] [C]` or even `[A] [B] [C]` and it wouldn't make a difference to the client. So in the implementation of BatchCallContext, the client waited for all requests in the batch individually. If the server didn't respond to some of the requests in the batch, the client would eventually just time out (if a context was used). With the addition of batch limits into the server, we anticipate that people will hit this kind of error way more often. To handle this properly, the client now waits for a single response batch and expects it to contain all responses to the requests. --------- Co-authored-by: Felix Lange <fjl@twurst.com> Co-authored-by: Martin Holst Swende <martin@swende.se>

@fjl

) This PR adds server-side limits for JSON-RPC batch requests. Before this change, batches were limited only by processing time. The server would pick calls from the batch and answer them until the response timeout occurred, then stop processing the remaining batch items. Here, we are adding two additional limits which can be configured: - the 'item limit': batches can have at most N items - the 'response size limit': batches can contain at most X response bytes These limits are optional in package rpc. In Geth, we set a default limit of 1000 items and 25MB response size. When a batch goes over the limit, an error response is returned to the client. However, doing this correctly isn't always possible. In JSON-RPC, only method calls with a valid `id` can be responded to. Since batches may also contain non-call messages or notifications, the best effort thing we can do to report an error with the batch itself is reporting the limit violation as an error for the first method call in the batch. If a batch is too large, but contains only notifications and responses, the error will be reported with a null `id`. The RPC client was also changed so it can deal with errors resulting from too large batches. An older client connected to the server code in this PR could get stuck until the request timeout occurred when the batch is too large. **Upgrading to a version of the RPC client containing this change is strongly recommended to avoid timeout issues.** For some weird reason, when writing the original client implementation, @fjl worked off of the assumption that responses could be distributed across batches arbitrarily. So for a batch request containing requests `[A B C]`, the server could respond with `[A B C]` but also with `[A B] [C]` or even `[A] [B] [C]` and it wouldn't make a difference to the client. So in the implementation of BatchCallContext, the client waited for all requests in the batch individually. If the server didn't respond to some of the requests in the batch, the client would eventually just time out (if a context was used). With the addition of batch limits into the server, we anticipate that people will hit this kind of error way more often. To handle this properly, the client now waits for a single response batch and expects it to contain all responses to the requests. --------- Co-authored-by: Felix Lange <fjl@twurst.com> Co-authored-by: Martin Holst Swende <martin@swende.se>

This reverts commit 5061259. Timeout handling for batches was improved upstream in this PR ethereum/go-ethereum#26681 which will be merged in in the next commit. Reverting this commit to make the merge cleaner.

rpc/handler.go: conflict with upstream's PR (ethereum/go-ethereum#26681) to add batch request and response size limits, and our own implementation of it. Removed our implementation (#198, which was moved inside batchCallBuffer.pushResponse in the v1.11.2 merge #205) in favor of upstream.

@fjl

) This PR adds server-side limits for JSON-RPC batch requests. Before this change, batches were limited only by processing time. The server would pick calls from the batch and answer them until the response timeout occurred, then stop processing the remaining batch items. Here, we are adding two additional limits which can be configured: - the 'item limit': batches can have at most N items - the 'response size limit': batches can contain at most X response bytes These limits are optional in package rpc. In Geth, we set a default limit of 1000 items and 25MB response size. When a batch goes over the limit, an error response is returned to the client. However, doing this correctly isn't always possible. In JSON-RPC, only method calls with a valid `id` can be responded to. Since batches may also contain non-call messages or notifications, the best effort thing we can do to report an error with the batch itself is reporting the limit violation as an error for the first method call in the batch. If a batch is too large, but contains only notifications and responses, the error will be reported with a null `id`. The RPC client was also changed so it can deal with errors resulting from too large batches. An older client connected to the server code in this PR could get stuck until the request timeout occurred when the batch is too large. **Upgrading to a version of the RPC client containing this change is strongly recommended to avoid timeout issues.** For some weird reason, when writing the original client implementation, @fjl worked off of the assumption that responses could be distributed across batches arbitrarily. So for a batch request containing requests `[A B C]`, the server could respond with `[A B C]` but also with `[A B] [C]` or even `[A] [B] [C]` and it wouldn't make a difference to the client. So in the implementation of BatchCallContext, the client waited for all requests in the batch individually. If the server didn't respond to some of the requests in the batch, the client would eventually just time out (if a context was used). With the addition of batch limits into the server, we anticipate that people will hit this kind of error way more often. To handle this properly, the client now waits for a single response batch and expects it to contain all responses to the requests. --------- Co-authored-by: Felix Lange <fjl@twurst.com> Co-authored-by: Martin Holst Swende <martin@swende.se>

### Description replacing `rpc` module with the upstream version ### Changes Focus PR: * ethereum/go-ethereum#26681 * ethereum/go-ethereum#27447 --------- Co-authored-by: Brandon Liu <lzqcn2000@126.com> Co-authored-by: Adrian Sutton <adrian@oplabs.co>

@fjl

) This PR adds server-side limits for JSON-RPC batch requests. Before this change, batches were limited only by processing time. The server would pick calls from the batch and answer them until the response timeout occurred, then stop processing the remaining batch items. Here, we are adding two additional limits which can be configured: - the 'item limit': batches can have at most N items - the 'response size limit': batches can contain at most X response bytes These limits are optional in package rpc. In Geth, we set a default limit of 1000 items and 25MB response size. When a batch goes over the limit, an error response is returned to the client. However, doing this correctly isn't always possible. In JSON-RPC, only method calls with a valid `id` can be responded to. Since batches may also contain non-call messages or notifications, the best effort thing we can do to report an error with the batch itself is reporting the limit violation as an error for the first method call in the batch. If a batch is too large, but contains only notifications and responses, the error will be reported with a null `id`. The RPC client was also changed so it can deal with errors resulting from too large batches. An older client connected to the server code in this PR could get stuck until the request timeout occurred when the batch is too large. **Upgrading to a version of the RPC client containing this change is strongly recommended to avoid timeout issues.** For some weird reason, when writing the original client implementation, @fjl worked off of the assumption that responses could be distributed across batches arbitrarily. So for a batch request containing requests `[A B C]`, the server could respond with `[A B C]` but also with `[A B] [C]` or even `[A] [B] [C]` and it wouldn't make a difference to the client. So in the implementation of BatchCallContext, the client waited for all requests in the batch individually. If the server didn't respond to some of the requests in the batch, the client would eventually just time out (if a context was used). With the addition of batch limits into the server, we anticipate that people will hit this kind of error way more often. To handle this properly, the client now waits for a single response batch and expects it to contain all responses to the requests. --------- Co-authored-by: Felix Lange <fjl@twurst.com> Co-authored-by: Martin Holst Swende <martin@swende.se>

…ereum#26681)" This reverts commit 189a756.

limit the number of batch requests to 100

7f3d7e6

mmsqe marked this pull request as ready for review February 14, 2023 02:34

mmsqe requested review from fjl and holiman as code owners February 14, 2023 02:34

mmsqe marked this pull request as draft February 14, 2023 02:37

limit the size of the response packet to 10MB

667a408

mmsqe force-pushed the add-rpc-limit branch from 18d10a2 to 667a408 Compare February 14, 2023 02:45

mmsqe marked this pull request as ready for review February 14, 2023 02:52

fjl reviewed Feb 14, 2023

View reviewed changes

rpc/errors.go Show resolved Hide resolved

mmsqe added 3 commits February 14, 2023 21:03

add batch limit related config

21aec8f

update doc

6b8b39d

Merge branch 'master' into add-rpc-limit

b6993c4

mmsqe added 3 commits February 14, 2023 21:29

apply limit for server & client

c4ac65c

make batch related limit configurable

c9015fa

Merge branch 'master' into add-rpc-limit

6d2ce24

mmsqe added 2 commits February 16, 2023 13:01

add SetBatchLimits for client with default limit

2c04aa0

Merge branch 'master' into add-rpc-limit

22bc552

mmsqe force-pushed the add-rpc-limit branch from 5e45e95 to 22bc552 Compare February 16, 2023 05:09

holiman reviewed Feb 16, 2023

View reviewed changes

mmsqe added 3 commits February 16, 2023 17:55

rename namespace

a43fda5

Merge branch 'master' into add-rpc-limit

754137c

allow set limit with dial after client get init

d7c8673

mmsqe force-pushed the add-rpc-limit branch from 0d01b40 to 38a859e Compare February 16, 2023 13:24

set limit when init client

7fd2b77

mmsqe force-pushed the add-rpc-limit branch from 38a859e to 7fd2b77 Compare February 16, 2023 14:11

mmsqe requested a review from fjl February 16, 2023 16:01

holiman added this to the 1.11.2 milestone Feb 17, 2023

holiman added this to the 1.12.1 milestone May 25, 2023

holiman and others added 4 commits May 31, 2023 03:32

Merge branch 'master' into add-rpc-limit

bd5dfa6

cmd/utils: fix docs on flags

47557d1

rpc: minor refactor of tests

8e6018f

fjl added 4 commits June 8, 2023 15:49

rpc: remove default limits

f0688d6

I think adding these limits might break people's setups, so best to avoid configuring them by default in package rpc.

rpc: remove added blank lines in invalid-batch.js

cd73291

rpc: remove special error handling for HTTP batch response length

7048bfc

rpc: rename error

6841858

holiman approved these changes Jun 13, 2023

View reviewed changes

fjl merged commit f3314bb into ethereum:master Jun 13, 2023
1 of 2 checks passed

mmsqe mentioned this pull request Jun 13, 2023

rpc: add limit for batch request items and response size crypto-org-chain/ethermint#268

Open

11 tasks

nisdas mentioned this pull request Aug 14, 2023

Increase the Default of Batch Response Max Size #27923

Closed

SEJeff mentioned this pull request Oct 4, 2023

Ccq/p2p with single host wormhole-foundation/wormhole#3356

Closed

0xcb9ff9 mentioned this pull request Oct 30, 2023

rpc: replace rpc module dogechain-lab/dbsc#42

Merged

sieniven mentioned this pull request Nov 3, 2023

RPC: Add default and configurable response size limit to eth JSON-RPC server DeFiCh/ain#2672

Merged

9 tasks

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "rpc: add limit for batch request items and response size (eth…

d8911d6

…ereum#26681)" This reverts commit 189a756.

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "rpc: add limit for batch request items and response size (eth…

eb0e6eb

…ereum#26681)" This reverts commit 189a756.

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "rpc: add limit for batch request items and response size (eth…

1c38dbe

…ereum#26681)" This reverts commit 189a756.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: add limit for batch request and response size #26681

rpc: add limit for batch request and response size #26681

mmsqe commented Feb 14, 2023 •

edited by fjl

fjl left a comment •

edited

mmsqe commented Feb 14, 2023

fjl commented Feb 14, 2023

mmsqe commented Feb 15, 2023

holiman Feb 16, 2023

fjl commented Jun 8, 2023

fjl commented Jun 8, 2023

holiman left a comment

rpc: add limit for batch request and response size #26681

rpc: add limit for batch request and response size #26681

Conversation

mmsqe commented Feb 14, 2023 • edited by fjl

fjl left a comment • edited

Choose a reason for hiding this comment

mmsqe commented Feb 14, 2023

fjl commented Feb 14, 2023

mmsqe commented Feb 15, 2023

holiman Feb 16, 2023

Choose a reason for hiding this comment

fjl commented Jun 8, 2023

fjl commented Jun 8, 2023

holiman left a comment

Choose a reason for hiding this comment

mmsqe commented Feb 14, 2023 •

edited by fjl

fjl left a comment •

edited