fix: charge the node for ingest-decidable atomic-batch inner due-diligence failures - #26804
Conversation
…er due-diligence failures (hiero-ledger#26615) Signed-off-by: Ruslan Velkov <ruslan.velkov@limechain.tech>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #26804 +/- ##
============================================
- Coverage 70.49% 70.48% -0.02%
- Complexity 11670 11671 +1
============================================
Files 2583 2583
Lines 108300 108310 +10
Branches 12109 12111 +2
============================================
- Hits 76345 76340 -5
- Misses 27977 27990 +13
- Partials 3978 3980 +2
... and 19 files with indirect coverage changes 🚀 New features to boost your workflow:
|
✅ All tests passed ✅🏷️ Commit: 16bacc3 Learn more about TestLens at testlens.app. |
| private static final Set<ResponseCodeEnum> STATE_DEPENDENT_DUE_DILIGENCE_CODES = | ||
| EnumSet.of(PAYER_ACCOUNT_NOT_FOUND, PAYER_ACCOUNT_DELETED, INSUFFICIENT_PAYER_BALANCE); |
There was a problem hiding this comment.
I don’t think we should skip charging the node for these exceptions. We charge the node for the same due-diligence failures in regular transactions, and batch transactions should behave consistently. Otherwise, a node could submit a transaction with an invalid payer or a payer with zero balance, and neither the node nor the payer would be charged.
There was a problem hiding this comment.
These are state-dependent, i.e. the Node couldn't foresee at ingest - the account ID might have been deleted after ingest, but before handle, by some other txn; or the account ID doesn't exist (ingest only checks about the valid shape, e.g. positive numbers for shard, realm, etc., existence is checked at handle, but a deleted account might have been garbage-collected - so the account ID passes ingest but is not found at handle); or the payer has insufficient balance - this could definitely happen because the same payer might have been charged multiple times by previous txns from the same batch, previous txns from a different batch, or previous top-level txns - at ingest we don't simulate how the same payer is going to be affected after multiple txns (and we can't because there might be smart contract payments involved), we just perform the check: for each individual txn in the batch see whether the payer can pay for that particular txn.
There was another case: txn expired for scheduled txns but we've got a time buffer at ingest of 10s by default, i.e. at ingest txns that are about to expire within the next 10s should not be passed to consensus (and 10s is a lot of time), so an expired txn at handle (in a batch or top-level) should be treated as a Node due diligence error. But for insufficient balance in a batch - not (this cannot be determined with 100% confidence at ingest, so it's not the Node's fault if such an error propagates at handle).
| dispatch.feeChargingOrElse(appFeeCharging) | ||
| .customized(dispatch) | ||
| .charge( | ||
| dispatch.creatorInfo().accountId(), | ||
| new Fees(0, dispatch.fees().networkFee(), 0), | ||
| null); |
There was a problem hiding this comment.
This only charges the node for the failing inner transaction. Fees from earlier inner transactions are still replayed and charged to their payers. If any inner has a due-diligence failure, the node should have rejected the entire batch at ingest. I think the whole batch should be treated as a node due-diligence failure: do not charge the inner payers, and charge the submitting node the applicable network fees.
There was a problem hiding this comment.
Treating the whole batch as a Node Due Diligence changes the ticket (Acceptance criteria 3: Batch resolution (INNER_TRANSACTION_FAILED) and inner record statuses remain externally consistent). Also, not charging the earlier inner payers would run against the HIP-551 invariant that already-processed inners pay their fees even when the batch rolls back — they did valid work; only the failing inner is the node's fault. And per my previous reply, there are situations in which it wasn't even the node's fault.
So changing this behavior will require further discussion.
3 participants
What
Inner transactions of an atomic batch bypassed node due-diligence charging. When an inner failed a node due-diligence check at pre-handle,
DispatchProcessor#chargeCreatorset the inner's status but returned without charging forBATCH_INNERdispatches — so the cost of letting the batch reach consensus fell on the batch payer instead of the submitting node. The same operation submitted top-level charges the node; wrapping it in a batch inverted who pays.Fixes #26615.
How
chargeCreatornow charges the submitting node for aBATCH_INNERdue-diligence failure when the failure is ingest-decidable — determinable from the payer and transaction body alone (structural/pure-check failures, oversize/parse, invalid node account, invalid payer signature). The charge is routed through the batch's recorded fee-charging context rather than the fee accumulator directly, so it is captured by the batch's rollback-and-replay and survives the batch failing — exactly like the batch payer's fee. A direct fee-accumulator charge would be discarded with the inner's savepoint.State-dependent failures the node could not have foreseen at ingest are left uncharged. These depend on mutable ledger state that can change after submission by any means (a separate transaction, another batch, or an earlier inner in the same batch):
The batch's external resolution (
INNER_TRANSACTION_FAILED) and the inner record statuses are unchanged — only who is charged shifts. The rationale for this code set is recorded in a comment on #26615.Acceptance criteria
INNER_TRANSACTION_FAILED) and inner record statuses remain externally consistent.Testing
DispatchProcessorTest(unit): ingest-decidable inner → node charged via the recorded context; each state-dependent code (PAYER_ACCOUNT_DELETED,INSUFFICIENT_PAYER_BALANCE) → node not charged.GovernanceTransactionsPostIngestTests(embedded): (a) an unbalanced inner → node charged, batchINNER_TRANSACTION_FAILED, inner recordsINVALID_ACCOUNT_AMOUNTS; (b) an inner drained below its network fee by an earlier inner → node not charged.:appunit + embedded batch suites green locally; full subprocess batch suites left to CI.