Request Response has hard coded limits for message sizes

[tqwewe]

Description

The request/response module has a 1 MB limit on requests, and 10 MB limit on responses, both hard coded as fixed limits.

rust-libp2p/protocols/request-response/src/cbor.rs

Lines 58 to 61 in 40550dc

	/// Max request size in bytes
	const REQUEST_SIZE_MAXIMUM: u64 = 1024 * 1024;
	/// Max response size in bytes
	const RESPONSE_SIZE_MAXIMUM: u64 = 10 * 1024 * 1024;

It would be preferable if this was configurable.

Is there a reason why these limits are set to these values, are there any obvious downsides to having much higher limits, or no limits at all?

Motivation

Messages which exceed these limits fail with a confusing Eof { name: "u8", expect: Small(1) } error. Sending messages larger than these, such as files, etc would be useful.

Current Implementation

Limits messages to 1 MB for requests, and 10 MB for responses for both JSON and CBOR implementations.

Are you planning to do it yourself in a pull request?

Maybe

[dariusc93]

Hey! I believe it was hard coded so it would be less knobs that need configuring. I did leave a comment on a discussion #5710 (comment) that pointed to the changes I made to make it configurable.

[tqwewe]

Thanks for the response! Out of curiosity, what would be the downside of simply using .read_to_end() so there's no limit at all? For untrusted peers, I can understand there may be a security issue with no limit, but for trusted peers it may be nice to have no limit at all.

[elenaf9]

Without a limit we'd have unbounded memory allocation on the receiver side, since we use a Vec to store the received data until EOF.
We don't differentiate on this layer between trusted and untrusted peers, but even with trusted peers we try to bound everything, see https://github.com/libp2p/rust-libp2p/blob/master/docs/coding-guidelines.md#bound-everything,

[moofone]

I noticed when extending this limit, we run into OutboundFailure::Timeout. Which seems to be due to substream_timeout on the worker_streams also in request-response protocol handler

 worker_streams: futures_bounded::FuturesMap::new(
                substream_timeout,
                max_concurrent_streams,
            )

Is this one configurable? I raised the limit and my timeout went away on very large payloads. I believe we'll need both substream_timeout and the response size limit to be configurable

[tqwewe]

@moofone it seems like its available already under Config::with_request_timeout

[anilaltuner]

Hey @tqwewe, is it fix with latest updates? I got this error too but I sent very small vector. It seems there is a problem on io.

[tqwewe]

@anilaltuner it should be fixed with the merged PR #5792, but I haven't tested it