Skip to content

feat(API): refactor merge rpc - #6977

Open
SeriousCoding789 wants to merge 4 commits into
tronprotocol:release_v4.8.3from
Little-Peony:refactor_merge_rpc
Open

SeriousCoding789 wants to merge 4 commits into
tronprotocol:release_v4.8.3from
Little-Peony:refactor_merge_rpc

Conversation

@SeriousCoding789

Copy link
Copy Markdown
Contributor

What does this PR do?

Implements #6927 — the gRPC counterpart of the HTTP servlet dedup in #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 change — Wallet#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 clients — onError 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

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.
- CursorInterceptorScopeTest read the cursor on thread A right after
  interceptCall, before thread B ran onHalfClose, so it passed even when
  the cursor was never reset. Model the cursor as a ThreadLocal, as
  Chainbase does, and read it on thread B after onHalfClose returns.
- CursorInterceptorServerTest asserted the reset as soon as the blocking
  call returned, but the client can get the response before the server
  leaves onHalfClose's finally block. Wait for the reset on a latch.
* <p>The {@code finally} reset is mandatory: the fixed thread pool is reused, so a leftover cursor
* leaks into the next call on that thread.
*/
public abstract class CursorServerInterceptor implements ServerInterceptor {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[NIT] Method-set parity is now implicit — record the new contract semantics

After this dedup, any method added to protocol.WalletSolidity / protocol.Database in the future automatically appears on all three ports (HEAD / SOLIDITY / PBFT). The per-port opt-out that the mirrored delegations provided — intentionally or not, e.g. the two methods the PBFT port lacked until now — no longer exists; each port's method set is now defined solely by the proto service. That is a reasonable semantics, but it should be recorded: one sentence in this class's javadoc or the PR description, e.g. "new proto methods default to all-port parity; per-port exclusion must be implemented explicitly".

Suggestion: add the one-line contract note to the class javadoc (or PR description); no code change needed.

* is free to do. An implementation that scoped the cursor around {@code interceptCall} instead of
* {@code onHalfClose} then fails here by construction rather than by luck.
*/
public class CursorInterceptorScopeTest {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[NIT] Add non-singleton N/A declarations to the four new test classes

The project's test conventions ask each test class to state explicitly when its target bean is not a singleton and no @After reset is needed, so future maintainers can tell "no reset required" apart from "reset forgotten". The four test classes added by this PR construct fresh instances (local new / mocks with instance-field injection) and mutate no global state — which is correct — but none of them says so:

  • CursorInterceptorScopeTest.java (~line 31)
  • CursorInterceptorServerTest.java (~line 42)
  • CursorInterceptorWiringTest.java (~line 40)
  • RpcApiServiceErrorPathTest.java (~line 33)

Suggestion: add one javadoc line per class, e.g. "Target bean is not a singleton; each test builds a fresh instance, so no @after reset is needed."

/**
* WalletSolidityApi.
* WalletSolidityApi is the full implementation of the {@code protocol.WalletSolidity} gRPC
* service. Every method here is read-only and also present on {@link WalletApi}, so each one

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[NIT] Documentation & metadata backlog (3 items rolled up)

Grouped as one comment since all are doc-or-metadata asks with no behavior impact.

  1. WalletSolidityApi javadoc overstates the delegation scope (RpcApiService.java:367-369). "...so each one delegates to the shared WalletApi singleton" — in fact 41 of the 47 methods delegate to WalletApi, while 6 (getTransactionCountByBlockNum, getRewardInfo, getBrokerageInfo, getBurnTrx, triggerConstantContract, getBlock) route through the shared *Common / callContract helpers instead. Suggest qualifying the wording, e.g. "most delegate to the shared WalletApi singleton; six route through the shared *Common / callContract helpers".

  2. Dangling test reference (CursorInterceptorWiringTest.java:33). The javadoc calls this test "the gRPC counterpart of CursorFilterInstallationTest", but no such class exists anywhere in the tree — the HTTP counterpart ([Feature] Deduplicate HTTP servlet stacks with cursor filters and a declarative endpoint registry #6922) used servlet subclassing + futureGet, not a CursorFilter. Please point the reference at the real HTTP-side pattern/test, or drop the sentence.

  3. Cross-cutting metadata note (no code line): branch naming. The head branch refactor_merge_rpc has no /-separated type prefix (the documented convention is feature/ / hotfix/). Upstream history is lax about this and the branch is already pushed, so nothing to do for this PR — just flagging it for future branches.

Suggestion: fix the two javadoc wordings; keep the branch-naming convention in mind for future branches.

verify(builder, never()).addService(any(BindableService.class));
ArgumentCaptor<ServerServiceDefinition> registered =
ArgumentCaptor.forClass(ServerServiceDefinition.class);
verify(builder, times(2)).addService(registered.capture());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[NIT] This pins which addService overload is called, not that the cursor interceptor is attached. addService(rpcApiService.getWalletSolidityApi().bindService()) also goes through addService(ServerServiceDefinition), so the class javadoc's "registering without the interceptor binds the addService(BindableService) overload" (L35-36) covers only one of the two ways to drop it.

I tried it locally. With both lines of RpcApiServiceOnSolidity.addService changed to serverBuilder.addService(rpcApiService.getXxxApi().bindService()), both tests here still pass, while that port would serve HEAD. Wrapping with an unrelated interceptor would pass as well. If only one of the two lines regresses, the ServerInterceptors import stays used, so checkstyle won't flag it either. CursorInterceptorServerTest builds its own server, so nothing else covers the production addService.

Suggestion: give the interceptor a mock Manager. For each captured definition, take one method's ServerCallHandler, call startCall(mockCall, new Metadata()) (stub getMethodDescriptor() on the call), then call onHalfClose() on the returned listener, and verify setCursor(SOLIDITY) / setCursor(PBFT) followed by resetCursor().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

[Feature] Deduplicate gRPC cursor services with a ServerInterceptor

5 participants