Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace MessageRunMode with a struct #373

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

magicxyyz
Copy link
Contributor

@magicxyyz magicxyyz commented Nov 8, 2024

This PR:

  • replaces MessageRunMode enum with MessageRunContext struct, that apart from run mode now contains wasm cache tag and wasm targets list
  • Refactors passing of wasm targets and wasm cache tag, so they are no longer wired through cachingDB and ethdb.Database

Resolves NIT-2827

@cla-bot cla-bot bot added the s CLA signed label Nov 8, 2024
@magicxyyz magicxyyz marked this pull request as ready for review November 15, 2024 15:05
@@ -1167,19 +1167,19 @@ func doCall(ctx context.Context, b Backend, args TransactionArgs, state *state.S
}

// Arbitrum: a tx can schedule another (see retryables)
result, err = runScheduledTxes(ctx, b, state, header, blockCtx, core.MessageGasEstimationMode, result)
result, err = runScheduledTxes(ctx, b, state, header, blockCtx, core.NewMessageGasEstimationContext(), result) // TODO: why do we use GasEstimationMode here?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flagging this TODO question

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PlasmaPower it seems that here we set it to GasEstimationMode because in the function runScheduledTxes (just below this one) we only run scheduled Txes for gas estimation mode.. but I don't understand that condition itself (currently, we call runScheduledTxes from two places I found, both set runmode argument to GasEstimation so this condition isn't really doing anything)

Copy link
Contributor Author

@magicxyyz magicxyyz Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runScheduledTxes was added in: 018bd54

it addressed following comment: #304 (comment)

this is how it looked like before:

// Arbitrum: a tx can schedule another (see retryables)
scheduled := result.ScheduledTxes
for runMode == core.MessageGasEstimationMode && len(scheduled) > 0 {
// This will panic if the scheduled tx is signed, but we only schedule unsigned ones
msg, err := core.TransactionToMessage(scheduled[0], types.NewArbitrumSigner(nil), header.BaseFee)
if err != nil {
return nil, err
}
// The scheduling transaction will "use" all of the gas available to it,
// but it's really just passing it on to the scheduled tx, so we subtract it out here.
if result.UsedGas >= msg.GasLimit {
result.UsedGas -= msg.GasLimit
} else {
log.Warn("Scheduling tx used less gas than scheduled tx has available", "usedGas", result.UsedGas, "scheduledGas", msg.GasLimit)
result.UsedGas = 0
}
msg.TxRunMode = runMode
// make a new EVM for the scheduled Tx (an EVM must never be reused)
evm := b.GetEVM(ctx, msg, state, header, &vm.Config{NoBaseFee: true}, &blockCtx)
go func() {
<-ctx.Done()
evm.Cancel()
}()
scheduledTxResult, err := core.ApplyMessage(evm, msg, gp)
if err != nil {
return nil, err // Bail out
}
if err := state.Error(); err != nil {
return nil, err
}
if scheduledTxResult.Failed() {
return scheduledTxResult, nil
}
// Add back in any gas used by the scheduled transaction.
result.UsedGas += scheduledTxResult.UsedGas
scheduled = append(scheduled[1:], scheduledTxResult.ScheduledTxes...)
}
return result, nil

Copy link
Contributor Author

@magicxyyz magicxyyz Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I think we need to just pass runCtx here, instead of overwriting it with gas estimation context

wasmTargets []rawdb.WasmTarget
}

func NewMessageCommitContext(wasmTargets []rawdb.WasmTarget) *MessageRunContext {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we always need loclaTarget.
Maybe make the argument extraWasmTargets, and then the function adds local if not already in there, and in most invocations you can just pass nil?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another option is to special case len(wasmTargets) == 0 and in that case add localTarget

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

went with the second option, if len(wasmTargets) == 0 then wasmTargets = [localTarget]

cmd/evm/internal/t8ntool/execution.go Outdated Show resolved Hide resolved
@magicxyyz magicxyyz requested a review from tsahee January 13, 2025 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s CLA signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants