feat(api): optimize and harden HTTP/JSON-RPC API layer#6693
Open
bladehan1 wants to merge 17 commits intotronprotocol:developfrom
Open
feat(api): optimize and harden HTTP/JSON-RPC API layer#6693bladehan1 wants to merge 17 commits intotronprotocol:developfrom
bladehan1 wants to merge 17 commits intotronprotocol:developfrom
Conversation
…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
bladehan1
changed the title
317787106
reviewed
Apr 20, 2026
Sunny6889
reviewed
Apr 20, 2026
Sunny6889
reviewed
Apr 20, 2026
Sunny6889
reviewed
Apr 20, 2026
Collaborator
|
[NIT]
|
0xbigapple
reviewed
Apr 20, 2026
0xbigapple
reviewed
Apr 20, 2026
2 tasks
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.
Collaborator
Author
Done |
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
6 participants
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 the full protobuf → JSON → object round-trip; construct the JSON field-by-field insteadClass.forNamewith a strict whitelist inRateLimiterServlet; unknown adapter names now fail-fast viaTronError(RATE_LIMITER_INIT)from@PostConstruct, so a misconfigured node aborts startup instead of silently running with the default limiterJsonRpcApiUtil.parseBlockNumber(blockNumOrTag)helper enforces a 100 hex-char max and maps parse failures toJsonRpcInvalidParamsException; used at five call sites inTronJsonRpcImpl/WalletWallet.broadcastTransactionNonUniqueObjectExceptionfrom ERROR to DEBUG inRpcApiService.getAssetIssueByName— thenamefield is no longer unique since the business-logic change, so collisions are an expected conditionWallet.getMerkleTreeOfBlockandWallet.deployContractWhy are these changes required?
Class.forNameallowed arbitrary class loading via a tampered config file. A failed adapter instantiation should abort startup (fail-fast) to surface the misconfiguration rather than silently degrade security policyBigIntegerparsing, enabling DoS. The length validation belongs in the API input layer, not in the generic low-level byte utility —ByteArray.hexToBigIntegeris left unchangedFollow-ups (not in this PR):
doPostentry. An earlier attempt added this as a catch inUtil.checkGetParam, but the utility layer conflated "malformed body" with "missing key" — rolled back per review, to be revisited at the correct layer in a separate PR.This PR has been tested by:
RateLimiterServletTest,UtilMockTest,JsonRpcApiUtilTest)Closes #6606