Skip to content

Commit

Permalink
Validate and fix fuzzy records in HollowAccountFinalizationSuite (#…
Browse files Browse the repository at this point in the history 
…9904)

Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
Co-authored-by: Michael Tinker <michael.tinker@swirldslabs.com>
Co-authored-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
  • Loading branch information
@Neeharika-Sompalli @tinker-michaelj @MiroslavGatsanoga
3 people committed Nov 17, 2023
1 parent cd7e715 commit 791ab23
Show file tree
Hide file tree
Showing 46 changed files with 466 additions and 171 deletions.
22 changes: 22 additions & 0 deletions ...-node/hedera-app-spi/src/main/java/com/hedera/node/app/spi/workflows/HandleException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,33 @@
* {@link IllegalArgumentException} as appropriate.
*/
public class HandleException extends RuntimeException {
private final ShouldRollbackStack shouldRollbackStack;
private final ResponseCodeEnum status;
/**
* Whether the stack should be rolled back. In case of a ContractCall if it reverts, the gas charged
* should not be rolled back
*/
public enum ShouldRollbackStack {
YES,
NO
}

public HandleException(final ResponseCodeEnum status) {
this(status, ShouldRollbackStack.YES);
}

public HandleException(final ResponseCodeEnum status, final ShouldRollbackStack shouldRollbackStack) {
super(status.protoName());
this.status = status;
this.shouldRollbackStack = shouldRollbackStack;
}

/**
* Returns whether the stack should be rolled back. In case of a ContractCall if it reverts, the gas charged
* should not be rolled back
*/
public boolean shouldRollbackStack() {
return shouldRollbackStack == ShouldRollbackStack.YES;
}

/**
Expand Down
168 changes: 98 additions & 70 deletions ...ra-node/hedera-app/src/main/java/com/hedera/node/app/workflows/handle/HandleWorkflow.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
import com.hedera.node.app.records.BlockRecordManager;
import com.hedera.node.app.service.token.ReadableAccountStore;
import com.hedera.node.app.service.token.api.TokenServiceApi;
import com.hedera.node.app.service.token.records.CryptoUpdateRecordBuilder;
import com.hedera.node.app.service.token.records.ParentRecordFinalizer;
import com.hedera.node.app.services.ServiceScopeLookup;
import com.hedera.node.app.signature.DefaultKeyVerifier;
Expand All @@ -77,7 +78,6 @@
import com.hedera.node.app.spi.workflows.InsufficientNonFeeDebitsException;
import com.hedera.node.app.spi.workflows.InsufficientServiceFeeException;
import com.hedera.node.app.spi.workflows.PreCheckException;
import com.hedera.node.app.spi.workflows.record.SingleTransactionRecordBuilder;
import com.hedera.node.app.state.HederaRecordCache;
import com.hedera.node.app.state.HederaState;
import com.hedera.node.app.throttle.NetworkUtilizationManager;
Expand Down Expand Up @@ -109,7 +109,6 @@
import java.time.Instant;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
Expand Down Expand Up @@ -360,7 +359,7 @@ private void handleUserTransaction(
transactionInfo.functionality(),
signatureMapSize,
payer,
preHandleResult.payerKey(),
preHandleResult.getPayerKey(),
networkInfo,
TransactionCategory.USER,
recordBuilder,
Expand Down Expand Up @@ -433,7 +432,7 @@ private void handleUserTransaction(
try {
// Any hollow accounts that must sign to have all needed signatures, need to be finalized
// as a result of transaction being handled.
finalizeHollowAccounts(context, configuration, preHandleResult.hollowAccounts(), verifier);
finalizeHollowAccounts(context, configuration, preHandleResult.getHollowAccounts(), verifier);

networkUtilizationManager.trackTxn(transactionInfo, consensusNow, stack);
// If the payer is authorized to waive fees, then we don't charge them
Expand Down Expand Up @@ -486,15 +485,17 @@ private void handleUserTransaction(
dualStateUpdateFacility.handleTxBody(stack, dualState, txBody);

} catch (final HandleException e) {
rollback(e.getStatus(), stack, recordListBuilder);
// In case of a ContractCall when it reverts, the gas charged should not be rolled back
rollback(e.shouldRollbackStack(), e.getStatus(), stack, recordListBuilder);
if (!hasWaivedFees) {
feeAccumulator.chargeFees(payer, creator.accountId(), fees);
}
}
}
} catch (final Exception e) {
logger.error("An unexpected exception was thrown during handle", e);
rollback(ResponseCodeEnum.FAIL_INVALID, stack, recordListBuilder);
// We should always rollback stack including gas charges when there is an unexpected exception
rollback(true, ResponseCodeEnum.FAIL_INVALID, stack, recordListBuilder);
if (payer != null && fees != null) {
try {
feeAccumulator.chargeFees(payer, creator.accountId(), fees);
Expand Down Expand Up @@ -563,8 +564,11 @@ private void finalizeHollowAccounts(
.build();
// Note the null key verification callback below; we bypass signature
// verifications when doing hollow account finalization
context.dispatchPrecedingTransaction(
syntheticUpdateTxn, SingleTransactionRecordBuilder.class, null, context.payer());
final var recordBuilder = context.dispatchPrecedingTransaction(
syntheticUpdateTxn, CryptoUpdateRecordBuilder.class, null, context.payer());
// For some reason update accountId is set only for the hollow account finalizations and not
// for top level crypto update transactions. So we set it here.
recordBuilder.accountID(hollowAccount.accountId());
}
}
}
Expand Down Expand Up @@ -603,7 +607,7 @@ private ValidationResult validate(
final var payerID = txInfo.payerID();
final var functionality = txInfo.functionality();
final var txBody = txInfo.txBody();
boolean isPayerHollow = false;
boolean isPayerHollow;

// Check if pre-handle was successful
if (preHandleResult.status() != SO_FAR_SO_GOOD) {
Expand Down Expand Up @@ -662,22 +666,23 @@ private ValidationResult validate(
}

// Check all signature verifications. This will also wait, if validation is still ongoing.
if (preHandleResult.payerKey() != null) {
final var payerKeyVerification = verifier.verificationFor(preHandleResult.payerKey());
if (!isPayerHollow && payerKeyVerification.failed()) {
// If the payer is hollow the key will be null, so we skip the payer signature verification.
if (!isPayerHollow) {
final var payerKeyVerification = verifier.verificationFor(preHandleResult.getPayerKey());
if (payerKeyVerification.failed()) {
return new ValidationResult(NODE_DUE_DILIGENCE_FAILURE, INVALID_PAYER_SIGNATURE);
}
}

// verify all the keys
for (final var key : preHandleResult.requiredKeys()) {
for (final var key : preHandleResult.getRequiredKeys()) {
final var verification = verifier.verificationFor(key);
if (verification.failed()) {
return new ValidationResult(PRE_HANDLE_FAILURE, INVALID_SIGNATURE);
}
}
// If there are any hollow accounts whose signatures need to be verified, verify them
for (final var hollowAccount : preHandleResult.hollowAccounts()) {
for (final var hollowAccount : preHandleResult.getHollowAccounts()) {
final var verification = verifier.verificationFor(hollowAccount.alias());
if (verification.failed()) {
return new ValidationResult(PRE_HANDLE_FAILURE, INVALID_SIGNATURE);
Expand All @@ -690,11 +695,22 @@ private ValidationResult validate(
private record ValidationResult(
@NonNull PreHandleResult.Status status, @NonNull ResponseCodeEnum responseCodeEnum) {}

/**
* Rolls back the stack and sets the status of the transaction in case of a failure.
* @param rollbackStack whether to rollback the stack. Will be false when the failure is due to a
* {@link HandleException} that is due to a contract call revert.
* @param status the status to set
* @param stack the save point stack to rollback
* @param recordListBuilder the record list builder to revert
*/
private void rollback(
final boolean rollbackStack,
@NonNull final ResponseCodeEnum status,
@NonNull final SavepointStackImpl stack,
@NonNull final RecordListBuilder recordListBuilder) {
stack.rollbackFullStack();
if (rollbackStack) {
stack.rollbackFullStack();
}
final var userTransactionRecordBuilder = recordListBuilder.userTransactionRecordBuilder();
userTransactionRecordBuilder.status(status);
recordListBuilder.revertChildrenOf(userTransactionRecordBuilder);
Expand Down Expand Up @@ -764,86 +780,98 @@ private PreHandleResult addMissingSignatures(

// extract keys and hollow accounts again
final var context = new PreHandleContextImpl(storeFactory, txBody, configuration, dispatcher);
dispatcher.dispatchPreHandle(context);
// Need to add payer key first in the list of required hollow accounts here, because this order
// determines the order of hollow account finalization records. The payer key must be finalized first.
// to easily compare results in differential testing
try {
final var payer = solvencyPreCheck.getPayerAccount(storeFactory, previousResult.payer());
final var payerKey = payer.key();
if (isHollow(payer)) {
context.requireSignatureForHollowAccount(payer);
}

// re-expand keys only if any of the keys have changed
final var previousResults = previousResult.verificationResults();
final var currentRequiredNonPayerKeys = context.requiredNonPayerKeys();
final var currentOptionalNonPayerKeys = context.optionalNonPayerKeys();
final var anyKeyChanged = haveKeyChanges(previousResults, context);
// If none of the keys changed then non need to re-expand all signatures.
if (!anyKeyChanged) {
return previousResult;
}
dispatcher.dispatchPreHandle(context);
// re-expand keys only if any of the keys have changed
final var currentRequiredNonPayerKeys = context.requiredNonPayerKeys();
final var currentOptionalNonPayerKeys = context.optionalNonPayerKeys();
final var anyKeyChanged = haveKeyChanges(previousResult, context);
// If none of the keys changed then non need to re-expand all signatures.
if (!anyKeyChanged) {
return previousResult;
}
// prepare signature verification
final var verifications = new HashMap<Key, SignatureVerificationFuture>();

// expand all keys
final var expanded = new HashSet<ExpandedSignaturePair>();
signatureExpander.expand(sigPairs, expanded);
if (payerKey != null && !isHollow(payer)) {
signatureExpander.expand(payerKey, sigPairs, expanded);
}

// prepare signature verification
final var verifications = new HashMap<Key, SignatureVerificationFuture>();
final var payer = solvencyPreCheck.getPayerAccount(storeFactory, previousResult.payer());
final var payerKey = payer.key();

// expand all keys
final var expanded = new HashSet<ExpandedSignaturePair>();
signatureExpander.expand(sigPairs, expanded);
if (payerKey != null && !isHollow(payer)) {
signatureExpander.expand(payerKey, sigPairs, expanded);
} else if (isHollow(payer)) {
context.requireSignatureForHollowAccount(payer);
}
signatureExpander.expand(currentRequiredNonPayerKeys, sigPairs, expanded);
signatureExpander.expand(currentOptionalNonPayerKeys, sigPairs, expanded);

// remove all keys that were already verified
for (final var it = expanded.iterator(); it.hasNext(); ) {
final var entry = it.next();
final var oldVerification = previousResult.verificationResults().get(entry.key());
if (oldVerification != null) {
verifications.put(oldVerification.key(), oldVerification);
it.remove();
signatureExpander.expand(currentRequiredNonPayerKeys, sigPairs, expanded);
signatureExpander.expand(currentOptionalNonPayerKeys, sigPairs, expanded);

// remove all keys that were already verified
for (final var it = expanded.iterator(); it.hasNext(); ) {
final var entry = it.next();
final var oldVerification = previousResult.verificationResults().get(entry.key());
if (oldVerification != null) {
verifications.put(oldVerification.key(), oldVerification);
it.remove();
}
}
}

// start verification for remaining keys
if (!expanded.isEmpty()) {
verifications.putAll(signatureVerifier.verify(signedBytes, expanded));
}
// start verification for remaining keys
if (!expanded.isEmpty()) {
verifications.putAll(signatureVerifier.verify(signedBytes, expanded));
}

return new PreHandleResult(
previousResult.payer(),
payerKey,
previousResult.status(),
previousResult.responseCode(),
previousResult.txInfo(),
context.requiredNonPayerKeys(),
context.requiredHollowAccounts(),
verifications,
previousResult.innerResult(),
previousResult.configVersion());
return new PreHandleResult(
previousResult.payer(),
payerKey,
previousResult.status(),
previousResult.responseCode(),
previousResult.txInfo(),
context.requiredNonPayerKeys(),
context.optionalNonPayerKeys(),
context.requiredHollowAccounts(),
verifications,
previousResult.innerResult(),
previousResult.configVersion());
} catch (final PreCheckException e) {
return previousResult;
}
}

/**
* Checks if any of the keys changed from previous result to current result.
* Only if keys changed we need to re-expand and re-verify the signatures.
*
* @param previousResults previous result from signature verification
* @param previousResult previous pre-handle result
* @param context current context
* @return true if any of the keys changed
*/
private boolean haveKeyChanges(
final Map<Key, SignatureVerificationFuture> previousResults, final PreHandleContextImpl context) {
private boolean haveKeyChanges(final PreHandleResult previousResult, final PreHandleContextImpl context) {
final var currentRequiredNonPayerKeys = context.requiredNonPayerKeys();
final var currentOptionalNonPayerKeys = context.optionalNonPayerKeys();
final var currentPayerKey = context.payerKey();

// keys from previous pre-handle result
final var previousResultRequiredKeys = previousResult.getRequiredKeys();
final var previousResultOptionalKeys = previousResult.getOptionalKeys();
final var previousResultPayerKey = previousResult.getPayerKey();

for (final var key : currentRequiredNonPayerKeys) {
if (!previousResults.containsKey(key)) {
if (!previousResultRequiredKeys.contains(key)) {
return true;
}
}
for (final var key : currentOptionalNonPayerKeys) {
if (!previousResults.containsKey(key)) {
if (!previousResultOptionalKeys.contains(key)) {
return true;
}
}
return !previousResults.containsKey(currentPayerKey);
return !previousResultPayerKey.equals(currentPayerKey);
}
}
4 changes: 3 additions & 1 deletion .../java/com/hedera/node/app/workflows/handle/record/SingleTransactionRecordBuilderImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
import com.hedera.node.app.service.token.records.CryptoCreateRecordBuilder;
import com.hedera.node.app.service.token.records.CryptoDeleteRecordBuilder;
import com.hedera.node.app.service.token.records.CryptoTransferRecordBuilder;
import com.hedera.node.app.service.token.records.CryptoUpdateRecordBuilder;
import com.hedera.node.app.service.token.records.GenesisAccountRecordBuilder;
import com.hedera.node.app.service.token.records.NodeStakeUpdateRecordBuilder;
import com.hedera.node.app.service.token.records.TokenAccountWipeRecordBuilder;
Expand Down Expand Up @@ -126,7 +127,8 @@ public class SingleTransactionRecordBuilderImpl
ContractDeleteRecordBuilder,
GenesisAccountRecordBuilder,
GasFeeRecordBuilder,
TokenAccountWipeRecordBuilder {
TokenAccountWipeRecordBuilder,
CryptoUpdateRecordBuilder {
private static final Comparator<TokenAssociation> TOKEN_ASSOCIATION_COMPARATOR =
Comparator.<TokenAssociation>comparingLong(a -> a.tokenId().tokenNum())
.thenComparingLong(a -> a.accountIdOrThrow().accountNum());
Expand Down
Loading

0 comments on commit 791ab23

Please sign in to comment.