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

Fixed: some touched accounts were not verified for eip161 deletion #625

Merged
merged 3 commits into from
Dec 13, 2016

Conversation

zilm13
Copy link
Collaborator

@zilm13 zilm13 commented Dec 12, 2016

Let's clarify
ethereum/EIPs#161

An account changes state when:

  1. it is the target or refund of a SUICIDE operation for zero or more value;
  2. it is the source or destination of a CALL operation or message-call transaction transferring zero or more value;
  3. it is the source or newly-creation of a CREATE operation or contract-creation transaction endowing zero or more value;
  4. as the block author ("miner") it is recipient of block-rewards or transaction-fees of zero or more.

Looks like we could answer YES on all of this, but you'd better check too. So:

  • Do we now check and merge results of all cases?
  • Should we move touched accounts to ProgramResult?

@zilm13
Copy link
Collaborator Author

zilm13 commented Dec 12, 2016

@Nashatyrev should fix #622

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 57.131% when pulling ea856dd on fix/receipt-hash-622 into ee31a28 on develop.

@Nashatyrev
Copy link
Member

Nashatyrev commented Dec 12, 2016

Do we now check and merge results of all cases?

I added merge for regular call but forgot about create. Do you mean any other cases?

Should we move touched accounts to ProgramResult?

Yep probably. Also it is possible that we shouldn't add touched accounts in case of throw. But if this will work on live then it actually doesn't matter much, since all accounts are cleared now and this execution branch will not be executed anymore

@zilm13
Copy link
Collaborator Author

zilm13 commented Dec 12, 2016

I added merge for regular call but forgot about create. Do you mean any other cases?

Just asking you to check it too according to specs to clarify that we didn't forgot anything else.

Also it is possible that we shouldn't add touched accounts in case of throw.

Yeah, that's why I've asked about ProgramResult. So I will refactor it a bit now.

@zilm13
Copy link
Collaborator Author

zilm13 commented Dec 12, 2016

Please, check

@@ -465,6 +465,7 @@ public void createContract(DataWord value, DataWord memStart, DataWord memSize)
if (!byTestingSuite())
track.commit();
getResult().addDeleteAccounts(result.getDeleteAccounts());
getResult().addTouchAccounts(result.getTouchedAccounts());
Copy link
Member

Choose a reason for hiding this comment

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

M.b. make it a part of ProgramResult.merge() ?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 56.99% when pulling 0861487 on fix/receipt-hash-622 into ee31a28 on develop.

@zilm13
Copy link
Collaborator Author

zilm13 commented Dec 12, 2016

@Nashatyrev Updated to use merge

@Nashatyrev Nashatyrev merged commit b9f2ea7 into develop Dec 13, 2016
@zilm13 zilm13 deleted the fix/receipt-hash-622 branch December 13, 2016 08:00
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 this pull request may close these issues.

3 participants