Skip to content

Commit

Permalink
fix: (mono) reclaim entity id after failed auto-create; avoid `FAIL_I…
Browse files Browse the repository at this point in the history 
…NVALID` on invalid token id (#12322)

Signed-off-by: Michael Tinker <[email protected]>
Co-authored-by: Iris Simon <[email protected]>
  • Loading branch information
@tinker-michaelj @iwsimon @imalygin
2 people authored and imalygin committed Apr 17, 2024
1 parent 23862bf commit 2fae105
Show file tree
Hide file tree
Showing 8 changed files with 78 additions and 39 deletions.
3 changes: 2 additions & 1 deletion .../hapi-utils/src/main/java/com/hedera/node/app/hapi/utils/forensics/OrderedComparison.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,8 @@ static List<DifferingEntries> diff(
"Additional modular record found at "
+ secondEntries.get(i).consensusTime() + " for transactionID : "
+ secondEntries.get(i).txnRecord().getTransactionID() + " transBody : "
+ secondEntries.get(i).body()));
+ secondEntries.get(i).body()
+ "\n -> \n" + secondEntries.get(i).txnRecord()));
continue;
}
final var secondEntry = entryWithMatchableRecord(secondEntries, i, firstEntry);
Expand Down
20 changes: 12 additions & 8 deletions ...a/com/hedera/node/app/service/mono/txns/crypto/CryptoApproveAllowanceTransitionLogic.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.SUCCESS;

import com.hedera.node.app.service.evm.exceptions.InvalidTransactionException;
import com.hedera.node.app.service.mono.context.TransactionContext;
import com.hedera.node.app.service.mono.context.primitives.StateView;
import com.hedera.node.app.service.mono.store.AccountStore;
Expand Down Expand Up @@ -82,13 +83,16 @@ public Function<TransactionBody, ResponseCodeEnum> semanticCheck() {
private ResponseCodeEnum validate(TransactionBody cryptoAllowanceTxn) {
final AccountID payer = cryptoAllowanceTxn.getTransactionID().getAccountID();
final var op = cryptoAllowanceTxn.getCryptoApproveAllowance();
final var payerAccount = accountStore.loadAccount(Id.fromGrpcAccount(payer));

return allowanceChecks.allowancesValidation(
op.getCryptoAllowancesList(),
op.getTokenAllowancesList(),
op.getNftAllowancesList(),
payerAccount,
workingView);
try {
final var payerAccount = accountStore.loadAccount(Id.fromGrpcAccount(payer));
return allowanceChecks.allowancesValidation(
op.getCryptoAllowancesList(),
op.getTokenAllowancesList(),
op.getNftAllowancesList(),
payerAccount,
workingView);
} catch (InvalidTransactionException e) {
return e.getResponseCode();
}
}
}
2 changes: 1 addition & 1 deletion .../src/main/java/com/hedera/node/app/service/token/impl/handlers/CryptoTransferHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ public void handle(@NonNull final HandleContext context) throws HandleException
new TransferContextImpl(context, enforceMonoServiceRestrictionsOnAutoCreationCustomFeePayments);

// (TEMPORARY) Remove this after diff testing
transferContext.validateHbarAllowances();
transferContext.validateTopLevelAllowances();
transferContext.validateAccountIds();

// Replace all aliases in the transaction body with its account ids
Expand Down
1 change: 0 additions & 1 deletion ...in/java/com/hedera/node/app/service/token/impl/handlers/transfer/NFTOwnersChangeStep.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,5 @@ private void updateOwnership(
tokenRelStore.put(senderRelCopy.balance(fromTokenRelBalance - 1).build());
tokenRelStore.put(receiverRelCopy.balance(toTokenRelBalance + 1).build());
nftStore.put(nftCopy.build());
// TODO: make sure finalize is capturing all these in transfer list
}
}
4 changes: 2 additions & 2 deletions ...c/main/java/com/hedera/node/app/service/token/impl/handlers/transfer/TransferContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,11 @@ public interface TransferContext {
boolean isEnforceMonoServiceRestrictionsOnAutoCreationCustomFeePayments();

/**
* Validates hbar allowances for the top-level operation in this transfer context.
* Validates allowances for the top-level operation in this transfer context.
*
* <p> (FUTURE) Remove this, only needed for diff testing and has no logical priority.
*/
void validateHbarAllowances();
void validateTopLevelAllowances();

/**
* Validates account IDs for the top-level operation in this transfer context.
Expand Down
21 changes: 20 additions & 1 deletion ...in/java/com/hedera/node/app/service/token/impl/handlers/transfer/TransferContextImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,18 @@
import static com.hedera.hapi.node.base.ResponseCodeEnum.SPENDER_DOES_NOT_HAVE_ALLOWANCE;
import static com.hedera.node.app.service.mono.utils.EntityIdUtils.EVM_ADDRESS_SIZE;
import static com.hedera.node.app.service.token.AliasUtils.isSerializedProtoKey;
import static com.hedera.node.app.service.token.impl.handlers.transfer.NFTOwnersChangeStep.validateSpenderHasAllowance;
import static com.hedera.node.app.spi.workflows.HandleException.validateTrue;
import static java.util.Collections.emptyList;

import com.hedera.hapi.node.base.AccountID;
import com.hedera.hapi.node.base.NftID;
import com.hedera.hapi.node.base.TokenAssociation;
import com.hedera.hapi.node.base.TokenID;
import com.hedera.hapi.node.base.TransferList;
import com.hedera.hapi.node.state.token.Account;
import com.hedera.hapi.node.transaction.AssessedCustomFee;
import com.hedera.node.app.service.token.ReadableNftStore;
import com.hedera.node.app.service.token.impl.WritableAccountStore;
import com.hedera.node.app.spi.workflows.HandleContext;
import com.hedera.node.app.spi.workflows.HandleException;
Expand Down Expand Up @@ -177,7 +181,7 @@ public boolean isEnforceMonoServiceRestrictionsOnAutoCreationCustomFeePayments()
}

@Override
public void validateHbarAllowances() {
public void validateTopLevelAllowances() {
final var topLevelPayer = context.payer();
final var op = context.body().cryptoTransferOrThrow();
for (final var aa : op.transfersOrElse(TransferList.DEFAULT).accountAmountsOrElse(emptyList())) {
Expand All @@ -186,6 +190,21 @@ public void validateHbarAllowances() {
accountStore.get(aa.accountIDOrElse(AccountID.DEFAULT)), topLevelPayer, aa.amount());
}
}
final var nftStore = context.readableStore(ReadableNftStore.class);
for (final var tokenTransfers : op.tokenTransfersOrElse(emptyList())) {
final var tokenId = tokenTransfers.tokenOrElse(TokenID.DEFAULT);
for (final var oc : tokenTransfers.nftTransfersOrElse(emptyList())) {
if (oc.isApproval()) {
final var maybeOwner = accountStore.get(oc.senderAccountIDOrElse(AccountID.DEFAULT));
if (maybeOwner != null) {
final var maybeNft = nftStore.get(new NftID(tokenId, oc.serialNumber()));
if (maybeNft != null) {
validateSpenderHasAllowance(maybeOwner, topLevelPayer, tokenId, maybeNft);
}
}
}
}
}
}

@Override
Expand Down
12 changes: 9 additions & 3 deletions ...test-clients/src/main/java/com/hedera/services/bdd/suites/consensus/TopicCreateSuite.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,6 @@ final HapiSpec signingRequirementsEnforced() {
.payingWith("payer")
.signedBy("wrongKey")
.hasPrecheck(INVALID_SIGNATURE),
// In hedera-app, we'll allow contracts with admin keys to be auto-renew accounts
createTopic("withContractAutoRenew").autoRenewAccountId(contractWithAdminKey),
// But contracts without admin keys will get INVALID_SIGNATURE (can't sign!)
createTopic("NotToBe")
.autoRenewAccountId(PAY_RECEIVABLE_CONTRACT)
Expand Down Expand Up @@ -197,7 +195,11 @@ final HapiSpec signingRequirementsEnforced() {
.autoRenewAccountId("autoRenewAccount")
/* SigMap missing signature from adminKey. */
.signedBy("payer", "autoRenewAccount")
.hasKnownStatus(INVALID_SIGNATURE))
.hasKnownStatus(INVALID_SIGNATURE),
// In hedera-app, we'll allow contracts with admin keys to be auto-renew accounts
createTopic("withContractAutoRenew")
.adminKeyName("adminKey")
.autoRenewAccountId(contractWithAdminKey))
.then(
createTopic("noAdminKeyNoAutoRenewAccount"),
getTopicInfo("noAdminKeyNoAutoRenewAccount")
Expand All @@ -213,6 +215,10 @@ final HapiSpec signingRequirementsEnforced() {
getTopicInfo("explicitAdminKeyExplicitAutoRenewAccount")
.hasAdminKey("adminKey")
.hasAutoRenewAccount("autoRenewAccount")
.logged(),
getTopicInfo("withContractAutoRenew")
.hasAdminKey("adminKey")
.hasAutoRenewAccount(contractWithAdminKey)
.logged());
}

Expand Down
54 changes: 32 additions & 22 deletions .../com/hedera/services/bdd/suites/contract/precompile/LazyCreateThroughPrecompileSuite.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,17 +200,23 @@ HapiSpec resourceLimitExceededRevertsAllRecords() {
IntStream.range(0, n)
.mapToObj(i -> ByteString.copyFromUtf8(ONE_TIME + i))
.toList()))
.when(sourcing(() -> contractCall(
AUTO_CREATION_MODES,
"createSeveralDirectly",
headlongFromHexed(nftMirrorAddr.get()),
nCopiesOfSender(n, mirrorAddrWith(civilianId.get())),
nNonMirrorAddressFrom(n, civilianId.get() + 3_050_000),
LongStream.iterate(1L, l -> l + 1).limit(n).toArray())
.via(creationAttempt)
.gas(GAS_TO_OFFER)
.alsoSigningWithFullPrefix(CIVILIAN)
.hasKnownStatusFrom(MAX_CHILD_RECORDS_EXCEEDED, CONTRACT_REVERT_EXECUTED)))
.when(
cryptoApproveAllowance()
.payingWith(CIVILIAN)
.addNftAllowance(CIVILIAN, nft, AUTO_CREATION_MODES, true, List.of()),
sourcing(() -> contractCall(
AUTO_CREATION_MODES,
"createSeveralDirectly",
headlongFromHexed(nftMirrorAddr.get()),
nCopiesOfSender(n, mirrorAddrWith(civilianId.get())),
nNonMirrorAddressFrom(n, civilianId.get() + 3_050_000),
LongStream.iterate(1L, l -> l + 1)
.limit(n)
.toArray())
.via(creationAttempt)
.gas(GAS_TO_OFFER)
.alsoSigningWithFullPrefix(CIVILIAN)
.hasKnownStatusFrom(MAX_CHILD_RECORDS_EXCEEDED, CONTRACT_REVERT_EXECUTED)))
.then(
// mono-service did not do an "orderly shutdown" of the EVM transaction when it hit
// a resource limit exception like MAX_CHILD_RECORDS_EXCEEDED, instead throwing an
Expand Down Expand Up @@ -257,17 +263,21 @@ HapiSpec autoCreationFailsWithMirrorAddress() {
.exposingCreatedIdTo(
idLit -> nftMirrorAddr.set(asHexedSolidityAddress(asToken(idLit)))),
mintToken(nft, List.of(ByteString.copyFromUtf8(ONE_TIME))))
.when(sourcing(() -> contractCall(
AUTO_CREATION_MODES,
CREATE_DIRECTLY,
headlongFromHexed(nftMirrorAddr.get()),
mirrorAddrWith(civilianId.get()),
mirrorAddrWith(civilianId.get() + 1_000_001),
1L,
false)
.via(creationAttempt)
.gas(GAS_TO_OFFER)
.hasKnownStatus(CONTRACT_REVERT_EXECUTED)))
.when(
cryptoApproveAllowance()
.payingWith(CIVILIAN)
.addNftAllowance(CIVILIAN, nft, AUTO_CREATION_MODES, true, List.of()),
sourcing(() -> contractCall(
AUTO_CREATION_MODES,
CREATE_DIRECTLY,
headlongFromHexed(nftMirrorAddr.get()),
mirrorAddrWith(civilianId.get()),
mirrorAddrWith(civilianId.get() + 1_000_001),
1L,
false)
.via(creationAttempt)
.gas(GAS_TO_OFFER)
.hasKnownStatus(CONTRACT_REVERT_EXECUTED)))
.then(childRecordsCheck(
creationAttempt, CONTRACT_REVERT_EXECUTED, recordWith().status(INVALID_ALIAS_KEY)));
}
Expand Down

0 comments on commit 2fae105

Please sign in to comment.