Merge #4200

4200: Simplify transaction preprocessing r=pattyshack a=pattyshack

Pause/Resume nested txn was a premature optimization.  Removing pause/resume, and reordering execution back to normal ordering simplify interim read set computation, and removes unnecessary assumptions.

Co-authored-by: Patrick Lee <[email protected]>

Author: bors[bot]

fvm/state/transaction_state.go

@@ -128,27 +128,6 @@ type NestedTransaction interface {
 		error,
 	)
 
-	// PauseNestedTransaction detaches the current nested transaction from the
-	// parent transaction, and returns the paused nested transaction state.
-	// The paused nested transaction may be resume via Resume.
-	//
-	// WARNING: Pause and Resume are intended for implementing continuation
-	// passing style behavior for the transaction executor, with the assumption
-	// that the states accessed prior to pausing remain valid after resumption.
-	// The paused nested transaction should not be reused across transactions.
-	// IT IS NOT SAFE TO PAUSE A NESTED TRANSACTION IN GENERAL SINCE THAT
-	// COULD LEAD TO PHANTOM READS.
-	PauseNestedTransaction(
-		expectedId NestedTransactionId,
-	) (
-		*ExecutionState,
-		error,
-	)
-
-	// ResumeNestedTransaction attaches the paused nested transaction (state)
-	// to the current transaction.
-	ResumeNestedTransaction(pausedState *ExecutionState)
-
 	// AttachAndCommitNestedTransaction commits the changes from the cached
 	// nested transaction execution snapshot to the current (nested)
 	// transaction.
@@ -373,30 +352,6 @@ func (txnState *transactionState) CommitParseRestrictedNestedTransaction(
 	return txnState.mergeIntoParent()
 }
 
-func (txnState *transactionState) PauseNestedTransaction(
-	expectedId NestedTransactionId,
-) (
-	*ExecutionState,
-	error,
-) {
-	if !txnState.IsCurrent(expectedId) {
-		return nil, fmt.Errorf(
-			"cannot pause unexpected nested transaction: id mismatch",
-		)
-	}
-
-	if txnState.IsParseRestricted() {
-		return nil, fmt.Errorf(
-			"cannot Pause parse restricted nested transaction")
-	}
-
-	return txnState.pop("pause")
-}
-
-func (txnState *transactionState) ResumeNestedTransaction(pausedState *ExecutionState) {
-	txnState.push(pausedState, nil)
-}
-
 func (txnState *transactionState) AttachAndCommitNestedTransaction(
 	cachedSnapshot *ExecutionSnapshot,
 ) error {

fvm/state/transaction_state_test.go

@@ -480,50 +480,6 @@ func TestParseRestrictedCannotCommitLocationMismatch(t *testing.T) {
 	require.True(t, txn.IsCurrent(id))
 }
 
-func TestPauseAndResume(t *testing.T) {
-	txn := newTestTransactionState()
-
-	key1 := flow.NewRegisterID("addr", "key")
-	key2 := flow.NewRegisterID("addr2", "key2")
-
-	val, err := txn.Get(key1)
-	require.NoError(t, err)
-	require.Nil(t, val)
-
-	id1, err := txn.BeginNestedTransaction()
-	require.NoError(t, err)
-
-	err = txn.Set(key1, createByteArray(2))
-	require.NoError(t, err)
-
-	val, err = txn.Get(key1)
-	require.NoError(t, err)
-	require.NotNil(t, val)
-
-	pausedState, err := txn.PauseNestedTransaction(id1)
-	require.NoError(t, err)
-
-	val, err = txn.Get(key1)
-	require.NoError(t, err)
-	require.Nil(t, val)
-
-	txn.ResumeNestedTransaction(pausedState)
-
-	val, err = txn.Get(key1)
-	require.NoError(t, err)
-	require.NotNil(t, val)
-
-	err = txn.Set(key2, createByteArray(2))
-	require.NoError(t, err)
-
-	_, err = txn.CommitNestedTransaction(id1)
-	require.NoError(t, err)
-
-	val, err = txn.Get(key2)
-	require.NoError(t, err)
-	require.NotNil(t, val)
-}
-
 func TestFinalizeMainTransactionFailWithUnexpectedNestedTransactions(
 	t *testing.T,
 ) {

fvm/transactionInvoker.go

@@ -60,8 +60,8 @@ type transactionExecutor struct {
 
 	errs *errors.ErrorsCollector
 
-	nestedTxnId state.NestedTransactionId
-	pausedState *state.ExecutionState
+	startedTransactionBodyExecution bool
+	nestedTxnId                     state.NestedTransactionId
 
 	cadenceRuntime  *reusableRuntime.ReusableCadenceRuntime
 	txnBodyExecutor runtime.Executor
@@ -91,13 +91,14 @@ func newTransactionExecutor(
 		TransactionVerifier: TransactionVerifier{
 			VerificationConcurrency: 4,
 		},
-		ctx:            ctx,
-		proc:           proc,
-		txnState:       txnState,
-		span:           span,
-		env:            env,
-		errs:           errors.NewErrorsCollector(),
-		cadenceRuntime: env.BorrowCadenceRuntime(),
+		ctx:                             ctx,
+		proc:                            proc,
+		txnState:                        txnState,
+		span:                            span,
+		env:                             env,
+		errs:                            errors.NewErrorsCollector(),
+		startedTransactionBodyExecution: false,
+		cadenceRuntime:                  env.BorrowCadenceRuntime(),
 	}
 }
 
@@ -131,22 +132,53 @@ func (executor *transactionExecutor) handleError(
 }
 
 func (executor *transactionExecutor) Preprocess() error {
+	return executor.handleError(executor.preprocess(), "preprocess")
+}
+
+func (executor *transactionExecutor) Execute() error {
+	return executor.handleError(executor.execute(), "executing")
+}
+
+func (executor *transactionExecutor) preprocess() error {
+	if executor.AuthorizationChecksEnabled {
+		err := executor.CheckAuthorization(
+			executor.ctx.TracerSpan,
+			executor.proc,
+			executor.txnState,
+			executor.AccountKeyWeightThreshold)
+		if err != nil {
+			executor.errs.Collect(err)
+			return executor.errs.ErrorOrNil()
+		}
+	}
+
+	if executor.SequenceNumberCheckAndIncrementEnabled {
+		err := executor.CheckAndIncrementSequenceNumber(
+			executor.ctx.TracerSpan,
+			executor.proc,
+			executor.txnState)
+		if err != nil {
+			executor.errs.Collect(err)
+			return executor.errs.ErrorOrNil()
+		}
+	}
+
 	if !executor.TransactionBodyExecutionEnabled {
 		return nil
 	}
 
-	err := executor.PreprocessTransactionBody()
-	return executor.handleError(err, "preprocessing")
-}
+	executor.errs.Collect(executor.preprocessTransactionBody())
+	if executor.errs.CollectedFailure() {
+		return executor.errs.ErrorOrNil()
+	}
 
-func (executor *transactionExecutor) Execute() error {
-	return executor.handleError(executor.execute(), "executing")
+	return nil
 }
 
-// PreprocessTransactionBody preprocess parts of a transaction body that are
+// preprocessTransactionBody preprocess parts of a transaction body that are
 // infrequently modified and are expensive to compute.  For now this includes
 // reading meter parameter overrides and parsing programs.
-func (executor *transactionExecutor) PreprocessTransactionBody() error {
+func (executor *transactionExecutor) preprocessTransactionBody() error {
 	meterParams, err := getBodyMeterParameters(
 		executor.ctx,
 		executor.proc,
@@ -160,6 +192,7 @@ func (executor *transactionExecutor) PreprocessTransactionBody() error {
 	if err != nil {
 		return err
 	}
+	executor.startedTransactionBodyExecution = true
 	executor.nestedTxnId = txnId
 
 	executor.txnBodyExecutor = executor.cadenceRuntime.NewTransactionExecutor(
@@ -173,93 +206,23 @@ func (executor *transactionExecutor) PreprocessTransactionBody() error {
 	// by the transaction body.
 	err = executor.txnBodyExecutor.Preprocess()
 	if err != nil {
-		executor.errs.Collect(
-			fmt.Errorf(
-				"transaction preprocess failed: %w",
-				err))
-
-		// We shouldn't early exit on non-failure since we need to deduct fees.
-		if executor.errs.CollectedFailure() {
-			return executor.errs.ErrorOrNil()
-		}
-
-		// NOTE: We need to restart the nested transaction in order to pause
-		// for fees deduction.
-		err = executor.txnState.RestartNestedTransaction(txnId)
-		if err != nil {
-			return err
-		}
-	}
-
-	// Pause the transaction body's nested transaction in order to interleave
-	// auth and seq num checks.
-	pausedState, err := executor.txnState.PauseNestedTransaction(txnId)
-	if err != nil {
-		return err
+		return fmt.Errorf(
+			"transaction preprocess failed: %w",
+			err)
 	}
-	executor.pausedState = pausedState
 
 	return nil
 }
 
 func (executor *transactionExecutor) execute() error {
-	if executor.AuthorizationChecksEnabled {
-		err := executor.CheckAuthorization(
-			executor.ctx.TracerSpan,
-			executor.proc,
-			executor.txnState,
-			executor.AccountKeyWeightThreshold)
-		if err != nil {
-			executor.errs.Collect(err)
-			executor.errs.Collect(executor.abortPreprocessed())
-			return executor.errs.ErrorOrNil()
-		}
+	if !executor.startedTransactionBodyExecution {
+		return executor.errs.ErrorOrNil()
 	}
 
-	if executor.SequenceNumberCheckAndIncrementEnabled {
-		err := executor.CheckAndIncrementSequenceNumber(
-			executor.ctx.TracerSpan,
-			executor.proc,
-			executor.txnState)
-		if err != nil {
-			executor.errs.Collect(err)
-			executor.errs.Collect(executor.abortPreprocessed())
-			return executor.errs.ErrorOrNil()
-		}
-	}
-
-	if executor.TransactionBodyExecutionEnabled {
-		err := executor.ExecuteTransactionBody()
-		if err != nil {
-			return err
-		}
-	}
-
-	return nil
-}
-
-func (executor *transactionExecutor) abortPreprocessed() error {
-	if !executor.TransactionBodyExecutionEnabled {
-		return nil
-	}
-
-	executor.txnState.ResumeNestedTransaction(executor.pausedState)
-
-	// There shouldn't be any update, but drop all updates just in case.
-	err := executor.txnState.RestartNestedTransaction(executor.nestedTxnId)
-	if err != nil {
-		return err
-	}
-
-	// We need to commit the aborted state unconditionally to include
-	// the touched registers in the execution receipt.
-	_, err = executor.txnState.CommitNestedTransaction(executor.nestedTxnId)
-	return err
+	return executor.ExecuteTransactionBody()
 }
 
 func (executor *transactionExecutor) ExecuteTransactionBody() error {
-	executor.txnState.ResumeNestedTransaction(executor.pausedState)
-
 	var invalidator derived.TransactionInvalidator
 	if !executor.errs.CollectedError() {