Skip to content

fix(jsonrpc): handle null parameters consistently - #6984

Merged
lvs0075 merged 4 commits into
tronprotocol:release_v4.8.3from
waynercheung:feat/jsonrpc-null-param
Oct 12, 2026
Merged

lvs0075 merged 4 commits into
tronprotocol:release_v4.8.3from
waynercheung:feat/jsonrpc-null-param

Conversation

@waynercheung

@waynercheung waynercheung commented Sep 21, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

Rejects an explicit null with -32602 at 10 identified top-level parameter positions that could previously trigger unchecked dereferences, normalizes seven optional DTO fields whose explicit JSON null currently behaves differently from omission, and aligns eth_uninstallFilter lookup misses with go-ethereum. The per-parameter decision basis is recorded in #6951.

  • Handle the 10 affected positions without unchecked dereferences. For source-gated methods the request-source check (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 by fullTransactionObjects, before any block lookup:
    • eth_getLogs / eth_newFilter with a null filter object -> -32602 "invalid filter request"
    • eth_call / eth_estimateGas / buildTransaction with a null argument object -> -32602 "invalid params"
    • eth_uninstallFilter / eth_getFilterChanges / eth_getFilterLogs with a null filter ID -> -32602 "invalid params"
    • eth_getBlockByHash / eth_getBlockByNumber with a null fullTransactionObjects -> -32602 "invalid params"; an invalid block hash or selector keeps its existing error
  • eth_call validates its required transaction argument before the block selector, so [null, null] returns -32602 instead of -32600.
  • @JsonSetter(nulls = Nulls.SKIP) on the six optional BuildArguments fields (tokenId, tokenValue, consumeUserResourcePercent, originEnergyLimit, permissionId, extraData) and on CallArguments.from, so an explicit JSON null keeps the field default exactly as omitting the field does.
  • One requireParam helper performs the null checks above. The two -32602 messages, invalid params and invalid filter request, are private constants in TronJsonRpcImpl.
  • eth_uninstallFilter returns false for unknown or already removed IDs, including empty and non-hexadecimal strings that miss the lookup. It returns true only when remove actually removes an installed event or block filter, without a separate containsKey check. ID normalization and request-source selection are unchanged; no format validation or locking is added.
  • Remove this method's obsolete ItemNotFoundException declaration from TronJsonRpc and TronJsonRpcImpl, and its corresponding -32000 annotation. Other methods' mappings are unchanged.

Remaining method-level error responses keep their existing @JsonRpcErrors mappings: data stays "{}" and the request id is 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: null violates 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), data exposes 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 explicit null failed with -32001 or -32000 depending on the path, while CallArguments.from returned -32602 where 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 from null, 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 through ObjectMapper: an explicit null and an omitted field produce the same BuildArguments / CallArguments.

  • JsonRpcCallAndEstimateGasTest - service-level results for null eth_call / eth_estimateGas argument objects. JsonRpcTest - a null entry inside a topic OR-list still returns invalid topic(s): null.

  • JsonrpcServiceTest - wire-level contract through a real JsonRpcServer: code / message / data for every case above, the eth_call argument-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 contains java. or an exception class name. Includes wire regressions for the four hash-parameter methods fixed by fix(jsonrpc): harden RPC/HTTP parameter validation #6828.

  • JsonrpcServiceTest also covers event/block filter creation and removal through the real server, repeated removal, unknown/empty/non-hexadecimal IDs, strict Boolean results without an error field, preserved ID normalization, unchanged get-filter errors for unknown IDs and concurrent removal of one installed filter.

  • JsonrpcServiceTest also covers null fullTransactionObjects for 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 mocked Wallet that 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 null fullTransactionObjects returns -32602 on PBFT, where the block-query methods remain available.

On 2026-09-08, :framework:cleanTest :framework:test --no-build-cache for org.tron.core.jsonrpc.* and org.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 onto release_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-Wallet check. On 2026-10-09 the branch was rebased onto release_v4.8.3 (6f7b83a6d6), and the tests added by this PR were adapted to the three-argument TronJsonRpcImpl constructor from #6990. With that adaptation and c74d9ea (shared null-check helper, and removal of the unreachable LogFilter constructor 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 from release_v4.8.3. :framework:checkstyleMain, :framework:checkstyleTest and git diff --check also 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:

  • required object parameters and null filter IDs change from the unmapped -32001 fallback to -32602;
  • a null fullTransactionObjects changes from -32001 to -32602; in addition, eth_getBlockByNumber with a non-existent block and a null fullTransactionObjects changes from result: null to -32602;
  • eth_uninstallFilter with an unknown, already removed, empty or non-hexadecimal string ID changes from -32000 "filter not found" to result: false. Existing filters still return true on removal; simultaneous removals no longer both report success for the same installed entry;
  • explicit null DTO fields recover the omitted-field semantics;
  • eth_call([null, null]) goes from -32600 to -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 ItemNotFoundException declaration 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. On TronJsonRpcImpl, uninstallFilter and getFilterChanges now also declare JsonRpcInvalidParamsException, which TronJsonRpc already 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 from uninstallFilter; unknown-ID errors from eth_getFilterChanges and eth_getFilterLogs remain unchanged.

Unchanged: successful valid-call results, HTTP status, request-source precedence, data on remaining mapped errors, result: null for a non-existent block with a non-null fullTransactionObjects, filter-object null wildcards, validation of non-null arguments (for example the field checks in eth_estimateGas and the finalized tag check in eth_newFilter), unknown-ID errors from eth_getFilterChanges / eth_getFilterLogs, gRPC and non-JSON-RPC HTTP APIs.

Follow up

Extra details

This PR and #7004, the first PR for #6676, both change TronJsonRpc, TronJsonRpcImpl and the JSON-RPC tests, and currently conflict in TronJsonRpcImpl and JsonrpcServiceTest. This PR also shares TronJsonRpc and TronJsonRpcImpl with 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:

  • Google Java Style; Checkstyle passes on main and test sources
  • No debug code, temporary comments or TODOs
  • No numeric computation or narrowing casts introduced
  • No logging added or changed
  • No DB, consensus, config or dependency changes
  • Comments explain the non-obvious points: @JsonSetter protects Jackson input only, and positional vs. OR-list null topics in LogFilter

Closes #6951
Refs #6676

@waynercheung

Copy link
Copy Markdown
Collaborator Author

Updated in dccf4b3001: an explicit null filter ID or fullTransactionObjects now returns -32602 "invalid params", instead of false / "filter not found" / treating the flag as false, following go-ethereum v1.17.6 and Besu 26.9.0. For the two block-query methods, the block hash or selector is still validated first, and the flag is checked before any block lookup. The rationale is in #6951 (comment), and the description above has been updated to match.

Comment thread framework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpcImpl.java Outdated
Comment thread framework/src/main/java/org/tron/core/services/jsonrpc/filters/LogFilter.java Outdated
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.
@waynercheung
waynercheung force-pushed the feat/jsonrpc-null-param branch from dccf4b3 to 010242a Compare October 9, 2026 10:24
@waynercheung
waynercheung requested a review from bladehan1 October 9, 2026 10:32
@0xbigapple

Copy link
Copy Markdown
Collaborator

[NIT] Not blocking, more a question for later. The null positions are covered by hand (10 parameters, 7 DTO fields, following the audit in #6951), so a method or DTO field added later could bring the unmapped NPE back without any test noticing. Do you see a good way to guard against that? One idea is a per-method table of valid sample arguments that must cover TronJsonRpc.class.getMethods(), with each position nulled in turn and the thrown exception checked against the method's @JsonRpcErrors via an InvocationListener, but the table is its own maintenance cost. If nothing simpler comes to mind, fine to leave it for a follow-up discussion.

@waynercheung

Copy link
Copy Markdown
Collaborator Author

@0xbigapple Thanks, I agree that the current tests protect the audited cases but do not force new methods or DTO fields to receive null-handling coverage.

One possible follow-up is a coverage check comparing RPC method signatures and Jackson-deserializable DTO properties against a test matrix, with an explicit null policy or documented exclusion for each parameter and property. This would flag additions without requiring an execution fixture for every method, though the matrix would still need maintenance and the check alone would not prove runtime behavior.

The per-position sweep and InvocationListener check could complement, rather than replace, the existing exact-response tests: a mapped exception can still have the wrong semantics, and an early return can leave the relevant path untested. DTO fields would also need separate JSON-field mutations; nulling the whole argument does not exercise them.

I suggest keeping this PR scoped to the audited fixes and discussing the broader coverage mechanism separately, as you proposed.

@0xbigapple

Copy link
Copy Markdown
Collaborator

@0xbigapple Thanks, I agree that the current tests protect the audited cases but do not force new methods or DTO fields to receive null-handling coverage.

One possible follow-up is a coverage check comparing RPC method signatures and Jackson-deserializable DTO properties against a test matrix, with an explicit null policy or documented exclusion for each parameter and property. This would flag additions without requiring an execution fixture for every method, though the matrix would still need maintenance and the check alone would not prove runtime behavior.

The per-position sweep and InvocationListener check could complement, rather than replace, the existing exact-response tests: a mapped exception can still have the wrong semantics, and an early return can leave the relevant path untested. DTO fields would also need separate JSON-field mutations; nulling the whole argument does not exercise them.

I suggest keeping this PR scoped to the audited fixes and discussing the broader coverage mechanism separately, as you proposed.

@waynercheung Agreed. I've approved the PR.

@lvs0075
lvs0075 merged commit 0d5b9e5 into tronprotocol:release_v4.8.3 Oct 12, 2026
13 checks passed
Little-Peony added a commit to Little-Peony/java-tron that referenced this pull request Oct 12, 2026
* release_v4.8.3:
  fix(jsonrpc): normalize error responses and fatal handling (tronprotocol#6985)
  fix(jsonrpc): handle null parameters consistently (tronprotocol#6984)
Little-Peony added a commit to Little-Peony/java-tron that referenced this pull request Oct 13, 2026
* release_v4.8.3:
  feat(crypto): harden ECKey validation (tronprotocol#6996)
  fix(config): fail startup on invalid DNS publish parameters (tronprotocol#7019)
  fix(jsonrpc): normalize error responses and fatal handling (tronprotocol#6985)
  fix(jsonrpc): handle null parameters consistently (tronprotocol#6984)
  refactor(trie): remove unused RLP utilities (tronprotocol#6991)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants