ci(config): enforce lowerCamelCase and max depth in reference.conf#29
ci(config): enforce lowerCamelCase and max depth in reference.conf#29bladehan1 wants to merge 7 commits into
Conversation
* fix(config): remove redundant JSON-RPC config fields and consolidate parameter binding Remove maxBlockFilterNum, maxAddressSize, and maxRequestTimeout from NodeConfig/CommonParameter since they were superseded by the jsonRpcMaxBatchSize/jsonRpcMaxResponseSize parameters. Consolidate the config binding path so all JSON-RPC size limits flow through a single reference.conf entry, and clean up stale test-config fixtures. * fix bug of NodeConfigTest * remove allowShieldedTransactionApi from reference.conf * add testcase of external.ip is null * change comment * fix the bug of --es and 7 failed testcases
…onprotocol#6760) Pre-3.0.0(The previous event-plugin public release is 2.2.0) event-plugin builds still link against com.alibaba.fastjson, which was removed from java-tron in tronprotocol#6701. When such a plugin is loaded, the NoClassDefFoundError surfaces inside the plugin's own worker thread and is swallowed by its catch(Throwable) handler. The node keeps running while silently dropping every trigger, leaving operators with no signal that the event subscription is broken. Enforce a minimum Plugin-Version at startup in EventPluginLoader.startPlugin using pf4j's VersionManager (semver). When the descriptor version is below 3.0.0, log a clear upgrade hint and return false; the existing call chain in Manager.startEventSubscribing wraps that into TronError(EVENT_SUBSCRIBE_INIT) and aborts node startup instead of silently degrading.
…rs (tronprotocol#6761) - Add `rate.limiter.apiNonBlocking` switch (default false). When off, callers wait for a permit; when on, they reject immediately and shed load. - Wire the switch through `RateLimiterConfig` → `Args` → `CommonParameter`; update `reference.conf` / `config.conf`. - Add blocking `acquire()` paths on `IRateLimiter`, `GlobalRateLimiter`, and `QpsStrategy` / `IPQpsStrategy` / `GlobalPreemptibleStrategy`. Only the semaphore-based strategy is bounded (2s); the QPS-based paths use unbounded Guava `RateLimiter.acquire()`. - Introduce `acquirePermit()` dispatcher (default method on `IRateLimiter`; static on `GlobalRateLimiter`) that picks blocking vs non-blocking based on the switch. - Swap `tryAcquire` → `acquirePermit` at the `RateLimiterServlet` and `RateLimiterInterceptor` call sites; preserve per-endpoint-before-global ordering to avoid consuming global quota on per-endpoint rejection. - Extract `loadIpLimiter()` in `GlobalRateLimiter` and `loadLimiter()` in `IPQpsStrategy` to share cache+exception handling between `tryAcquire` and `acquire`. - Add unit tests covering dispatcher routing (both directions), acquire IP-before-global ordering, and IP-loader-failure fail-closed behaviour.
Add a CI gate that scans common/src/main/resources/reference.conf and fails the build when any new key violates lowerCamelCase (^[a-z][a-zA-Z0-9]*$ per dot-separated segment) or exceeds the maximum hierarchy depth of 5. Runs as a new step in the existing checkstyle job of pr-check.yml (no extra runner spin-up). Four legacy PBFT* keys are grandfathered via an in-script allowlist so the gate fails only on new violations. The script emits a consolidated GHA error annotation listing every offending key/line, and uses sys.exit(1) to drive step failure.
📝 WalkthroughWalkthroughThis PR refactors configuration binding and rate-limiter acquisition semantics, adds runtime validation for event plugin versions, and hardens JSON-RPC request processing. Configuration classes normalize non-standard keys before auto-binding instead of manual field assignment. Rate-limiters now dispatch acquire requests through a conditional ChangesConfiguration validation, rate-limiter routing, and JSON-RPC hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 559 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="common/build.gradle">
<violation number="1" location="common/build.gradle:24">
P2: Avoid using a SNAPSHOT dependency in `api`; it makes builds non-reproducible and can introduce unexpected changes without code updates.</violation>
</file>
<file name="chainbase/src/main/java/org/tron/core/capsule/BlockCapsule.java">
<violation number="1" location="chainbase/src/main/java/org/tron/core/capsule/BlockCapsule.java:233">
P1: `validateMerkleRoot()` memoizes success but never invalidates it when the block is mutated, so later merkle-root mismatches can be skipped.</violation>
</file>
Note: This PR contains a large number of files. cubic only reviews up to 100 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
On a pro plan you can use ultrareview for larger PRs.
Re-trigger cubic
| } | ||
|
|
||
| public void validateMerkleRoot() throws BadBlockException { | ||
| if (merkleValidated) { |
There was a problem hiding this comment.
P1: validateMerkleRoot() memoizes success but never invalidates it when the block is mutated, so later merkle-root mismatches can be skipped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At chainbase/src/main/java/org/tron/core/capsule/BlockCapsule.java, line 233:
<comment>`validateMerkleRoot()` memoizes success but never invalidates it when the block is mutated, so later merkle-root mismatches can be skipped.</comment>
<file context>
@@ -225,6 +229,19 @@ public Sha256Hash calcMerkleRoot() {
}
+ public void validateMerkleRoot() throws BadBlockException {
+ if (merkleValidated) {
+ return;
+ }
</file context>
| api 'org.aspectj:aspectjweaver:1.9.8' | ||
| api 'org.aspectj:aspectjtools:1.9.8' | ||
| api group: 'io.github.tronprotocol', name: 'libp2p', version: '2.2.7',{ | ||
| api group: 'com.github.tronprotocol', name: 'libp2p', version: 'release-v2.2.8-SNAPSHOT',{ |
There was a problem hiding this comment.
P2: Avoid using a SNAPSHOT dependency in api; it makes builds non-reproducible and can introduce unexpected changes without code updates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At common/build.gradle, line 24:
<comment>Avoid using a SNAPSHOT dependency in `api`; it makes builds non-reproducible and can introduce unexpected changes without code updates.</comment>
<file context>
@@ -21,7 +21,8 @@ dependencies {
api 'org.aspectj:aspectjweaver:1.9.8'
api 'org.aspectj:aspectjtools:1.9.8'
- api group: 'io.github.tronprotocol', name: 'libp2p', version: '2.2.7',{
+ api group: 'com.github.tronprotocol', name: 'libp2p', version: 'release-v2.2.8-SNAPSHOT',{
+ //api group: 'io.github.tronprotocol', name: 'libp2p', version: '2.2.7',{
exclude group: 'io.grpc', module: 'grpc-context'
</file context>
| api group: 'com.github.tronprotocol', name: 'libp2p', version: 'release-v2.2.8-SNAPSHOT',{ | |
| api group: 'io.github.tronprotocol', name: 'libp2p', version: '2.2.7',{ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
framework/src/test/java/org/tron/common/logsfilter/EventLoaderTest.java (1)
22-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse an ephemeral port in
launchNativeQueuetest to remove CI flakiness.Line 25 hardcodes
5555, and the current pipeline already fails withErrno 48: Address already in use. Pick a free ephemeral port at runtime instead.Suggested fix
+import java.io.IOException; +import java.net.ServerSocket; ... `@Test` public void launchNativeQueue() { EventPluginConfig config = new EventPluginConfig(); config.setSendQueueLength(1000); - config.setBindPort(5555); + config.setBindPort(findFreePort()); config.setUseNativeQueue(true); ... } + + private static int findFreePort() { + try (ServerSocket socket = new ServerSocket(0)) { + return socket.getLocalPort(); + } catch (IOException e) { + throw new AssertionError("Unable to allocate a free test port", e); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/test/java/org/tron/common/logsfilter/EventLoaderTest.java` around lines 22 - 27, The test launchNativeQueue hardcodes port 5555 via EventPluginConfig.setBindPort(5555), causing CI flakiness; change it to pick an ephemeral free port at runtime by opening a ServerSocket on port 0 to obtain the assigned port (socket.getLocalPort()), close the socket and then call config.setBindPort(foundPort) so the test uses a free ephemeral port; update launchNativeQueue to use this dynamic port acquisition before starting the native queue.framework/src/main/resources/config.conf (1)
744-754:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign
constantCallTimeoutMsdocs with zero-allowed behavior.Line 747 says the value must be positive, but Line 753 sets default to
0and current validation accepts0. Please make this wording consistent to avoid operator confusion.Suggested doc fix
- # routed through the constant-call path. When set, must be a positive - # integer that fits VM deadline conversion and is used verbatim as the + # routed through the constant-call path. When set, must be a non-negative + # integer that fits VM deadline conversion and is used verbatim as the🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/resources/config.conf` around lines 744 - 754, The docs for the config property constantCallTimeoutMs are inconsistent: the text currently says "must be a positive integer" but the default and validation allow 0; update the wording to state it accepts zero (e.g., "must be a non-negative integer (0 disables sharing and uses a per-call deadline)") or "must be an integer ≥ 0" and mention the default is 0 so operators aren't confused; ensure the change is applied to the constantCallTimeoutMs description in config.conf.
🧹 Nitpick comments (1)
common/src/main/java/org/tron/core/config/args/NodeConfig.java (1)
452-468: 💤 Low valuePotential exception if
discovery.external.ippath is missing.
getIsNull(externalIpPath)on line 463 will throwConfigException.Missingif the path doesn't exist in the config. Whilereference.confguarantees this path exists, a defensivehasPathcheck would prevent failures from malformed user configs.Suggested defensive check
private static Config normalizeNonStandardKeys(Config section) { if (section.hasPath("isOpenFullTcpDisconnect")) { section = section.withValue("openFullTcpDisconnect", section.getValue("isOpenFullTcpDisconnect")); } String externalIpPath = "discovery.external.ip"; - if (section.getIsNull(externalIpPath) + if (!section.hasPath(externalIpPath) + || section.getIsNull(externalIpPath) || "null".equalsIgnoreCase(section.getString(externalIpPath))) { section = section.withValue(externalIpPath, ConfigValueFactory.fromAnyRef("")); } return section; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/src/main/java/org/tron/core/config/args/NodeConfig.java` around lines 452 - 468, normalizeNonStandardKeys can throw ConfigException.Missing when "discovery.external.ip" is absent; guard the null/string check with a hasPath check. Modify normalizeNonStandardKeys to first check section.hasPath("discovery.external.ip") before calling section.getIsNull(...) or section.getString(...), and only call withValue(...) to replace the value when the path exists and is null or equals "null" (case-insensitive); keep the existing handling for "isOpenFullTcpDisconnect" unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@framework/src/main/java/org/tron/core/services/ratelimiter/strategy/IPQpsStrategy.java`:
- Around line 29-35: The current IPQpsStrategy.acquire method calls
limiter.acquire() which can block indefinitely; change it to use a bounded call
limiter.tryAcquire(timeout, unit) and return false on timeout so threads don't
starve. Inside IPQpsStrategy.acquire, after obtaining the RateLimiter from
loadLimiter(ip), replace the blocking limiter.acquire() with
limiter.tryAcquire(<configuredTimeout>, <TimeUnit>) (use an existing
timeout/config property or add a private field like rateLimiterTimeout and
TimeUnit.MILLISECONDS) and if tryAcquire returns false then return false;
otherwise proceed and return true. Ensure behavior respects the
isRateLimiterApiNonBlocking flag (when non-blocking is required, use a short
timeout and fail fast) and reference the methods loadLimiter and acquire in your
change.
In
`@framework/src/main/java/org/tron/core/services/ratelimiter/strategy/QpsStrategy.java`:
- Around line 33-35: Replace the indefinite blocking call in
QpsStrategy.acquire() with a bounded wait: use rateLimiter.tryAcquire(timeout,
unit) (following the same timeout/unit used by GlobalPreemptibleStrategy) and
return false when tryAcquire times out; ensure the method returns true only on
successful acquisition and mirrors the non-blocking behavior when apiNonBlocking
is false by respecting the same timeout policy as GlobalPreemptibleStrategy.
---
Outside diff comments:
In `@framework/src/main/resources/config.conf`:
- Around line 744-754: The docs for the config property constantCallTimeoutMs
are inconsistent: the text currently says "must be a positive integer" but the
default and validation allow 0; update the wording to state it accepts zero
(e.g., "must be a non-negative integer (0 disables sharing and uses a per-call
deadline)") or "must be an integer ≥ 0" and mention the default is 0 so
operators aren't confused; ensure the change is applied to the
constantCallTimeoutMs description in config.conf.
In `@framework/src/test/java/org/tron/common/logsfilter/EventLoaderTest.java`:
- Around line 22-27: The test launchNativeQueue hardcodes port 5555 via
EventPluginConfig.setBindPort(5555), causing CI flakiness; change it to pick an
ephemeral free port at runtime by opening a ServerSocket on port 0 to obtain the
assigned port (socket.getLocalPort()), close the socket and then call
config.setBindPort(foundPort) so the test uses a free ephemeral port; update
launchNativeQueue to use this dynamic port acquisition before starting the
native queue.
---
Nitpick comments:
In `@common/src/main/java/org/tron/core/config/args/NodeConfig.java`:
- Around line 452-468: normalizeNonStandardKeys can throw
ConfigException.Missing when "discovery.external.ip" is absent; guard the
null/string check with a hasPath check. Modify normalizeNonStandardKeys to first
check section.hasPath("discovery.external.ip") before calling
section.getIsNull(...) or section.getString(...), and only call withValue(...)
to replace the value when the path exists and is null or equals "null"
(case-insensitive); keep the existing handling for "isOpenFullTcpDisconnect"
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e7decea-1803-4ee3-a394-a842f0902d1d
📒 Files selected for processing (46)
.github/scripts/check_reference_conf.py.github/workflows/pr-check.ymlcodecov.ymlcommon/src/main/java/org/tron/common/parameter/CommonParameter.javacommon/src/main/java/org/tron/core/config/args/CommitteeConfig.javacommon/src/main/java/org/tron/core/config/args/EventConfig.javacommon/src/main/java/org/tron/core/config/args/NodeConfig.javacommon/src/main/java/org/tron/core/config/args/Overlay.javacommon/src/main/java/org/tron/core/config/args/RateLimiterConfig.javacommon/src/main/java/org/tron/core/config/args/Storage.javacommon/src/main/java/org/tron/core/config/args/StorageConfig.javacommon/src/main/java/org/tron/core/config/args/VmConfig.javacommon/src/main/resources/reference.confcommon/src/test/java/org/tron/core/config/args/CommitteeConfigTest.javacommon/src/test/java/org/tron/core/config/args/EventConfigTest.javacommon/src/test/java/org/tron/core/config/args/NodeConfigTest.javacommon/src/test/java/org/tron/core/config/args/RateLimiterConfigTest.javacommon/src/test/java/org/tron/core/config/args/VmConfigTest.javaframework/src/main/java/org/tron/common/logsfilter/EventPluginLoader.javaframework/src/main/java/org/tron/core/config/args/Args.javaframework/src/main/java/org/tron/core/net/peer/PeerConnection.javaframework/src/main/java/org/tron/core/services/http/RateLimiterServlet.javaframework/src/main/java/org/tron/core/services/jsonrpc/JsonRpcServlet.javaframework/src/main/java/org/tron/core/services/ratelimiter/GlobalRateLimiter.javaframework/src/main/java/org/tron/core/services/ratelimiter/RateLimiterInterceptor.javaframework/src/main/java/org/tron/core/services/ratelimiter/adapter/DefaultBaseQqsAdapter.javaframework/src/main/java/org/tron/core/services/ratelimiter/adapter/GlobalPreemptibleAdapter.javaframework/src/main/java/org/tron/core/services/ratelimiter/adapter/IPQPSRateLimiterAdapter.javaframework/src/main/java/org/tron/core/services/ratelimiter/adapter/IRateLimiter.javaframework/src/main/java/org/tron/core/services/ratelimiter/adapter/QpsRateLimiterAdapter.javaframework/src/main/java/org/tron/core/services/ratelimiter/strategy/GlobalPreemptibleStrategy.javaframework/src/main/java/org/tron/core/services/ratelimiter/strategy/IPQpsStrategy.javaframework/src/main/java/org/tron/core/services/ratelimiter/strategy/QpsStrategy.javaframework/src/main/java/org/tron/program/FullNode.javaframework/src/main/resources/config.confframework/src/test/java/org/tron/common/ParameterTest.javaframework/src/test/java/org/tron/common/logsfilter/EventLoaderTest.javaframework/src/test/java/org/tron/core/config/args/ArgsTest.javaframework/src/test/java/org/tron/core/config/args/OverlayTest.javaframework/src/test/java/org/tron/core/net/messagehandler/ChainInventoryMsgHandlerTest.javaframework/src/test/java/org/tron/core/services/http/RateLimiterServletTest.javaframework/src/test/java/org/tron/core/services/jsonrpc/JsonRpcServletTest.javaframework/src/test/java/org/tron/core/services/ratelimiter/GlobalRateLimiterTest.javaframework/src/test/java/org/tron/core/services/ratelimiter/RateLimiterInterceptorTest.javaframework/src/test/java/org/tron/core/services/ratelimiter/adaptor/AdaptorTest.javaprotocol/src/main/protos/api/api.proto
💤 Files with no reviewable changes (7)
- protocol/src/main/protos/api/api.proto
- common/src/main/java/org/tron/core/config/args/Overlay.java
- framework/src/test/java/org/tron/core/config/args/OverlayTest.java
- common/src/main/java/org/tron/core/config/args/Storage.java
- codecov.yml
- common/src/test/java/org/tron/core/config/args/NodeConfigTest.java
- framework/src/test/java/org/tron/common/ParameterTest.java
| public boolean acquire(String ip) { | ||
| RateLimiter limiter = loadLimiter(ip); | ||
| if (limiter == null) { | ||
| return false; | ||
| } | ||
| limiter.acquire(); | ||
| return true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "IPQpsStrategy.java" -type fRepository: bladehan1/java-tron
Length of output: 152
🏁 Script executed:
cat -n ./framework/src/main/java/org/tron/core/services/ratelimiter/strategy/IPQpsStrategy.javaRepository: bladehan1/java-tron
Length of output: 2365
🏁 Script executed:
rg "\.acquire\(" --type java -B2 -A2 | head -100Repository: bladehan1/java-tron
Length of output: 9431
🏁 Script executed:
cat -n ./framework/src/main/java/org/tron/core/services/ratelimiter/GlobalRateLimiter.javaRepository: bladehan1/java-tron
Length of output: 2558
🏁 Script executed:
rg "acquirePermit\|\.acquire\(" --type java -l | head -20Repository: bladehan1/java-tron
Length of output: 45
🏁 Script executed:
rg "acquirePermit" --type java -B2 -A2 | head -80Repository: bladehan1/java-tron
Length of output: 8920
🏁 Script executed:
cat -n ./framework/src/main/java/org/tron/core/services/http/RateLimiterServlet.javaRepository: bladehan1/java-tron
Length of output: 7551
🏁 Script executed:
find . -name "IRateLimiter.java" -type fRepository: bladehan1/java-tron
Length of output: 150
🏁 Script executed:
cat -n ./framework/src/main/java/org/tron/core/services/ratelimiter/adapter/IRateLimiter.javaRepository: bladehan1/java-tron
Length of output: 612
🏁 Script executed:
rg "isRateLimiterApiNonBlocking" --type java -B2 -A2 | head -40Repository: bladehan1/java-tron
Length of output: 1258
🏁 Script executed:
rg "RateLimiter" --type java | grep -i "import\|timeout\|tryacquire" | head -20Repository: bladehan1/java-tron
Length of output: 2955
🏁 Script executed:
cat -n ./framework/src/main/java/org/tron/core/services/ratelimiter/adapter/IPQPSRateLimiterAdapter.javaRepository: bladehan1/java-tron
Length of output: 845
🌐 Web query:
Guava RateLimiter tryAcquire timeout semantics API documentation
💡 Result:
The Guava RateLimiter tryAcquire method with timeout parameters is designed to attempt to acquire a specified number of permits within a defined time limit [1][2][3]. Its semantics are as follows: 1. Acquisition Attempt: The method attempts to acquire the requested number of permits from the RateLimiter [1][2][3]. 2. Timeout Behavior: If the permits can be obtained without exceeding the specified timeout, the method acquires them and returns true [1][2][3]. If the permits cannot be granted before the timeout expires, the method returns false immediately without waiting further [1][2][3]. 3. Negative Timeouts: Any negative timeout values provided to the method are treated as zero, meaning the method will return false immediately if the permits are not available at that exact moment [1][2][3]. 4. Overloads: - tryAcquire(long timeout, TimeUnit unit): Attempts to acquire a single permit within the specified timeout [1][2][3]. - tryAcquire(int permits, long timeout, TimeUnit unit): Attempts to acquire the specified number of permits within the specified timeout [1][2][3]. - tryAcquire(Duration timeout): A more modern alternative (available since Guava 28.0) that accepts a java.time.Duration object [1][3]. The method is non-blocking in the sense that it will not wait longer than the specified duration, providing a mechanism to avoid indefinite blocking when permits are unavailable [1][2][3].
Citations:
- 1: https://guava.dev/releases/snapshot/api/docs/com/google/common/util/concurrent/RateLimiter.html
- 2: https://guava.dev/releases/19.0/api/docs/com/google/common/util/concurrent/RateLimiter.html
- 3: https://github.com/google/guava/blob/master/guava/src/com/google/common/util/concurrent/RateLimiter.java
Replace unbounded RateLimiter.acquire() with bounded tryAcquire(timeout, unit) to prevent thread starvation.
Line 34 calls RateLimiter.acquire(), which blocks indefinitely. When blocking mode is enabled (via isRateLimiterApiNonBlocking=false), this exhausts servlet worker threads during sustained overload. Use tryAcquire(timeout, unit) to fail fast on timeout.
Suggested patch
public class IPQpsStrategy extends Strategy {
+ public static final int DEFAULT_ACQUIRE_TIMEOUT_SECONDS = 2;
@@
public boolean acquire(String ip) {
RateLimiter limiter = loadLimiter(ip);
if (limiter == null) {
return false;
}
- limiter.acquire();
- return true;
+ return limiter.tryAcquire(DEFAULT_ACQUIRE_TIMEOUT_SECONDS, TimeUnit.SECONDS);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@framework/src/main/java/org/tron/core/services/ratelimiter/strategy/IPQpsStrategy.java`
around lines 29 - 35, The current IPQpsStrategy.acquire method calls
limiter.acquire() which can block indefinitely; change it to use a bounded call
limiter.tryAcquire(timeout, unit) and return false on timeout so threads don't
starve. Inside IPQpsStrategy.acquire, after obtaining the RateLimiter from
loadLimiter(ip), replace the blocking limiter.acquire() with
limiter.tryAcquire(<configuredTimeout>, <TimeUnit>) (use an existing
timeout/config property or add a private field like rateLimiterTimeout and
TimeUnit.MILLISECONDS) and if tryAcquire returns false then return false;
otherwise proceed and return true. Ensure behavior respects the
isRateLimiterApiNonBlocking flag (when non-blocking is required, use a short
timeout and fail fast) and reference the methods loadLimiter and acquire in your
change.
| public boolean acquire() { | ||
| rateLimiter.acquire(); | ||
| return true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "QpsStrategy.java" | head -20Repository: bladehan1/java-tron
Length of output: 150
🏁 Script executed:
find . -type f -name "GlobalPreemptibleStrategy.java" | head -20Repository: bladehan1/java-tron
Length of output: 164
🏁 Script executed:
cat -n ./framework/src/main/java/org/tron/core/services/ratelimiter/strategy/QpsStrategy.javaRepository: bladehan1/java-tron
Length of output: 1418
🏁 Script executed:
cat -n ./framework/src/main/java/org/tron/core/services/ratelimiter/strategy/GlobalPreemptibleStrategy.javaRepository: bladehan1/java-tron
Length of output: 2066
🏁 Script executed:
rg "\.acquire\(\)" --type java | grep -i qpsRepository: bladehan1/java-tron
Length of output: 410
🏁 Script executed:
rg "QpsStrategy" --type java -A 3 -B 3 | head -50Repository: bladehan1/java-tron
Length of output: 5896
🌐 Web query:
Guava RateLimiter tryAcquire timeout TimeUnit API
💡 Result:
The Guava RateLimiter class provides the tryAcquire method with timeout support to allow threads to attempt to acquire permits without blocking indefinitely [1][2][3]. API Signatures The RateLimiter class offers two primary overloaded methods for acquiring permits with a timeout [1][2]: 1. boolean tryAcquire(long timeout, TimeUnit unit): Attempts to acquire a single permit within the specified timeout [1][4]. 2. boolean tryAcquire(int permits, long timeout, TimeUnit unit): Attempts to acquire the specified number of permits within the specified timeout [1][2]. Behavior - If the requested permit(s) can be acquired before the timeout expires, the method returns true [1][3]. - If the permit(s) cannot be acquired within the specified timeout, the method returns false immediately upon the expiration of that duration [1][2]. - Negative timeout values are treated as zero, meaning the method will return false immediately if no permits are currently available [1][2][4]. - The method is thread-safe [5]. Example Usage The following example demonstrates how to use tryAcquire to avoid indefinite blocking [3]: RateLimiter rateLimiter = RateLimiter.create(1.0); // 1 permit per second boolean acquired = rateLimiter.tryAcquire(1, 500, TimeUnit.MILLISECONDS); if (acquired) { // Proceed with the task } else { // Handle the case where the permit could not be acquired in time } Important Considerations When using tryAcquire, ensure that the timeout duration is appropriate for your application's latency requirements [6]. If the timeout is set too high, the thread may block for longer than expected, potentially leading to thread pool exhaustion or increased response times under high load [6][7]. Conversely, if the timeout is too short, you may experience a high rate of false returns even when the system is operating within its configured limits [6].
Citations:
- 1: https://guava.dev/releases/snapshot/api/docs/com/google/common/util/concurrent/RateLimiter.html
- 2: https://guava.dev/releases/19.0/api/docs/com/google/common/util/concurrent/RateLimiter.html
- 3: https://www.baeldung.com/guava-rate-limiter
- 4: https://github.com/google/guava/blob/master/guava/src/com/google/common/util/concurrent/RateLimiter.java
- 5: https://guava.dev/releases/19.0/api/docs/src-html/com/google/common/util/concurrent/RateLimiter.html
- 6: https://stackoverflow.com/questions/66384775/guava-ratelimiter-doesnt-work-when-timeout-set-to-1-second
- 7: https://dev.to/asifthewebguy/api-rate-limiting-and-security-best-practices-for-2026-dfb
Use bounded wait instead of indefinite RateLimiter.acquire().
The acquire() method blocks indefinitely, which can stall request-handling threads under sustained rate pressure when apiNonBlocking=false. Use bounded tryAcquire(timeout, unit) and return false on timeout, matching the pattern in GlobalPreemptibleStrategy.
Suggested patch
import com.google.common.util.concurrent.RateLimiter;
import java.util.HashMap;
import java.util.Map;
+import java.util.concurrent.TimeUnit;
@@
public class QpsStrategy extends Strategy {
@@
+ public static final int DEFAULT_ACQUIRE_TIMEOUT_SECONDS = 2;
@@
public boolean acquire() {
- rateLimiter.acquire();
- return true;
+ return rateLimiter.tryAcquire(DEFAULT_ACQUIRE_TIMEOUT_SECONDS, TimeUnit.SECONDS);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public boolean acquire() { | |
| rateLimiter.acquire(); | |
| return true; | |
| public boolean acquire() { | |
| return rateLimiter.tryAcquire(DEFAULT_ACQUIRE_TIMEOUT_SECONDS, TimeUnit.SECONDS); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@framework/src/main/java/org/tron/core/services/ratelimiter/strategy/QpsStrategy.java`
around lines 33 - 35, Replace the indefinite blocking call in
QpsStrategy.acquire() with a bounded wait: use rateLimiter.tryAcquire(timeout,
unit) (following the same timeout/unit used by GlobalPreemptibleStrategy) and
return false when tryAcquire times out; ensure the method returns true only on
successful acquisition and mirrors the non-blocking behavior when apiNonBlocking
is false by respecting the same timeout policy as GlobalPreemptibleStrategy.
4 participants
What does this PR do?
Adds a CI gate that scans
common/src/main/resources/reference.confand fails the build on:^[a-z][a-zA-Z0-9]*$(lowerCamelCase).The check runs as a new step (
Validate reference.conf key names and depth) inside the existingcheckstylejob of.github/workflows/pr-check.yml, so it reuses the existing checkout and adds no new runner.Why are these changes required?
reference.confkeys must auto-bind toXxxConfig.javabean fields viaConfigBeanFactory, which requires lowerCamelCase. Without an automated gate the file has drifted (4 legacyPBFT*keys with leading uppercase, grandfathered via an in-script allowlist), and deeply nested hierarchies can creep in unnoticed (current max depth is 4; the gate allows up to 5 for headroom).This PR has been tested by:
reference.conf(exit 0, 266 keys, max depth 4 ≤ 5) and against synthetic bad inputs (1 format + 1 depth violation → exit 1, single consolidated::errorannotation emitted).Follow up
This PR is for fork-internal smoke-test of the gate itself, not for upstream. It validates that the new step runs on GHA against an unmodified
reference.confand exits clean.Extra details
node.http.PBFTEnable,node.http.PBFTPort,node.rpc.PBFTEnable,node.rpc.PBFTPort. Renaming them would break userconfig.conffiles and is tracked separately.python3already onubuntu-24.04-armrunners — nosetup-pythonaction.Summary by cubic
Adds a CI gate to validate
reference.confkeys (lowerCamelCase, depth ≤5) and introduces a switchable blocking mode for API rate limiting; also enforces a minimum event‑plugin version and improves JSON‑RPC compliance.New Features
.github/scripts/check_reference_conf.pyand runs it in thecheckstylejob to enforce per-segment^[a-z][a-zA-Z0-9]*$, max depth 5; four legacy keys are allowlisted (node.http.PBFTEnable,node.http.PBFTPort,node.rpc.PBFTEnable,node.rpc.PBFTPort). Emits one::errorwith all offenders and fails on violations.rate.limiter.apiNonBlocking(default false). Servlet and gRPC paths use a newacquirePermitthat chooses blocking or non‑blocking per this switch; per-endpoint checks still run before global limits.Bug Fixes
--esnow reliably starts event subscription by applying CLI before event config;useNativeQueuedefault restored to false.application/json-rpc, input limits are applied via a single binding path, and redundant config keys were removed; parser constraints now respect node settings.Written for commit 2e2f3e0. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Release Notes
New Features
Configuration Updates
Bug Fixes