feat(db): support solidity conditional shutdown#70
feat(db): support solidity conditional shutdown#70
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 35 minutes and 39 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change relocates the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="framework/src/main/java/org/tron/program/SolidityNode.java">
<violation number="1" location="framework/src/main/java/org/tron/program/SolidityNode.java:69">
P1: Shutdown can hang because stop flag is not honored inside the inner retry loops (`getBlockByNum` / `getLastSolidityBlockNum`).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
317787106
force-pushed
the
feature/solidity_shutdown
branch
from
3fc0053 to
0e5a48f
Compare
317787106
changed the title
| resolveCompatibilityIssueIfUsingFullNodeDatabase(); | ||
| ID.set(chainBaseManager.getDynamicPropertiesStore().getLatestSolidifiedBlockNum()); | ||
| databaseGrpcClient = new DatabaseGrpcClient(CommonParameter.getInstance().getTrustNodeAddr()); | ||
| remoteBlockNum.set(getLastSolidityBlockNum()); |
There was a problem hiding this comment.
[MUST] Move blocking remote RPC out of @PostConstruct
init() is executed synchronously during ApplicationContext.refresh(). It calls new DatabaseGrpcClient(trustNodeAddr) followed by remoteBlockNum.set(getLastSolidityBlockNum()), and getLastSolidityBlockNum() now runs a while (flag) retry loop that can only exit when flag becomes false (i.e. via @PreDestroy). If the trust node is unreachable at startup, the loop never returns, context.refresh() never completes, and context.registerShutdownHook() in FullNode.main is never registered — the process becomes unresponsive to SIGTERM. This is a regression vs. the old SolidityNode.start() which invoked this call after appT.startup(), when the shutdown hook was already wired.
Suggestion: Move databaseGrpcClient creation and remoteBlockNum initialization out of @PostConstruct into run() (or first iteration of getBlock). Keep @PostConstruct limited to local field initialization.
| public void pushVerifiedBlock(BlockCapsule block) throws P2pException { | ||
| block.generatedByMyself = true; | ||
| long start = System.currentTimeMillis(); | ||
| processBlock(block, true); |
There was a problem hiding this comment.
[MUST] hitDown path leaves callers writing a fake success
When latestSolidityNumShutDown is hit, processBlock flips hitDown=true, unparks the hit-thread, and returns normally. The caller SolidityNode.loopProcessBlock treats that as success, then executes saveLatestSolidifiedBlockNum(blockNum) and logs "Success to process block", while the block was never actually pushed into BlockStore. Depending on the race between the hit-thread's System.exit(0) and the current thread's save, LATEST_SOLIDIFIED_BLOCK_NUM may be advanced to a block number that does not exist in the store — producing an inconsistent DB state and misleading logs.
Suggestion: Throw a dedicated sentinel (e.g. P2pException with a new TypeEnum, or a HitDownException) from processBlock when entering the hitDown branch, and let loopProcessBlock break out without calling saveLatestSolidifiedBlockNum. Alternatively, let the caller check tronNetDelegate.isHitDown() before writing state.
| new Thread(this::getBlock).start(); | ||
| new Thread(this::processBlock).start(); | ||
| getBlockExecutor.submit(this::getBlock); | ||
| processBlockExecutor.submit(this::processBlock); |
There was a problem hiding this comment.
[SHOULD] Use ExecutorServiceManager.submit for TronError propagation
getBlockExecutor.submit(this::getBlock) and processBlockExecutor.submit(this::processBlock) use the raw ExecutorService.submit. Any uncaught Throwable (OOM, AssertionError, unexpected RuntimeException) is captured inside the FutureTask and swallowed — the solidity sync threads die silently, with no log and no JVM exit. The project-wide convention is ExecutorServiceManager.submit(es, task) (see DposTask.java:78, TransactionsMsgHandler.java:90/120, Manager.java:562/567/573), which wraps the task in a try/catch that routes TronError through ExitManager::logAndExit.
Suggestion: Replace both submits with ExecutorServiceManager.submit(getBlockExecutor, this::getBlock) and the equivalent for processBlockExecutor.
| sleep(exceptionSleepTime); | ||
| } | ||
| } | ||
| throw new RuntimeException("SolitityNode is closing."); |
There was a problem hiding this comment.
[SHOULD] Fix typo and avoid raw RuntimeException for shutdown path
Two issues on the shutdown exit of getBlockByNum (line 157) and getLastSolidityBlockNum (line 174):
- The string
"SolitityNode is closing."has a typo —Solidityis misspelled asSolitity. This text surfaces inlogger.error(e.getMessage())downstream and will hamper future log grepping. - Using a bare
RuntimeExceptionto signal graceful shutdown makes it indistinguishable from a real error in callers that catchException— the normal shutdown ends up being logged at ERROR level.
Suggestion: Correct the spelling, and either define a private sentinel exception (e.g. ShuttingDownException extends RuntimeException) or change the method to return null / return -1 and have callers check + break explicitly.
| sleep(exceptionSleepTime); | ||
| } | ||
| } | ||
| throw new RuntimeException("SolitityNode is closing."); |
There was a problem hiding this comment.
[SHOULD] Re-interrupt after catching InterruptedException in sleep()
(Anchored here because sleep() at line 177-183 is outside the diff hunk.)
sleep(long) catches Exception and only logs e.getMessage(), dropping the interrupt flag when InterruptedException is the cause. During @PreDestroy, ExecutorServiceManager.shutdownAndAwaitTermination relies on shutdownNow() interrupting worker threads to end them quickly; by swallowing the interrupt bit, the worker may continue iterating while (flag) for an extra cycle and slow shutdown.
Suggestion: Catch InterruptedException specifically and call Thread.currentThread().interrupt(); before/after the log, and keep a separate catch (Exception e) for other cases if needed.
| Args.getInstance().setTrustNodeAddr(trustNodeAddr); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
[SHOULD] Restore coverage lost with testSolidityArgs and add tests for new surfaces
This PR deletes testSolidityArgs (which asserted TronError(SOLID_NODE_INIT) when trustNodeAddr is missing), deletes a pushVerifiedBlock assertion in ManagerTest.transactionTest, and introduces new surfaces without any test:
TronNetDelegate.pushVerifiedBlock(BlockCapsule)(new method, zero tests)SolidityNodelifecycle (@PostConstruct/@PreDestroy/run()) (zero integration tests)- The
hitDownshutdown path end-to-end
The PR main behavioral promise — "shutdown signal is now reachable from solidity mode" — has no automated coverage; any future change to processBlock hitDown branch or the solidity lifecycle will regress without CI catching it.
Suggestion: Add at minimum (1) a happy-path and a hitDown test for TronNetDelegate.pushVerifiedBlock verifying that LATEST_SOLIDIFIED_BLOCK_NUM is NOT advanced on hitDown; (2) an integration test for SolidityNode lifecycle; (3) a test at FullNode level for the missing-trust-node case that replaces the deleted testSolidityArgs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (3)
framework/src/main/java/org/tron/program/SolidityNode.java (2)
177-183:⚠️ Potential issue | 🟡 Minor
sleepswallowsInterruptedExceptionand drops the interrupt flag.
shutdown()relies onExecutorServiceManager.shutdownAndAwaitTermination→shutdownNow()interrupting workers. CatchingExceptionand only logginge.getMessage()here clears the interrupt status, sogetBlock/processSolidityBlockmay iterate one morewhile (flag)cycle and even re-enterblockQueue.put(...)/blockQueue.take()before noticingflag = false, slowing teardown.🛡️ Suggested change
public void sleep(long time) { try { Thread.sleep(time); - } catch (Exception e1) { - logger.error(e1.getMessage()); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/tron/program/SolidityNode.java` around lines 177 - 183, The sleep method in SolidityNode currently catches Exception and swallows InterruptedException, clearing the thread's interrupt status; change the catch to specifically catch InterruptedException, call Thread.currentThread().interrupt() to restore the interrupt flag, optionally log the interruption with the exception, and return immediately so workers in getBlock/processSolidityBlock and blocking calls (blockQueue.put/take) can observe the interruption during ExecutorServiceManager.shutdownAndAwaitTermination → shutdownNow() triggered from shutdown().
138-158:⚠️ Potential issue | 🟡 MinorAvoid signaling normal shutdown via bare
RuntimeException.Typo
"Solitity"is now fixed, 👍. The remaining concern stands: callers (getBlock,loopProcessBlock,processSolidityBlock) allcatch (Exception e)andlogger.error(e.getMessage()), so a clean shutdown surfaces asERROR: SolidityNode is closing.and is indistinguishable from a real failure. InloopProcessBlockthe rethrow at line 133 also propagates up out of thetryblock without being suppressed during shutdown.Either define a private sentinel type (e.g.
class ShuttingDownException extends RuntimeException) and special-case it (or skip logging), or change these helpers to returnOptional<Block>/OptionalLongand have callers checkflagand break.Also applies to: 160-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/tron/program/SolidityNode.java` around lines 138 - 158, Replace the bare RuntimeException used to signal normal shutdown with a sentinel exception and handle it specially: add a private static class ShuttingDownException extends RuntimeException (no stack-trace needed) and throw new ShuttingDownException() from getBlockByNum (and the similar shutdown sites around 160-175) instead of new RuntimeException("SolidityNode is closing."); then update callers such as loopProcessBlock, processSolidityBlock and getBlock to catch ShuttingDownException separately (or rethrow it) so they do not log it as an error — treat it as a normal shutdown signal (skip logger.error and break/return) while other Exceptions continue to be logged and handled as before.framework/src/main/java/org/tron/core/net/TronNetDelegate.java (1)
236-245:⚠️ Potential issue | 🔴 Critical
pushVerifiedBlocksucceeds silently whenprocessBlockenters thehitDownbranch.When
processBlockhits the shutdown condition (lines 248-258), it setshitDown = true, unparkshitThread, and returns normally — no exception.pushVerifiedBlockthen logs"Push block cost: ..."and returns. The caller inSolidityNode.loopProcessBlock(lines 121-136) treats this as a successful push and proceeds tosaveLatestSolidifiedBlockNum(blockNum), advancingLATEST_SOLIDIFIED_BLOCK_NUMto a block number that was never persisted toBlockStore. Depending on the race betweenhitThread'sSystem.exit(0)and the caller's save, the DB ends up in an inconsistent state with misleading INFO logs.Either propagate a sentinel (e.g. dedicated
P2pExceptionTypeEnumor aHitDownException) fromprocessBlockso the caller can break without saving, or checkisHitDown()here before logging success and have callers honor it.🛡️ One possible shape
public void pushVerifiedBlock(BlockCapsule block) throws P2pException { block.generatedByMyself = true; long start = System.currentTimeMillis(); processBlock(block, true); + if (hitDown) { + throw new P2pException(TypeEnum.SHUTDOWN, "shutdown condition reached"); + } logger.info("Push block cost: {} ms, blockNum: {}, blockHash: {}, trx count: {}.", System.currentTimeMillis() - start, block.getNum(), block.getBlockId(), block.getTransactions().size()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/tron/core/net/TronNetDelegate.java` around lines 236 - 245, pushVerifiedBlock currently treats processBlock return as success even when processBlock sets hitDown and unparks hitThread; update pushVerifiedBlock to detect shutdown and avoid logging/success behavior or change processBlock to propagate a sentinel exception: either (A) have processBlock throw a dedicated P2pException subtype (e.g., HitDownException / TypeEnum.HIT_DOWN) when it enters the hitDown branch and let pushVerifiedBlock rethrow it so callers such as SolidityNode.loopProcessBlock can skip saveLatestSolidifiedBlockNum, or (B) after calling processBlock(block, true) check process.isHitDown() (or the hitDown flag exposed) and if true skip the success log and return/throw so callers do not persist the unprocessed block; update SolidityNode.loopProcessBlock to honor the exception/flag and avoid advancing LATEST_SOLIDIFIED_BLOCK_NUM when hitDown is observed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@framework/src/main/java/org/tron/core/net/TronNetDelegate.java`:
- Around line 236-245: pushVerifiedBlock currently treats processBlock return as
success even when processBlock sets hitDown and unparks hitThread; update
pushVerifiedBlock to detect shutdown and avoid logging/success behavior or
change processBlock to propagate a sentinel exception: either (A) have
processBlock throw a dedicated P2pException subtype (e.g., HitDownException /
TypeEnum.HIT_DOWN) when it enters the hitDown branch and let pushVerifiedBlock
rethrow it so callers such as SolidityNode.loopProcessBlock can skip
saveLatestSolidifiedBlockNum, or (B) after calling processBlock(block, true)
check process.isHitDown() (or the hitDown flag exposed) and if true skip the
success log and return/throw so callers do not persist the unprocessed block;
update SolidityNode.loopProcessBlock to honor the exception/flag and avoid
advancing LATEST_SOLIDIFIED_BLOCK_NUM when hitDown is observed.
In `@framework/src/main/java/org/tron/program/SolidityNode.java`:
- Around line 177-183: The sleep method in SolidityNode currently catches
Exception and swallows InterruptedException, clearing the thread's interrupt
status; change the catch to specifically catch InterruptedException, call
Thread.currentThread().interrupt() to restore the interrupt flag, optionally log
the interruption with the exception, and return immediately so workers in
getBlock/processSolidityBlock and blocking calls (blockQueue.put/take) can
observe the interruption during
ExecutorServiceManager.shutdownAndAwaitTermination → shutdownNow() triggered
from shutdown().
- Around line 138-158: Replace the bare RuntimeException used to signal normal
shutdown with a sentinel exception and handle it specially: add a private static
class ShuttingDownException extends RuntimeException (no stack-trace needed) and
throw new ShuttingDownException() from getBlockByNum (and the similar shutdown
sites around 160-175) instead of new RuntimeException("SolidityNode is
closing."); then update callers such as loopProcessBlock, processSolidityBlock
and getBlock to catch ShuttingDownException separately (or rethrow it) so they
do not log it as an error — treat it as a normal shutdown signal (skip
logger.error and break/return) while other Exceptions continue to be logged and
handled as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ed13c09-5186-4dfa-a41f-48a7572e20f7
📒 Files selected for processing (6)
framework/src/main/java/org/tron/core/db/Manager.javaframework/src/main/java/org/tron/core/net/TronNetDelegate.javaframework/src/main/java/org/tron/program/FullNode.javaframework/src/main/java/org/tron/program/SolidityNode.javaframework/src/test/java/org/tron/core/db/ManagerTest.javaframework/src/test/java/org/tron/program/SolidityNodeTest.java
💤 Files with no reviewable changes (3)
- framework/src/test/java/org/tron/core/db/ManagerTest.java
- framework/src/main/java/org/tron/core/db/Manager.java
- framework/src/test/java/org/tron/program/SolidityNodeTest.java
2 participants
Summary
Closes tronprotocol#6610.
Refactors
SolidityNodeinto a Spring-managed component with proper lifecycle support, enabling it to respond to conditional shutdown signals (block height, block time, block count) already implemented inTronNetDelegate.processBlock. The previous staticstart()approach bypassed Spring entirely, so the shutdown conditions could never be reached during solidity sync.Problem
SolidityNodepreviously initialized its ownTronApplicationContext, created aSolidityNodeinstance manually, and calledappT.blockUntilShutdown()in a separate JVM entry path. This architecture had two consequences:TronNetDelegate.processBlock(checkinglatestSolidityNumShutDown), butSolidityNodecalledManager.pushVerifiedBlock -> Manager.pushBlock-- a path that never passes throughTronNetDelegate. The shutdown signal was therefore silently ignored.@PreDestroylifecycle.Changes
SolidityNode(framework)@Componentwith@Conditional(SolidityCondition.class)-- only registered when--solidityflag is set.@PostConstruct init(): initialises namedExecutorServicethread pools viaExecutorServiceManagerand the gRPC client.@PreDestroy shutdown(): setsflag = false, shuts down both executors, and closes the gRPC client cleanly.getBlockByNum()andgetLastSolidityBlockNum()loops now checkflag; they throwRuntimeExceptioninstead of looping forever when the node is shutting down.loopProcessBlock()now callstronNetDelegate.pushVerifiedBlock()instead ofdbManager.pushVerifiedBlock(), routing blocks through the shutdown-condition check inprocessBlock.TronNetDelegate(framework)pushVerifiedBlock(BlockCapsule): setsgeneratedByMyself = trueand delegates toprocessBlock(block, true), which already contains the conditional-shutdown logic. Includes timing log consistent with the removedManagermethod.Manager(framework)pushVerifiedBlock(). Block push for solidity sync now goes throughTronNetDelegate.FullNode(framework)trustNodeAddrvalidation andp2pDisable = trueare now enforced before the Spring context is created.appT.startup(), retrieves theSolidityNodebean from the context and callsrun()-- reusing the same application lifecycle rather than spawning a second context.Test
SolidityNodeTest#testSolidityGrpcCall-- verifies gRPC connectivity through the runningRpcApiService.SolidityNodeTest#testSolidityNodeHttpApiService-- verifies HTTP service start/stop idempotency.ManagerTest#transactionTest(partial) -- thepushVerifiedBlockassertion is commented out as the method no longer exists onManager; the rest of the test is unaffected.Summary by CodeRabbit
Refactor
Tests