make the ws buffer size configurable #10013

kianenigma · 2021-10-12T15:05:42Z

I know this is not populare, but hear me out:

I am running into similar issues as in paritytech/polkadot#2039 in staking miners, and I don't see any other clear way to fix this.

In the issue, it was mentioned that this happened due to the speed of the consumer and producer not matching. Given the 10mb default, I am pretty sure it my case it is happening because a single storage item is somewhere above 10mb.

If at least one of @tomusdrw or @bkchr give a thumb up, will remove draft and document the code a bit better. I can also group all RPC-server related params into one struct to make it cleaner.

Polkadot companion: paritytech/polkadot#4078

tomusdrw

lgtm (modulo missing docs, and refinement), we are far beyond the "don't add any RPC-related CLI options" point anyway, and config files are nowhere around the corner, so I'm okay with adding it, especially that it addresses a particular pain point you are having.

tomusdrw · 2021-10-13T09:20:51Z

client/cli/src/commands/run_cmd.rs

+	/// Set the the maximum RPC output buffer size. Default is 10MiB.
+	#[structopt(long = "rpc-max-in-buffer-capacity")]
+	pub ws_max_in_buffer_capacity: Option<usize>,


Is there a use case for in buffer? We already have max_rpc_payload which limits that, no?

Not in my case, but I thought it would be cleaner to make both configurable.

client/cli/src/commands/run_cmd.rs

tomusdrw · 2021-10-13T09:23:00Z

client/cli/src/commands/run_cmd.rs

+	#[structopt(long = "rpc-max-in-buffer-capacity")]
+	pub ws_max_in_buffer_capacity: Option<usize>,
+
+	/// Set the the maximum RPC output buffer size. Default is 10MiB.


How does it relate to max payload setting? Wouldn't restricting it here further restrict the output max payload?

Suggested change

/// Set the the maximum RPC output buffer size. Default is 10MiB.

/// Set the the maximum RPC output buffer size in MiB. Default is 10.

The buffer seems to be the pre-cursor to the max-payload, at least for the output buffer. I presume you want to have your buffer size be x and your max payload size 80% of x in most cases. But now, we allow max payload to be high like 100 MiB (don't ask why, assume some teams that don't care about DOS need it :p), but with a buffer of 10 MiB your max payload is useless.

but with a buffer of 10 MiB your max payload is useless.

Right, perhaps best to detect such misconfiguration and quit with an error then? I think it might be confusing to see large requests fail because they are limited by some other factor.

I can do this if you agree with setting the default buffer size to 16 MiB, with the default max payload remaining 15 MiB. Then I can add a warning if the max out buffer is ever less than max payload.

applied. I think it is pretty safe, WDYT?

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

client/cli/src/commands/run_cmd.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

…figurable-ws-buffer-size

client/cli/src/commands/run_cmd.rs

…ch/substrate into kiz-configurable-ws-buffer-size

…figurable-ws-buffer-size

tomusdrw

Few grumbles, but lgtm otherwise!

client/cli/src/commands/run_cmd.rs

client/rpc-servers/src/lib.rs

client/cli/src/commands/run_cmd.rs

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

kianenigma · 2021-10-14T10:53:47Z

bot merge

ghost · 2021-10-14T10:53:56Z

Waiting for commit status.

* make the ws buffer size configurable * Update client/cli/src/commands/run_cmd.rs Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * Update client/cli/src/commands/run_cmd.rs Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * Update client/cli/src/commands/run_cmd.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Final touches * Apply suggestions from code review * fix bench * remove in buffer * Apply suggestions from code review Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

make the ws buffer size configurable

0bd9e8e

github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Oct 12, 2021

kianenigma requested a review from bkchr October 12, 2021 18:33

tomusdrw reviewed Oct 13, 2021

View reviewed changes

kianenigma and others added 2 commits October 13, 2021 10:29

Update client/cli/src/commands/run_cmd.rs

0d1331d

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

Update client/cli/src/commands/run_cmd.rs

3f4174a

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

bkchr approved these changes Oct 13, 2021

View reviewed changes

client/cli/src/commands/run_cmd.rs Outdated Show resolved Hide resolved

kianenigma and others added 2 commits October 13, 2021 12:12

Update client/cli/src/commands/run_cmd.rs

48c5bc0

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Merge branch 'master' of github.com:paritytech/substrate into kiz-con…

1474ea8

…figurable-ws-buffer-size

kianenigma marked this pull request as ready for review October 13, 2021 12:19

github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Oct 13, 2021

kianenigma added B5-clientnoteworthy C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Oct 13, 2021

kianenigma requested a review from tomusdrw October 13, 2021 12:25

Final touches

ea20a2e

github-actions bot added the A7-needspolkadotpr label Oct 13, 2021

kianenigma commented Oct 13, 2021

View reviewed changes

client/cli/src/commands/run_cmd.rs Outdated Show resolved Hide resolved

kianenigma commented Oct 13, 2021

View reviewed changes

client/cli/src/commands/run_cmd.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

3072b12

kianenigma mentioned this pull request Oct 14, 2021

default ws note #10014

Closed

kianenigma added 3 commits October 14, 2021 10:50

fix bench

46a6d96

Merge branch 'kiz-configurable-ws-buffer-size' of github.com:parityte…

311d8b5

…ch/substrate into kiz-configurable-ws-buffer-size

Merge branch 'master' of github.com:paritytech/substrate into kiz-con…

c1e4d1b

…figurable-ws-buffer-size

tomusdrw approved these changes Oct 14, 2021

View reviewed changes

client/cli/src/commands/run_cmd.rs Outdated Show resolved Hide resolved

client/rpc-servers/src/lib.rs Outdated Show resolved Hide resolved

client/rpc-servers/src/lib.rs Show resolved Hide resolved

client/cli/src/commands/run_cmd.rs Outdated Show resolved Hide resolved

kianenigma and others added 2 commits October 14, 2021 12:48

remove in buffer

61ef27c

Apply suggestions from code review

1563c5d

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

kianenigma removed the A7-needspolkadotpr label Oct 14, 2021

ghost merged commit 00629de into master Oct 14, 2021

ghost deleted the kiz-configurable-ws-buffer-size branch October 14, 2021 11:44

ghzlatarev mentioned this pull request Nov 2, 2021

[Manta-PC] Update dependencies to v0.9.12 Manta-Network/Manta#242

Merged

19 tasks

github-actions bot mentioned this pull request Nov 2, 2021

Update substrate/polkadot/cumulus from v0.9.11 to v0.9.12 moonbeam-foundation/moonbeam#951

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make the ws buffer size configurable #10013

make the ws buffer size configurable #10013

kianenigma commented Oct 12, 2021 •

edited

Loading

tomusdrw left a comment •

edited

Loading

tomusdrw Oct 13, 2021

kianenigma Oct 13, 2021

tomusdrw Oct 13, 2021

kianenigma Oct 13, 2021

tomusdrw Oct 13, 2021

kianenigma Oct 13, 2021

kianenigma Oct 13, 2021

tomusdrw left a comment

kianenigma commented Oct 14, 2021

ghost commented Oct 14, 2021

	/// Set the the maximum RPC output buffer size. Default is 10MiB.
	/// Set the the maximum RPC output buffer size in MiB. Default is 10.

make the ws buffer size configurable #10013

make the ws buffer size configurable #10013

Conversation

kianenigma commented Oct 12, 2021 • edited Loading

tomusdrw left a comment • edited Loading

Choose a reason for hiding this comment

tomusdrw Oct 13, 2021

Choose a reason for hiding this comment

kianenigma Oct 13, 2021

Choose a reason for hiding this comment

tomusdrw Oct 13, 2021

Choose a reason for hiding this comment

kianenigma Oct 13, 2021

Choose a reason for hiding this comment

tomusdrw Oct 13, 2021

Choose a reason for hiding this comment

kianenigma Oct 13, 2021

Choose a reason for hiding this comment

kianenigma Oct 13, 2021

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

kianenigma commented Oct 14, 2021

ghost commented Oct 14, 2021

kianenigma commented Oct 12, 2021 •

edited

Loading

tomusdrw left a comment •

edited

Loading