feat(db): support solidity conditional shutdown#70
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoves Changes
Sequence Diagram(s)sequenceDiagram
participant SolidityNode
participant DBGrpcClient
participant TronNetDelegate
participant Manager
participant BlockchainDB
SolidityNode->>DBGrpcClient: fetch BlockCapsule
DBGrpcClient-->>SolidityNode: return BlockCapsule
SolidityNode->>TronNetDelegate: pushVerifiedBlock(block)
note right of TronNetDelegate: set generatedByMyself = true\ncall processBlock(..., isSync = true)
TronNetDelegate->>Manager: processBlock / pushBlock(...)
Manager->>BlockchainDB: persist block / update state
BlockchainDB-->>Manager: ack
Manager-->>TronNetDelegate: result
TronNetDelegate-->>SolidityNode: result / log
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.
There was a problem hiding this comment.
Very good suggestion, fix it.
| 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
There was a problem hiding this comment.
♻️ Duplicate comments (3)
framework/src/main/java/org/tron/program/SolidityNode.java (3)
166-183:⚠️ Potential issue | 🟡 MinorDon't use a plain
RuntimeExceptionfor normal shutdown.This is an expected stop signal, but the callers catch it as a generic failure, log at ERROR, and sleep once more before exiting. A dedicated shutdown exception, or a sentinel return that lets the loops break cleanly, would keep shutdown quiet and deterministic.
🤖 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 166 - 183, Replace the thrown generic RuntimeException in getLastSolidityBlockNum with a clear shutdown signal: either create and throw a dedicated unchecked exception (e.g., NodeShutdownException or SolidityNodeShutdownException) so callers can catch that specific type instead of treating it as an error, or change the method to return a sentinel value (e.g., -1) and update callers to check for that sentinel and break their loops cleanly; update references to flag and any callers of getLastSolidityBlockNum to handle the new exception or sentinel consistently so shutdown is deterministic and not logged as an error.
81-82:⚠️ Potential issue | 🟠 MajorUse
ExecutorServiceManager.submit(...)for these workers.
ExecutorService.submithides uncaughtThrowableinside the returnedFuture, so either sync loop can die silently. Route both tasks through the manager wrapper instead.Proposed fix
- getBlockExecutor.submit(this::getBlock); - processBlockExecutor.submit(this::processSolidityBlock); + ExecutorServiceManager.submit(getBlockExecutor, this::getBlock); + ExecutorServiceManager.submit(processBlockExecutor, this::processSolidityBlock);🤖 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 81 - 82, Replace direct calls to getBlockExecutor.submit(this::getBlock) and processBlockExecutor.submit(this::processSolidityBlock) with the ExecutorServiceManager.submit(...) wrapper so uncaught Throwables are not swallowed by plain ExecutorService futures; locate the invocations around getBlockExecutor and processBlockExecutor in SolidityNode and call ExecutorServiceManager.submit(getBlockExecutor, this::getBlock) and ExecutorServiceManager.submit(processBlockExecutor, this::processSolidityBlock) (or the project’s equivalent static/instance submit method) to ensure exceptions in getBlock and processSolidityBlock are propagated/handled by the manager.
186-191:⚠️ Potential issue | 🟡 MinorPreserve interrupts in
sleep().Catching
Exceptionhere clearsInterruptedException, which works against the shutdown path. CatchInterruptedExceptionexplicitly, restore the interrupt flag, and return.Proposed fix
public void sleep(long time) { try { Thread.sleep(time); - } catch (Exception e1) { - logger.error(e1.getMessage()); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + logger.info("sleep interrupted."); + } catch (Exception e) { + logger.error(e.getMessage()); } }🤖 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 186 - 191, The sleep(long time) method currently catches Exception which swallows InterruptedException; change it to catch InterruptedException explicitly in SolidityNode.sleep, call Thread.currentThread().interrupt() to restore the interrupt flag, and return immediately instead of continuing, and remove the broad Exception catch so other runtime exceptions propagate; keep the logger usage for non-interrupt cases if needed (e.g., log the interruption via logger.warn or logger.debug).
🤖 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/program/SolidityNode.java`:
- Around line 166-183: Replace the thrown generic RuntimeException in
getLastSolidityBlockNum with a clear shutdown signal: either create and throw a
dedicated unchecked exception (e.g., NodeShutdownException or
SolidityNodeShutdownException) so callers can catch that specific type instead
of treating it as an error, or change the method to return a sentinel value
(e.g., -1) and update callers to check for that sentinel and break their loops
cleanly; update references to flag and any callers of getLastSolidityBlockNum to
handle the new exception or sentinel consistently so shutdown is deterministic
and not logged as an error.
- Around line 81-82: Replace direct calls to
getBlockExecutor.submit(this::getBlock) and
processBlockExecutor.submit(this::processSolidityBlock) with the
ExecutorServiceManager.submit(...) wrapper so uncaught Throwables are not
swallowed by plain ExecutorService futures; locate the invocations around
getBlockExecutor and processBlockExecutor in SolidityNode and call
ExecutorServiceManager.submit(getBlockExecutor, this::getBlock) and
ExecutorServiceManager.submit(processBlockExecutor, this::processSolidityBlock)
(or the project’s equivalent static/instance submit method) to ensure exceptions
in getBlock and processSolidityBlock are propagated/handled by the manager.
- Around line 186-191: The sleep(long time) method currently catches Exception
which swallows InterruptedException; change it to catch InterruptedException
explicitly in SolidityNode.sleep, call Thread.currentThread().interrupt() to
restore the interrupt flag, and return immediately instead of continuing, and
remove the broad Exception catch so other runtime exceptions propagate; keep the
logger usage for non-interrupt cases if needed (e.g., log the interruption via
logger.warn or logger.debug).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6d5da9e-9a68-406f-be7d-e7ad20310629
📒 Files selected for processing (3)
framework/src/main/java/org/tron/core/db/Manager.javaframework/src/main/java/org/tron/program/SolidityNode.javaframework/src/test/java/org/tron/core/db/ManagerTest.java
💤 Files with no reviewable changes (2)
- framework/src/test/java/org/tron/core/db/ManagerTest.java
- framework/src/main/java/org/tron/core/db/Manager.java
There was a problem hiding this comment.
♻️ Duplicate comments (1)
framework/src/main/java/org/tron/core/net/TronNetDelegate.java (1)
236-245:⚠️ Potential issue | 🔴 CriticalStill reports a successful push after shutdown short-circuit
At Line 239,
processBlock(...)can return early when hitDown is reached (Lines 248-262), but this method still proceeds to the success-style log at Line 240. That keeps the “processed successfully” ambiguity alive for callers and logs.🤖 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, The pushVerifiedBlock method currently logs a successful push unconditionally even though processBlock(block, true) can short-circuit when hitDown is reached; change processBlock to return a boolean (true if fully processed, false if short-circuited by hitDown) and update pushVerifiedBlock to capture that return value and only emit the success-style logger.info (and keep block.generatedByMyself as appropriate) when the result is true; if false, emit a distinct shutdown/short-circuit log and avoid reporting the block as successfully processed.
🤖 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: The pushVerifiedBlock method currently logs a successful
push unconditionally even though processBlock(block, true) can short-circuit
when hitDown is reached; change processBlock to return a boolean (true if fully
processed, false if short-circuited by hitDown) and update pushVerifiedBlock to
capture that return value and only emit the success-style logger.info (and keep
block.generatedByMyself as appropriate) when the result is true; if false, emit
a distinct shutdown/short-circuit log and avoid reporting the block as
successfully processed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed545f03-5841-4cda-94ff-6d8d9146dee3
📒 Files selected for processing (1)
framework/src/main/java/org/tron/core/net/TronNetDelegate.java
There was a problem hiding this comment.
♻️ Duplicate comments (3)
framework/src/main/java/org/tron/program/SolidityNode.java (3)
193-198:⚠️ Potential issue | 🟠 MajorPreserve thread interrupt status in
sleep(long).Line 196 catches broad
Exceptionand drops interrupts. That makes shutdown via executor interruption less reliable/slower.Suggested patch
public void sleep(long time) { try { Thread.sleep(time); - } catch (Exception e1) { - logger.error(e1.getMessage()); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + logger.info("Sleep interrupted."); + } catch (Exception e1) { + logger.error("Sleep failed.", e1); } }🤖 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 193 - 198, The sleep(long) method in class SolidityNode currently catches a broad Exception and swallows interrupts; modify sleep(long) to catch InterruptedException separately, call Thread.currentThread().interrupt() to preserve the thread's interrupt status and then return or rethrow as appropriate, while keeping the existing catch-all for other Exceptions to handle non-interrupt errors; update references inside SolidityNode.sleep(long) so interrupted shutdown via executor interruption is honored.
88-89:⚠️ Potential issue | 🟠 MajorUse
ExecutorServiceManager.submit(...)to avoid silent worker death.Line 88 and Line 89 still use raw
ExecutorService.submit(...); uncaught throwables can get trapped inFutureTaskand kill the worker without process-level handling.Suggested patch
- getBlockExecutor.submit(this::getBlock); - processBlockExecutor.submit(this::processSolidityBlock); + ExecutorServiceManager.submit(getBlockExecutor, this::getBlock); + ExecutorServiceManager.submit(processBlockExecutor, this::processSolidityBlock);🤖 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 88 - 89, The code in SolidityNode is submitting runnables via ExecutorService.submit(...) which can swallow exceptions; replace those calls with ExecutorServiceManager.submit(...) so submitted tasks get wrapped/handled by the central manager (use the same task/lambda you currently pass to ExecutorService.submit and call ExecutorServiceManager.submit(...) instead), ensuring you update any imports and remove direct ExecutorService.submit(...) usages in the SolidityNode class so uncaught throwables are propagated/handled by the manager.
173-174:⚠️ Potential issue | 🟡 MinorAvoid using generic
RuntimeExceptionfor normal shutdown flow.Line 173 and Line 190 throw generic runtime errors for graceful stop; these propagate into generic
catch (Exception)paths and get logged as failures, creating noisy false-error shutdown logs.Also applies to: 190-190
🤖 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 173 - 174, Replace the generic RuntimeException used to signal a normal/graceful shutdown in SolidityNode with a dedicated exception type (e.g., GracefulShutdownException or use InterruptedException) so shutdown control flow is explicit; update the throw sites in SolidityNode (the lines currently throwing RuntimeException around the graceful stop) to throw that specific exception and adjust any surrounding catch blocks that currently catch Exception to handle the new exception separately (or rethrow it) so it isn't logged as an error in normal shutdown paths.
🤖 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/program/SolidityNode.java`:
- Around line 193-198: The sleep(long) method in class SolidityNode currently
catches a broad Exception and swallows interrupts; modify sleep(long) to catch
InterruptedException separately, call Thread.currentThread().interrupt() to
preserve the thread's interrupt status and then return or rethrow as
appropriate, while keeping the existing catch-all for other Exceptions to handle
non-interrupt errors; update references inside SolidityNode.sleep(long) so
interrupted shutdown via executor interruption is honored.
- Around line 88-89: The code in SolidityNode is submitting runnables via
ExecutorService.submit(...) which can swallow exceptions; replace those calls
with ExecutorServiceManager.submit(...) so submitted tasks get wrapped/handled
by the central manager (use the same task/lambda you currently pass to
ExecutorService.submit and call ExecutorServiceManager.submit(...) instead),
ensuring you update any imports and remove direct ExecutorService.submit(...)
usages in the SolidityNode class so uncaught throwables are propagated/handled
by the manager.
- Around line 173-174: Replace the generic RuntimeException used to signal a
normal/graceful shutdown in SolidityNode with a dedicated exception type (e.g.,
GracefulShutdownException or use InterruptedException) so shutdown control flow
is explicit; update the throw sites in SolidityNode (the lines currently
throwing RuntimeException around the graceful stop) to throw that specific
exception and adjust any surrounding catch blocks that currently catch Exception
to handle the new exception separately (or rethrow it) so it isn't logged as an
error in normal shutdown paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e13b5f38-4b4d-444e-8a22-7691889ec815
📒 Files selected for processing (2)
framework/src/main/java/org/tron/core/net/TronNetDelegate.javaframework/src/main/java/org/tron/program/SolidityNode.java
There was a problem hiding this comment.
♻️ Duplicate comments (2)
framework/src/main/java/org/tron/program/SolidityNode.java (2)
88-89:⚠️ Potential issue | 🟠 MajorUse
ExecutorServiceManager.submit(...)to avoid silent worker death.At Line 88 and Line 89, raw
ExecutorService.submit(...)can hide fatal task failures inside the returnedFuture, which risks silent sync stoppage. Please route submissions through the project wrapper here as well.Proposed fix
- getBlockExecutor.submit(this::getBlock); - processBlockExecutor.submit(this::processSolidityBlock); + ExecutorServiceManager.submit(getBlockExecutor, this::getBlock); + ExecutorServiceManager.submit(processBlockExecutor, this::processSolidityBlock);#!/bin/bash # Verify this class uses raw submit and compare with project convention. rg -n -C2 'getBlockExecutor\.submit|processBlockExecutor\.submit' framework/src/main/java/org/tron/program/SolidityNode.java rg -n -C1 'ExecutorServiceManager\.submit\(' --type=java🤖 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 88 - 89, The code submits tasks with raw ExecutorService.submit using getBlockExecutor.submit(this::getBlock) and processBlockExecutor.submit(this::processSolidityBlock); replace these with the project wrapper by routing submissions through ExecutorServiceManager.submit(...) so task failures aren't swallowed. Locate the calls to getBlockExecutor and processBlockExecutor in SolidityNode and change them to use ExecutorServiceManager.submit(getBlockExecutor, this::getBlock) and ExecutorServiceManager.submit(processBlockExecutor, this::processSolidityBlock) (or the wrapper's equivalent overload) so the manager can handle exceptions and logging.
196-201:⚠️ Potential issue | 🟠 MajorPreserve interrupt status in
sleep()for clean shutdown behavior.At Line 199, catching generic
ExceptiondropsInterruptedExceptionstate; this can slow or distort shutdown flow for worker threads.Proposed fix
public void sleep(long time) { try { Thread.sleep(time); - } catch (Exception e1) { - logger.error(e1.getMessage()); + } catch (InterruptedException e1) { + Thread.currentThread().interrupt(); + logger.info("Sleep interrupted."); + } catch (Exception e1) { + logger.error(e1.getMessage(), e1); } }#!/bin/bash # Verify interrupt swallowing in sleep() and its call sites. rg -n -C3 'void sleep\(long time\)|Thread\.sleep|catch \(Exception e1\)' framework/src/main/java/org/tron/program/SolidityNode.java rg -n -C2 'sleep\(exceptionSleepTime\)|sleep\(BLOCK_PRODUCED_INTERVAL\)' framework/src/main/java/org/tron/program/SolidityNode.java🤖 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 196 - 201, The sleep(long time) method currently catches Exception and swallows InterruptedException; change the catch to specifically handle InterruptedException by restoring the thread's interrupt status (Thread.currentThread().interrupt()) and logging appropriately, and handle other unexpected Throwables separately if needed; locate the sleep(long time) method (uses Thread.sleep(time) and logger.error) and replace the generic catch with a catch (InterruptedException ie) { Thread.currentThread().interrupt(); logger.error("Interrupted while sleeping", ie); } plus an optional catch (Exception e) { logger.error("Unexpected error in sleep", e); } to preserve shutdown behavior.
🧹 Nitpick comments (1)
framework/src/main/java/org/tron/program/SolidityNode.java (1)
83-97: Consider makingrun()idempotent.If
run()is called more than once, it can enqueue duplicate long-lived workers on the same executors. A simple started-flag guard would make lifecycle safer.Possible refactor
+ private final java.util.concurrent.atomic.AtomicBoolean started = new java.util.concurrent.atomic.AtomicBoolean(false); + public void run() { + if (!started.compareAndSet(false, true)) { + logger.warn("SolidityNode.run() called more than once; ignoring."); + return; + } try { databaseGrpcClient = new DatabaseGrpcClient(CommonParameter.getInstance().getTrustNodeAddr()); remoteBlockNum.set(getLastSolidityBlockNum()); @@ } catch (Exception e) { + started.set(false); logger.error("Failed to start solid node, address: {}.", CommonParameter.getInstance().getTrustNodeAddr()); throw new TronError(e, TronError.ErrCode.SOLID_NODE_INIT); } }🤖 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 83 - 97, run() can be invoked multiple times and will submit duplicate long-lived tasks to getBlockExecutor and processBlockExecutor; make it idempotent by adding a private AtomicBoolean (e.g., started) checked at the top of run() and set atomically (compareAndSet(false, true)) so the body only executes once, and return immediately if already started; ensure the existing initialization of databaseGrpcClient, remoteBlockNum.set(...), and the two submit(...) calls (getBlockExecutor.submit(this::getBlock) and processBlockExecutor.submit(this::processSolidityBlock)) are guarded by this flag so duplicate workers are not enqueued.
🤖 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/program/SolidityNode.java`:
- Around line 88-89: The code submits tasks with raw ExecutorService.submit
using getBlockExecutor.submit(this::getBlock) and
processBlockExecutor.submit(this::processSolidityBlock); replace these with the
project wrapper by routing submissions through
ExecutorServiceManager.submit(...) so task failures aren't swallowed. Locate the
calls to getBlockExecutor and processBlockExecutor in SolidityNode and change
them to use ExecutorServiceManager.submit(getBlockExecutor, this::getBlock) and
ExecutorServiceManager.submit(processBlockExecutor, this::processSolidityBlock)
(or the wrapper's equivalent overload) so the manager can handle exceptions and
logging.
- Around line 196-201: The sleep(long time) method currently catches Exception
and swallows InterruptedException; change the catch to specifically handle
InterruptedException by restoring the thread's interrupt status
(Thread.currentThread().interrupt()) and logging appropriately, and handle other
unexpected Throwables separately if needed; locate the sleep(long time) method
(uses Thread.sleep(time) and logger.error) and replace the generic catch with a
catch (InterruptedException ie) { Thread.currentThread().interrupt();
logger.error("Interrupted while sleeping", ie); } plus an optional catch
(Exception e) { logger.error("Unexpected error in sleep", e); } to preserve
shutdown behavior.
---
Nitpick comments:
In `@framework/src/main/java/org/tron/program/SolidityNode.java`:
- Around line 83-97: run() can be invoked multiple times and will submit
duplicate long-lived tasks to getBlockExecutor and processBlockExecutor; make it
idempotent by adding a private AtomicBoolean (e.g., started) checked at the top
of run() and set atomically (compareAndSet(false, true)) so the body only
executes once, and return immediately if already started; ensure the existing
initialization of databaseGrpcClient, remoteBlockNum.set(...), and the two
submit(...) calls (getBlockExecutor.submit(this::getBlock) and
processBlockExecutor.submit(this::processSolidityBlock)) are guarded by this
flag so duplicate workers are not enqueued.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c6ec84a-8de4-477a-b0ad-d73d007b57a2
📒 Files selected for processing (2)
framework/src/main/java/org/tron/program/SolidityNode.javaframework/src/test/java/org/tron/program/SolidityNodeTest.java
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
…shutdown - Convert SolidityNode from static utility to @conditional @component so it only registers when --solidity is passed - Implement ApplicationListener<ContextClosedEvent> to set flag=false before any @PreDestroy method fires, stopping worker threads promptly - Add pushVerifiedBlock() to TronNetDelegate to route solidity sync blocks through the conditional-shutdown check (<=, not ==, because a single batch write can advance the DB header past the target number) - Guard saveLatestSolidifiedBlockNum in loopProcessBlock: skip the write when hitDown is true, preventing a phantom block-num from persisting when the block was never actually pushed to BlockStore - Add unit tests covering lifecycle hooks, all retry paths, interrupt handling, and the hitDown save-guard; SolidityNode line coverage 28% -> 81% Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
317787106
force-pushed
the
feature/solidity_shutdown
branch
from
3c5c2d2 to
7de0f2a
Compare
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.ApplicationListener<ContextClosedEvent>: setsflag = falseat the very start of context close, before any@PreDestroymethods run on other beans, preventing block pushes into a partially-shut-down system.@PostConstruct init(): initialises namedExecutorServicethread pools viaExecutorServiceManager. gRPC client andremoteBlockNumare initialised inrun()to keep init lightweight.@PreDestroy shutdown(): setsflag = false(safety net), shuts down both executors viaExecutorServiceManager.shutdownAndAwaitTermination, and closes the gRPC client cleanly.processSolidityBlock()usesblockQueue.poll(timeout)instead oftake(), and handlesInterruptedExceptionexplicitly so thread interruption during executor shutdown is clean.getBlockByNum(): loop changed towhile (flag); throwsRuntimeException("SolidityNode is closing.")when flag is false — avoidsNullPointerExceptionfromblockQueue.put(null).getLastSolidityBlockNum(): loop changed towhile (flag); returns0when flag is false — lets the caller'swhile (flag)loop exit quietly without a misleading error log.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.processBlockuses<=instead of==becausepushBlockmay commit multiple blocks in a single batch write, causing the DB header number to jump past the target block number and never equal it exactly.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.Tests
testSolidityGrpcCall— verifies gRPC connectivity through the runningRpcApiService.testSolidityNodeHttpApiService— verifies HTTP service start/stop idempotency.testExecutorsInitializedOnStartup—@PostConstructcreates both executors beforerun()is called.testOnApplicationEventSetsFlagFalse—ContextClosedEventsetsflag = falsebefore any@PreDestroyruns.testGetBlockByNumThrowsWhenClosed—RuntimeException(notnull) is thrown whenflag = false.testGetLastSolidityBlockNumReturnsZeroWhenClosed— returns0whenflag = falsefor a quiet loop exit.testSolidityConditionMatchesWhenSolidityFlagSet—SolidityCondition.matches()returnstruewith--solidity.