-
Notifications
You must be signed in to change notification settings - Fork 221
fix: charge the node for ingest-decidable atomic-batch inner due-diligence failures #26804
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,8 +8,11 @@ | |
| import static com.hedera.hapi.node.base.ResponseCodeEnum.AUTHORIZATION_FAILED; | ||
| import static com.hedera.hapi.node.base.ResponseCodeEnum.ENTITY_NOT_ALLOWED_TO_DELETE; | ||
| import static com.hedera.hapi.node.base.ResponseCodeEnum.FAIL_INVALID; | ||
| import static com.hedera.hapi.node.base.ResponseCodeEnum.INSUFFICIENT_PAYER_BALANCE; | ||
| import static com.hedera.hapi.node.base.ResponseCodeEnum.INVALID_SIGNATURE; | ||
| import static com.hedera.hapi.node.base.ResponseCodeEnum.NOT_SUPPORTED; | ||
| import static com.hedera.hapi.node.base.ResponseCodeEnum.PAYER_ACCOUNT_DELETED; | ||
| import static com.hedera.hapi.node.base.ResponseCodeEnum.PAYER_ACCOUNT_NOT_FOUND; | ||
| import static com.hedera.hapi.node.base.ResponseCodeEnum.SUCCESS; | ||
| import static com.hedera.hapi.node.base.ResponseCodeEnum.UNAUTHORIZED; | ||
| import static com.hedera.node.app.spi.workflows.HandleContext.TransactionCategory.BATCH_INNER; | ||
|
|
@@ -239,16 +242,40 @@ private void rollbackAndRechargeFee( | |
| dispatchUsageManager.trackFeePayments(dispatch); | ||
| } | ||
|
|
||
| /** | ||
| * The inner due-diligence response codes that depend on mutable ledger state — the payer's existence or its | ||
| * balance — which can change between a batch's submission and consensus by any means (an earlier inner, a | ||
| * separate transaction, and so on). The submitting node's only checkpoint for these is ingest; a failure that | ||
| * surfaces only at consensus is not attributable to the node, so it must not be charged. See #26615. | ||
| */ | ||
| private static final Set<ResponseCodeEnum> STATE_DEPENDENT_DUE_DILIGENCE_CODES = | ||
| EnumSet.of(PAYER_ACCOUNT_NOT_FOUND, PAYER_ACCOUNT_DELETED, INSUFFICIENT_PAYER_BALANCE); | ||
|
|
||
| /** | ||
| * Charges the creator for the network fee. This will be called when there is a due diligence failure. | ||
| * | ||
| * @param dispatch the dispatch to be processed | ||
| * @param validation the validation of the charging scenario | ||
| */ | ||
| private void chargeCreator(@NonNull final Dispatch dispatch, @NonNull final FeeCharging.Validation validation) { | ||
| dispatch.streamBuilder().status(validation.errorStatusOrThrow()); | ||
| // If the transaction is a batch inner transaction, we don't charge the creator | ||
| final var errorStatus = validation.errorStatusOrThrow(); | ||
| dispatch.streamBuilder().status(errorStatus); | ||
| if (dispatch.category() == BATCH_INNER) { | ||
| // State-dependent failures the node could not have foreseen at ingest (e.g. an inner payer removed, | ||
| // or drained below the network fee, after submission) leave the node uncharged. | ||
| if (STATE_DEPENDENT_DUE_DILIGENCE_CODES.contains(errorStatus)) { | ||
| return; | ||
| } | ||
| // The node should have rejected the batch at ingest, so charge it the inner's network fee, as a top-level | ||
| // due-diligence failure would. Route the charge through the (recorded) fee-charging context rather than | ||
| // the fee accumulator so it is captured by the batch's rollback-and-replay and survives the batch failing; | ||
| // a direct fee-accumulator charge would be discarded with the inner's savepoint. See #26615. | ||
| dispatch.feeChargingOrElse(appFeeCharging) | ||
| .customized(dispatch) | ||
| .charge( | ||
| dispatch.creatorInfo().accountId(), | ||
| new Fees(0, dispatch.fees().networkFee(), 0), | ||
| null); | ||
|
Comment on lines
+273
to
+278
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| return; | ||
| } | ||
| dispatch.feeAccumulator() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).