From bdc74228b959af89c8c7f32673c812e3a030aa43 Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Wed, 9 Aug 2023 15:46:21 -0500 Subject: [PATCH] Try using top level changes to avoid env mutations --- miner/algo_common.go | 51 ++++++++++++++++------- miner/env_changes.go | 96 +++++++++++++++++++++++++++++++++++--------- 2 files changed, 114 insertions(+), 33 deletions(-) diff --git a/miner/algo_common.go b/miner/algo_common.go index 57aab284ab..f3e49cfc31 100644 --- a/miner/algo_common.go +++ b/miner/algo_common.go @@ -367,19 +367,31 @@ func BuildMultiTxSnapBlock( buildBlockErrors []error ) + changes, err := newEnvChanges(inputEnvironment) + if err != nil { + return nil, nil, err + } + opMap := map[bool]func() error{ + true: changes.env.state.MultiTxSnapshotRevert, + false: changes.env.state.MultiTxSnapshotCommit, + } + for { order := orders.Peek() if order == nil { break } - orderFailed = false - changes, err := newEnvChanges(inputEnvironment) - // if changes cannot be instantiated, return early - if err != nil { - log.Error("Failed to create changes", "err", err) + if err = changes.env.state.NewMultiTxSnapshot(); err != nil { return nil, nil, err } + orderFailed = false + //changes, err := newEnvChanges(inputEnvironment) + // if changes cannot be instantiated, return early + //if err != nil { + // log.Error("Failed to create changes", "err", err) + // return nil, nil, err + //} if tx := order.Tx(); tx != nil { _, skip, err := changes.commitTx(tx, chData) @@ -423,17 +435,26 @@ func BuildMultiTxSnapBlock( panic("unsupported order type found") } - if orderFailed { - if err = changes.discard(); err != nil { - log.Error("Failed to discard changes with multi-transaction snapshot", "err", err) - buildBlockErrors = append(buildBlockErrors, fmt.Errorf("failed to discard changes: %w", err)) - } - } else { - if err = changes.apply(); err != nil { - log.Error("Failed to apply changes with multi-transaction snapshot", "err", err) - buildBlockErrors = append(buildBlockErrors, fmt.Errorf("failed to apply changes: %w", err)) - } + if err = opMap[orderFailed](); err != nil { + log.Error("Failed to apply changes with multi-transaction snapshot", "err", err) + buildBlockErrors = append(buildBlockErrors, fmt.Errorf("failed to apply changes: %w", err)) } + //if orderFailed { + // if err = changes.discard(); err != nil { + // log.Error("Failed to discard changes with multi-transaction snapshot", "err", err) + // buildBlockErrors = append(buildBlockErrors, fmt.Errorf("failed to discard changes: %w", err)) + // } + //} else { + // if err = changes.apply(); err != nil { + // log.Error("Failed to apply changes with multi-transaction snapshot", "err", err) + // buildBlockErrors = append(buildBlockErrors, fmt.Errorf("failed to apply changes: %w", err)) + // } + //} + } + + if err = changes.apply(); err != nil { + log.Error("Failed to apply changes with multi-transaction snapshot", "err", err) + buildBlockErrors = append(buildBlockErrors, fmt.Errorf("failed to apply changes: %w", err)) } return usedBundles, usedSbundles, errors.Join(buildBlockErrors...) diff --git a/miner/env_changes.go b/miner/env_changes.go index fd85b39ada..ff2c02d0c3 100644 --- a/miner/env_changes.go +++ b/miner/env_changes.go @@ -120,6 +120,8 @@ func (c *envChanges) commitBundle(bundle *types.SimulatedBundle, chData chainDat ) for _, tx := range bundle.OriginalBundle.Txs { + gasUsed := c.usedGas + gasPool := new(core.GasPool).AddGas(c.gasPool.Gas()) txHash := tx.Hash() // TODO: Checks for base fee and dynamic fee txs should be moved to the transaction pool, // similar to mev-share bundles. See SBundlesPool.validateTx() for reference. @@ -140,36 +142,94 @@ func (c *envChanges) commitBundle(bundle *types.SimulatedBundle, chData chainDat break } } + if err := c.env.state.NewMultiTxSnapshot(); err != nil { + bundleErr = err + break + } receipt, _, err := c.commitTx(tx, chData) - - if err != nil { + switch err { + case nil: + switch { + case receipt == nil: + panic("receipt is nil") + case receipt.Status == types.ReceiptStatusFailed: + isRevertibleTx := bundle.OriginalBundle.RevertingHash(txHash) + // if drop enabled, and revertible tx has error on commit, we skip the transaction and continue with next one + if algoConf.DropRevertibleTxOnErr && isRevertibleTx { + log.Trace("Found error on commit for revertible tx, but discard on err is enabled so skipping.", + "tx", txHash, "err", err) + c.usedGas = gasUsed + c.gasPool = gasPool + if err := c.env.state.MultiTxSnapshotRevert(); err != nil { + panic(err) + } + } else { + bundleErr = errors.New("bundle tx revert") + } + case receipt.Status == types.ReceiptStatusSuccessful: + fallthrough + default: + if err := c.env.state.MultiTxSnapshotCommit(); err != nil { + panic(err) + } + } + default: isRevertibleTx := bundle.OriginalBundle.RevertingHash(txHash) // if drop enabled, and revertible tx has error on commit, we skip the transaction and continue with next one if algoConf.DropRevertibleTxOnErr && isRevertibleTx { log.Trace("Found error on commit for revertible tx, but discard on err is enabled so skipping.", "tx", txHash, "err", err) - continue - } - log.Trace("Bundle tx error", "bundle", bundle.OriginalBundle.Hash, "tx", txHash, "err", err) - bundleErr = err - break - } - - if receipt != nil { - if receipt.Status == types.ReceiptStatusFailed && !bundle.OriginalBundle.RevertingHash(txHash) { - // if transaction reverted and isn't specified as reverting hash, return error - log.Trace("Bundle tx failed", "bundle", bundle.OriginalBundle.Hash, "tx", txHash, "err", err) - bundleErr = errors.New("bundle tx revert") + c.usedGas = gasUsed + c.gasPool = gasPool + if err := c.env.state.MultiTxSnapshotRevert(); err != nil { + panic(err) + } + } else { + bundleErr = err } - } else { - // NOTE: The expectation is that a receipt is only nil if an error occurred. - // If there is no error but receipt is nil, there is likely a programming error. - bundleErr = errors.New("invalid receipt when no error occurred") } if bundleErr != nil { + if err := c.env.state.MultiTxSnapshotRevert(); err != nil { + panic(err) + } break } + + //if err != nil { + // isRevertibleTx := bundle.OriginalBundle.RevertingHash(txHash) + // // if drop enabled, and revertible tx has error on commit, we skip the transaction and continue with next one + // if algoConf.DropRevertibleTxOnErr && isRevertibleTx { + // log.Trace("Found error on commit for revertible tx, but discard on err is enabled so skipping.", + // "tx", txHash, "err", err) + // if err := c.env.state.MultiTxSnapshotRevert(); err != nil { + // panic(err) + // } + // continue + // } + // log.Trace("Bundle tx error", "bundle", bundle.OriginalBundle.Hash, "tx", txHash, "err", err) + // bundleErr = err + // break + //} + // + //if receipt != nil { + // if receipt.Status == types.ReceiptStatusFailed && !bundle.OriginalBundle.RevertingHash(txHash) { + // // if transaction reverted and isn't specified as reverting hash, return error + // log.Trace("Bundle tx failed", "bundle", bundle.OriginalBundle.Hash, "tx", txHash, "err", err) + // bundleErr = errors.New("bundle tx revert") + // } + //} else { + // // NOTE: The expectation is that a receipt is only nil if an error occurred. + // // If there is no error but receipt is nil, there is likely a programming error. + // bundleErr = errors.New("invalid receipt when no error occurred") + //} + // + //if bundleErr != nil { + // if err := c.env.state.MultiTxSnapshotRevert(); err != nil { + // panic(err) + // } + // break + //} } if bundleErr != nil {