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

Validate the argument range of getBlockHash #336

Closed
wants to merge 1 commit into from
Closed

Conversation

axic
Copy link
Member

@axic axic commented Aug 19, 2018

No description provided.

@@ -261,6 +261,8 @@ namespace hera {

HERA_DEBUG << "getBlockHash " << hex << number << " " << resultOffset << dec << "\n";

ensureCondition(number >= 0 && number <= 256, ArgumentOutOfRange, "getBlockHash only accepts a block number between 0 and 256");
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's not correct. It accepts range of (current_block_number - 1, current_block_number - 257) .

Copy link
Member Author

Choose a reason for hiding this comment

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

Btw, we could also change getblockhash in EVMC to return an error code for reason of failure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@axic
Copy link
Member Author

axic commented Aug 19, 2018

Actually this PR doesn't make sense. We have getBlockHash to return an error code, that's the way it should return if it is out of range.

The ArgumentOutOfRange exception makes more sense of invalid gas value supplied to useGas and call.

@axic axic closed this Aug 19, 2018
@axic axic deleted the blockhash-argrange branch August 19, 2018 21:00
@axic axic removed the in progress label Aug 19, 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.

None yet

2 participants