Rationalize treatment of auto-create MAX_CHILD_RECORDS_EXCEEDED (#9966

) Signed-off-by: Michael Tinker <[email protected]> Signed-off-by: Neeharika-Sompalli <[email protected]> Co-authored-by: Neeharika-Sompalli <[email protected]>
hashgraph · Nov 17, 2023 · d13b121 · d13b121
1 parent b6bf36f
commit d13b121
Show file tree

Hide file tree

Showing 9 changed files with 113 additions and 16 deletions.
diff --git a/...ono-service/src/jmh/java/com/hedera/node/app/service/mono/mocks/MockRecordsHistorian.java b/...ono-service/src/jmh/java/com/hedera/node/app/service/mono/mocks/MockRecordsHistorian.java
@@ -98,6 +98,11 @@ public void trackFollowingChildRecord(
         // No-op
     }
 
+    @Override
+    public boolean canTrackPrecedingChildRecords(int n) {
+        return false;
+    }
+
     @Override
     public void trackPrecedingChildRecord(
             final int sourceId, final Builder syntheticBody, final ExpirableTxnRecord.Builder recordSoFar) {

diff --git a/...era-mono-service/src/main/java/com/hedera/node/app/service/mono/ledger/TransferLogic.java b/...era-mono-service/src/main/java/com/hedera/node/app/service/mono/ledger/TransferLogic.java
@@ -27,6 +27,7 @@
 import static com.hedera.node.app.service.mono.ledger.properties.NftProperty.SPENDER;
 import static com.hedera.node.app.service.mono.state.submerkle.EntityId.MISSING_ENTITY_ID;
 import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.INSUFFICIENT_PAYER_BALANCE;
+import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.MAX_CHILD_RECORDS_EXCEEDED;
 import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.OK;
 
 import com.hedera.node.app.service.evm.exceptions.InvalidTransactionException;
@@ -111,25 +112,32 @@ public void doZeroSum(final List<BalanceChange> changes) {
         var autoCreationFee = 0L;
         var updatedPayerBalance = Long.MIN_VALUE;
         boolean failedAutoCreation = false;
+        boolean hasSuccessfulAutoCreation = false;
+        int numAutoCreationsSoFar = 0;
         for (final var change : changes) {
             // If the change consists of any repeated aliases, replace the alias with the account
             // number
             replaceAliasWithIdIfExisting(change);
-
             // create a new account for alias when the no account is already created using the alias
             if (change.hasAlias()) {
                 if (autoCreationLogic == null) {
                     throw new IllegalStateException(
                             "Cannot auto-create account from " + change + " with null autoCreationLogic");
                 }
-                final var result = autoCreationLogic.create(change, accountsLedger, changes);
-                validity = result.getKey();
-                // We break this loop on the first non-OK validity
-                failedAutoCreation = validity != OK;
-                autoCreationFee += result.getValue();
-                if (validity == OK && (change.isForToken())) {
-                    validity = tokenStore.tryTokenChange(change);
+                numAutoCreationsSoFar++;
+                if (recordsHistorian.canTrackPrecedingChildRecords(numAutoCreationsSoFar)) {
+                    final var result = autoCreationLogic.create(change, accountsLedger, changes);
+                    validity = result.getKey();
+                    // We break this loop on the first non-OK validity
+                    hasSuccessfulAutoCreation |= validity == OK;
+                    autoCreationFee += result.getValue();
+                    if (validity == OK && (change.isForToken())) {
+                        validity = tokenStore.tryTokenChange(change);
+                    }
+                } else {
+                    validity = MAX_CHILD_RECORDS_EXCEEDED;
                 }
+                failedAutoCreation = validity != OK;
             } else if (change.isForHbar()) {
                 validity = accountsLedger.validate(change.accountId(), scopedCheck.setBalanceChange(change));
                 if (change.affectsAccount(topLevelPayer)) {
@@ -160,6 +168,10 @@ public void doZeroSum(final List<BalanceChange> changes) {
             adjustBalancesAndAllowances(changes);
             if (autoCreationFee > 0) {
                 payAutoCreationFee(autoCreationFee);
+            }
+            // If the auto creation is successful submit the records to historian,
+            // even if auto creation fee is 0 (which can be the case if the payer is a superuser)
+            if (hasSuccessfulAutoCreation) {
                 autoCreationLogic.submitRecordsTo(recordsHistorian);
             }
         } else {

diff --git a/...mono-service/src/main/java/com/hedera/node/app/service/mono/records/RecordsHistorian.java b/...mono-service/src/main/java/com/hedera/node/app/service/mono/records/RecordsHistorian.java
@@ -147,6 +147,14 @@ void trackFollowingChildRecord(
     void trackPrecedingChildRecord(
             int sourceId, TransactionBody.Builder syntheticBody, ExpirableTxnRecord.Builder recordSoFar);
 
+    /**
+     * Returns whether the active transaction has the capacity to track the given number of preceding children.
+     *
+     * @param n the number of preceding children to track
+     * @return whether the active transaction has the capacity to track the given number of preceding children
+     */
+    boolean canTrackPrecedingChildRecords(int n);
+
     /**
      * Reverts all records created by the given source.
      *

diff --git a/...vice/src/main/java/com/hedera/node/app/service/mono/records/TxnAwareRecordsHistorian.java b/...vice/src/main/java/com/hedera/node/app/service/mono/records/TxnAwareRecordsHistorian.java
@@ -215,6 +215,11 @@ public void trackFollowingChildRecord(
         followingChildRecords.add(inProgress);
     }
 
+    @Override
+    public boolean canTrackPrecedingChildRecords(final int n) {
+        return consensusTimeTracker.isAllowablePrecedingOffset(precedingChildRecords.size() + (long) n);
+    }
+
     @Override
     public void trackPrecedingChildRecord(
             final int sourceId,

diff --git a/...rvice/src/test/java/com/hedera/node/app/service/mono/ledger/LedgerBalanceChangesTest.java b/...rvice/src/test/java/com/hedera/node/app/service/mono/ledger/LedgerBalanceChangesTest.java
@@ -31,6 +31,7 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.BDDMockito.given;
@@ -433,6 +434,7 @@ void happyPathTransfersWithAutoCreation() {
         given(validator.expiryStatusGiven(anyLong(), anyBoolean(), anyBoolean()))
                 .willReturn(OK);
         given(aliasManager.lookupIdBy(aliasA.toByteString())).willReturn(EntityNum.MISSING_NUM);
+        given(historian.canTrackPrecedingChildRecords(anyInt())).willReturn(true);
 
         subject.begin();
         assertDoesNotThrow(() -> subject.doZeroSum(changes));
@@ -488,6 +490,7 @@ void invalidTransfersWithAutoCreationDrainsCapacityIfSelfSubmitted() {
         given(txnCtx.isSelfSubmitted()).willReturn(true);
         given(autoCreationLogic.create(any(), eq(accountsLedger), any())).willReturn(Pair.of(INVALID_ACCOUNT_ID, 100L));
         given(aliasManager.lookupIdBy(aliasA.toByteString())).willReturn(EntityNum.MISSING_NUM);
+        given(historian.canTrackPrecedingChildRecords(anyInt())).willReturn(true);
 
         subject.begin();
         assertFailsWith(() -> subject.doZeroSum(changes), INVALID_ACCOUNT_ID);
@@ -532,6 +535,7 @@ void invalidTransfersWithAutoCreationDrainsNoCapacityIfNotSelfSubmitted() {
         given(txnCtx.activePayer()).willReturn(payer);
         given(autoCreationLogic.create(any(), eq(accountsLedger), any())).willReturn(Pair.of(INVALID_ACCOUNT_ID, 100L));
         given(aliasManager.lookupIdBy(aliasA.toByteString())).willReturn(EntityNum.MISSING_NUM);
+        given(historian.canTrackPrecedingChildRecords(anyInt())).willReturn(true);
 
         subject.begin();
         assertFailsWith(() -> subject.doZeroSum(changes), INVALID_ACCOUNT_ID);

diff --git a/...mono-service/src/test/java/com/hedera/node/app/service/mono/ledger/TransferLogicTest.java b/...mono-service/src/test/java/com/hedera/node/app/service/mono/ledger/TransferLogicTest.java
@@ -24,13 +24,15 @@
 import static com.hedera.test.utils.TxnUtils.assertFailsWith;
 import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.INSUFFICIENT_ACCOUNT_BALANCE;
 import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.INSUFFICIENT_PAYER_BALANCE;
+import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.MAX_CHILD_RECORDS_EXCEEDED;
 import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.OK;
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.BDDMockito.given;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
@@ -216,13 +218,34 @@ void cleansUpOnFailedAutoCreation() {
         accountsLedger.begin();
         accountsLedger.create(mockCreation);
         given(autoCreationLogic.reclaimPendingAliases()).willReturn(true);
+        given(recordsHistorian.canTrackPrecedingChildRecords(anyInt())).willReturn(true);
 
         assertFailsWith(() -> subject.doZeroSum(changes), INSUFFICIENT_ACCOUNT_BALANCE);
 
         verify(autoCreationLogic).reclaimPendingAliases();
         assertTrue(accountsLedger.getCreatedKeys().isEmpty());
     }
 
+    @Test
+    void behavesAsExpectedOnAutoCreationWithInsufficientChildRecords() {
+        final var mockCreation = IdUtils.asAccount("0.0.1234");
+        final var firstAmount = 1_000L;
+        final var firstAlias = ByteString.copyFromUtf8("fake");
+        final var failingTrigger = BalanceChange.changingHbar(aliasedAa(firstAlias, firstAmount), payer);
+        final var changes = List.of(failingTrigger);
+        given(aliasManager.lookupIdBy(firstAlias)).willReturn(EntityNum.MISSING_NUM);
+
+        accountsLedger.begin();
+        accountsLedger.create(mockCreation);
+        given(autoCreationLogic.reclaimPendingAliases()).willReturn(true);
+
+        assertFailsWith(() -> subject.doZeroSum(changes), MAX_CHILD_RECORDS_EXCEEDED);
+
+        verify(autoCreationLogic).reclaimPendingAliases();
+        assertTrue(accountsLedger.getCreatedKeys().isEmpty());
+        verify(recordsHistorian, never()).trackPrecedingChildRecord(anyInt(), any(), any());
+    }
+
     @Test
     void autoCreatesWithNftTransferToAlias() {
         final var mockCreation = IdUtils.asAccount("0.0.1234");
@@ -247,6 +270,7 @@ void autoCreatesWithNftTransferToAlias() {
         given(tokenStore.tryTokenChange(any())).willReturn(OK);
         given(txnCtx.activePayer()).willReturn(payer);
         given(aliasManager.lookupIdBy(firstAlias)).willReturn(EntityNum.MISSING_NUM);
+        given(recordsHistorian.canTrackPrecedingChildRecords(anyInt())).willReturn(true);
 
         subject.doZeroSum(changes);
 
@@ -278,6 +302,7 @@ void autoCreatesWithFungibleTokenTransferToAlias() {
         given(tokenStore.tryTokenChange(any())).willReturn(OK);
         given(txnCtx.activePayer()).willReturn(payer);
         given(aliasManager.lookupIdBy(firstAlias)).willReturn(EntityNum.MISSING_NUM);
+        given(recordsHistorian.canTrackPrecedingChildRecords(anyInt())).willReturn(true);
 
         subject.doZeroSum(changes);
 
@@ -358,6 +383,7 @@ void createsAccountsAsExpected() {
         given(txnCtx.activePayer()).willReturn(payer);
         given(aliasManager.lookupIdBy(firstAlias)).willReturn(EntityNum.MISSING_NUM);
         given(aliasManager.lookupIdBy(secondAlias)).willReturn(EntityNum.MISSING_NUM);
+        given(recordsHistorian.canTrackPrecedingChildRecords(anyInt())).willReturn(true);
         subject.doZeroSum(changes);
 
         assertEquals(2 * autoFee, (long) accountsLedger.get(funding, AccountProperty.BALANCE));
@@ -406,6 +432,7 @@ void failsIfPayerDoesntHaveEnoughBalance() {
         given(txnCtx.activePayer()).willReturn(payer);
         given(aliasManager.lookupIdBy(firstAlias)).willReturn(EntityNum.MISSING_NUM);
         given(aliasManager.lookupIdBy(secondAlias)).willReturn(EntityNum.MISSING_NUM);
+        given(recordsHistorian.canTrackPrecedingChildRecords(anyInt())).willReturn(true);
         final var ex = assertThrows(InvalidTransactionException.class, () -> subject.doZeroSum(changes));
         assertEquals(INSUFFICIENT_PAYER_BALANCE, ex.getResponseCode());
 
@@ -442,6 +469,7 @@ void failsIfPayerDoesntHaveEnoughBalanceAfterTransfersFromHisAccount() {
         given(txnCtx.activePayer()).willReturn(payer);
         given(aliasManager.lookupIdBy(firstAlias)).willReturn(EntityNum.MISSING_NUM);
         given(autoCreationLogic.reclaimPendingAliases()).willReturn(true);
+        given(recordsHistorian.canTrackPrecedingChildRecords(anyInt())).willReturn(true);
 
         final var ex = assertThrows(InvalidTransactionException.class, () -> subject.doZeroSum(changes));
 

diff --git a/...st-clients/src/main/java/com/hedera/services/bdd/spec/assertions/TransferListAsserts.java b/...st-clients/src/main/java/com/hedera/services/bdd/spec/assertions/TransferListAsserts.java
@@ -34,10 +34,29 @@
 import java.util.Set;
 import java.util.function.Function;
 import java.util.function.LongSupplier;
+import java.util.function.ToLongFunction;
 import java.util.stream.Stream;
 import org.junit.jupiter.api.Assertions;
 
 public class TransferListAsserts extends BaseErroringAssertsProvider<TransferList> {
+    // non-standard initialization
+    @SuppressWarnings("java:S3599")
+    public static TransferListAsserts noCreditAboveNumber(ToLongFunction<HapiSpec> provider) {
+        return new TransferListAsserts() {
+            {
+                registerProvider((spec, o) -> {
+                    TransferList actual = (TransferList) o;
+                    long maxAllowed = provider.applyAsLong(spec);
+                    Assertions.assertTrue(
+                            actual.getAccountAmountsList().stream()
+                                    .filter(aa -> aa.getAmount() > 0)
+                                    .allMatch(aa -> aa.getAccountID().getAccountNum() <= maxAllowed),
+                            "Transfers include a credit above account 0.0." + maxAllowed);
+                });
+            }
+        };
+    }
+
     public static TransferListAsserts exactParticipants(Function<HapiSpec, List<AccountID>> provider) {
         return new ExactParticipantsAssert(provider);
     }

diff --git a/hedera-node/test-clients/src/main/java/com/hedera/services/bdd/spec/utilops/UtilVerbs.java b/hedera-node/test-clients/src/main/java/com/hedera/services/bdd/spec/utilops/UtilVerbs.java
@@ -607,15 +607,25 @@ public static HapiSpecOperation childRecordsCheck(
             final String parentTxnId,
             final ResponseCodeEnum parentalStatus,
             final TransactionRecordAsserts... childRecordAsserts) {
+        return childRecordsCheck(parentTxnId, parentalStatus, parentRecordAsserts -> {}, childRecordAsserts);
+    }
+
+    public static HapiSpecOperation childRecordsCheck(
+            final String parentTxnId,
+            final ResponseCodeEnum parentalStatus,
+            final Consumer<TransactionRecordAsserts> parentRecordAssertsSpec,
+            final TransactionRecordAsserts... childRecordAsserts) {
         return withOpContext((spec, opLog) -> {
             final var lookup = getTxnRecord(parentTxnId);
             allRunFor(spec, lookup);
             final var parentId = lookup.getResponseRecord().getTransactionID();
+            final var parentRecordAsserts = recordWith().status(parentalStatus).txnId(parentId);
+            parentRecordAssertsSpec.accept(parentRecordAsserts);
             allRunFor(
                     spec,
                     getTxnRecord(parentTxnId)
                             .andAllChildRecords()
-                            .hasPriority(recordWith().status(parentalStatus).txnId(parentId))
+                            .hasPriority(parentRecordAsserts)
                             .hasChildRecords(parentId, childRecordAsserts)
                             .logged());
         });

diff --git a/...s/src/main/java/com/hedera/services/bdd/suites/crypto/HollowAccountFinalizationSuite.java b/...s/src/main/java/com/hedera/services/bdd/suites/crypto/HollowAccountFinalizationSuite.java
@@ -21,6 +21,7 @@
 import static com.hedera.services.bdd.spec.HapiSpec.defaultHapiSpec;
 import static com.hedera.services.bdd.spec.assertions.AccountInfoAsserts.accountWith;
 import static com.hedera.services.bdd.spec.assertions.TransactionRecordAsserts.recordWith;
+import static com.hedera.services.bdd.spec.assertions.TransferListAsserts.noCreditAboveNumber;
 import static com.hedera.services.bdd.spec.keys.TrieSigMapGenerator.uniqueWithFullPrefixesFor;
 import static com.hedera.services.bdd.spec.queries.QueryVerbs.getAccountBalance;
 import static com.hedera.services.bdd.spec.queries.QueryVerbs.getAliasedAccountInfo;
@@ -50,13 +51,11 @@
 import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.INSUFFICIENT_ACCOUNT_BALANCE;
 import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.INVALID_ALIAS_KEY;
 import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.MAX_CHILD_RECORDS_EXCEEDED;
-import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.REVERTED_SUCCESS;
 import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.SUCCESS;
 
 import com.esaulpaugh.headlong.abi.Tuple;
 import com.google.protobuf.ByteString;
 import com.hedera.services.bdd.junit.HapiTest;
-import com.hedera.services.bdd.junit.HapiTestSuite;
 import com.hedera.services.bdd.spec.HapiSpec;
 import com.hedera.services.bdd.spec.HapiSpecOperation;
 import com.hedera.services.bdd.spec.queries.crypto.HapiGetAccountInfo;
@@ -71,12 +70,13 @@
 import edu.umd.cs.findbugs.annotations.NonNull;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Function;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
-@HapiTestSuite
+// @HapiTestSuite
 public class HollowAccountFinalizationSuite extends HapiSuite {
     private static final Logger LOG = LogManager.getLogger(HollowAccountFinalizationSuite.class);
     private static final String ANOTHER_SECP_256K1_SOURCE_KEY = "anotherSecp256k1Alias";
@@ -684,14 +684,17 @@ private HapiSpec txnWith2CompletionsAndAnother2PrecedingChildRecords() {
         final var ecdsaKey2 = "ecdsaKey2";
         final var recipientKey = "recipient";
         final var recipientKey2 = "recipient2";
+        final var receiverId = new AtomicLong();
         return defaultHapiSpec("txnWith2CompletionsAndAnother2PrecedingChildRecords")
                 .given(
                         newKeyNamed(SECP_256K1_SOURCE_KEY).shape(SECP_256K1_SHAPE),
                         newKeyNamed(ecdsaKey2).shape(SECP_256K1_SHAPE),
                         newKeyNamed(recipientKey).shape(SECP_256K1_SHAPE),
                         newKeyNamed(recipientKey2).shape(SECP_256K1_SHAPE),
                         cryptoCreate(LAZY_CREATE_SPONSOR).balance(INITIAL_BALANCE * ONE_HBAR),
-                        cryptoCreate(CRYPTO_TRANSFER_RECEIVER).balance(INITIAL_BALANCE * ONE_HBAR))
+                        cryptoCreate(CRYPTO_TRANSFER_RECEIVER)
+                                .balance(INITIAL_BALANCE * ONE_HBAR)
+                                .exposingCreatedIdTo(id -> receiverId.set(id.getAccountNum())))
                 .when(withOpContext((spec, opLog) -> {
                     final var op1 = sendToEvmAddressFromECDSAKey(spec, SECP_256K1_SOURCE_KEY, TRANSFER_TXN);
                     final var op2 = sendToEvmAddressFromECDSAKey(spec, ecdsaKey2, "randomTxn");
@@ -722,10 +725,13 @@ private HapiSpec txnWith2CompletionsAndAnother2PrecedingChildRecords() {
                     final var childRecordCheck = childRecordsCheck(
                             TRANSFER_TXN_2,
                             MAX_CHILD_RECORDS_EXCEEDED,
+                            // Ensure there are no credits to auto-created accounts
+                            parentAsserts -> parentAsserts.transfers(noCreditAboveNumber(ignore -> spec.registry()
+                                    .getAccountID(SECP_256K1_SOURCE_KEY)
+                                    .getAccountNum())),
                             recordWith().status(SUCCESS),
-                            recordWith().status(SUCCESS),
-                            recordWith().status(REVERTED_SUCCESS));
-                    //                    // assert that the payer has been finalized
+                            recordWith().status(SUCCESS));
+                    // assert that the payer has been finalized
                     final var ecdsaKey = spec.registry().getKey(SECP_256K1_SOURCE_KEY);
                     final var payerEvmAddress = ByteString.copyFrom(recoverAddressFromPubKey(
                             ecdsaKey.getECDSASecp256K1().toByteArray()));