[Feature] Align timeout and task-cancellation handling across parallel verification precompiles

[yanghang8612]

Summary

The two parallel-verification precompiles BatchValidateSign and VerifyTransferProof in PrecompiledContracts.java follow the same "submit tasks → CountDownLatch.await(cpuBudget) → collect results" pattern but complete the timeout branch differently. This issue proposes aligning the two call-sites so the CPU-budget contract is expressed consistently and in-flight worker tasks are released symmetrically. The change is semantically equivalent to current behaviour (see # Impact below) and does not require a fork gate.

Problem

Motivation

Shielded transfer verification and batch signature verification are both CPU-intensive precompile paths and both track the remaining CPU budget on a CountDownLatch. For predictability and easier reasoning about the CPU-budget contract, the two sites should respond to an exhausted budget in the same way.

Current State

• BatchValidateSign.doExecute (actuator/src/main/java/org/tron/core/vm/PrecompiledContracts.java, L1083–L1089) checks the return value of countDownLatch.await(...) and raises Program.Exception.notEnoughTime("call BatchValidateSign precompile method") before touching any Future.
• VerifyTransferProof.execute (same file, L1471–L1477) computes the same withNoTimeout local but does not read it; control falls through to future.get(), which is invoked without a timeout argument and without cancelling the outstanding tasks.

As a result, once the CountDownLatch budget is exhausted, the two code paths behave differently at the call-site: one surfaces an explicit notEnoughTime signal; the other blocks the block-processing thread on future.get() until the worker returns on its own, before the VM main loop's next-opcode CPU check fires.

Limitations or Risks

The two precompiles are conceptually identical in structure, so the divergence is surprising for readers. On the VerifyTransferProof path, the block-processing thread is held on future.get() until the slowest worker completes even though the CPU budget is already exhausted, and worker threads continue running proof verifications whose results will be discarded by the imminent OutOfTimeException.

Proposed Solution

Proposed Design

Adopt the BatchValidateSign pattern in VerifyTransferProof:

1. Check the return value of countDownLatch.await(...) and raise notEnoughTime when it is false, before the result-collection loop.
2. Before raising, cancel outstanding tasks via future.cancel(true) so worker threads are reclaimed promptly and no longer compute results that will be discarded.

Optionally, factor the "submit → await CPU budget → collect" sequence into a small shared helper so that any future parallel-verification precompile inherits the same semantics by default.

Key Changes

• Module: org.tron.core.vm.PrecompiledContracts (VerifyTransferProof.execute, and optionally BatchValidateSign.doExecute to share the helper).
• API: none; only internal control flow.
• Test: add coverage for the timeout path (latch exhausted → notEnoughTime, cancelled futures reported as cancelled).

Impact

This change is consensus-neutral. Every observable field of the transaction receipt is identical between the two paths, because when the CountDownLatch returns false the CPU budget is by construction already exhausted:

• The timeout argument is getCPUTimeLeftInNanoSecond() = vmShouldEndInUs * 1000 - System.nanoTime() (see PrecompiledContracts.java L458–L462), so await(...) == false is equivalent to System.nanoTime() ≥ vmShouldEndInUs.
• Under the current code, control returns to the VM main loop and the very next iteration calls Program.checkCPUTimeLimit(opName) (see VM.java L85, Program.java L1241–L1260), which throws notEnoughTime for the same reason.
• Both OutOfTimeException call-sites funnel into the same handler in VMActuator.execute (L272–L278), which calls program.spendAllEnergy() and sets contractResult.OUT_OF_TIME on the receipt; state changes are reverted in both cases (insertLeaves writes on the current path are discarded by the revert).

So the proposed change only relocates where the identical OutOfTimeException is thrown — from the VM's post-precompile check to inside the precompile itself. It does not introduce any new class of transaction outcome and does not interact with TransactionTrace.checkNeedRetry. The --debug and solidity-node paths short-circuit checkCPUTimeLimit and are not on the consensus path.

Concrete benefits:

• Reliability: the block-processing thread is no longer held on future.get() waiting for workers whose results are already destined to be discarded.
• Worker utilization: cancel(true) releases pool threads promptly instead of letting them run orphaned native verification calls.
• Readability: the two precompiles read the same way, and withNoTimeout stops being an unused local.

Compatibility

• Breaking Change: No.
• Default Behavior Change: No. Every consensus-visible field (contractResult, energyUsed, state, logs) is identical to the pre-change path (see # Impact).
• Migration Required: No.

References (Optional)

• actuator/src/main/java/org/tron/core/vm/PrecompiledContracts.java
  • BatchValidateSign.doExecute — timeout check and notEnoughTime throw (L1083–L1089).
  • VerifyTransferProof.execute — await return value currently unused; future.get() without timeout / cancel (L1471–L1477).
  • getCPUTimeLeftInNanoSecond helper shared by both call-sites (L458–L462).
• actuator/src/main/java/org/tron/core/vm/VM.java — per-opcode program.checkCPUTimeLimit(opName) call (L85).
• actuator/src/main/java/org/tron/core/vm/program/Program.java — checkCPUTimeLimit (L1241–L1260).
• actuator/src/main/java/org/tron/core/actuator/VMActuator.java — OutOfTimeException handling (L272–L278).

Additional Notes

• Do you have ideas regarding implementation? Yes.
• Are you willing to implement this feature? Yes.

[freddie-lele]

@yanghang8612 Thanks for the detailed analysis. Aligning the timeout handling across parallel precompiles makes the codebase much more predictable for anyone building on or auditing the core logic. I especially like the idea of a 'shared helper' to standardize the execution pattern for future parallel precompile tasks. This kind of symmetry is key to a robust VM implementation.

[alan-eth]

Really appreciate that the test plan already covers both sides of the timeout path — "latch exhausted → notEnoughTime" and "cancelled futures reported as cancelled". That second one is a nice touch in particular, since it pins down the actual cancel(true) behavior rather than just the exception being thrown. If useful, two low-priority scenarios that might complement those nicely:

1. Queued tasks after cancel. Since workersInNonConstantCall is a fixed 5-thread pool and shielded transactions can easily submit more tasks than that, some do end up queued in practice. Wondering if it'd be worth a quick probe (e.g., an AtomicInteger incremented inside a test-only task subclass's call()) to confirm queued-but-not-yet-started tasks really don't run once the timeout fires. Would turn the "releases worker slots" benefit from a structural claim into a verifiable fact. Totally optional — feel free to skip if it feels like overkill for this change.

2. Mixed completion states at timeout. Could be nice to have a scenario or two where await(...) == false happens while some futures have already returned — one with all-true partials, one with mixed true/false. Mostly a safety net: guards against a future refactor accidentally letting control slip to the checkResult fold or insertLeaves when the latch already timed out. Not strictly needed to validate this change, more about locking in the eager-throw contract for later.

Either way, really looking forward to seeing this one land. 🚀

[yanghang8612]

@freddie-lele Thanks! Glad the symmetry framing lands. Given the point about future parallel-verification precompiles inheriting the same semantics, I'll fold both call-sites onto the shared helper in the same PR rather than leaving it as an optional follow-up.

[yanghang8612]

@alan-eth Thanks — both are worth having and I'll fold them into the test plan.

For (1), I'll submit more subtasks than workersInNonConstantCall has threads and use a test-only subclass with an AtomicInteger.incrementAndGet() at the top of call(), then assert the counter stays ≤ pool size after the latch timeout fires. FutureTask.cancel(true) flips queued tasks to CANCELLED before a worker dequeues them, so call() shouldn't be entered for anything still queued — the counter makes that observable.

For (2), I'll add both the all-true and mixed true/false partial cases with await(...) == false, asserting Program.Exception.notEnoughTime surfaces and neither the checkResult fold nor insertLeaves runs.

Really appreciate the careful read.

[waynercheung]

@yanghang8612
One implementation detail looks worth pinning down before the helper lands.

Today the observable difference between these two precompiles is not quite "BatchValidateSign already surfaces OOT while VerifyTransferProof does not". BatchValidateSign.doExecute() does throw on latch timeout, but BatchValidateSign.execute() currently catches Throwable and returns zero bytes, so the OutOfTimeException does not escape the public precompile boundary either. VerifyTransferProof.execute() also swallows Throwable. From the VM call-site, the real divergence today is that VerifyTransferProof can still block the block-processing thread on future.get() after the CPU budget is already exhausted, while BatchValidateSign avoids that extra wait.

That suggests a couple of things for the shared helper / PR shape:

1. If the intended end-state is "the precompile itself raises OOT", both execute() wrappers need to passthrough OutOfTimeException explicitly (similar to ValidateMultiSign). Otherwise the helper can throw internally and still get swallowed at the outer boundary.
2. The cancel path should also cover getCPUTimeLeftInNanoSecond() itself throwing before await() begins. In that case the futures have already been submitted, so cancelling only on await(...) == false still leaves submitted work running.
3. I would also cancel outstanding futures on InterruptedException for the same resource-release reason.

One nuance on the proposed queued-task test: with the current input constraints, a single VerifyTransferProof call can submit at most 5 tasks (2 spend + 2 output + 1 binding), which exactly matches the fixed pool size of 5. So deterministic per-call queueing is not observable through the existing end-to-end input shape. If we want to verify queued futures becoming CANCELLED, a small package-private helper test with a dedicated tiny executor would be much more deterministic than an end-to-end proof test, and it avoids flakiness from the shared static pools.

Also, for the PR description I’d phrase cancel(true) as best-effort reclamation for running native verification calls, while treating “queued tasks do not start” as the stronger guarantee.

[Federico2014]

Additional concern: solidity-node vs full-node state divergence from the precompile-internal timeout

Before landing the proposed change, I'd like to flag a pre-existing asymmetry that this PR would unintentionally extend to VerifyTransferProof.

TVM has two independent wall-clock timeout checks:

1. Opcode-level — Program.checkCPUTimeLimit (Program.java:1241-1260) short-circuits on solidity nodes and debug mode:

   if (CommonParameter.getInstance().isDebug())        { return; }
   if (CommonParameter.getInstance().isSolidityNode()) { return; }

   This is intentional: solidity nodes aren't on the consensus path and must reproduce mainnet state regardless of local replay latency.

2. Precompile-internal — PrecompiledContracts.getCPUTimeLeftInNanoSecond (PrecompiledContracts.java:458-465) has no such short-circuit. It is used by BatchValidateSign at L1091 and by VerifyTransferProof at L1478 to bound a CountDownLatch.await(...).

Because vmShouldEndInUs is still computed normally on solidity nodes (VMActuator.java:401-405), the await can genuinely time out on a solidity node under GC / disk / scheduling pressure even when the same transaction succeeded on the producing SR.

For BatchValidateSign, the timeout path is:

• L1091 await returns withNoTimeout=false
• L1095 throws notEnoughTime
• L1041-1046 outer catch (Throwable) swallows it and returns Pair.of(true, new byte[32]) (all-zero result)
• Control returns to the VM loop; the next opcode's checkCPUTimeLimit short-circuits on a solidity node, so execution continues with the zero-byte result
• The contract branches on a value that differs from mainnet → divergent SSTORE / LOG / REVERT → persisted state diverges from the SR's block

On a full node the same timeout would also land in the outer catch, but the very next checkCPUTimeLimit throws OOT and the transaction reverts cleanly — so only the solidity node silently forks.

VerifyTransferProof is currently not affected: it ignores withNoTimeout and falls through to future.get(), so workers always finish and the real result is returned (solidity node is just slower). This PR's proposal to have it honor withNoTimeout and short-circuit on timeout would remove that safety property and introduce the same divergence surface that BatchValidateSign already has.

Suggested fix

Gate the precompile-internal timeout on the same condition as checkCPUTimeLimit, so the two checks stay in sync:

protected long getCPUTimeLeftInNanoSecond() {
  CommonParameter p = CommonParameter.getInstance();
  if (p.isDebug() || p.isSolidityNode()) {
    return Long.MAX_VALUE;
  }
  long left = getVmShouldEndInUs() * VMConstant.ONE_THOUSAND - System.nanoTime();
  if (left <= 0) {
    throw Program.Exception.notEnoughTime("call");
  }
  return left;
}

Effect:

• Full nodes: behavior unchanged — both precompiles still time out as today / as proposed.
• Solidity / debug nodes: precompile workers are awaited to completion, matching the existing checkCPUTimeLimit short-circuit semantics. The cost is that a heavy replay transaction can take longer on a solidity node, but that is already the accepted trade-off for every other opcode on the replay path.

This one change covers both precompiles and closes the divergence for BatchValidateSign at the same time as making the VerifyTransferProof refactor safe to land.

Happy to open a separate issue for the BatchValidateSign part if the maintainers prefer to keep this PR focused.

[alan-eth]

@yanghang8612 Perfect — thanks for folding both in, and the FutureTask.cancel(true) framing for queued tasks is exactly right. One small heads-up though: @waynercheung's point below is a useful guardrail on the pool-saturation scenario — a single VerifyTransferProof call caps at 5 submitted tasks (2 spend + 2 output + 1 binding), which exactly equals workersInNonConstantCall's pool size, so the queued-vs-dequeued distinction isn't really observable through an end-to-end proof-shaped input.