Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

Problem: state transition code is duplicated #672

Closed
yihuang opened this issue Oct 14, 2021 · 0 comments · Fixed by #674
Closed

Problem: state transition code is duplicated #672

yihuang opened this issue Oct 14, 2021 · 0 comments · Fixed by #674

Comments

@yihuang
Copy link
Contributor

yihuang commented Oct 14, 2021

The key thing here is to make ApplyMessage more self-contained and easier to use.

ApplyMessage is called in several sites:

  • ApplyTransaction, tx execution
    • the fee is already deducted in ante handler, so should do gas refund after message executed
    • should commit the context stack.
  • EthCall/EthEstimateGas query handler
    • don't need to refund gas,
    • don't need to commit context stack
  • ApplyNativeMessage, called by native modules to execute evm message
    • no need to refund gas
    • gas is accumulated naturally by cosmos-sdk
    • should commit context stack

The other thing is the context stack commit/flatten logic is pretty complicated, one way to simplify it is to keep the context stack logic inside the ApplyMessage, when ApplyMessage returns, the stack is either committed or discarded.

Proposal

Extract the common ground of the above scenarios, we should make the following changes to ApplyMessage:

  1. ApplyMessage only calculates the gas refund amount and adjusts the gasUsed, but don't do refund action, move that to ApplyTransaction.
  2. Move EVM construction into ApplyMessage, so the caller doesn't need to do it by themselves.
  3. Simplify the parameters of ApplyMessage, by encapsulating the common ones into a struct.
  4. Always commit/revert the context stack when ApplyMessage returns, so the caller doesn't need to worry about the context stack commit, flatten, revert stuff.
  5. Remove ApplyNativeMessage, since the implementation is so trivial.
// External interface to execute a message:

k.WithContext(ctx)
msg := build core.Message
res, err := k.ApplyMessage(msg, nil, true) // nil means use default tracer, true means commit the context stack.
if err != nil {
    return err
}
yihuang added a commit to yihuang/ethermint that referenced this issue Oct 22, 2021
Closes: evmos#672

Solution:
- move gas refund out from ApplyMessage
- move check into ApplyMessage
- move evm construction into ApplyMessage
- ensure context stack is clean after ApplyMessage return

fix unit tests

undo rename

add underflow check
fedekunze pushed a commit that referenced this issue Oct 22, 2021
* Problem: state transition code is duplicated

Closes: #672

Solution:
- move gas refund out from ApplyMessage
- move check into ApplyMessage
- move evm construction into ApplyMessage
- ensure context stack is clean after ApplyMessage return

fix unit tests

undo rename

add underflow check

* improve performance

- don't duplicate params loading
- passing EVMConfig around as pointer
yihuang added a commit to yihuang/ethermint that referenced this issue Nov 15, 2021
* Problem: state transition code is duplicated

Closes: evmos#672

Solution:
- move gas refund out from ApplyMessage
- move check into ApplyMessage
- move evm construction into ApplyMessage
- ensure context stack is clean after ApplyMessage return

fix unit tests

undo rename

add underflow check

* improve performance

- don't duplicate params loading
- passing EVMConfig around as pointer
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant