feat(API): refactor merge rpc - #6977
SeriousCoding789 wants to merge 2 commits into
Conversation
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.
| DatabaseGrpc.newBlockingStub(channel).getNowBlock(EmptyMessage.getDefaultInstance()); | ||
|
|
||
| Assert.assertEquals("cursor must be set exactly once per call", 1, setOn.size()); | ||
| Assert.assertEquals("cursor must be restored exactly once per call", 1, resetOn.size()); |
There was a problem hiding this comment.
[SHOULD] The blocking RPC returning does not guarantee that the server has executed resetCursor() in the interceptor’s finally block. The client can receive the response first, so this assertion may observe resetOn.size() == 0 and fail intermittently.
Please count down a latch after recording the reset, then await it with a timeout before checking the reset count and thread.
| final List<String> setOn = new CopyOnWriteArrayList<>(); | ||
| final List<String> resetOn = new CopyOnWriteArrayList<>(); | ||
| final String[] handlerOn = new String[1]; | ||
|
|
||
| Manager manager = mock(Manager.class); | ||
| doAnswer(inv -> setOn.add(Thread.currentThread().getName())) | ||
| .when(manager).setCursor(any(Chainbase.Cursor.class)); | ||
| doAnswer(inv -> resetOn.add(Thread.currentThread().getName())) | ||
| .when(manager).resetCursor(); |
There was a problem hiding this comment.
[SHOULD] cursorAfterCall is captured on thread A immediately after interceptCall(), before thread B executes onHalfClose(). This assertion therefore checks the initial state rather than whether the cursor was restored. The test still passes when the reset mock does nothing.
Please capture the cursor on thread B after listener.onHalfClose() returns. Using a ThreadLocal<Cursor> would also better reflect the production behavior.
What does this PR do?
Implements #6927 — the gRPC counterpart of the HTTP servlet dedup in #6922.
RpcApiServiceOnSolidityandRpcApiServiceOnPBFTeach re-declare the whole read surface as per-method delegations whose only job is to switch the per-thread read cursor: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:CursorServerInterceptorwithSolidityCursorInterceptor/PbftCursorInterceptor. It bracketsListener.onHalfClose()— the callback gRPC runs a unary handler inline from — setting the cursor before and restoring it in afinally.DatabaseApi/WalletSolidityApisingletons directly, wrapped inServerInterceptors.intercept(...).RpcApiServiceOnSoliditygoes from 488 lines to 36,RpcApiServiceOnPBFTfrom 495 to 36.RpcApiServiceitself:WalletSolidityApi(servingprotocol.WalletSolidity) re-implements read handlersWalletApi(servingprotocol.Wallet) already has. 41 of its 47 methods now delegate to the sharedWalletApisingleton; the remaining 6 already routed through shared*Common/callContracthelpers, so no duplicated handler body is left. Java is single-inheritance and gRPC generates oneImplBaseper proto service, so both classes have to stay — only the bodies go.responseObserver.onError(...)and then falls through toresponseObserver.onCompleted()closes the call twice; the secondclose()hitscheckState(!closeCalled, "call already closed")in gRPC'sServerCallImpland throws.RpcApiServicehad 32 handlers shaped that way ondevelop— 8 disappear with the duplicatedWalletSolidityApibodies, and the remaining 24 get an explicitreturn, 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 ondevelopand 18 differ. Of those 18:WalletApi— it gained areturnafterresponseObserver.onError(...)andWalletSolidityApidid not, so on the error path the copy serving the Solidity and PBFT ports falls through toonCompleted()and terminates the call twice (getMerkleTreeVoucherInfo,isSpend,scanAndMarkNoteByIvk,scanNoteByIvk,scanNoteByOvk,isShieldedTRC20ContractNoteSpent,scanShieldedTRC20NotesByIvk,scanShieldedTRC20NotesByOvk). The last two additionally have aBadItemException | ZksnarkExceptionbranch with logging that onlyWalletApiever received.getBlockByNum,getBlockByNum2) drifted the other way:WalletSolidityApihas anum >= 0guardWalletApilacks.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
returnhardening was never finished onWalletApieither — 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
developMethod sets, per port. The base
WalletSolidityApi(47 methods) andDatabaseApi(4) expose exactly the same methods before and after — only bodies changed.RpcApiService)The PBFT port gains
getPaginatedNowWitnessListandgetTransactionInfoByBlockNum— the only two methodsRpcApiServiceOnPBFTnever mirrored fromRpcApiServiceOnSolidity; they returnedUNIMPLEMENTEDthere before. Both are ordinary reads and resolve against the PBFT snapshot like every other read on that port.Handler bodies. Only three
WalletApibodies changed beyond the addedreturns:getBlockByNum/getBlockByNum2adopt the solidity copy'snum >= 0guard. No response change —Wallet#getBlockByNumalready catches theStoreExceptionand returnsnullfor a negative number, so both paths reachonNext(null); the guard only skips a futile store lookup and its log line.getAssetIssueByNamedrops the"FullNode "prefix from onelogger.debugline, which was the only difference between the two copies.Error paths. Every handler that used to emit
onErrorfollowed byonCompletednow emits a single terminal event, on all three ports. Not visible to clients —onErrorhad already closed the call with the error status and the secondclose()threw before sending anything; what goes away is one server-sideIllegalStateExceptionper failed call. Two groups:WalletischeckAllowShieldedTransactionApi(), andnode.allowShieldedTransactionApidefaults tofalse, so on a default node every such call took the double-close path.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#setCursorstill computes theheadNum - pbftNumoffset for PBFT, exactly asWalletOnPBFT.futureGetdid.Scope. gRPC only.
WalletOnCursor/WalletOnSolidity/WalletOnPBFTstay, because the HTTP and JSON-RPC servlets still callfutureGet; 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:
CursorInterceptorScopeTest(2) drivesinterceptCall()on one thread and the returned listener'sonHalfClose()on another — which is what gRPC'sSerializingExecutoris free to do. An implementation that scoped the cursor aroundinterceptCallfails here by construction rather than by luck. Also covers thefinallyreset 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 fromonHalfClose, 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 realaddServiceof both cursor services against a mock builder and asserts each shared read service is registered as an intercepted definition. DroppingServerInterceptors.interceptleaves every other test green while the port silently serves HEAD, so this is the gRPC counterpart ofCursorFilterInstallationTest.RpcApiServiceErrorPathTest(2) drives every unary handler ofWalletApiandWalletSolidityApiwith collaborators that throw, and asserts none of them terminates the call more than once. Reverting the addedreturns makes it fail ongetDelegatedResourceV2,getPendingSizeandgetBlock, so it reaches the shared*Commonhelpers as well.RpcApiServicesTest— the existing suite drives all three ports end to end over 129 tests; it now also callsgetPaginatedNowWitnessListandgetTransactionInfoByBlockNumon the PBFT stub, pinning the one intentional behaviour change.