Repository navigation
[Feature] Deduplicate gRPC cursor services with a ServerInterceptor #6927
Description
Summary
The same gRPC read surface is implemented several times over: RpcApiService, RpcApiServiceOnSolidity and RpcApiServiceOnPBFT are ~490 lines each of methods that do nothing but call the base service and switch the per-thread read cursor. This proposes replacing all of that per-method delegation with a single cursor-parameterized ServerInterceptor, and letting the read-only WalletSolidityApi handlers delegate to the shared WalletApi rather than re-implementing them. About 900 lines go away, with no change to the gRPC service definitions, ports, or responses.
Problem
Motivation
A read handler currently lives in up to four places — twice inside RpcApiService (WalletApi and WalletSolidityApi) and once each in the Solidity and PBFT services — so one RPC change may have to be mirrored four times. Miss one and the same RPC behaves differently depending on which port a client hits. This cross-port drift is hard to spot, and it has already happened: an error-handling fix was applied to WalletApi only and never reached the copy serving the Solidity and PBFT ports.
Current State
The Solidity and PBFT services each re-declare the whole read surface, and every method body is the same shape — wrap a call to the base service's singleton in futureGet so the thread reads from the solidified / PBFT snapshot instead of HEAD:
@Override
public void getAccount(Account req, StreamObserver<Account> obs) {
walletOnSolidity.futureGet(
() -> rpcApiService.getWalletSolidityApi().getAccount(req, obs));
}That is roughly 100 delegations carrying no logic. The cursor itself is a ThreadLocal, so switching it is a property of the calling thread, not of the class the handler happens to sit in — which is what makes a single interceptor able to replace all of them.
How the FullNode and standalone SolidityNode processes use these services. The standalone SolidityNode has no dedicated service class; the base RpcApiService branches by run mode, and the two cursor services only start on a FullNode:
| Service | enable condition |
FullNode | SolidityNode |
|---|---|---|---|
RpcApiService |
isRpcEnable() |
registers WalletApi (HEAD) |
registers WalletSolidityApi (HEAD) |
RpcApiServiceOnSolidity |
isFullNode() && isRpcSolidityEnable() |
registers WalletSolidityApi (SOLIDITY) |
not started |
RpcApiServiceOnPBFT |
isFullNode() && isRpcPBFTEnable() |
registers WalletPBFTApi (PBFT) |
not started |
So on a FullNode the two cursor services already borrow the base service's singletons — the refactor keeps exactly that and only adds the interceptor. The standalone SolidityNode is out of scope: its database holds solidified data only, so it needs no cursor switching and never runs the cursor services.
A second duplication — the WalletApi / WalletSolidityApi read handlers. Inside the base RpcApiService, WalletApi (protocol.Wallet) and WalletSolidityApi (protocol.WalletSolidity) are two separate proto services whose read methods re-implement the same logic twice. gRPC generates one ImplBase per proto service and Java is single-inheritance, so both classes have to exist — but their duplicated handler bodies are a second axis of duplication, per proto service rather than per port, and it is where the drift noted in Motivation actually happened.
Limitations or Risks
- Every RPC addition or change has to be repeated in each copy, and forgetting one silently produces different behavior per port.
- Reviews and tests have to cover the same handler several times over.
Proposed Solution
Proposed Design
Add a cursor-parameterized ServerInterceptor. The Solidity and PBFT services stop declaring their own handlers, register the base service's WalletSolidity / Database singletons directly, and attach the interceptor that sets the cursor for the duration of the call and restores it afterwards.
public abstract class CursorServerInterceptor implements ServerInterceptor {
// sets the cursor before the handler runs and restores it in a finally block
}Two properties decide whether this works, and both should be verified before any code is deleted:
- The cursor must be switched on the thread that runs the handler. gRPC dispatches a call's callbacks through the server's executor without pinning them to one thread, so the cursor has to be set where the handler actually runs, not when the call arrives. Getting this wrong fails silently — the port serves HEAD data with no error.
- Handlers that read the database must be synchronous. The cursor is restored as soon as the handler returns, so anything deferring its reads to another thread would read after the restore.
Separately, the read-only WalletSolidityApi handlers delegate to the shared WalletApi singleton instead of repeating the same bodies. The two generated ImplBase classes and their signatures stay; only the duplicated bodies go. This is safe for the same reason as the interceptor: which snapshot a read resolves against depends on the calling thread's cursor, not on which class the handler lives in.
Key Changes
- Module:
framework - Add: a cursor
ServerInterceptorplus its Solidity and PBFT subclasses, and tests covering the two properties above - Modify: service and interceptor registration in the two cursor services; the read-only handler bodies in
RpcApiService - Delete: ~100 per-method delegations and the duplicated inner service classes
- Config / API: unchanged — same ports, same switches, same proto definitions
Impact
- Developer experience: an RPC change goes from "edit in four places" back to one, removing the drift risk entirely.
- Security: the existing interceptor chain (rate limiting, API access, lite-fullnode filtering, metrics) is untouched.
- Performance: one interceptor per call replaces one lambda per method; overhead is comparable.
Compatibility
- Breaking change: No. Proto definitions, ports and responses are unchanged.
- Default behavior change: One. Because both services now share the same singleton, the PBFT port also exposes
getPaginatedNowWitnessListandgetTransactionInfoByBlockNum. PBFT lacked them only because the two services were written separately; both are ordinary read-only queries that resolve against the PBFT snapshot like every other read, so the proposal aligns rather than shields them. - Migration required: No.
References
- [Feature] Deduplicate HTTP servlet stacks with cursor filters and a declarative endpoint registry #6922 — the equivalent HTTP servlets dedup
Additional Notes
- Do you have ideas regarding implementation? Yes
- Are you willing to implement this feature? Yes
Metadata
Metadata
Assignees
Labels
Type
Projects
- StatusShow more project fieldsNo status
Activity
xxo1shine commentedon Aug 26, 2026
@Sunny6889, this deduplication direction makes sense, especially because it can reduce behavior drift between FullNode gRPC ports caused by duplicated implementations.
Could you clarify where the new cursor interceptor will be inserted in the existing interceptor chain? Have you verified that its execution order and behavior remain unchanged with rate limiting, API access, lite-fullnode filtering, metrics, and the other existing interceptors?
SeriousCoding789 commentedon Aug 26, 2026
Thanks @xxo1shine — that's exactly the right thing to pin down, and it was one of the first things I worked through.
Placement. The cursor interceptor is registered before
super.addInterceptor(...), which makes it the innermost one — in gRPC the first-registered interceptor ends up closest to the handler. The existing four (rate limiter, API access, lite-fullnode query, Prometheus) keep their current registration, and their order relative to each other is untouched.Behaviour. The inbound path is therefore unchanged: every existing interceptor still runs, in the same order, before the cursor interceptor. A call rejected by rate limiting, API access or the lite-fullnode filter is rejected exactly as it is today, before the cursor is ever touched. The cursor interceptor brackets the handler alone — it switches the cursor on the thread that actually runs the handler and restores it in a
finally, including when the handler throws.I verified the ordering empirically against gRPC 1.83.0 rather than assuming it, and that conclusion is asserted in a test, so a future gRPC upgrade that flipped the semantics would fail the build instead of silently re-ordering the chain. The same applies to the thread affinity the cursor depends on.
The PR is close and I'll post it shortly — the ordering and threading tests ship with it, so this is easy to verify rather than take on trust.
waynercheung commentedon Aug 26, 2026
Thanks @Sunny6889 - I checked the claims against develop and they hold up. I support the cursor-interceptor direction, with the correctness and sequencing points below.
The drift is real and traceable
WalletApi.isSpend(RpcApiService.java:2339) returns afteronError(); theWalletSolidityApi.isSpendcopy (RpcApiService.java:726) does not. On the Solidity and PBFT ports it therefore falls through toonCompleted()and can triggerIllegalStateException: call already closed.git blametraces the fix to5486ec80ca("process some potential exceptions", 2020-06-05), whose 23 addedreturn;statements all landed on the WalletApi side. Eight shielded read methods are affected:getMerkleTreeVoucherInfo,scanNoteByIvk,scanAndMarkNoteByIvk,scanNoteByOvk,isSpend,scanShieldedTRC20NotesByIvk,scanShieldedTRC20NotesByOvk,isShieldedTRC20ContractNoteSpent. That is six years of divergence between port 50051 and ports 50061/50073, so the maintenance problem described here is not hypothetical.Property 1 is where this can fail silently
On grpc-java 1.83.0,
interceptCall()runs in the1HandleServerCalltask while the unary handler runs in a separate1HalfClosedtask, both submitted through aSerializingExecutorbacked by the application executor.SerializingExecutorguarantees ordering, not thread affinity, andrpcThreadNumdefaults to(cores + 1) / 2. The cursor must therefore be set and restored around the wrappedServerCall.Listener.onHalfClose(), not aroundinterceptCall()ornext.startCall().Please also reword "for the duration of the call" to "for the duration of the synchronous handler callback". A call spans multiple callbacks and may span multiple threads, so call‑lifetime scoping is not a safe model for a ThreadLocal.
The test should be deterministic rather than relying on a multi‑threaded pool:
interceptCall()on thread A.onHalfClose()on a known, different thread B.An implementation that scopes the cursor only around
interceptCall()then fails by construction, rather than depending on thread‑pool scheduling luck.Property 2 holds today for the relevant WalletSolidity and Database RPCs: they are all unary, and no executor, thread, or future handoff appears in the audited read‑handler paths. This is still an implementation invariant rather than a type‑level guarantee, so it is worth documenting on the interceptor – a future handler that moves a database read to another executor will need explicit cursor propagation.
Interceptor ordering (@xxo1shine)
ServerImplwalks the interceptor array forward, wrapping the handler on each iteration, and the array preservesintercept()registration order. The last interceptor added is therefore outermost and executes first. With Prometheus enabled the effective order isprometheus -> liteFnQuery -> apiAccess -> rateLimiter -> handler.Rather than choosing a slot in that order, I would attach the cursor interceptor directly to the shared service definitions –
ServerInterceptors.intercept(rpcApiService.getWalletSolidityApi(), cursorInterceptor), and the same forDatabaseApi. Service‑level interceptors are part of theServerServiceDefinitionand remain inside the server‑level chain, which makes "the existing chain is untouched" a structural property rather than an ordering assumption. It also keeps the cursor interceptor offProtoReflectionServicewhen reflection is enabled – relevant for PBFT, becauseManager.setCursor(PBFT)readsgetHeadBlockNum()andgetLatestPbftBlockNum()on every switch. For the existing methods nothing changes:WalletPBFTApialready extendsWalletSolidityImplBase, so the shared singleton exposes the sameprotocol.WalletSolidityservice, andLiteFnQueryGrpcInterceptor,RpcApiAccessInterceptor, and the rate limiter continue to see identical endpoint names and method descriptors.Requested scope and sequencing
Land the missing
return;statements as a separate bug‑fix PR first. It is an independently valid bug affecting two ports and is worth backporting on its own schedule. Keeping it separate also prevents the later handler‑deduplication PR from mixing behavior fixes with code movement.Split cursor‑service deduplication from handler deduplication. The two have different risk profiles: the cursor‑interceptor work deletes pure delegation and is mechanically checkable, while the
WalletSolidityApi/WalletApideduplication changes handler ownership and behavior. Of the 47 shared methods, 28 are textually identical and 19 differ. Most differences are cosmetic, but the 19 also include the eight missing‑return;cases above and the "Solidity" versus "FullNode" diagnostic prefix ingetAssetIssueByName. Please include that 19‑method pre‑refactor inventory in the handler‑deduplication PR description, marking each difference as intentional, accidental, or resolved by the prerequisite bug‑fix PR, so reviewers do not have to reconstruct it.Consider a neutral shared read‑handler component for the second PR. The
WalletSolidityImplBasedescriptor still keeps write RPCs off the wire, so direct delegation toWalletApidoes not widen the transport surface. It does, however, couple the restricted read semantics to an implementation that also serves HEAD. I would treat the neutral component as a design trade‑off rather than a prerequisite: it adds another delegation layer but makes the snapshot‑neutral semantics explicit. If direct delegation is retained, please document the snapshot‑neutral invariant and keep the WalletSolidity read surface explicitly enumerated.Document the PBFT surface additions.
getPaginatedNowWitnessListandgetTransactionInfoByBlockNummove from UNIMPLEMENTED to real PBFT‑snapshot responses. Aligning them is the right direction, but it is still an observable API‑surface change: it needs a changelog entry, a review of known first‑party clients, and a compatibility note for clients that may use UNIMPLEMENTED to identify a PBFT endpoint.On rate limiting and access control there is nothing to do:
RateLimiterInterceptor.init()enumerates methods from the live server, so both pick up the default QPS limiter automatically, andRpcApiAccessInterceptormatches on method name, so it already covers them.Lite‑fullnode filtering is worth a closer look. Neither method is in
LiteFnQueryGrpcInterceptor, and forgetTransactionInfoByBlockNumthat looks accidental rather than deliberate: the project's ownLiteFnQueryHttpFilterdoes block/wallet/gettransactioninfobyblocknum,/walletsolidity/gettransactioninfobyblocknum, and/walletpbft/gettransactioninfobyblocknum, so the two surfaces disagree about whether this is a history query. Comparing the two filter sets end to end, it is the only such divergence – every other HTTP‑filtered endpoint either has a matching gRPC entry or no gRPC counterpart at all. (getPaginatedNowWitnessListis not a history query, so its absence from both filters looks correct.) This predates the refactor, but exposing the method on a third port is a good moment to fix it. I would suggest a separate issue rather than folding a filter‑policy change into the deduplication PR.Test expectations
RpcApiServicesTestalready starts all three servers, but most existing assertions only useassertNotNull(), which catches neither the error‑handling divergence above nor an interceptor that silently reads HEAD. The tests need to cover two distinct properties:onError()path must not subsequently callonCompleted().Beyond that I would expect coverage for the cursor observed inside the handler when
interceptCall()andonHalfClose()run on different threads; cursor restoration to HEAD after both normal and exceptional execution; no leakage under concurrent calls and pooled‑thread reuse; service‑descriptor parity including the two newly exposed PBFT methods; and unchanged behavior for the standalone SolidityNode.With these points covered, I am supportive of the refactor and happy to review the individual PRs.
SeriousCoding789 commentedon Aug 26, 2026
Thanks @waynercheung — thorough review, and the
5486ec80catrace is a cleaner articulation of the drift than mine. I'll fold it in.On the implementation details:
onHalfClose(), notinterceptCall()/next.startCall(), and restored in afinallyincluding when the handler throws. On ordering my reading of 1.83.0 matches yours: last-registered is outermost, givingprometheus -> liteFnQuery -> apiAccess -> rateLimiter -> handler.ProtoReflectionService, which is bidi-streaming and would otherwise sit inside the bracket; your PBFT point makes that more than cosmetic. Will go that way.return;bug-fix, (2) cursor-service dedup, (3) handler dedup with the per-method inventory in the description. My count reads 29 identical / 18 differing of the 47 shared methods rather than 28/19 — likely whitespace normalisation; I'll publish the list so we can reconcile it there.getTransactionInfoByBlockNumfilter divergence is pre-existing and belongs in its own issue.On the neutral shared read-handler component I'd like to think further before committing either way; if direct delegation stays, I'll document the snapshot-neutral invariant and keep the WalletSolidity read surface explicitly enumerated.
waynercheung commentedon Sep 2, 2026
Thanks @Sunny6889. A few clarifications to close the remaining design questions.
Count reconciliation. The 28/19 vs. 29/18 discrepancy is normalization-only on
develop@4a21592f95:getAvailableUnfreezeCountdiffers only in indentation. Ignoring whitespace leaves 15 method bodies with non-whitespace differences:return;statements afteronError();getAssetIssueByNamediagnostic prefix;getAccount,getAccountById,getNowBlock2,getBlockByNum,getBlockByNum2, andgetTransactionById.The two TRC20 scan methods also differ in exception logging, while the negative guards in
getBlockByNum*avoid a failed lookup and log entry. These are operational/logging differences rather than wire-level differences and should be recorded in the handler-dedup PR. After the missing-return fix, I see no remaining wire-level semantic divergence.Shared read handler. I would not block the handler-dedup PR on introducing a neutral component. None of the 47 handlers contains snapshot-specific logic; the relevant reads execute synchronously and resolve through the calling thread's cursor. Direct delegation is sufficient if that invariant and the explicit
WalletSoliditysurface remain documented.Sequencing. The missing-return fix is independent and a backport candidate, so it should be submitted separately. The
getTransactionInfoByBlockNumlite-fullnode filter divergence is orthogonal and should be tracked in a separate issue rather than folded into either refactor.