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

Make sure selfDestruct test expects destroyed account to not exist at the end of the transaction #5

Merged
merged 1 commit into from
Mar 11, 2018

Conversation

jwasinger
Copy link

No description provided.

@hugo-dc
Copy link
Member

hugo-dc commented Jan 23, 2018

This change looks good, but I have some questions related to this test case.

At the beginning both accounts has a balance of 100000000000, so when the second account destruct itself the total balance should be passed to the caller, I would expect 200000000000 in the first account or 200000000000 - 21000 (Tx cost) = 199999979000, but instead we are getting 199999989500 (200000000000 - 10500), I'm not sure what those 10500 means and if it is ok to not reduce the transaction cost, also I see that in the post state we have a new account:

https://github.com/ewasm/tests/blob/self-destruct/GeneralStateTests/stEWASMTests/selfDestruct.json#L29-L34

And by the way the balance of that new account is 10500.

Copy link
Member

@lrettig lrettig left a comment

Choose a reason for hiding this comment

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

Change looks good.

Agree with @hugo-dc, I would like to understand the gas semantics here.

One more thing: should we test this behavior of selfDestruct?

Note: the contract shall halt execution after this call.

@jwasinger
Copy link
Author

Yeah @lrettig that is a good idea. If we could add some code after the selfdestruct (maybe a call to another account where the executed code causes a state change), that would be great.

@axic
Copy link
Member

axic commented Jan 24, 2018

Maybe the gas issues are due to this: ethereum/aleth#4731 ?

Copy link
Member

@hugo-dc hugo-dc left a comment

Choose a reason for hiding this comment

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

I think we should merge this change, and document the gas issues in @jwasinger #7

Copy link
Member

@lrettig lrettig left a comment

Choose a reason for hiding this comment

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

add some code after the selfdestruct (maybe a call to another account where the executed code causes a state change)

…t self destructs does not exist after the transaction
@hugo-dc hugo-dc merged commit eaba2db into wasm-tests Mar 11, 2018
@axic axic deleted the self-destruct branch March 11, 2018 23:23
gumb0 pushed a commit that referenced this pull request Apr 29, 2021
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.

None yet

4 participants