feat(api): optimize and harden HTTP/JSON-RPC API layer#20
Open
feat(api): optimize and harden HTTP/JSON-RPC API layer#20
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded input guards and tests for hex-to-BigInteger parsing; removed two Wallet APIs and lowered some log levels; replaced reflection-based rate-limiter adapter loading with a static whitelist and centralized builder (with tests); rebuilt block→JSON assembly and hardened JSON parsing; expanded unit tests across these areas. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
common/src/main/java/org/tron/common/utils/ByteArray.java (1)
163-176:⚠️ Potential issue | 🟠 MajorAdd validation for empty and malformed hex inputs to prevent unchecked parse exceptions.
The current guards only check
nulland prefix, allowing inputs like0x(empty hex),0xgg(invalid hex digits), and very large values to escape asNumberFormatExceptionat parse time instead of the declared exception types.
- Add length check to reject inputs where hex part is empty
- Wrap
Long.parseLong()andInteger.parseInt()in try-catch to convertNumberFormatExceptionto the declared exception typeProposed fix
public static long jsonHexToLong(String x) throws JsonRpcInvalidParamsException { - if (x == null || !x.startsWith("0x")) { + if (x == null || !x.startsWith("0x") || x.length() <= 2) { throw new JsonRpcInvalidParamsException("Incorrect hex syntax"); } - x = x.substring(2); - return Long.parseLong(x, 16); + try { + return Long.parseLong(x.substring(2), 16); + } catch (NumberFormatException e) { + throw new JsonRpcInvalidParamsException("Incorrect hex syntax"); + } } public static int jsonHexToInt(String x) throws Exception { - if (x == null || !x.startsWith("0x")) { + if (x == null || !x.startsWith("0x") || x.length() <= 2) { throw new Exception("Incorrect hex syntax"); } - x = x.substring(2); - return Integer.parseInt(x, 16); + try { + return Integer.parseInt(x.substring(2), 16); + } catch (NumberFormatException e) { + throw new Exception("Incorrect hex syntax", e); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/org/tron/common/utils/ByteArray.java` around lines 163 - 176, Both jsonHexToLong and jsonHexToInt accept "0x" prefix but don't validate empty or malformed hex bodies and let NumberFormatException leak; update jsonHexToLong(String x) and jsonHexToInt(String x) to (1) after stripping the "0x" prefix verify the remaining string length > 0 and throw JsonRpcInvalidParamsException (for jsonHexToLong) or Exception (for jsonHexToInt) with "Incorrect hex syntax" if empty, and (2) wrap Long.parseLong(...,16) and Integer.parseInt(...,16) in try-catch(NumberFormatException) that converts the parse error into the method's declared exception type with an "Incorrect hex syntax" message so invalid digits or out-of-range values are reported consistently.
🧹 Nitpick comments (4)
framework/src/main/java/org/tron/core/services/RpcApiService.java (1)
419-421: Consider adding metrics to track NonUniqueObjectException occurrences.Downgrading this exception from error to debug level reduces visibility into potential data integrity issues. While this change aligns WalletSolidityApi with the existing WalletApi pattern (Line 1582) and may reduce log noise, consider implementing a counter metric to track how often this condition occurs. This would maintain operational visibility without polluting error logs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/tron/core/services/RpcApiService.java` around lines 419 - 421, In the NonUniqueObjectException catch block inside RpcApiService (the catch handling NonUniqueObjectException that currently calls responseObserver.onNext(null) and logger.debug(...)), increment a dedicated counter metric each time this exception occurs so occurrences are tracked without raising log noise; add or reuse a Metric/Counter (e.g., nonUniqueObjectExceptionCounter) and call its increment method in that catch path before the responseObserver and debug log, ensuring the counter is declared/initialized on the RpcApiService class (or injected metrics registry) so it records counts for monitoring/alerts.framework/src/test/java/org/tron/core/services/http/RateLimiterServletWhitelistTest.java (1)
32-38: Exercise the init path instead of the private field.This only proves the shape of
ALLOWED_ADAPTERS. It will still pass ifaddRateContainer()stops instantiating one of these classes correctly, changes its error wrapping, or a whitelisted adapter loses the required(String)constructor. A small harness that drives servlet initialization with known/unknown strategies would protect the actual hardening behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/core/services/http/RateLimiterServletWhitelistTest.java` around lines 32 - 38, The test currently reads the private ALLOWED_ADAPTERS field directly; instead, change the test to exercise RateLimiterServlet.init(...) so it goes through addRateContainer() and validates real behavior: instantiate a RateLimiterServlet, call its init with a mock ServletConfig/ServletContext supplying init-params for a known good strategy and a known-bad strategy, assert the good strategy results in a working adapter (e.g., an instance retrievable or successful request handling) and the bad strategy is rejected/logged/throws as expected; reference RateLimiterServlet.init, addRateContainer(), and ALLOWED_ADAPTERS when updating the test to drive servlet initialization rather than reflectively reading the field.common/src/main/java/org/tron/common/utils/ByteArray.java (1)
151-160: ClarifyhexToBigIntegerinput contract (hex-only vs decimal fallback).This method now enforces a hex-length-based limit, but still accepts non-
0xdecimal input. That mixed contract is hard to reason about and may surprise callers.💡 Suggested direction (hex-only contract)
public static BigInteger hexToBigInteger(String input) { - if (input == null || input.length() > MAX_HEX_BIG_INTEGER_LEN) { + if (input == null || !input.startsWith("0x") + || input.length() > MAX_HEX_BIG_INTEGER_LEN) { throw new IllegalArgumentException( - "hex input is null or exceeds max length: " + MAX_HEX_BIG_INTEGER_LEN); + "hex input is invalid or exceeds max length: " + MAX_HEX_BIG_INTEGER_LEN); } - if (input.startsWith("0x")) { - return new BigInteger(input.substring(2), 16); - } else { - return new BigInteger(input, 10); - } + return new BigInteger(input.substring(2), 16); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/org/tron/common/utils/ByteArray.java` around lines 151 - 160, The hexToBigInteger method mixes a hex-length check with a decimal fallback which is confusing; change hexToBigInteger to be hex-only: require the input to start with "0x", validate length against MAX_HEX_BIG_INTEGER_LEN excluding the "0x" prefix, parse with new BigInteger(hexSubstring, 16), and throw IllegalArgumentException with a clear message when input is null, doesn't start with "0x", or exceeds the max hex length; update any existing callers or tests that relied on decimal parsing accordingly.framework/src/test/java/org/tron/core/services/jsonrpc/JsonRpcServletTest.java (1)
39-46: Avoid NPE-as-success assertions in pass-through tests.Using expected
NullPointerExceptionas proof of pass-through is brittle; unrelated NPEs would also pass. Prefer injecting a mockedJsonRpcServerand assertinghandle(...)invocation directly.Also applies to: 79-83, 92-96, 109-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/core/services/jsonrpc/JsonRpcServletTest.java` around lines 39 - 46, Replace the brittle NPE-based "pass-through" assertions in JsonRpcServletTest (where servlet.doPost(req, resp) is wrapped expecting a NullPointerException and relying on sendBatchExceedError not being called) by injecting a mocked JsonRpcServer into the servlet and verifying its handle(...) method is invoked; specifically, update the test cases around servlet.doPost to set a mock JsonRpcServer, call servlet.doPost(req, resp) without catching NPEs, and assert that JsonRpcServer.handle(...) was called and that sendBatchExceedError was not invoked, doing the same replacement for the other similar blocks (lines covering 79-83, 92-96, 109-113).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/src/main/java/org/tron/core/config/args/Args.java`:
- Around line 268-276: Args.java currently assigns raw integers from config into
PARAMETER.jsonRpcMaxBatchSize and PARAMETER.jsonRpcMaxAddressSize without
validation, allowing negative values (e.g., -1) to bypass enforcement that
checks only for >0. Fix by validating the retrieved int before assignment: after
reading config.getInt(ConfigKey.NODE_JSONRPC_MAX_BATCH_SIZE) and
config.getInt(ConfigKey.NODE_JSONRPC_MAX_ADDRESS_SIZE) check if value < 0 and if
so throw an IllegalArgumentException (or log an error and exit) with a clear
message including the config key and invalid value; otherwise assign to
PARAMETER.jsonRpcMaxBatchSize / PARAMETER.jsonRpcMaxAddressSize. Ensure these
checks are placed where the current assignments occur (the block handling
config.hasPath for those keys).
In `@framework/src/main/java/org/tron/core/services/http/RateLimiterServlet.java`:
- Around line 62-63: The servlet currently only falls back to defaults when item
is null, but leaves empty or blank strategy/params through which causes
initialization failure; in RateLimiterServlet change the logic that sets cName
and params to treat null or blank/empty strings as "missing" by checking
item.getStrategy() and item.getParams() for null/empty (e.g., trim().isEmpty()
or StringUtils.isBlank) and substitute DEFAULT_ADAPTER_NAME and
QpsStrategy.DEFAULT_QPS_PARAM respectively when blank; update the assignments
that reference item, cName and params to use these guarded checks so partial
configs behave like missing ones.
In `@framework/src/main/java/org/tron/core/services/jsonrpc/JsonRpcServlet.java`:
- Around line 83-88: The pre-parse call to MAPPER.readTree in the maxBatchSize
check can throw and bypass the JSON-RPC error handling; wrap the
MAPPER.readTree(body) call inside a try/catch that on parsing exceptions
delegates to rpcServer.handle (the same parse-error path used elsewhere in
doPost) instead of letting the exception escape, and only proceed to the size
check/sendBatchExceedError when parsing succeeds; reference the maxBatchSize
check, isBatchRequest(body), MAPPER.readTree(body), sendBatchExceedError(...),
rpcServer.handle(...), and doPost to locate and update the logic.
---
Outside diff comments:
In `@common/src/main/java/org/tron/common/utils/ByteArray.java`:
- Around line 163-176: Both jsonHexToLong and jsonHexToInt accept "0x" prefix
but don't validate empty or malformed hex bodies and let NumberFormatException
leak; update jsonHexToLong(String x) and jsonHexToInt(String x) to (1) after
stripping the "0x" prefix verify the remaining string length > 0 and throw
JsonRpcInvalidParamsException (for jsonHexToLong) or Exception (for
jsonHexToInt) with "Incorrect hex syntax" if empty, and (2) wrap
Long.parseLong(...,16) and Integer.parseInt(...,16) in
try-catch(NumberFormatException) that converts the parse error into the method's
declared exception type with an "Incorrect hex syntax" message so invalid digits
or out-of-range values are reported consistently.
---
Nitpick comments:
In `@common/src/main/java/org/tron/common/utils/ByteArray.java`:
- Around line 151-160: The hexToBigInteger method mixes a hex-length check with
a decimal fallback which is confusing; change hexToBigInteger to be hex-only:
require the input to start with "0x", validate length against
MAX_HEX_BIG_INTEGER_LEN excluding the "0x" prefix, parse with new
BigInteger(hexSubstring, 16), and throw IllegalArgumentException with a clear
message when input is null, doesn't start with "0x", or exceeds the max hex
length; update any existing callers or tests that relied on decimal parsing
accordingly.
In `@framework/src/main/java/org/tron/core/services/RpcApiService.java`:
- Around line 419-421: In the NonUniqueObjectException catch block inside
RpcApiService (the catch handling NonUniqueObjectException that currently calls
responseObserver.onNext(null) and logger.debug(...)), increment a dedicated
counter metric each time this exception occurs so occurrences are tracked
without raising log noise; add or reuse a Metric/Counter (e.g.,
nonUniqueObjectExceptionCounter) and call its increment method in that catch
path before the responseObserver and debug log, ensuring the counter is
declared/initialized on the RpcApiService class (or injected metrics registry)
so it records counts for monitoring/alerts.
In
`@framework/src/test/java/org/tron/core/services/http/RateLimiterServletWhitelistTest.java`:
- Around line 32-38: The test currently reads the private ALLOWED_ADAPTERS field
directly; instead, change the test to exercise RateLimiterServlet.init(...) so
it goes through addRateContainer() and validates real behavior: instantiate a
RateLimiterServlet, call its init with a mock ServletConfig/ServletContext
supplying init-params for a known good strategy and a known-bad strategy, assert
the good strategy results in a working adapter (e.g., an instance retrievable or
successful request handling) and the bad strategy is rejected/logged/throws as
expected; reference RateLimiterServlet.init, addRateContainer(), and
ALLOWED_ADAPTERS when updating the test to drive servlet initialization rather
than reflectively reading the field.
In
`@framework/src/test/java/org/tron/core/services/jsonrpc/JsonRpcServletTest.java`:
- Around line 39-46: Replace the brittle NPE-based "pass-through" assertions in
JsonRpcServletTest (where servlet.doPost(req, resp) is wrapped expecting a
NullPointerException and relying on sendBatchExceedError not being called) by
injecting a mocked JsonRpcServer into the servlet and verifying its handle(...)
method is invoked; specifically, update the test cases around servlet.doPost to
set a mock JsonRpcServer, call servlet.doPost(req, resp) without catching NPEs,
and assert that JsonRpcServer.handle(...) was called and that
sendBatchExceedError was not invoked, doing the same replacement for the other
similar blocks (lines covering 79-83, 92-96, 109-113).
🪄 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: 370b2fa7-8090-4971-ae76-d99ead4b3d4a
📒 Files selected for processing (15)
common/src/main/java/org/tron/common/parameter/CommonParameter.javacommon/src/main/java/org/tron/common/utils/ByteArray.javaframework/src/main/java/org/tron/core/Wallet.javaframework/src/main/java/org/tron/core/config/args/Args.javaframework/src/main/java/org/tron/core/config/args/ConfigKey.javaframework/src/main/java/org/tron/core/services/RpcApiService.javaframework/src/main/java/org/tron/core/services/http/RateLimiterServlet.javaframework/src/main/java/org/tron/core/services/http/Util.javaframework/src/main/java/org/tron/core/services/jsonrpc/JsonRpcServlet.javaframework/src/main/java/org/tron/core/services/jsonrpc/filters/LogFilter.javaframework/src/test/java/org/tron/common/utils/ByteArrayTest.javaframework/src/test/java/org/tron/core/jsonrpc/JsonRpcTest.javaframework/src/test/java/org/tron/core/services/http/RateLimiterServletWhitelistTest.javaframework/src/test/java/org/tron/core/services/http/UtilMockTest.javaframework/src/test/java/org/tron/core/services/jsonrpc/JsonRpcServletTest.java
💤 Files with no reviewable changes (1)
- framework/src/main/java/org/tron/core/Wallet.java
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 15 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="framework/src/main/java/org/tron/core/services/http/RateLimiterServlet.java">
<violation number="1" location="framework/src/main/java/org/tron/core/services/http/RateLimiterServlet.java:62">
P2: When `item` is non-null but `item.getStrategy()` is null or empty, `ALLOWED_ADAPTERS.get(cName)` returns null and throws, causing a startup regression for previously tolerated configs. Fall back to the default adapter when the strategy field is blank.</violation>
</file>
<file name="framework/src/test/java/org/tron/core/services/jsonrpc/JsonRpcServletTest.java">
<violation number="1" location="framework/src/test/java/org/tron/core/services/jsonrpc/JsonRpcServletTest.java:41">
P2: These pass-through tests are non-verifying: they catch `NullPointerException` but assert nothing if no exception occurs, so regressions can slip through while tests still pass.</violation>
</file>
<file name="framework/src/main/java/org/tron/core/services/jsonrpc/JsonRpcServlet.java">
<violation number="1" location="framework/src/main/java/org/tron/core/services/jsonrpc/JsonRpcServlet.java:84">
P1: Handle JSON parse failures in batch-size precheck; otherwise malformed batch payloads escape JSON-RPC error handling and can return a generic server error.</violation>
</file>
<file name="framework/src/main/java/org/tron/core/config/args/Args.java">
<violation number="1" location="framework/src/main/java/org/tron/core/config/args/Args.java:269">
P2: Negative config values silently disable protections. Since enforcement uses `maxBatchSize > 0` / `maxAddressSize > 0`, setting `-1` in config bypasses both the batch-size and address-size limits. Add a validation guard rejecting negative values (e.g., throw `IllegalArgumentException` if `< 0`).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
bladehan1
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
framework/src/test/java/org/tron/core/services/http/UtilMockTest.java (1)
261-273: Consider extracting a helper for mocked request bodies.The anonymous
ServletInputStreamsetup is duplicated across both tests. A small helper (e.g.,mockRequestBody(String body)) would reduce repetition and improve readability.Also applies to: 287-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/core/services/http/UtilMockTest.java` around lines 261 - 273, The test duplicates an anonymous ServletInputStream when stubbing request.getInputStream(); extract that logic into a reusable helper (e.g., mockRequestBody(String body)) inside the UtilMockTest class and replace both anonymous implementations with calls to mockRequestBody("not a json") (and the other test's body) so request.getInputStream() returns the same mocked ServletInputStream; ensure the helper constructs a javax.servlet.ServletInputStream wrapping a ByteArrayInputStream and implements read(), isFinished(), isReady(), and setReadListener() just like the current anonymous inner classes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/src/test/java/org/tron/common/utils/ByteArrayTest.java`:
- Around line 111-117: The test uses a 100-char hex length cap but the code
should enforce uint256-size inputs (0x + 64 hex chars = 66 total); update the
constant MAX_HEX_BIG_INTEGER_LEN in ByteArray (e.g., set to 66) and adjust the
ByteArrayTest expectations and fixtures (replace the 98/99 char builders and the
assertEquals(100) with builders/assertions that produce 64 hex chars plus "0x"
and assertEquals(66)) so ByteArray.hexToBigInteger is validated against the
correct uint256-length boundary.
In `@framework/src/test/java/org/tron/core/services/http/UtilMockTest.java`:
- Around line 266-272: The anonymous ServletInputStream overrides in
UtilMockTest (methods read(), isFinished(), isReady(), setReadListener()) use
one-line method bodies that violate LeftCurly/RightCurlyAlone checkstyle; change
each one-line override into a multi-line form with the opening brace on its own
line and the closing brace alone on its line and place the return or empty body
on the next line (e.g., convert "public boolean isFinished() { return false; }"
to a three-line form), and apply the same fix at both occurrences (around the
overrides at lines shown in the diff).
- Around line 15-17: Reorder the imports in UtilMockTest so they satisfy
CustomImportOrder: move the javax.servlet.http.HttpServletRequest import to
appear before org.mockito.Mockito; i.e., update the import block in the
UtilMockTest file so javax.servlet.http.HttpServletRequest is listed prior to
org.mockito.Mockito to fix the Checkstyle import order violation.
---
Nitpick comments:
In `@framework/src/test/java/org/tron/core/services/http/UtilMockTest.java`:
- Around line 261-273: The test duplicates an anonymous ServletInputStream when
stubbing request.getInputStream(); extract that logic into a reusable helper
(e.g., mockRequestBody(String body)) inside the UtilMockTest class and replace
both anonymous implementations with calls to mockRequestBody("not a json") (and
the other test's body) so request.getInputStream() returns the same mocked
ServletInputStream; ensure the helper constructs a
javax.servlet.ServletInputStream wrapping a ByteArrayInputStream and implements
read(), isFinished(), isReady(), and setReadListener() just like the current
anonymous inner classes.
🪄 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: c37eeadd-2dd2-4845-963c-2a26f114b25a
📒 Files selected for processing (8)
common/src/main/java/org/tron/common/utils/ByteArray.javaframework/src/main/java/org/tron/core/Wallet.javaframework/src/main/java/org/tron/core/services/RpcApiService.javaframework/src/main/java/org/tron/core/services/http/RateLimiterServlet.javaframework/src/main/java/org/tron/core/services/http/Util.javaframework/src/test/java/org/tron/common/utils/ByteArrayTest.javaframework/src/test/java/org/tron/core/services/http/RateLimiterServletWhitelistTest.javaframework/src/test/java/org/tron/core/services/http/UtilMockTest.java
💤 Files with no reviewable changes (1)
- framework/src/main/java/org/tron/core/Wallet.java
✅ Files skipped from review due to trivial changes (1)
- framework/src/main/java/org/tron/core/services/RpcApiService.java
🚧 Files skipped from review as they are similar to previous changes (3)
- framework/src/main/java/org/tron/core/services/http/Util.java
- framework/src/test/java/org/tron/core/services/http/RateLimiterServletWhitelistTest.java
- common/src/main/java/org/tron/common/utils/ByteArray.java
bladehan1
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
framework/src/test/java/org/tron/common/utils/ByteArrayTest.java (1)
142-147: Add decimal length boundary assertions to prevent regressions.This covers decimal parsing correctness, but it doesn’t yet verify the unified max-length guard for decimal inputs (exactly-at-limit vs over-limit), which is part of the intended behavior.
Suggested test extension
`@Test` public void testHexToBigIntegerDecimalPath() { assertEquals(BigInteger.valueOf(12345), ByteArray.hexToBigInteger("12345")); assertEquals(BigInteger.ZERO, ByteArray.hexToBigInteger("0")); assertEquals(BigInteger.ONE, ByteArray.hexToBigInteger("1")); + + // decimal at limit (100 chars) should pass + String decimalAtLimit = new String(new char[100]).replace('\0', '9'); + ByteArray.hexToBigInteger(decimalAtLimit); + + // decimal over limit (101 chars) should fail + String decimalTooLong = "1" + new String(new char[100]).replace('\0', '0'); + IllegalArgumentException ex = assertThrows( + IllegalArgumentException.class, + () -> ByteArray.hexToBigInteger(decimalTooLong)); + assertTrue(ex.getMessage().contains("exceeds max length")); }Based on learnings: the 100-char cap is a unified upper bound for both hex and decimal representations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/common/utils/ByteArrayTest.java` around lines 142 - 147, Extend testHexToBigIntegerDecimalPath to assert the unified 100-character input cap for decimal strings: construct a 100-char decimal string (e.g., 100 '9' characters) and assert ByteArray.hexToBigInteger(...) parses it successfully to a BigInteger of that value, and construct a 101-char decimal string (101 '9's) and assert that ByteArray.hexToBigInteger(...) rejects it (use assertThrows expecting the method's current exception type, e.g., IllegalArgumentException). Reference the test method name testHexToBigIntegerDecimalPath and the target method ByteArray.hexToBigInteger when adding these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@framework/src/test/java/org/tron/common/utils/ByteArrayTest.java`:
- Around line 142-147: Extend testHexToBigIntegerDecimalPath to assert the
unified 100-character input cap for decimal strings: construct a 100-char
decimal string (e.g., 100 '9' characters) and assert
ByteArray.hexToBigInteger(...) parses it successfully to a BigInteger of that
value, and construct a 101-char decimal string (101 '9's) and assert that
ByteArray.hexToBigInteger(...) rejects it (use assertThrows expecting the
method's current exception type, e.g., IllegalArgumentException). Reference the
test method name testHexToBigIntegerDecimalPath and the target method
ByteArray.hexToBigInteger when adding these assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11b89e34-9b71-4159-8524-1414332c6b23
📒 Files selected for processing (8)
common/src/main/java/org/tron/common/utils/ByteArray.javaframework/src/main/java/org/tron/core/Wallet.javaframework/src/main/java/org/tron/core/services/RpcApiService.javaframework/src/main/java/org/tron/core/services/http/RateLimiterServlet.javaframework/src/main/java/org/tron/core/services/http/Util.javaframework/src/test/java/org/tron/common/utils/ByteArrayTest.javaframework/src/test/java/org/tron/core/services/http/RateLimiterServletWhitelistTest.javaframework/src/test/java/org/tron/core/services/http/UtilMockTest.java
💤 Files with no reviewable changes (1)
- framework/src/main/java/org/tron/core/Wallet.java
✅ Files skipped from review due to trivial changes (3)
- framework/src/main/java/org/tron/core/services/RpcApiService.java
- framework/src/main/java/org/tron/core/services/http/Util.java
- framework/src/test/java/org/tron/core/services/http/RateLimiterServletWhitelistTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- framework/src/test/java/org/tron/core/services/http/UtilMockTest.java
- common/src/main/java/org/tron/common/utils/ByteArray.java
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
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/src/main/java/org/tron/common/utils/ByteArray.java">
<violation number="1">
P1: `jsonHexToLong` now throws `NullPointerException` for null input because the null check was removed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…llet.deployContract Both methods have zero callers in production code. getMerkleTreeOfBlock was an orphaned shielded transaction helper; deployContract was a no-op stub that simply returned the input transaction unchanged.
Replace reflection-based adapter loading with a static whitelist map to prevent arbitrary class instantiation via tampered config. Only pre-approved rate limiter adapters can now be loaded.
…intBlockToJSON 1. remove redundant full BlockList serialization in printBlockList 2. serialize only block_header instead of entire block in printBlockToJSON 3. enhance testPrintBlockList to verify JSON structure after refactoring
…og level Wrap JSON.parseObject in checkGetParam with try-catch to gracefully handle invalid JSON input instead of propagating the exception. Downgrade Solidity NonUniqueObjectException log from error to debug as it is an expected condition. Add tests for invalid JSON and JSON array inputs.
BigInteger(String, 16) has O(n²) complexity on large inputs. A multi-MB hex string within the 4MB HTTP body limit can consume seconds of CPU. Add a 66-char limit (0x + 64 hex = uint256 max) to hexToBigInteger, covering all 6 JSON-RPC call sites. Also add null checks to jsonHexToLong and jsonHexToInt (no length guards needed — parseLong and parseInt self-protect via per-character overflow detection).
…quality - Relax MAX_HEX_BIG_INTEGER_LEN from 66 to 100 to cover both hex and decimal uint256 representations - Split null/length error messages in hexToBigInteger for clarity - Change ALLOWED_ADAPTERS to package-private, remove reflection from RateLimiterServletWhitelistTest - Remove unused BufferedReader in UtilMockTest - Add test for empty string input to hexToBigInteger - Add test for printBlockToJSON with empty transactions
- Add comment explaining intentional MAX_HEX_BIG_INTEGER_LEN = 100 - Fall back to default adapter when strategy/params is null or empty in RateLimiterServlet to avoid startup regression - Fix import order in UtilMockTest (javax before org) - Expand one-line ServletInputStream overrides to satisfy checkstyle
…ger tests Cover two gaps identified in review: - printBlockToJSON with a block containing transactions verifies the transactions array structure (txID, raw_data) - hexToBigInteger decimal path verifies the non-hex branch works correctly for "0", "1", and "12345"
…null guards - Wallet: downgrade broadcast user-input validation failures from WARN to INFO (sig error, contract validate, resource insufficient, etc. are expected client errors) - RateLimiterServlet: unknown/invalid adapter falls back to default with WARN log instead of throwing TronError - ByteArray: remove redundant null checks in jsonHexToLong/jsonHexToInt - Util: simplify JSONException catch to use unqualified import - Tests: add rate limiter fallback coverage; assert block_header.raw_data presence; rename test method for clarity
- RateLimiterServletWhitelistTest: replace assertFalse(true) with fail() - ByteArrayTest: simplify testJsonHexToInt_ValidHex to use throws instead of try/catch; add NPE tests for jsonHexToLong/jsonHexToInt null input after null-check removal - UtilMockTest: extract duplicated ServletInputStream into mockPostJsonRequest helper
Address PR review (Sunny6889/waynercheung) separating concerns between low-level byte utility and API-level validation. - ByteArray.hexToBigInteger: revert length/null guards; the utility is generic and should not encode API-specific DoS policy. - JsonRpcApiUtil: new parseBlockNumber(blockNumOrTag) helper enforces MAX_BLOCK_NUM_HEX_LEN (100) and maps parse failures to JsonRpcInvalidParamsException with a stable "invalid block number" message. Comment explains the DoS motivation explicitly. - TronJsonRpcImpl / Wallet: use parseBlockNumber at five call sites, removing duplicated try/catch-and-wrap blocks. - Wallet: drop stale IncrementalMerkleTree import left over from getMerkleTreeOfBlock removal. - Util.checkGetParam: rollback JSONException catch (waynercheung MUST); malformed JSON must not be silently treated as missing keys. - ByteArrayTest: replace long2Bytes System.out.println with assertEquals; move length/null tests to JsonRpcApiUtilTest where the guard now lives. - JsonRpcApiUtilTest: new tests cover hex, decimal, max-length (100), oversized (101), null, malformed hex, and empty string.
Pre-whitelist code used Class.forName(ADAPTER_PREFIX + cName), so a misspelled or unknown strategy name raised ClassNotFoundException, which addRateContainer's catch lifted into TronError and aborted Spring startup. The whitelist refactor softened this to warn+fallback and masked misconfigured nodes. Restore original semantics: - buildAdapter throws TronError (via throwTronError) when the name is not in the whitelist or the constructor fails; the no-fallback behavior matches the original Class.forName chain exactly. - addRateContainer branches strictly on item == null (original behavior) rather than per-field null/empty defaulting; a configured but empty strategy name is a configuration bug and fails fast. - Drop the now-invalid `throws Exception` on buildAdapter; make throwTronError static and return TronError so callers use `throw throwTronError(...)` for compiler reachability. Tests: - Rename RateLimiterServletWhitelistTest -> RateLimiterServletTest to match the <ProductionClass>Test convention used elsewhere (UtilTest, UpdateAccountServletTest, etc.). - Replace fallback-assertion test with testUnknownAdapterThrowsTronError asserting RATE_LIMITER_INIT and servlet/adapter name in message. - Rename the DEFAULT_ADAPTER_NAME happy-path test for clarity. - Add testEmptyAdapterNameThrowsTronError covering the empty-strategy fail-fast branch reached via item != null with empty getStrategy(). - Add testBuildsEachWhitelistedAdapter exercising newInstance(String) for QpsRateLimiterAdapter, IPQPSRateLimiterAdapter, and GlobalPreemptibleAdapter so a signature or strategy-class break on any entry fails in CI instead of at node startup.
There was a problem hiding this comment.
5 issues found across 11 files (changes from recent commits).
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="framework/src/main/java/org/tron/core/services/http/Util.java">
<violation number="1">
P2: Handle `JSONException` when parsing request JSON; otherwise malformed POST payloads can propagate as server errors instead of returning `null`.</violation>
</file>
<file name="framework/src/main/java/org/tron/core/services/jsonrpc/JsonRpcApiUtil.java">
<violation number="1" location="framework/src/main/java/org/tron/core/services/jsonrpc/JsonRpcApiUtil.java:539">
P1: `parseBlockNumber` does not enforce non-negative `long` range, so oversized inputs can overflow/truncate when callers use `.longValue()` and resolve the wrong block.</violation>
</file>
<file name="framework/src/main/java/org/tron/core/services/http/RateLimiterServlet.java">
<violation number="1" location="framework/src/main/java/org/tron/core/services/http/RateLimiterServlet.java:68">
P2: `strategy`/`params` no longer default when empty, so partially configured rate-limiter entries can now fail servlet initialization.</violation>
<violation number="2" location="framework/src/main/java/org/tron/core/services/http/RateLimiterServlet.java:75">
P1: Adapter instantiation failures now throw instead of falling back to the default limiter, reducing fault tolerance for misconfigured params.</violation>
<violation number="3" location="framework/src/main/java/org/tron/core/services/http/RateLimiterServlet.java:82">
P1: Unknown adapter names now throw `TronError` instead of falling back to the default limiter, making config mistakes fatal.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| try { | ||
| container.add(KEY_PREFIX_HTTP, name, buildAdapter(cName, params, name)); | ||
| } catch (Exception e) { | ||
| throw throwTronError(cName, params, name, e); |
There was a problem hiding this comment.
P1: Adapter instantiation failures now throw instead of falling back to the default limiter, reducing fault tolerance for misconfigured params.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/core/services/http/RateLimiterServlet.java, line 75:
<comment>Adapter instantiation failures now throw instead of falling back to the default limiter, reducing fault tolerance for misconfigured params.</comment>
<file context>
@@ -55,42 +55,44 @@ public abstract class RateLimiterServlet extends HttpServlet {
container.add(KEY_PREFIX_HTTP, name, buildAdapter(cName, params, name));
} catch (Exception e) {
- this.throwTronError(cName, params, name, e);
+ throw throwTronError(cName, params, name, e);
}
}
</file context>
| static IRateLimiter buildAdapter(String cName, String params, String name) { | ||
| Class<? extends IRateLimiter> c = ALLOWED_ADAPTERS.get(cName); | ||
| if (c == null) { | ||
| throw throwTronError(cName, params, name, |
There was a problem hiding this comment.
P1: Unknown adapter names now throw TronError instead of falling back to the default limiter, making config mistakes fatal.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/core/services/http/RateLimiterServlet.java, line 82:
<comment>Unknown adapter names now throw `TronError` instead of falling back to the default limiter, making config mistakes fatal.</comment>
<file context>
@@ -55,42 +55,44 @@ public abstract class RateLimiterServlet extends HttpServlet {
- logger.warn("Unknown rate limiter adapter '{}' for servlet '{}', "
- + "falling back to default.", cName, name);
+ if (c == null) {
+ throw throwTronError(cName, params, name,
+ new IllegalArgumentException("unknown rate limiter adapter; allowed="
+ + ALLOWED_ADAPTERS.keySet()));
</file context>
| @@ -95,7 +95,7 @@ public static String printErrorMsg(Exception e) { | |||
|
|
|||
There was a problem hiding this comment.
P2: Handle JSONException when parsing request JSON; otherwise malformed POST payloads can propagate as server errors instead of returning null.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/core/services/http/Util.java, line 546:
<comment>Handle `JSONException` when parsing request JSON; otherwise malformed POST payloads can propagate as server errors instead of returning `null`.</comment>
<file context>
@@ -543,13 +543,9 @@ private static String checkGetParam(HttpServletRequest request, String key) thro
- }
- } catch (JSONException e) {
- logger.debug("Invalid JSON input: {}", e.getMessage());
+ JSONObject jsonObject = JSON.parseObject(value);
+ if (jsonObject != null) {
+ return jsonObject.getString(key);
</file context>
Suggested change
| JSONObject jsonObject; | |
| try { | |
| jsonObject = JSON.parseObject(value); | |
| } catch (JSONException e) { | |
| logger.debug("Invalid JSON input: {}", e.getMessage()); | |
| return null; | |
| } |
Comment on lines
+68
to
+69
| cName = item.getStrategy(); | ||
| params = item.getParams(); |
There was a problem hiding this comment.
P2: strategy/params no longer default when empty, so partially configured rate-limiter entries can now fail servlet initialization.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/core/services/http/RateLimiterServlet.java, line 68:
<comment>`strategy`/`params` no longer default when empty, so partially configured rate-limiter entries can now fail servlet initialization.</comment>
<file context>
@@ -55,42 +55,44 @@ public abstract class RateLimiterServlet extends HttpServlet {
+ cName = DEFAULT_ADAPTER_NAME;
+ params = QpsStrategy.DEFAULT_QPS_PARAM;
+ } else {
+ cName = item.getStrategy();
+ params = item.getParams();
+ }
</file context>
Suggested change
| cName = item.getStrategy(); | |
| params = item.getParams(); | |
| cName = Strings.isNullOrEmpty(item.getStrategy()) | |
| ? DEFAULT_ADAPTER_NAME | |
| : item.getStrategy(); | |
| params = Strings.isNullOrEmpty(item.getParams()) | |
| ? QpsStrategy.DEFAULT_QPS_PARAM | |
| : item.getParams(); |
Add two regression tests that pin the optimized printBlockToJSON path
against the visible=true/false semantics of the prior implementation,
which the field-by-field assembly now has to thread through JsonFormat
explicitly.
- testPrintBlockToJSONVisibleFalse: assert structural invariants
(blockID, block_header, transactions) under visible=false, mirroring
the existing visible=true cases.
- testPrintBlockToJSONVisibleFlagAffectsAddressEncoding: build a block
with a mainnet 0x41-prefixed witness_address and assert
- blockID is identical under either flag (byte-level hash),
- block_header JSON differs between the two (witness_address is
re-encoded), and
- visible=true renders witness_address as Base58 ("T...") while
visible=false does not.
The hexToBigInteger length guard and tests were moved to JsonRpcApiUtil in 4c6df2d; ByteArray.java net diff is already zero. Remaining ByteArrayTest edits (System.out.println -> assertEquals cleanup, new null/empty/decimal tests for unchanged ByteArray methods) are drive-by refactors unrelated to the PR goal. Restore the file to develop.
Coverage added/consolidated around the hand-rolled printBlockToJSON from 0980488 so future regressions surface in tests, not at runtime. - Merge testPrintBlockToJSONVisibleFalse into testPrintBlockToJSONWithTransactions via a boolean[]{true,false} loop; flag-driven encoding difference remains covered by testPrintBlockToJSONVisibleFlagAffectsAddressEncoding. - testPrintBlockToJSONTransactionsKeyMatchesLegacyImpl: pin parity with the prior JsonFormat.printToString(block, ...) output for the transactions-key presence in both empty and non-empty cases. - testPrintBlockToJSONCoversAllProtoTopLevelFields: reflect over Block's descriptor to fail fast if a new top-level proto field is added without extending the hand-rolled assembler.
parseBlockNumber previously returned BigInteger, letting callers narrow with .longValue() and silently wrap past Long.MAX_VALUE. Return long directly and enforce two invariants at the API boundary so corrupted inputs never reach downstream block lookups: - signum check rejects negative inputs (both "-1" and "0x-1"), a protocol-level constraint block numbers can never violate - longValueExact rejects values that overflow signed 64-bit, including uint64 wraparounds like 0xffffffffffffffff Callers in Wallet and TronJsonRpcImpl drop the obsolete .longValue() call; the other three call sites already discarded the return value. Regression tests cover negatives, 0x7fffffffffffffff (max long), 0x8000000000000000 (just past), and 0xffffffffffffffff (uint64).
Replace the raw Class[] literal (which the compiler cannot type-check and emits an unchecked-conversion warning for) with Arrays.asList so the element type Class<? extends IRateLimiter> is inferred explicitly. No behavior change; ALLOWED_ADAPTERS keeps the same four entries.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
printBlockList/printBlockToJSON: remove full protobuf → JSON → object round-trip; construct JSON field-by-field insteadClass.forNamewith a strict whitelist inRateLimiterServlet; unknown or misconfigured adapters fall back to the default limiter instead of throwingByteArray.hexToBigIntegerto reject oversized hex inputJSONExceptioninUtil.checkGetParamso malformed POST bodies return null instead of propagating an exceptionWallet.broadcastTransactionWallet.getMerkleTreeOfBlockandWallet.deployContractWhy are these changes required?
Class.forNameallows arbitrary class loading via a tampered config file; failed adapter instantiation should not crash the serverBigIntegerparsing, enabling DoSJSONExceptionpropagated as an ERROR-level stack trace for every malformed client request, polluting logsThis PR has been tested by:
Closes tronprotocol#6606