rpc: Add experimental config params to allow for subscription buffer size control (tm v0.34.x)

[thanethomson]

Follows from #7188 and related to #6729.

The aim here is to provide a short-term workaround for #6729 in the v0.34.x series of Tendermint that allows for tuning of the internal buffer sizes relating to event subscription. The following configuration parameters are introduced in the [rpc] section:

[rpc]

# Experimental parameter to specify the maximum number of events a node will
# buffer, per subscription, before returning an error and closing the
# subscription. Must be set to at least 100, but higher values will accommodate
# higher event throughput rates (and will use more memory).
experimental_subscription_buffer_size = 1000

# Experimental parameter to specify the maximum number of RPC responses that
# can be buffered per WebSocket client. If clients cannot read from the
# WebSocket endpoint fast enough, they will be disconnected, so increasing this
# parameter may reduce the chances of them being disconnected (but will cause
# the node to use more memory).
#
# Must be at least the same as "experimental_subscription_buffer_size",
# otherwise connections could be dropped unnecessarily. This value should
# ideally be somewhat higher than "experimental_subscription_buffer_size" to
# accommodate non-subscription-related RPC responses.
experimental_websocket_write_buffer_size = 1000

# If a WebSocket client cannot read fast enough, at present we may
# silently drop events instead of generating an error or disconnecting the
# client.
#
# Enabling this experimental parameter will cause the WebSocket connection to
# be closed instead if it cannot read fast enough, allowing for greater
# predictability in subscription behaviour.
experimental_close_on_slow_client = false

[thanethomson]

Seems like the new linting changes in 8a2dcba are picking up a whole bunch of spelling mistakes throughout the codebase 🙂

[tychoish]

Seems like the new linting changes in 8a2dcba are picking up a whole bunch of spelling mistakes throughout the codebase 

They're less mistakes and more conventions :p

Anyway, those should be fixed if you rebase/etc.

[thanethomson]

So we've now managed to confirm that:

1. With the latest version of Hermes, we can still reproduce the RPC disconnection with the official Gaia v4.2.1 release (Tendermint v0.34.9).
2. We can reproduce the RPC disconnection with Gaia v4.2.1 built with Tendermint from the v0.34.x branch.
3. We can reproduce the RPC disconnection with Gaia v4.2.1 built with Tendermint from this PR's branch (thane/6729-websocket-bufsize2) with experimental buffer sizes set to 100.
4. Using the same setup as in (3), but setting the buffer sizes to 1000 results in us not being able to reproduce the RPC disconnection. We do not get disconnected from Gaia, even when submitting 10,000 transactions as quickly as we can with Hermes (all processes running on the same machine).

Beyond enabling these experimental config parameters, I'd like to propose we increase the default subscription buffer size to 1000 to better handle the loads we're seeing in live networks.

[creachadair]

Overall this change seems mechanically fine. My one main concern is this: If we add these experimental settings only to the v0.34 line, it becomes an upgrade blocker for v0.35.

The biggest users are the ones most likely to need this knob, and there are enough other quality of life improvements in the v0.35 series that we'd prefer not to add any additional impediments to their upgrading.

It looks to me like the config plumbing parts of this PR could be safely added to v0.35.x for the next point release, which would avert that. I think it'd be fine to land them separately rather than as a backport from v0.35, though.

Your thoughts?

[creachadair]

Overall this change seems mechanically fine. My one main concern is this: If we add these experimental settings only to the v0.34 line, it becomes an upgrade blocker for v0.35.

The biggest users are the ones most likely to need this knob, and there are enough other quality of life improvements in the v0.35 series that we'd prefer not to add any additional impediments to their upgrading.

It looks to me like the config plumbing parts of this PR could be safely added to v0.35.x for the next point release, which would avert that. I think it'd be fine to land them separately rather than as a backport from v0.35, though.

Your thoughts?

Just to clarify: I'm not objecting to merging this change into v0.34 either way, I am just thinking about the path for someone who needs to upgrade.

[thanethomson]

It looks to me like the config plumbing parts of this PR could be safely added to v0.35.x for the next point release, which would avert that. I think it'd be fine to land them separately rather than as a backport from v0.35, though.

I'd be more than happy to submit a separate PR to introduce similar changes into v0.35.

[thanethomson]

Are the coverage tests known to be non-deterministic? It seems like each time I re-run the tests, a different set of coverage tests fail.

[creachadair]

Are the coverage tests known to be non-deterministic? It seems like each time I re-run the tests, a different set of coverage tests fail.

Some of the end-to-end tests are nondeterministic, but the unit tests should usually not be. This looks like a bug in the tests, though it is quite possible it's a bug we fixed already on trunk. Over the past few months we've cleaned up various issues with goroutine lifetime.

If it doesn't fail consistently, I don't think we should block over it.