Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Fix BlockReward contract "arithmetic operation overflow" #8611

Merged
merged 3 commits into from
May 14, 2018
Merged

Fix BlockReward contract "arithmetic operation overflow" #8611

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3bc8a18
Fix BlockReward contract "arithmetic operation overflow"
sorpaas May 12, 2018
ea88879
Add docs on how execute_as_system works
sorpaas May 14, 2018
e158f17
Fix typo
sorpaas May 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ethcore/src/machine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ impl EthereumMachine {
) -> Result<Vec<u8>, Error> {
let env_info = {
let mut env_info = block.env_info();
env_info.gas_limit = env_info.gas_used + gas;
env_info.gas_limit = env_info.gas_used.saturating_add(gas);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be great to document (and/or add a test for it) what happens in case of an overflow with this change. It fixes the panic, but is the new non-panicky behaviour legal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added more docs on how execute_as_system function actually works. I think this is mostly a definition issue -- system transactions are not normal transactions after all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sorpaas talking further about this with @ascjones we were wondering what the reason is this method accepts a gas param at all: as the system calls are "special" and can use however much gas they want, then perhaps this line could just become env_info.gas_limit = U256::max_value()?

cc @andresilva

Copy link
Collaborator Author

@sorpaas sorpaas May 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It does make sense. Sometimes we want to limit the gas can be used by system transactions. Like BLOCKHASH contract (https://eips.ethereum.org/EIPS/eip-210).

Copy link
Contributor

Choose a reason for hiding this comment

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

@dvdplm It's typically used for consensus-level transactions, e.g. finalizing changes to ValidatorSet, block reward contract transactions. In these cases we don't really need to cap the execution using gas since we assume the contracts are well behaved. Although we do provide U256::max_value() in most cases, for EIP210 we set the gas limit defined in the spec. So I think we should keep the gas parameter and don't assume it will always be U256::max_value().

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sorpaas @andresilva ok, I'll trust you on that. I am still not sure I fully understand what happens when the addition overflows though: wouldn't that risk making the gas_limit way too low causing execution to halt unexpectedly?
Again, thank you for taking the time to explain things! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that what the change to saturating_add in this PR is for?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ascjones you are right ofc, I was totally confused about what saturating_add actually does. I thought it wrapped around but of course it doesn't. My bad, please ignore me.

env_info
};

Expand Down