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

transactionFromNotExistingAccount does not reflect YellowPaper #503

Closed
germsvel opened this issue Sep 7, 2018 · 12 comments
Closed

transactionFromNotExistingAccount does not reflect YellowPaper #503

germsvel opened this issue Sep 7, 2018 · 12 comments

Comments

@germsvel
Copy link

germsvel commented Sep 7, 2018

I was looking through the transactionFromNotExistingAccount test, and it seems to me like it does not accurately reflect the Yellow Paper.

This test seems to execute a transaction with a valid signature (and thus we can get a sender address), but for which the sender's account does not exist in the blockchain db.

The transaction passes several of the requirements for a valid transaction specified under Section 6.2 of the Yellow Paper (currently eq. 66) by having a gas price of 0 and a valueof 0, but it does not seem to satisfy the requirement that S(T) != Null.

Since the equation numbers are liable to change, I include a copy of the equations here:

screen shot 2018-09-07 at 11 39 05 am

@holiman
Copy link
Contributor

holiman commented Sep 7, 2018

True. Would be nice to know is this has ever occurred (I suspect it may, we had brief no-cost-blocks a while back).

If it hasn't occurred, I think it's a pita to change/hardfork to make it different in the future.

If it hasn't occurred, we should change the test and retroactively consider it as always having been in place.

Just an opinion...

germsvel pushed a commit to mana-ethereum/mana that referenced this issue Sep 7, 2018
Why:

We should be using `Transaction.execute_with_validation` in our
blockchain block instead of skipping validations with
`Transaction.execute`. We were skipping these previously because it was
causing some blockchain common tests to fail but I think we want to
check our transactions are valid to get all the tests passing
eventually.

This PR:

Uses `Transaction.execute_with_validation` in
`apps/blockchain/lib/blockchain/block.ex` and makes the required changes
to our existing tests. Due to
ethereum/tests#503 we needed to introduce a
`check_account_validity` to handle cases where the sender account
doesn't exist. This removes the invalid case of `missing_account` as if
the account is missing but there is a cost for the transaction, the
transaction will still be invalid but for a different reason (such as
`over_gas_limit`)

Co-authored-by: Matthew Sumner <matt.m.sumner@gmail.com>
@germsvel
Copy link
Author

@holiman am I understanding your comment correctly?

Are you saying that:

  • if it has occurred, we should keep the test as is to document reality and make sure clients can sync from genesis.
  • if it has not occurred, we should change the test so that it reflects the S(T) != Null contraint.

If those are your points, I definitely agree. I think that makes sense.

@winsvega
Copy link
Collaborator

Looks like we agreed that if majority of the clients pass this test as it is right now, we should change the YP.

@holiman
Copy link
Contributor

holiman commented Sep 11, 2018

If those are your points, I definitely agree. I think that makes sense.

Yes those were my points :)

@germsvel
Copy link
Author

👍 changing the YP makes sense to me. Does anybody know how we go about doing that (open an issue and point back here)? And should I close this issue?

@winsvega
Copy link
Collaborator

winsvega commented Oct 11, 2018

sup with YP change? @holiman
@pirapira used to update the YP. not sure where is the latest repo now

@holiman
Copy link
Contributor

holiman commented Oct 11, 2018

I have no idea -- have never touched the YP .. :/ We need a new @pirapira

@germsvel
Copy link
Author

Would opening an issue in https://github.com/ethereum/yellowpaper be a good step? I'm guessing that's the first step in making that change?

@winsvega
Copy link
Collaborator

yes. I remember there was issue about whos yellow paper is more yellow. Gavin Wood had some authorship issues with us. I guess ethereum/yellowpaper is the latest version. it should also be autocompiled by some script into a webpage.

@winsvega
Copy link
Collaborator

winsvega commented Sep 1, 2019

any updates?

@germsvel
Copy link
Author

germsvel commented Sep 3, 2019

@winsvega I never opened an issue on https://github.com/ethereum/yellowpaper for this. I think I misread your answer back in October of last year, and I thought we weren't sure what would be the best place to fix this. Looking back at it now, I see the "yes" there.

I haven't dealt with ethereum in a little while. @ayrat555 would opening an issue for the yellow paper be something you'd be willing to do? If not, I can try to open one based on what I have described in this issue.

@winsvega
Copy link
Collaborator

winsvega commented Sep 3, 2019

I am quite busy with tests right now. if there is smth we should do about this issue please tell, otherwise could just close.

@germsvel germsvel closed this as completed Sep 3, 2019
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

No branches or pull requests

3 participants