Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Comment on lines +251 to +252

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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).


/**
* 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.
So changing this behavior will require further discussion.

return;
}
dispatch.feeAccumulator()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,17 @@
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_ACCOUNT_BALANCE;
import static com.hedera.hapi.node.base.ResponseCodeEnum.INSUFFICIENT_PAYER_BALANCE;
import static com.hedera.hapi.node.base.ResponseCodeEnum.INVALID_ACCOUNT_AMOUNTS;
import static com.hedera.hapi.node.base.ResponseCodeEnum.INVALID_PAYER_SIGNATURE;
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.SUCCESS;
import static com.hedera.hapi.node.base.ResponseCodeEnum.TOKEN_NOT_ASSOCIATED_TO_ACCOUNT;
import static com.hedera.hapi.node.base.ResponseCodeEnum.UNAUTHORIZED;
import static com.hedera.node.app.spi.authorization.SystemPrivilege.UNNECESSARY;
import static com.hedera.node.app.spi.workflows.HandleContext.TransactionCategory.BATCH_INNER;
import static com.hedera.node.app.spi.workflows.HandleContext.TransactionCategory.USER;
import static com.hedera.node.app.workflows.handle.dispatch.DispatchValidator.DuplicateStatus.NO_DUPLICATE;
import static com.hedera.node.app.workflows.handle.dispatch.DispatchValidator.ServiceFeeStatus.UNABLE_TO_PAY_SERVICE_FEE;
Expand All @@ -29,6 +33,7 @@
import static com.hedera.node.app.workflows.handle.dispatch.ValidationResult.newSuccess;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doCallRealMethod;
Expand Down Expand Up @@ -56,6 +61,7 @@
import com.hedera.node.app.signature.impl.SignatureVerificationImpl;
import com.hedera.node.app.spi.authorization.Authorizer;
import com.hedera.node.app.spi.authorization.SystemPrivilege;
import com.hedera.node.app.spi.fees.FeeCharging;
import com.hedera.node.app.spi.fees.Fees;
import com.hedera.node.app.spi.info.NetworkInfo;
import com.hedera.node.app.spi.info.NodeInfo;
Expand Down Expand Up @@ -216,6 +222,58 @@ void creatorErrorAsExpected() {
verify(opWorkflowMetrics, never()).incrementThrottled(any());
}

@Test
void batchInnerIngestDecidableDueDiligenceChargesCreator() {
final var feeCharging = mock(FeeCharging.class);
final var chargeContext = mock(FeeCharging.Context.class);
given(dispatch.fees()).willReturn(FEES);
given(dispatch.feeChargingOrElse(any())).willReturn(feeCharging);
given(feeCharging.customized(dispatch)).willReturn(chargeContext);
given(dispatchValidator.validateFeeChargingScenario(dispatch))
.willReturn(newCreatorError(CREATOR_ACCOUNT_ID, INVALID_ACCOUNT_AMOUNTS));
final var creatorInfo = mock(NodeInfo.class);
given(dispatch.creatorInfo()).willReturn(creatorInfo);
given(creatorInfo.accountId()).willReturn(CREATOR_ACCOUNT_ID);
given(dispatch.category()).willReturn(BATCH_INNER);

subject.processDispatch(dispatch);

// Ingest-decidable inner due-diligence failure -> the node is charged its network fee, routed through the
// recorded fee-charging context so the charge survives the batch's rollback-and-replay (#26615).
verify(chargeContext).charge(CREATOR_ACCOUNT_ID, new Fees(0, FEES.networkFee(), 0), null);
verify(recordBuilder).status(INVALID_ACCOUNT_AMOUNTS);
assertFinished(IsRootStack.NO);
}

@Test
void batchInnerStateDependentDueDiligenceDoesNotChargeCreator() {
given(dispatchValidator.validateFeeChargingScenario(dispatch))
.willReturn(newCreatorError(CREATOR_ACCOUNT_ID, PAYER_ACCOUNT_DELETED));
given(dispatch.category()).willReturn(BATCH_INNER);

subject.processDispatch(dispatch);

// State-dependent inner failure the node could not foresee -> the node is NOT charged (#26615).
verify(feeAccumulator, never()).chargeFee(any(), anyLong(), any());
verify(recordBuilder).status(PAYER_ACCOUNT_DELETED);
assertFinished(IsRootStack.NO);
}

@Test
void batchInnerInsufficientPayerBalanceDoesNotChargeCreator() {
given(dispatchValidator.validateFeeChargingScenario(dispatch))
.willReturn(newCreatorError(CREATOR_ACCOUNT_ID, INSUFFICIENT_PAYER_BALANCE));
given(dispatch.category()).willReturn(BATCH_INNER);

subject.processDispatch(dispatch);

// A balance shortfall is state-dependent (the payer can be drained after ingest, even by an earlier inner
// in the same batch), so the node is NOT charged. See #26615.
verify(feeAccumulator, never()).chargeFee(any(), anyLong(), any());
verify(recordBuilder).status(INSUFFICIENT_PAYER_BALANCE);
assertFinished(IsRootStack.NO);
}

@Test
void waivedFeesDoesNotCharge() {
given(dispatchValidator.validateFeeChargingScenario(dispatch))
Expand Down
Original file line number Diff line number Diff line change
@@ -1,23 +1,32 @@
// SPDX-License-Identifier: Apache-2.0
package com.hedera.services.bdd.suites.hip1300;

import static com.hedera.services.bdd.junit.ContextRequirement.SYSTEM_ACCOUNT_BALANCES;
import static com.hedera.services.bdd.junit.EmbeddedReason.MUST_SKIP_INGEST;
import static com.hedera.services.bdd.junit.hedera.embedded.EmbeddedMode.CONCURRENT;
import static com.hedera.services.bdd.spec.HapiSpec.hapiTest;
import static com.hedera.services.bdd.spec.assertions.AccountInfoAsserts.reducedFromSnapshot;
import static com.hedera.services.bdd.spec.assertions.AccountInfoAsserts.unchangedFromSnapshot;
import static com.hedera.services.bdd.spec.keys.KeyShape.listOf;
import static com.hedera.services.bdd.spec.keys.SigMapGenerator.Nature.UNIQUE_PREFIXES;
import static com.hedera.services.bdd.spec.queries.QueryVerbs.getAccountBalance;
import static com.hedera.services.bdd.spec.transactions.TxnVerbs.atomicBatch;
import static com.hedera.services.bdd.spec.transactions.TxnVerbs.createTopic;
import static com.hedera.services.bdd.spec.transactions.TxnVerbs.cryptoCreate;
import static com.hedera.services.bdd.spec.transactions.TxnVerbs.cryptoTransfer;
import static com.hedera.services.bdd.spec.transactions.crypto.HapiCryptoTransfer.tinyBarsFromTo;
import static com.hedera.services.bdd.spec.transactions.crypto.HapiCryptoTransfer.tinyBarsFromToWithInvalidAmounts;
import static com.hedera.services.bdd.spec.utilops.UtilVerbs.balanceSnapshot;
import static com.hedera.services.bdd.spec.utilops.UtilVerbs.newKeyNamed;
import static com.hedera.services.bdd.spec.utilops.UtilVerbs.overriding;
import static com.hedera.services.bdd.suites.HapiSuite.GENESIS;
import static com.hedera.services.bdd.suites.HapiSuite.ONE_HBAR;
import static com.hedera.services.bdd.suites.HapiSuite.ONE_HUNDRED_HBARS;
import static com.hedera.services.bdd.suites.HapiSuite.ONE_MILLION_HBARS;
import static com.hedera.services.bdd.suites.HapiSuite.SYSTEM_ADMIN;
import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.INNER_TRANSACTION_FAILED;
import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.INSUFFICIENT_PAYER_BALANCE;
import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.INVALID_ACCOUNT_AMOUNTS;
import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.SUCCESS;
import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.TRANSACTION_OVERSIZE;

Expand Down Expand Up @@ -46,6 +55,8 @@ public class GovernanceTransactionsPostIngestTests {
private static final String PAYER_KEY = "payer_key";
private static final String PAYER_KEY2 = "payer_key2";
private static final String RECEIVER = "receiver";
// 0.0.4 is a non-default node; submitting to it bypasses ingest in embedded mode.
private static final String SUBMITTING_NODE_ACCOUNT_ID = "4";
private static final String TOPIC = "topic";
private static final String TOPIC2 = "topic2";
private static final String SUBMIT_KEY = "submit_key";
Expand Down Expand Up @@ -147,6 +158,58 @@ public Stream<DynamicTest> governanceAccountCannotSubmitWhenPaidOnlyBatchIfEnabl
.hasKnownStatus(INNER_TRANSACTION_FAILED));
}

@LeakyEmbeddedHapiTest(reason = MUST_SKIP_INGEST, requirement = SYSTEM_ACCOUNT_BALANCES)
@DisplayName("Ingest-decidable inner due-diligence failure in a batch charges the submitting node")
public Stream<DynamicTest> batchInnerIngestDecidableFailureChargesNode() {
return hapiTest(
cryptoCreate(PAYER).balance(ONE_HUNDRED_HBARS),
cryptoCreate(RECEIVER),
// Fund the submitting node so its network-fee charge is observable.
cryptoTransfer(tinyBarsFromTo(GENESIS, SUBMITTING_NODE_ACCOUNT_ID, ONE_HBAR)),
balanceSnapshot("nodePre", SUBMITTING_NODE_ACCOUNT_ID),
// An unbalanced inner transfer is an ingest-decidable due-diligence failure (INVALID_ACCOUNT_AMOUNTS):
// an honest node rejects it at ingest, so its reaching consensus is the node's fault, not the payer's.
atomicBatch(cryptoTransfer(tinyBarsFromToWithInvalidAmounts(PAYER, RECEIVER, 1L))
.payingWith(PAYER)
.batchKey(PAYER)
.hasKnownStatus(INVALID_ACCOUNT_AMOUNTS))
.setNode(SUBMITTING_NODE_ACCOUNT_ID)
.payingWith(PAYER)
// Batch resolution stays INNER_TRANSACTION_FAILED and the inner records its own status
// (AC #3); only the charge for the ingest-decidable inner shifts to the node. See #26615.
.hasKnownStatus(INNER_TRANSACTION_FAILED),
// The submitting node is charged the network fee for the inner's due-diligence failure.
getAccountBalance(SUBMITTING_NODE_ACCOUNT_ID).hasTinyBars(reducedFromSnapshot("nodePre")));
}

@LeakyEmbeddedHapiTest(reason = MUST_SKIP_INGEST, requirement = SYSTEM_ACCOUNT_BALANCES)
@DisplayName("A batch inner drained by an earlier inner fails INSUFFICIENT_PAYER_BALANCE without charging the node")
public Stream<DynamicTest> batchInnerDrainedByEarlierInnerDoesNotChargeNode() {
return hapiTest(
cryptoCreate(PAYER).balance(ONE_HBAR),
cryptoCreate(RECEIVER).balance(0L),
cryptoTransfer(tinyBarsFromTo(GENESIS, SUBMITTING_NODE_ACCOUNT_ID, ONE_HBAR)),
balanceSnapshot("nodePre", SUBMITTING_NODE_ACCOUNT_ID),
atomicBatch(
// Inner #1 drains PAYER to a single tinybar. It is paid by GENESIS (fees waived), so
// PAYER loses only the transferred amount and no node fee is collected here.
cryptoTransfer(tinyBarsFromTo(PAYER, RECEIVER, ONE_HBAR - 1))
.payingWith(GENESIS)
.signedBy(GENESIS, PAYER)
.batchKey(GENESIS),
// Inner #2, paid by the now-drained PAYER, cannot cover its network fee at handle.
cryptoTransfer(tinyBarsFromTo(PAYER, RECEIVER, 1))
.payingWith(PAYER)
.batchKey(GENESIS)
.hasKnownStatus(INSUFFICIENT_PAYER_BALANCE))
.setNode(SUBMITTING_NODE_ACCOUNT_ID)
.payingWith(GENESIS)
.hasKnownStatus(INNER_TRANSACTION_FAILED),
// The shortfall is state-dependent (PAYER was solvent at submission, drained mid-batch), so the node
// is NOT charged -- unlike an ingest-decidable failure. See #26615.
getAccountBalance(SUBMITTING_NODE_ACCOUNT_ID).hasTinyBars(unchangedFromSnapshot("nodePre")));
}

@EmbeddedHapiTest(MUST_SKIP_INGEST)
@DisplayName(
"Governance account cannot submit more than 6KB batch transactions when only the inner transaction is paid")
Expand Down
Loading