Repository navigation
fix(jsonrpc): handle null parameters consistently - #6984
waynercheung wants to merge 4 commits into
Conversation
| }) | ||
| boolean uninstallFilter(String filterId) throws JsonRpcInvalidParamsException, | ||
| JsonRpcMethodNotFoundException, ItemNotFoundException; | ||
| JsonRpcMethodNotFoundException; |
There was a problem hiding this comment.
[NIT] Preserve source compatibility for uninstallFilter
Returning false on a lookup miss only requires the implementation to stop throwing ItemNotFoundException; removing the checked exception from this public interface also makes existing source callers that catch it and third-party implementations that still declare it fail to recompile. That is a separate compatibility cost from the intended JSON-RPC wire change.
Suggestion: Keep the existing ItemNotFoundException declaration and error mapping in this release while leaving the implementation non-throwing, and remove them only in a separately announced compatibility cleanup.
There was a problem hiding this comment.
Thanks for raising the source-compatibility concern. I agree that removing a checked exception can require Java callers that catch it, or implementations that declare it, to change when recompiled.
I'd prefer to keep the removal here. My understanding is that TronJsonRpc is an internal jsonrpc4j binding for the node's JSON-RPC endpoint, whose compatibility contract is the JSON-RPC wire protocol. TronJsonRpcImpl is its only implementation in the repository, and there are no direct calls to uninstallFilter in the main source set.
There is also precedent for changing this interface's exception declarations: #6732, included in GreatVoyage-v4.8.2, added JsonRpcExceedLimitException to newFilter without a deprecation step.
The built-in implementation no longer throws ItemNotFoundException: an unknown or already removed filter ID returns false. Removing the declaration and its -32000 mapping keeps the binding consistent with that behavior. The PR description already records the potential source-compatibility impact.
If maintaining source compatibility for external users of this Java interface is an intended project commitment, I can restore the declaration and the mapping. Otherwise, I would keep their removal in this PR.
|
Updated in |
| } | ||
|
|
||
| private static final String FILTER_NOT_FOUND = "filter not found"; | ||
| private static final String INVALID_PARAMS = "invalid params"; |
There was a problem hiding this comment.
[NIT] Minor follow-ups — 2 items rolled up
Grouped as one comment since both are small, non-blocking nits from this review.
-
Constant placement inconsistency.
INVALID_PARAMSis private inTronJsonRpcImpl(framework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpcImpl.java:125) whileINVALID_FILTER_REQUESTis shared viaJsonRpcApiUtil(framework/src/main/java/org/tron/core/services/jsonrpc/JsonRpcApiUtil.java:66) — both are user-visible -32602 messages. The split has a reason (theLogFilterconstructor needs cross-class access), but it is implicit, and future null-check sites may drift in wording or add yet another copy. Consider unifying these message constants inJsonRpcApiUtilin a follow-up cleanup. -
Unrestored dynamic property in test. In
JsonrpcServiceTest.executeBuiltTransaction(framework/src/test/java/org/tron/core/jsonrpc/JsonrpcServiceTest.java:2218),saveMaxCpuTimeOfOneTx(5_000L)is written but never restored, unlike the symmetric save/restore ofConfigLoader.disableandVMConfig.initVmHardForkin the same try block. SinceJsonrpcServiceTest extends BaseTestshares the Spring context and DB at class level, the value persists for later tests in the class. It is currently benign (more permissive direction, no downstream consumer), but it violates the "who pollutes, who cleans up" convention and can become a hidden ordering dependency for future CPU-limit-sensitive tests. Restore the original value in the finally block, as done forConfigLoader.disable.
Suggestion: unify the -32602 message constants in a follow-up cleanup, and restore maxCpuTimeOfOneTx in the test's finally block.
There was a problem hiding this comment.
Thanks for the review.
-
Addressed in c74d9ea, although in the other direction from moving them into
JsonRpcApiUtil. After removing the unreachableLogFilterconstructor check (discussion),INVALID_FILTER_REQUESTis only used byTronJsonRpcImpl. Both messages are now private constants there, together with the class's other method-level messages such asFILTER_NOT_FOUNDandJSON_ERROR, whileJsonRpcApiUtilkeeps the messages thrown by its own helpers, such asBLOCK_NUM_ERROR.JsonRpcApiUtilis no longer changed by this PR. The ten required-parameter null checks go through onerequireParamhelper with these two constants, and the message strings are unchanged. -
The CPU-limit write is inside an uncommitted revoking-store session (
try (ISession ignored = dbManager.getRevokingStore().buildSession())). The store is enabled duringManagerinitialization, so closing the session without a commit rolls the write back on both normal and exceptional exit. A probe on this branch read5000inside the session and the original50after it closed.ConfigLoader.disableand the hard-fork flag set byVMConfig.initVmHardFork(CommonParameter.ENERGY_LIMIT_HARD_FORK) are static values outside that mechanism, which is why only those two are restored infinally.This close-without-commit behavior is already covered by
SnapshotManagerTest.testClose. Given the existing session scope here, I suggest keeping the helper unchanged rather than adding a second restoration mechanism.
| return (b == null ? null : getBlockResult(b, fullTransactionObjects)); | ||
| } | ||
|
|
||
| private static void requireFullTransactionObjects(Boolean fullTransactionObjects) |
There was a problem hiding this comment.
[NIT] Optional: the "null means invalid params" rule is now written in three ways:
- six inline
if (x == null) throw new JsonRpcInvalidParamsException(INVALID_PARAMS)blocks (L702, L1034, L1369, L1536, L1562, L1605) requireFullTransactionObjectshere- two inline guards with
INVALID_FILTER_REQUEST(L1481, L1587)
One private static <T> T requireParam(T value, String message) would cover all of them. Keep both messages, since the tests assert them.
Related: ethGetBlockByHash (L366-368) re-inlines getBlockByJsonHash so it can check the flag before the lookup. A small getBlockByHash(byte[]) used by both would mirror the parseBlockSelector / getBlockBySelector split on the number path.
There was a problem hiding this comment.
Thanks, applied in c74d9ea. All ten call sites now use private static <T> T requireParam(T value, String message), and both messages are kept as they are. ethGetBlockByHash and getBlockByJsonHash share a new getBlockByHash(byte[]), so ethGetBlockByHash no longer inlines the lookup, mirroring the parseBlockSelector / getBlockBySelector split on the number path. The validation order is unchanged.
| * construct one LogFilter from part parameters of FilterRequest | ||
| */ | ||
| public LogFilter(FilterRequest fr) throws JsonRpcInvalidParamsException { | ||
| if (fr == null) { |
There was a problem hiding this comment.
[NIT] Optional: production can't reach this check. new LogFilter(fr) is only built via LogFilterWrapper, from getLogs and from newFilter (through LogFilterAndResult), and both now reject a null fr first (TronJsonRpcImpl L1481, L1587). newFilter needs its own check anyway because of fr.getFromBlock() at L1486. So this is a third guard for the same condition, and only testNullLogFilterRequestRejectedAsInvalidParams reaches it. Either drop it along with that test, or keep it as the only check for getLogs and remove the one at L1587.
There was a problem hiding this comment.
Agreed, applied the first option in c74d9ea: removed the constructor check and testNullLogFilterRequestRejectedAsInvalidParams. The checks stay at the eth_getLogs and eth_newFilter entry points, so eth_getLogs still rejects a null filter before reading the head block, and all ten positions are validated the same way. Null filter requests for both methods remain covered in JsonrpcServiceTest at the service level (testTopLevelNullParametersAtServiceLayer) and the wire level (testNullParameterWireContract). As a result, INVALID_FILTER_REQUEST no longer needs to be shared. It is now a private constant in TronJsonRpcImpl, and JsonRpcApiUtil is no longer changed by this PR.
Validate nullable JSON-RPC arguments before they are dereferenced. Return stable protocol errors instead of jsonrpc4j's unhandled -32001 fallback, which exposes Java exception types. Validate eth_call's required transaction argument before its block selector. The double-null case now returns -32602 instead of -32600. Align eth_uninstallFilter lookup misses with geth: null, unknown and already removed IDs return false. Empty and non-hexadecimal strings also return false when no filter matches, without new format checks. Non-null lookup misses now return false instead of -32000. Use the value returned by remove to report whether a filter was removed and avoid the race between checking and removing the same entry. Remove the obsolete uninstall ItemNotFoundException declaration and error mapping. Java binaries remain compatible, but source callers catching that checked exception, or implementations still declaring it, may need adjustment when recompiled. Keep getFilterChanges and getFilterLogs on their existing lookup-miss errors, and preserve request-source precedence and filter isolation. Treat null block-detail flags as false and retain DTO defaults for explicitly null optional fields. Preserve filter wildcard semantics. Add binding, service, wire and VM execution regressions for null arguments, error contracts, filter removal, source isolation and concurrent uninstall results.
Return invalid params for an explicit null filter ID in eth_uninstallFilter, eth_getFilterChanges and eth_getFilterLogs, and for a null fullTransactionObjects flag in eth_getBlockByHash and eth_getBlockByNumber. The block hash or selector is still validated first, and the flag is checked before any block lookup, so the result does not depend on whether the block exists. Unknown non-null filter IDs keep their existing results. This replaces the earlier null-as-false handling of the flag and the filter ID. go-ethereum v1.17.6 rejects null for these required arguments; earlier releases decoded it as a zero value.
Route the null checks for the ten required JSON-RPC parameter positions through one requireParam helper instead of repeating the same if/throw block at each site. The "invalid params" and "invalid filter request" messages are unchanged and are now both private constants in TronJsonRpcImpl. Remove the null check from the LogFilter constructor. In production code, LogFilter is only built through LogFilterWrapper from eth_getLogs and eth_newFilter, and both reject a null filter request before reaching it, so the constructor check and its unit test are dropped. Null filter requests remain covered at the service and wire level. Extract getBlockByHash(byte[]) and use it from ethGetBlockByHash and getBlockByJsonHash, so ethGetBlockByHash no longer inlines the block lookup. The validation order is unchanged. No behavior change.
Adapt the tests added for null parameter handling to the constructor that now takes the Manager, and drop the removed setManager call. Tests built only from mocks pass a mocked Manager, and tests backed by the shared database pass dbManager, matching the existing tests.
dccf4b3 to
010242a
Compare
3 participants
What does this PR do?
Rejects an explicit
nullwith-32602at 10 identified top-level parameter positions that could previously trigger unchecked dereferences, normalizes seven optional DTO fields whose explicit JSONnullcurrently behaves differently from omission, and alignseth_uninstallFilterlookup misses with go-ethereum. The per-parameter decision basis is recorded in #6951.disableInPBFT/ FullNode-only) stays first. Object and filter-ID arguments are validated before business processing. For the two block-query methods, the block hash or selector is validated first, followed byfullTransactionObjects, before any block lookup:eth_getLogs/eth_newFilterwith a null filter object ->-32602 "invalid filter request"eth_call/eth_estimateGas/buildTransactionwith a null argument object ->-32602 "invalid params"eth_uninstallFilter/eth_getFilterChanges/eth_getFilterLogswith a null filter ID ->-32602 "invalid params"eth_getBlockByHash/eth_getBlockByNumberwith a nullfullTransactionObjects->-32602 "invalid params"; an invalid block hash or selector keeps its existing erroreth_callvalidates its required transaction argument before the block selector, so[null, null]returns-32602instead of-32600.@JsonSetter(nulls = Nulls.SKIP)on the six optionalBuildArgumentsfields (tokenId,tokenValue,consumeUserResourcePercent,originEnergyLimit,permissionId,extraData) and onCallArguments.from, so an explicit JSONnullkeeps the field default exactly as omitting the field does.requireParamhelper performs the null checks above. The two-32602messages,invalid paramsandinvalid filter request, are private constants inTronJsonRpcImpl.eth_uninstallFilterreturnsfalsefor unknown or already removed IDs, including empty and non-hexadecimal strings that miss the lookup. It returnstrueonly whenremoveactually removes an installed event or block filter, without a separatecontainsKeycheck. ID normalization and request-source selection are unchanged; no format validation or locking is added.ItemNotFoundExceptiondeclaration fromTronJsonRpcandTronJsonRpcImpl, and its corresponding-32000annotation. Other methods' mappings are unchanged.Remaining method-level error responses keep their existing
@JsonRpcErrorsmappings:datastays"{}"and the requestidis echoed. Uninstall lookup misses now produce a Boolean result instead of the removed error mapping.Why are these changes required?
Before this PR, the null paths at these positions dereferenced the argument without a null check. Where this raised a
NullPointerException, the exception was not covered by@JsonRpcErrors, so jsonrpc4j's fallback returned:{"jsonrpc":"2.0","id":1,"error":{"code":-32001,"message":null,"data":"java.lang.NullPointerException"}}message: nullviolates JSON-RPC 2.0 (on JVMs where helpful NullPointerException messages are enabled, the default from JDK 15 onwards, it becomes a diagnostic string echoing internal class and method names),dataexposes the Java exception type, and a required-parameter error such as this example is reported under the code java-tron documents for internal errors. The seven DTO fields were inconsistent among themselves: an explicitnullfailed with-32001or-32000depending on the path, whileCallArguments.fromreturned-32602where omitting the field continues with the zero address.The per-parameter decisions follow the basis recorded in #6951: the Execution API
required/ schema first, then whether a zero value can be inferred fromnull, then go-ethereum (v1.17.6) / Besu (26.9.0) behavior as a reference. Details and reproduction steps are in #6951.This PR has been tested by:
Unit Tests
Manual Testing
JsonRpcArgumentNullBindingTest- Jackson binding throughObjectMapper: an explicitnulland an omitted field produce the sameBuildArguments/CallArguments.JsonRpcCallAndEstimateGasTest- service-level results for nulleth_call/eth_estimateGasargument objects.JsonRpcTest- a null entry inside a topic OR-list still returnsinvalid topic(s): null.JsonrpcServiceTest- wire-level contract through a realJsonRpcServer:code/message/datafor every case above, theeth_callargument-before-block ordering, filter-object null wildcard semantics unchanged (address: null,topics: null, positional[A, null]), optional DTO null fields, and an assertion that no response containsjava.or an exception class name. Includes wire regressions for the four hash-parameter methods fixed by fix(jsonrpc): harden RPC/HTTP parameter validation #6828.JsonrpcServiceTestalso covers event/block filter creation and removal through the real server, repeated removal, unknown/empty/non-hexadecimal IDs, strict Boolean results without anerrorfield, preserved ID normalization, unchanged get-filter errors for unknown IDs and concurrent removal of one installed filter.JsonrpcServiceTestalso covers nullfullTransactionObjectsfor existing and non-existent blocks through both block-query methods, keeps the block hash or selector error when both parameters are invalid, and verifies with a mockedWalletthat a null flag is rejected before any block is read.WalletCursorTest- FullNode/Solidity filter stores stay isolated; both filter types can be removed only from the selected source. PBFT rejects null, installed, unknown and malformed string IDs before removing anything. A nullfullTransactionObjectsreturns-32602on PBFT, where the block-query methods remain available.On 2026-09-08,
:framework:cleanTest :framework:test --no-build-cachefororg.tron.core.jsonrpc.*andorg.tron.core.services.jsonrpc.*passed 24 classes / 242 tests (0 failures, 0 errors, 0 skipped) on JDK 17 (arm64). On 2026-09-15 the same suite was re-run on the branch rebased ontorelease_v4.8.3(0d19485318, which already carries the slf4j 2.0.17 / logback 1.3.16 / jackson 2.18.10 upgrade from #6950 and the HTTP error sanitization from #6954) with the same result. On 2026-09-27, after the null-handling update in dccf4b3, the same suite passed 24 classes / 243 tests (0 failures, 0 errors, 0 skipped) on JDK 17 (arm64); the added test is the mocked-Walletcheck. On 2026-10-09 the branch was rebased ontorelease_v4.8.3(6f7b83a6d6), and the tests added by this PR were adapted to the three-argumentTronJsonRpcImplconstructor from #6990. With that adaptation and c74d9ea (shared null-check helper, and removal of the unreachableLogFilterconstructor check together with its unit test), the same suite passed 27 classes / 253 tests (0 failures, 0 errors, 0 skipped) on JDK 17 (arm64). Compared with 2026-09-27, one test was removed with the constructor check, and the 3 classes / 11 tests added come fromrelease_v4.8.3.:framework:checkstyleMain,:framework:checkstyleTestandgit diff --checkalso pass. This validates compilation of the repository's callers after the checked-exception changes; external Java source callers may still need adjustment. Java 8/x86 execution was not performed.Compatibility
Breaking, limited to the affected null-input and filter lookup-miss responses. Successful valid calls remain unchanged. Observable changes are:
-32001fallback to-32602;fullTransactionObjectschanges from-32001to-32602; in addition,eth_getBlockByNumberwith a non-existent block and a nullfullTransactionObjectschanges fromresult: nullto-32602;eth_uninstallFilterwith an unknown, already removed, empty or non-hexadecimal string ID changes from-32000 "filter not found"toresult: false. Existing filters still returntrueon removal; simultaneous removals no longer both report success for the same installed entry;eth_call([null, null])goes from-32600to-32602;eth_call([null, "earliest"])and similar shapes where parameter 0 and the block selector are both invalid keep-32602, but the message changes from the block error to"invalid params"because the required argument is now validated first.Clients matching on the old codes or messages for these paths, or branching on an uninstall error versus a result, need to adjust.
Removing the checked
ItemNotFoundExceptiondeclaration is Java binary-compatible, but source callers that specifically catch it around this call, or implementations that still declare it, may need adjustment when recompiled. OnTronJsonRpcImpl,uninstallFilterandgetFilterChangesnow also declareJsonRpcInvalidParamsException, whichTronJsonRpcalready declares for them: this adds no checked-exception requirement for callers that use the interface, but code that calls these methods through the implementation type may need to handle it. This PR removes one obsolete mapping fromuninstallFilter; unknown-ID errors frometh_getFilterChangesandeth_getFilterLogsremain unchanged.Unchanged: successful valid-call results, HTTP status, request-source precedence,
dataon remaining mapped errors,result: nullfor a non-existent block with a non-nullfullTransactionObjects, filter-object null wildcards, validation of non-null arguments (for example the field checks ineth_estimateGasand thefinalizedtag check ineth_newFilter), unknown-ID errors frometh_getFilterChanges/eth_getFilterLogs, gRPC and non-JSON-RPC HTTP APIs.Follow up
-32603 "Internal error"withoutdata, which moves only the "before" side of the comparison; the target behavior of this PR is the same either way.web3_sha3(null), unconditional transaction index validation.paramsmissing /null/[]retain existing dispatch and arity behavior and are not universally rejected: whether they are accepted depends on how many parameters the method declares. They are not touched here. Request-envelope validation is tracked by [Feature]Standardize JSON-RPC error handling(revert codes, LiteNode pruned-history responses, request fields validation) #6676; there is no dependency between them (see Extra details).Extra details
This PR and #7004, the first PR for #6676, both change
TronJsonRpc,TronJsonRpcImpland the JSON-RPC tests, and currently conflict inTronJsonRpcImplandJsonrpcServiceTest. This PR also sharesTronJsonRpcandTronJsonRpcImplwith the error-mapping change tracked by #6941. This PR does not require #6676 or #6941 to land first; whichever lands second rebases and re-runs the related regression tests.Pre-submit checklist:
@JsonSetterprotects Jackson input only, and positional vs. OR-list null topics inLogFilterCloses #6951
Refs #6676