Skip to content

feat(API): refactor merge rpc - #17

Open
SeriousCoding789 wants to merge 10 commits into
SeriousCoding789:developfrom
Little-Peony:refactor_merge_rpc
Open

SeriousCoding789 wants to merge 10 commits into
SeriousCoding789:developfrom
Little-Peony:refactor_merge_rpc

Conversation

@SeriousCoding789

@SeriousCoding789 SeriousCoding789 commented Aug 20, 2026

Copy link
Copy Markdown
Owner

What does this PR do?

Implements tronprotocol#6927 — the gRPC counterpart of the HTTP servlet dedup in tronprotocol#6922.

RpcApiServiceOnSolidity and RpcApiServiceOnPBFT each re-declare the whole read surface as per-method delegations whose only job is to switch the per-thread read cursor:

@Override
public void getAccount(Account req, StreamObserver<Account> obs) {
  walletOnSolidity.futureGet(() -> rpcApiService.getWalletSolidityApi().getAccount(req, obs));
}

There are exactly 100 of these — 51 in the Solidity service, 49 in the PBFT one — spread over four inner service classes. This PR replaces them with a cursor-parameterized ServerInterceptor:

  • Adds CursorServerInterceptor with SolidityCursorInterceptor / PbftCursorInterceptor. It brackets Listener.onHalfClose() — the callback gRPC runs a unary handler inline from — setting the cursor before and restoring it in a finally.
  • The two cursor services now register the base service's DatabaseApi / WalletSolidityApi singletons directly, wrapped in ServerInterceptors.intercept(...). RpcApiServiceOnSolidity goes from 488 lines to 36, RpcApiServiceOnPBFT from 495 to 36.
  • A second, independent axis of duplication inside RpcApiService itself: WalletSolidityApi (serving protocol.WalletSolidity) re-implements read handlers WalletApi (serving protocol.Wallet) already has. 41 of its 47 methods now delegate to the shared WalletApi singleton; the remaining 6 already routed through shared *Common / callContract helpers, so no duplicated handler body is left. Java is single-inheritance and gRPC generates one ImplBase per proto service, so both classes have to stay — only the bodies go.
  • Fixes the double-close on the error path, which the dedup made visible. A handler that calls responseObserver.onError(...) and then falls through to responseObserver.onCompleted() closes the call twice; the second close() hits checkState(!closeCalled, "call already closed") in gRPC's ServerCallImpl and throws. RpcApiService had 32 handlers shaped that way on develop — 8 disappear with the duplicated WalletSolidityApi bodies, and the remaining 24 get an explicit return, the shape the file already used elsewhere. The file now has none.

10 files changed, +945 / −1267.

Why are these changes required?

A read handler currently lives in up to four places, so one RPC change has to be mirrored four times, and missing one makes the same RPC behave differently depending on which port a client hits.

That has already happened. Of the 41 handlers being dedup'd inside RpcApiService, 23 are byte-identical between the two copies on develop and 18 differ. Of those 18:

  • 8 are hardening that only ever landed in WalletApi — it gained a return after responseObserver.onError(...) and WalletSolidityApi did not, so on the error path the copy serving the Solidity and PBFT ports falls through to onCompleted() and terminates the call twice (getMerkleTreeVoucherInfo, isSpend, scanAndMarkNoteByIvk, scanNoteByIvk, scanNoteByOvk, isShieldedTRC20ContractNoteSpent, scanShieldedTRC20NotesByIvk, scanShieldedTRC20NotesByOvk). The last two additionally have a BadItemException | ZksnarkException branch with logging that only WalletApi ever received.
  • 7 are cosmetic — a parameter name, a temporary variable, line wrapping.
  • 1 differs only in a log prefix.
  • 2 (getBlockByNum, getBlockByNum2) drifted the other way: WalletSolidityApi has a num >= 0 guard WalletApi lacks.

None of that was written deliberately; it is what happens when the same handler exists four times.

Lining the two copies up also showed that the return hardening was never finished on WalletApi either — 24 more handlers in the same file still fall through — so this PR completes it rather than leaving the file in two states.

Behaviour differences vs develop

Method sets, per port. The base WalletSolidityApi (47 methods) and DatabaseApi (4) expose exactly the same methods before and after — only bodies changed.

Port Before After Delta
HEAD (RpcApiService) 47 + 4 47 + 4 none
SOLIDITY 47 + 4 47 + 4 none
PBFT 45 + 4 47 + 4 +2

The PBFT port gains getPaginatedNowWitnessList and getTransactionInfoByBlockNum — the only two methods RpcApiServiceOnPBFT never mirrored from RpcApiServiceOnSolidity; they returned UNIMPLEMENTED there before. Both are ordinary reads and resolve against the PBFT snapshot like every other read on that port.

Handler bodies. Only three WalletApi bodies changed beyond the added returns:

  • getBlockByNum / getBlockByNum2 adopt the solidity copy's num >= 0 guard. No response changeWallet#getBlockByNum already catches the StoreException and returns null for a negative number, so both paths reach onNext(null); the guard only skips a futile store lookup and its log line.
  • getAssetIssueByName drops the "FullNode " prefix from one logger.debug line, which was the only difference between the two copies.

Error paths. Every handler that used to emit onError followed by onCompleted now emits a single terminal event, on all three ports. Not visible to clientsonError had already closed the call with the error status and the second close() threw before sending anything; what goes away is one server-side IllegalStateException per failed call. Two groups:

  • Fixed as a side effect of the dedup, on the SOLIDITY and PBFT ports: the 8 shielded handlers listed above. Worth noting these were not a rare corner — the first statement of the five sapling reads in Wallet is checkAllowShieldedTransactionApi(), and node.allowShieldedTransactionApi defaults to false, so on a default node every such call took the double-close path.
  • Fixed explicitly, on all three ports: 17 handlers in WalletApi (getPaginatedNowWitnessList, getTransactionInfoByBlockNum, getDelegatedResourceV2, getDelegatedResourceAccountIndex, getDelegatedResourceAccountIndexV2, getCanDelegatedMaxSize, getCanWithdrawUnfreezeAmount, getAvailableUnfreezeCount, getBandwidthPrices, getEnergyPrices, getMemoFee, getNodeInfo, getMarketOrderByAccount, getMarketOrderById, getMarketOrderListByPair, getMarketPriceByPair, getMarketPairList) and 7 shared helpers reachable from both services (getBlockCommon, getRewardInfoCommon, getBrokerageInfoCommon, getBurnTrxCommon, getPendingSizeCommon, getTransactionFromPendingCommon, getTransactionListFromPendingCommon).

Cursor semantics are unchanged. Manager#setCursor still computes the headNum - pbftNum offset for PBFT, exactly as WalletOnPBFT.futureGet did.

Scope. gRPC only. WalletOnCursor / WalletOnSolidity / WalletOnPBFT stay, because the HTTP and JSON-RPC servlets still call futureGet; removing those is a separate change. Ports, switches, proto definitions and the server-level interceptor chain (rate limiter, api access, lite-fullnode filter, prometheus) are untouched.

This PR has been tested by:

  • Unit Tests — 7 new tests plus two assertions added to the existing end-to-end suite, all passing. Each pins something that can actually go wrong:
    • CursorInterceptorScopeTest (2) drives interceptCall() on one thread and the returned listener's onHalfClose() on another — which is what gRPC's SerializingExecutor is free to do. An implementation that scoped the cursor around interceptCall fails here by construction rather than by luck. Also covers the finally reset when the handler throws.
    • CursorInterceptorServerTest (1) runs the production interceptor behind a real gRPC server and asserts the cursor is set and restored exactly once, on the handler's own thread. This is the end-to-end half: the whole design rests on gRPC running the handler inline from onHalfClose, and if that stops holding the cursor never reaches the read path and the port serves HEAD data with no error.
    • CursorInterceptorWiringTest (2) runs the real addService of both cursor services against a mock builder and asserts each shared read service is registered as an intercepted definition. Dropping ServerInterceptors.intercept leaves every other test green while the port silently serves HEAD, so this is the gRPC counterpart of CursorFilterInstallationTest.
    • RpcApiServiceErrorPathTest (2) drives every unary handler of WalletApi and WalletSolidityApi with collaborators that throw, and asserts none of them terminates the call more than once. Reverting the added returns makes it fail on getDelegatedResourceV2, getPendingSize and getBlock, so it reaches the shared *Common helpers as well.
    • RpcApiServicesTest — the existing suite drives all three ports end to end over 129 tests; it now also calls getPaginatedNowWitnessList and getTransactionInfoByBlockNum on the PBFT stub, pinning the one intentional behaviour change.
  • Manual Testing

@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: ffe05861-921e-4c7b-a687-957d9fc470f8


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@SeriousCoding789 SeriousCoding789 changed the title fix: refactor merge rpc feat(API): refactor merge rpc Aug 21, 2026
@SeriousCoding789
SeriousCoding789 changed the base branch from feat/refactor_merge_http_servlets to develop August 24, 2026 06:02
@Little-Peony
Little-Peony force-pushed the refactor_merge_rpc branch 3 times, most recently from 8b15128 to 224cfc6 Compare September 1, 2026 06:39
vaibhav8a and others added 9 commits September 3, 2026 18:12
Signed-off-by: Vaibhav Srivastava <vaibhavsri1712@gmail.com>
Clarify trigger behavior, log retention, Solidity API availability, and account-name rules.
* feat: optimize delegate and undelegate instruction handling (tronprotocol#6920)

Co-authored-by: Asuka <yanghang8612@163.com>

* perf: optimize jump table initialization and reuse (tronprotocol#6943)

Co-authored-by: Asuka <yanghang8612@163.com>

* feat: refine contract deployment transaction validation (tronprotocol#6945)

Co-authored-by: Asuka <yanghang8612@163.com>

* ci: run single-node smoke only and disable multinode (backport tronprotocol#6908 to release_v4.8.2.2) (tronprotocol#6953)

Backport of tronprotocol#6908 to the release_v4.8.2.2 branch so CI passes there:

- Switch the single-node integration CI to the smoke test subset
  (--clean --smoke instead of the full suite), renaming workflow,
  job, steps, and report artifact from "Full" to "Smoke"
- Remove the multinode integration CI workflow entirely; the full
  single-node suite and the multinode suite are unstable in CI today
  (hardened assertions don't match the troninfra/troninfra-ci image
  fixture), causing failures unrelated to PR code

---------

Co-authored-by: ouy95917 <ouy95917@gmail.com>
Co-authored-by: Asuka <yanghang8612@163.com>
Co-authored-by: Jeremy Zhang <50477615+warku123@users.noreply.github.com>
chore(branch): merge master into develop
…#6930)

Node and network API responses populated two fields from the wrong source values because of copy-and-paste mapping errors.

Map needSyncFromPeer from the corresponding peer state and assign UDP inbound traffic to the udpInTraffic protobuf field.
…me (tronprotocol#6950)

* chore(deps): upgrade grpc-java from 1.83.0 to 1.83.1

1. bump grpcVersion to 1.83.1 to pick up the upstream fix for
   grpc/grpc-java#12930 (PR grpc/grpc-java#12942), which enforces
   connection.remote().maxActiveStreams(maxStreams) at handler startup
2. drop GrpcNettyMaxConcurrentStreamsLimiter, the local protocol-negotiator
   shim that applied the same limit while 1.83.0 left the remote endpoint
   unbounded until the client acknowledged SETTINGS

* chore(deps): upgrade jackson from 2.18.6 to 2.18.10

bump jackson-databind from 2.18.6 to 2.18.10 to pick up cumulative fixes from the 2.18.x line

* chore(deps): upgrade logback to 1.3.16 and slf4j to 2.0.17

1. bump logback-classic from 1.2.13 to 1.3.16 and slf4j-api,
   jcl-over-slf4j, jul-to-slf4j from 1.7.36 to 2.0.17; logback 1.3
   requires the slf4j 2.0 provider model, and 1.3.16 is the last 1.3.x
   release and the ceiling for the x86_64 JDK 8 build, since 1.5.x
   requires JDK 11
2. rename DelayingShutdownHook to DefaultShutdownHook in the toolkit
   logback.xml; logback 1.3 removed the old class and only auto-maps
   the legacy name with a startup warning
3. drop the CONSOLE appender from the toolkit logback.xml; no logger
   ever referenced it, so it never emitted output on 1.2 either, and
   logback 1.3 now flags it with an unreferenced-appender warning
4. accept one known 1.3.x behavior change: SizeAndTimeBasedRollingPolicy
   now throttles its maxFileSize comparison to once per 60s
   (SimpleInvocationGate) instead of the adaptive ~100-800ms gate of
   1.2.13, so under sustained heavy logging a file can overshoot the
   500MB cap by up to 60s of writes before the %i rollover fires;
   time-based rollover and totalSizeCap/maxHistory cleanup are ungated
   and unaffected
5. note for operators running a custom --log-config file: well-formed
   1.2-era configs using standard elements keep working unchanged
   (jmxConfigurator degrades to an ignored-property warning, the legacy
   shutdown hook name is auto-mapped), and malformed XML still fails
   fast via TronError(LOG_LOAD) exactly as on 1.2; however, a config
   that references an uninstantiable class (e.g. a custom appender
   missing from the classpath) now aborts the whole appender-ref phase
   instead of losing just that one appender, so the node starts with no
   log output while the ERROR statuses are printed to stdout by
   LogService

* chore(deps): upgrade commons-lang3/collections4 and drop commons-math

1. bump commons-lang3 from 3.4 to 3.20.0; the runtime classpath already
   resolved 3.18.0 through libp2p 2.2.9's transitive requirement, so
   align the declaration with what actually ships and move past the
   CVE-2025-48924 range that the nominal 3.4 still sits in
2. bump commons-collections4 from 4.1 to 4.6.0
3. remove commons-math 2.2; no source file imports
   org.apache.commons.math and nothing else in the dependency graph
   requests it

* chore(deps): remove joda-time and use JDK time APIs

1. drop the joda-time 2.3 dependency.
2. replace the six new DateTime(millis) log-formatting call sites in
   DynamicPropertiesStore, DposTask and DposService with a new
   Time.getIsoTimeString helper backed by java.time; its formatter
   (yyyy-MM-dd'T'HH:mm:ss.SSSXXX in the system zone) reproduces joda's
   DateTime.toString() output byte for byte where the JDK and joda 2.3
   time-zone databases agree (UTC nodes are unaffected); zones whose
   rules changed after joda's 2013-era tzdb, e.g. Europe/Moscow, now
   render the corrected offset for the same instant.
3. replace DateTime.now() day arithmetic in four test classes with the
   java.time equivalent, ZonedDateTime.now().minusDays(n)/plusDays(n)
   .toInstant().toEpochMilli(), keeping joda's calendar semantics
   one-to-one, and map plain DateTime.now().getMillis() to
   System.currentTimeMillis()
* feat(api): sanitize HTTP API error responses

Standard HTTP error paths used to expose internal details to clients:
Util.processError prefixed every message with the Java exception class
name, several servlets printed raw Throwable.getMessage() directly, and
the two solidity query endpoints returned bare-text error bodies.

Centralize the client-facing text decision in Util.processError:

* keep the raw non-blank message only for the exact runtime types
  JsonFormat.ParseException, ContractValidateException and
  MaintenanceUnavailableException; a null, empty or whitespace-only
  message falls back to "internal server error"
* preserve the events-deprecation message only for the exact
  IllegalArgumentException type carrying EVENTS_DEPRECATED_MSG
* write the fixed rate-limit and INVALID address messages, along with
  existing GetBlock validation messages, through the package-private
  writeAuditedError helper; these audited callers bypass exception
  classification, and printErrorMsg is private to the shared writer
* return {"Error":"internal server error"} for every other exception,
  with no exception class name

Client-visible changes:

* all processError-based error bodies lose the "class <FQCN> : "
  prefix; unclassified raw messages become "internal server error"
* the rate-limit rejection body becomes
  {"Error":"lack of computing resources"} on every endpoint extending
  RateLimiterServlet, including full-node, solidity and PBFT /jsonrpc
* gettransactionbyid / gettransactioninfobyid on solidity return
  standard {"Error":...} JSON instead of bare text
* validateaddress, getBrokerage and getReward replace leaked library
  messages in their failure branches with existing fixed texts; the
  "INVALID address" body is now written via writeAuditedError and loses
  the space after the colon
* getblock keeps its exact error bodies (refactor only)

Cover Solidity transaction and transaction-info GET/POST input errors,
backend failures, successful lookups and missing records directly with
mocked Wallet calls and in-memory requests and responses. Replace the
transaction servlet tests that accidentally exercised POST in both cases,
changed global stdout and used a shared temporary response file.

Verify both endpoint and global rate-limit rejections across the three
JSON-RPC servlet variants, including status, response body and the absence
of business dispatch on rejection.

HTTP status codes, success responses, request validation rules and
gRPC behavior are unchanged. JSON-RPC behavior is unchanged except for
the shared HTTP rate-limit response described above.

Closes tronprotocol#6936

* fix(api): keep server-side failure logging at error level

The previous commit routed four catch-all blocks through the shared
processError entry point, which logs at debug. Those four catches cover
server-side work only: getburntrx, getnodeinfo and getpendingsize read
no request parameters, and in getreward malformed addresses are already
handled by the preceding DecoderException | IllegalArgumentException
catch. Their failures therefore left no trace under the default log
configuration, where the API topic is INFO.

Add a dedicated processServerError entry point that logs at error and
then applies the same sanitization, and use it at those four call sites.
Logging the exception once inside the helper keeps a single record at
any log level, instead of pairing an error log in the servlet with the
debug log in the shared path.

The shared Exception entry point keeps debug on purpose: its callers
also cover request parsing, so an unauthenticated client can fail it
cheaply and repeatedly, and an unconditional stack trace per request
would amplify that into log pressure. Distinguishing client from server
faults on that path is the parameter/internal split tracked as follow-up
in tronprotocol#6936.

Client-facing responses are unchanged.
Remove the duplicated wallet-solidity gRPC stack by serving the solidity
and PBFT surfaces from the shared service instances, with a cursor
interceptor selecting the store each call reads from.

- Add cursor server interceptors for the solidity and PBFT ports and bind
  them to the shared services.
- Serve solidity and PBFT gRPC through the shared service instances
  instead of separate handler implementations.
- Deduplicate the remaining wallet-solidity read handlers.
- Return after onError so a failed call is closed exactly once.
- Document that the wallet-solidity API is the read-only subset of wallet,
  and assert that subset relationship in tests.
- Cover the error path, cursor wiring and PBFT reads; drop probe tests
  that guarded nothing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants