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

EIP-210: Fixes for blockhash contracts #1094

Merged
merged 2 commits into from
Oct 15, 2018

Conversation

chfast
Copy link
Member

@chfast chfast commented May 18, 2018

  1. Fix blockhash contract for block number 1
    This fixes the issue in the contract when executed from SYSTEM account at block 1. Then the number of the previous block is 0 and the contract executes the loop forever.

  2. Correctly handle argument with negative value
    Fix fixes the issue in the blockhash contract where the negative value of the argument is not properly handled.

This fixes the issue in the contract when executed from SYSTEM account at block 1. Then the number of the previous block is 0 and the contract executes the loop forever.
Fix fixes the issue in the blockhash contract where the negative value of the argument is not properly handled.
@eip-automerger
Copy link

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

  • EIP 210 requires approval from one of (@vbuterin)

@chfast
Copy link
Member Author

chfast commented May 18, 2018

I'm not sure I extracted runtime code correctly. @chriseth @axic can you take a look?

@holiman
Copy link
Contributor

holiman commented May 18, 2018

That blocknum 1 issue is actually very important, because otherwise it'll produce very weird results in most test scenarios

@gumb0
Copy link
Member

gumb0 commented May 18, 2018

We still don't have any tests for the contract code itself, do we?

@chfast
Copy link
Member Author

chfast commented May 18, 2018

We do, but for old version of the contract: #641. When migrating to new code I discovered this block 1 bug.

@chfast
Copy link
Member Author

chfast commented May 18, 2018

Because these 2 bugs are quite obvious, I'd like to fix them first before progressing with tests.

@axic
Copy link
Member

axic commented Sep 20, 2018

I'm not sure I extracted runtime code correctly. @chriseth @axic can you take a look?

This seems to be Vyper so I am not sure, but it looks okay.

Can this be merged?

@Arachnid Arachnid merged commit e58c4d5 into ethereum:master Oct 15, 2018
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