Skip to content

core/vm: add a failed flag to detect tx execution status #14637

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

Closed
wants to merge 9 commits into from

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Jun 16, 2017

Motivation for this PR:

Now the gas is estimated through the gas is consumed all. But this method does not apply to all scenes. e.g. in the frontier contract deploys can fail without consuming all gas due to have not enough gas to store contract code.

So a failed flag returned by vm is useful to detect whether an error occurred during the execution of the transaction. It can help to estimate gas accurately.

@fjl fjl requested review from fjl and holiman June 21, 2017 12:51
@fjl fjl added this to the 1.6.7 milestone Jun 21, 2017
which is used to indicate whether state revert happen during
state transition.

if child's revert flag been set, it will also affect its parent
recursively
detect whether tx is reverted during state transition and save to receipt if
so.
@rjl493456442 rjl493456442 force-pushed the master branch 3 times, most recently from 37734c1 to f19d90f Compare June 28, 2017 04:39
@rjl493456442 rjl493456442 changed the title core/types: add a new field reverted in receipt core/vm: add a failed flag to detect tx execution status Jun 28, 2017
@rjl493456442 rjl493456442 force-pushed the master branch 2 times, most recently from cb7d283 to af0c5ef Compare June 28, 2017 08:06
Motivation:
    Current problem with gas estimation is not all cases is correct
    using gas metrics, e.g. in the frontier contract deploys can fail without
    consuming all gas due to have not enough gas to store contract code.
    So a `failed` flag is useful which can detect the vm execution
    status.

In Addition:
    the `Reverted` flag has been removed, since it is too sensitive to
    insert addtional field to it.
Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

LGTM

@Arachnid
Copy link
Contributor

This is a really useful improvement - but wouldn't it be a lot more useful if it bubbled up the actual error, rather than just a boolean indicating an error occurred?

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM!
Would have liked to combine this code with some feature to allow developers to know the source of a failure: a pc and an address, but I guess that can be implemented later on.

@fjl fjl modified the milestones: 1.6.7, 1.7.0 Jul 17, 2017
@fjl
Copy link
Contributor

fjl commented Aug 24, 2017

This is implemented in #15014 and #15030.

@fjl fjl closed this Aug 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants