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

rpc: increase max request content length #22012

Closed

Conversation

martinboehm
Copy link

In the ropsten network, there are multiple blocks with the size over 3MB (height 599275 - 3.35MB, 599281 - 6.52MB). The current max request content length limit of 5MB does not allow to fetch these blocks using the eth_getBlock API call. With the overhead of the hex encoding in the rpc protocol, the maximum block size possible to be fetched is only about 2.5MB.

Fixed by extending the max request content length size to 20MB - with some buffer room, as the working limit would be about 14-15MB.

In the ropsten network, there are multiple blocks with size over 3MB
(height 599275 - 3.35MB, 599281 - 6.52MB). The current limit of 5MB
does not allow to fetch these blocks using the eth_getBlock API call.
With the overhead of the hex encoding in the rpc protocol, the maximum
block size possible to be fetched is about 2.5MB.
@@ -32,7 +32,7 @@ import (
)

const (
maxRequestContentLength = 1024 * 1024 * 5
maxRequestContentLength = 1024 * 1024 * 20
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps maxRequestContentLength size should not only be increased, but also be made to be configurable like parity-ethereum. As to what concrete value should it increase to, maybe DDOS attack also needs to be considered.

@fjl fjl added this to the 1.10.0 milestone Dec 15, 2020
@fjl
Copy link
Contributor

fjl commented Jan 25, 2021

I don't understand why we need to increase the maximum request length to allow for very large responses. I just tested this with the client available in package rpc and it seems to have no problem dealing with large responses sent by the server:

// This checks that maxRequestContentLength is not applied to the response of a request.
func TestHTTPRespBodyLimit(t *testing.T) {
	const respLength = maxRequestContentLength * 3

	s := NewServer()
	s.RegisterName("test", largeRespService{respLength})
	ts := httptest.NewServer(s)
	defer ts.Close()

	c, err := DialHTTP(ts.URL)
	if err != nil {
		t.Fatal(err)
	}
	defer c.Close()

	var r string
	if err := c.Call(&r, "test_largeResp"); err != nil {
		t.Fatal(err)
	}
	if len(r) != respLength {
		t.Fatalf("response has wrong length %d, want %d", len(r), respLength)
	}
}

type largeRespService struct {
	length int
}

func (x largeRespService) LargeResp() string {
	return string(make([]byte, x.length))
}

@fjl
Copy link
Contributor

fjl commented Jan 25, 2021

@martinboehm can you provide more information about your issue? It would be nice to know under which circumstances you are hitting this limit. Which client library are you using to access ropsten blocks?

@fjl fjl removed the status:triage label Jan 25, 2021
@martinboehm
Copy link
Author

@fjl Hi, we are using websocket connection, not HTTP. The issue is happening in the blockchain indexer and simple explorer https://github.com/trezor/blockbook on initial indexing of the blockchain.
The problem in the code happens here https://github.com/trezor/blockbook/blob/d0a1cb29f67b6afa9f70e6960cd8098fef9979f0/bchain/coins/eth/ethrpc.go#L476.
The issue description including the crude workaround of using sed before compilation is here trezor/blockbook#490.

@fjl
Copy link
Contributor

fjl commented Jan 27, 2021

Thanks! So it looks like the issue is with this line: https://github.com/ethereum/go-ethereum/blob/master/rpc/websocket.go#L242. I'm OK with increasing this limit independently of the HTTP request body limit, will look into it.

@fjl fjl self-assigned this Feb 16, 2021
@fjl
Copy link
Contributor

fjl commented Feb 26, 2021

Replaced by #22385

@fjl fjl closed this Feb 26, 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.

4 participants