Skip to content

fix(rpc): apply RST_STREAM frame limit by default - #8

Open
waynercheung wants to merge 1 commit into
release_v4.8.3from
fix/rpc-rst-stream-limit
Open

waynercheung wants to merge 1 commit into
release_v4.8.3from
fix/rpc-rst-stream-limit

Conversation

@waynercheung

Copy link
Copy Markdown
Owner

What does this PR do?

Turns node.rpc.maxRstStream and node.rpc.secondsPerWindow into properly defaulted and validated options, and applies the gRPC HTTP/2 RST_STREAM frame limit unconditionally.

  • common (NodeConfig): default both options to 200 / 30, matching Netty's HTTP/2 server defaults. In postProcess, reject negative values for either option and reject Integer.MAX_VALUE specifically for maxRstStream (which grpc-java treats as disabling the limit), and independently fall back each 0 to its default with a warning.
  • framework (RpcService): read and validate the two values before allocating the RPC executor, then always call maxRstFramesPerWindow(...).
  • reference.conf, docs/configuration.md, config/README.md: document the new defaults and the 0 fallback kept for backward compatibility.

Why are these changes required?

Both options previously defaulted to 0, and RpcService configured maxRstFramesPerWindow only when both values were positive. As a result the RST_STREAM frame limit was left unset in a default deployment, and misconfigured values (negative for either option, or maxRstStream == Integer.MAX_VALUE) were accepted silently. This brings the two options in line with how the neighboring maxConcurrentCallsPerConnection option already behaves (a secure default plus validation), so the setting is consistent across the config, cannot be silently disabled, and fails fast on invalid input.

This PR has been tested by:

  • Unit Tests
    • NodeConfigTest: default resolves to 200/30; each 0 independently falls back to its default; negative values and maxRstStream == Integer.MAX_VALUE are rejected with PARAMETER_INIT; explicit positive values (including Integer.MAX_VALUE - 1 and an Integer.MAX_VALUE window) are preserved.
    • RpcServiceHttp2SecurityTest: the limit is enforced at the connection layer under the default, under the legacy 0/0, and under an explicit small limit; a connection at the limit still gets its PING acknowledged, and one more RST_STREAM past the limit is answered with GOAWAY(ENHANCE_YOUR_CALM); invalid runtime values are rejected before the RPC executor is allocated.
    • All 170 tests in the five selected configuration and RPC test classes passed, along with framework Checkstyle and reference.conf validation.
  • Manual Testing
    • No separate manual testing; transport behavior is covered by the automated raw HTTP/2 tests above.

Follow up

None.

Extra details

  • Backward compatibility: an explicit 0 for either option now falls back to its default (with a startup warning) instead of leaving the limit unset; maxRstStream = 2147483647 (Integer.MAX_VALUE) is rejected at startup. Upgrade notes are included in docs/configuration.md and common/src/main/java/org/tron/core/config/README.md.

@waynercheung
waynercheung changed the base branch from develop to release_v4.8.3 September 15, 2026 06:07
The node.rpc.maxRstStream and secondsPerWindow options defaulted to
0, which left the HTTP/2 RST_STREAM frame limit unset, and the limit
was configured only when both values were positive. Give them
sensible defaults, validate them at load time, and apply the limit
unconditionally so the configuration behaves consistently.

- NodeConfig: default maxRstStream/secondsPerWindow to 1000/5.
  In postProcess, reject negative values for either option and reject
  Integer.MAX_VALUE specifically for maxRstStream, which grpc-java
  treats as disabling the limit. Independently replace each zero with
  its default and log a warning.
- RpcService: read and validate the two values before allocating the
  executor, then always configure maxRstFramesPerWindow.
- reference.conf, docs/configuration.md and config/README.md:
  document the new defaults and the 0 fallback for compatibility.
- Tests: pin the Java and reference defaults, preserve positive
  overrides, and verify PING at the RST limit followed by GOAWAY
  after one additional reset using a prebuilt HTTP/2 frame burst.

Note: an explicit 0 now falls back to the default instead of leaving
the limit unset, and maxRstStream = 2147483647 is rejected at startup.
@waynercheung
waynercheung force-pushed the fix/rpc-rst-stream-limit branch from bd99b08 to cfdc57c Compare September 21, 2026 10:22
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.

1 participant