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

Byzantium updates #5855

Merged
merged 28 commits into from
Sep 15, 2017
Merged

Byzantium updates #5855

merged 28 commits into from
Sep 15, 2017

Conversation

arkpar
Copy link
Collaborator

@arkpar arkpar commented Jun 16, 2017

Contains a number of small updates to byzantium EIPs.

@arkpar arkpar added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Jun 16, 2017
@rphmeier rphmeier added the M4-core ⛓ Core client code / Rust. label Jun 16, 2017
@arkpar
Copy link
Collaborator Author

arkpar commented Jun 21, 2017

cc @pirapira

@arkpar arkpar mentioned this pull request Jun 21, 2017
@pirapira
Copy link

pirapira commented Aug 9, 2017

Thank you.

@debris
Copy link
Collaborator

debris commented Aug 27, 2017

This pr hasn't been updated for > 28 days and I'm closing it as a part of pull request cleanup. If it's still relevant, please rebase and reopen it.

@arkpar arkpar mentioned this pull request Sep 8, 2017
12 tasks
@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. B0-patch F0-consensus 💣 Issue can lead to a consensus failure. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Sep 12, 2017
@arkpar arkpar changed the title Metropolis updates Byzantium updates Sep 12, 2017
@5chdn 5chdn added the P2-asap 🌊 No need to stop dead in your tracks, however issue should be addressed as soon as possible. label Sep 14, 2017
"out" : "0x",
"network" : "Fronier",
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo, Frontier

EIP158ToByzantiumAt5,
FrontierToHomesteadAt5,
HomesteadToDaoAt5,
HomesteadToEIP150At5
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does At5 mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Transition at block 5. This comes from json test definition.

},
vm::ContractCreateResult::Reverted(gas_left, _) => {
trace!(target: "wasm", "runtime: create contract reverted");
self.gas_counter = self.gas_limit - gas_left.low_u64();
Copy link
Collaborator

Choose a reason for hiding this comment

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

can it underflow?

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 assume it can't because the gas was checked previously. Created branch above follows the same assumption.

Copy link
Contributor

Choose a reason for hiding this comment

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

a debug assertion could be useful

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to be added by @NikVolf

@@ -323,6 +332,11 @@ impl<'a, 'b> Runtime<'a, 'b> {
self.memory.set(result_ptr, &result)?;
Ok(Some(0i32.into()))
},
vm::MessageCallResult::Reverted(gas_left, _) => {
self.gas_counter = self.gas_limit - gas_left.low_u64();
Copy link
Collaborator

Choose a reason for hiding this comment

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

can it underflow?

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 assume it can't because the gas was checked previously. Success branch above follows the same assumption.

Copy link
Contributor

@NikVolf NikVolf Sep 14, 2017

Choose a reason for hiding this comment

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

I think I will change it to .checked_sub().expect("reason")

@@ -111,6 +111,8 @@ pub struct Schedule {
pub have_return_data: bool,
/// Kill basic accounts below this balance if touched.
pub kill_dust: CleanDustMode,
/// Enable EIP-86 rules
pub eip86: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This property is always set to false. Is it intended?

Copy link
Collaborator Author

@arkpar arkpar Sep 14, 2017

Choose a reason for hiding this comment

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

Yes, it will be enabled in Constantinople

Ok((params.gas - cost, ReturnData::empty()))
Ok(FinalizationResult {
gas_left: params.gas - cost,
return_data: ReturnData::new(output.to_owned(), 0, output.len()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't it dangerous to copy the output (via to_owned)? iirc, we've always avoided that at any cost

Copy link
Collaborator Author

@arkpar arkpar Sep 14, 2017

Choose a reason for hiding this comment

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

There is no way around it. output here a slice into callers memory. The built-ins don't have their own evm memory and the result has to be kept around after the call in return data buffer.

@@ -260,6 +264,8 @@ fn do_json_test_for(vm_type: &VMType, json_data: &[u8]) -> Vec<String> {
fail_unless(Some(res.gas_left) == vm.gas_left.map(Into::into), "gas_left is incorrect");
let vm_output: Option<Vec<u8>> = vm.output.map(Into::into);
fail_unless(Some(output) == vm_output, "output is incorrect");
// TODO: verify logs
//fail_unless(Some(res.logs) == vm.logs, "logs are incorrect");
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about this TODO?

@@ -124,7 +126,7 @@ impl CommonParams {
schedule.have_static_call = block_number >= self.eip214_transition;
schedule.have_return_data = block_number >= self.eip211_transition;
if block_number >= self.eip210_transition {
schedule.blockhash_gas = 350;
schedule.blockhash_gas = 800;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The latter function is used only in some tests and the value should not really be set there. I'll remove it

let eip658 = env_info.number >= engine.params().eip658_transition;
let no_intermediate_commits = (env_info.number >= engine.params().eip98_transition
&& env_info.number >= engine.params().validate_receipts_transition)
|| eip658;
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 more readable (and efficient) to change the order of those checks and move eip658 || ... to the front

} else if instruction == instructions::STATICCALL {
Some(U256::zero())
}
else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

style: why not keep it on the same line?

if ext.is_static() && value.map_or(false, |v| !v.is_zero()) {
return Err(vm::Error::MutableCallInStaticContext);
}
let has_balance = ext.balance(&params.address)? >= value.expect("value set for all but delegate call and staticcall; qed");
Copy link
Collaborator

Choose a reason for hiding this comment

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

The proof should stay the same (it's 0 for staticcall, not None)

(&params.address, &code_address, has_balance, CallType::Call)
},
instructions::CALLCODE => {
let has_balance = ext.balance(&params.address)? >= value.expect("value set for all but delegate call; qed");
let has_balance = ext.balance(&params.address)? >= value.expect("value set for all but delegate call and staticcall; qed");
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/ and staticcall//

@@ -328,6 +328,9 @@ impl<Cost: CostType> Interpreter<Cost> {
let contract_code = self.mem.read_slice(init_off, init_size);
let can_create = ext.balance(&params.address)? >= endowment && ext.depth() < ext.schedule().max_depth;

// clear return data buffer before crearing new call frame.
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo in crearing

MessageCallResult::Reverted(gas_left, data) => {
stack.push(U256::zero());
self.return_data = data;
Ok(InstructionResult::UnusedGas(Cost::from_u256(gas_left).expect("Gas left cannot be greater then current one")))
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/then/than/

ForkSpec::Metropolis | ForkSpec::Byzantium | ForkSpec::Constantinople => None,
ForkSpec::Byzantium => Some(&*BYZANTIUM),
ForkSpec::EIP158ToByzantiumAt5 => Some(&BYZANTIUM_TRANSITION),
_ => None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I preferred explicit enumeration here to get a compile error if you add a new one in ForkSpec.

@@ -215,8 +225,9 @@ impl Engine for Arc<Ethash> {
} else if block_number < self.ethash_params.eip150_transition {
Schedule::new_homestead()
} else {
let max_code_size = if block_number >= self.ethash_params.eip160_transition { self.ethash_params.max_code_size as usize } else { usize::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.

Isn't that a breaking change for some private chains? Should we include it in relnotes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True, although eip160 was added and enabled a long time ago. All non-ethash engines have it enabled unconditionally.

@@ -260,6 +264,8 @@ fn do_json_test_for(vm_type: &VMType, json_data: &[u8]) -> Vec<String> {
fail_unless(Some(res.gas_left) == vm.gas_left.map(Into::into), "gas_left is incorrect");
let vm_output: Option<Vec<u8>> = vm.output.map(Into::into);
fail_unless(Some(output) == vm_output, "output is incorrect");
// TODO: verify logs
//fail_unless(Some(res.logs) == vm.logs, "logs are incorrect");
Copy link
Collaborator

Choose a reason for hiding this comment

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

TODO?

self.ensure_cached(a, RequireCache::CodeSize, false, |a| a.map_or(false, |a| a.code_size().map_or(false, |size| size != 0)))
/// Determine whether an account exists and has code or non-zero nonce.
pub fn exists_and_has_code_or_nonce(&self, a: &Address) -> trie::Result<bool> {
self.ensure_cached(a, RequireCache::CodeSize, false, |a| a.map_or(false, |a| a.code_size().map_or(false, |size| size != 0 || !a.nonce().is_zero())))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't it use account_start_nonce from spec?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

EIP-684 explicitly says "nonzero nonce". IMO we should drop support for non-zero starting nonce as there are better ways of replay protection now.

@@ -70,7 +70,8 @@ lazy_static! {
pub static ref HOMESTEAD: spec::Spec = ethereum::new_homestead_test();
pub static ref EIP150: spec::Spec = ethereum::new_eip150_test();
pub static ref EIP161: spec::Spec = ethereum::new_eip161_test();
pub static ref _METROPOLIS: spec::Spec = ethereum::new_metropolis_test();
pub static ref BYZANTIUM: spec::Spec = ethereum::new_byzantium_test();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why CONSTANTINOPLE is not here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's no Constantinople spec yet. It will be added in a later PR

@gavofyork gavofyork added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 14, 2017
@gavofyork
Copy link
Contributor

labelling grumble for marek's questions...

@arkpar
Copy link
Collaborator Author

arkpar commented Sep 14, 2017

I think I've addressed all of them

@arkpar arkpar added B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Sep 14, 2017
@gavofyork gavofyork merged commit 25b35eb into master Sep 15, 2017
@gavofyork gavofyork deleted the metropolis-update branch September 15, 2017 19:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. F0-consensus 💣 Issue can lead to a consensus failure. M4-core ⛓ Core client code / Rust. P2-asap 🌊 No need to stop dead in your tracks, however issue should be addressed as soon as possible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants