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 all commits
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
10 changes: 8 additions & 2 deletions ethcore/src/machine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,13 @@ impl EthereumMachine {
}

impl EthereumMachine {
/// Execute a call as the system address.
/// Execute a call as the system address. Block environment information passed to the
/// VM is modified to have its gas limit bounded at the upper limit of possible used
/// gases including this system call, capped at the maximum value able to be
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure if I understand this phrase fully. Can you rephrase the part around "…upper limit of possible used gases including this system call"? Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you explain more which parts looks confusing? Would help a lot for me to rephrase it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Absolutely. And remember I'm new to much of this so it might just be me being ignorant. To me it's "upper limit of possible used gases" that is hard to understand.
Maybe something like: "This call executes a call as the system address and modifies the VM's gas limits to be bound to the upper limit, given by the gas amounts provided in the call. Gas is capped to the maximum value represented by a U256" – is that close to what you meant to say?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm TBH it feels a little bit confusing. I think maybe it's just that we're using different terminologies here.

So I think what I'm trying to say there is as follows:

  • At the given moment when this system transaction is being executed, the block contains a "used gas" information. This is accumulated from transactions executed before this system transaction.
  • Each transaction has a gas limit. This gas limit also defines the possible upper bound of used gas for that particular transaction.
  • In VM execution, you can query block's gas limit. System transaction might have a gas limit way beyond a block's gas limit. So in that line, what it tries to do is to make that VM-query-able value valid again for the particular system transaction.
    • We first try to add the block's actual used gas with the system transaction's gas limit. If in the worst case, the system transaction is executed using up all its gas limit, we're still having a valid VM-query-able value at that moment.
    • And at the same time, this value needs to be capped at U256::max_value(). If it's saturated, it apparently doesn't comply with the worst case gas property. However, no execution on the consensus level will break -- this particular value is not referenced back from the VM. So if the contract does not need it, it can be ignored. If the contract needs it, then it just needs to understand this consensus-level behavior correctly. For block reward contract, this value will actually always be U256::max_value() so it doesn't matter at all.

Mixing terms of gas limit and used gas can be confusing, so that's how I got the phrase "upper limit of possible used gases including this system call". So using your sentence, it would be "modifies the block-environment-block-gas-limit (this is a different term than VM's gas limit) to be bound to the upper limit, given by the block used gas at the moment the system transaction is executed, and the gas limit provided to the system call".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I think I understand a little better now, thank you for taking the time. I like your suggested change!

/// represented by U256. This system call modifies the block state, but discards other
/// information. If suicides, logs or refunds happen within the system call, they
/// will not be executed or recorded. Gas used by this system call will not be counted
/// on the block.
pub fn execute_as_system(
&self,
block: &mut ExecutedBlock,
Expand All @@ -132,7 +138,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