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

EIP-86 #4697

Merged
merged 2 commits into from
Apr 19, 2017
Merged

EIP-86 #4697

merged 2 commits into from
Apr 19, 2017

Conversation

arkpar
Copy link
Collaborator

@arkpar arkpar commented Feb 27, 2017

Implements ethereum/EIPs#208

@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. M4-core ⛓ Core client code / Rust. labels Feb 27, 2017
@arkpar arkpar removed the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Mar 7, 2017
@arkpar arkpar mentioned this pull request Mar 9, 2017
12 tasks
fn schedule(&self, _env_info: &EnvInfo) -> Schedule {
Schedule::new_post_eip150(usize::max_value(), true, true, true)
fn schedule(&self, block_number: BlockNumber) -> Schedule {
let eip86 = block_number >= self.params.eip98_transition;
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo 86 != 98

@@ -68,6 +70,7 @@ impl From<ethjson::spec::Params> for CommonParams {
min_gas_limit: p.min_gas_limit.into(),
fork_block: if let (Some(n), Some(h)) = (p.fork_block, p.fork_hash) { Some((n.into(), h.into())) } else { None },
eip98_transition: p.eip98_transition.map_or(0, Into::into),
eip86_transition: p.eip86_transition.map_or(0, Into::into),
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe it should be u64::max_value() instead of 0 until we release?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense to enable this when other metropolis changes are ready.

@debris debris added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 9, 2017
fn schedule(&self, _env_info: &EnvInfo) -> Schedule {
Schedule::new_post_eip150(usize::max_value(), true, true, true)
fn schedule(&self, block_number: BlockNumber) -> Schedule {
let eip86 = block_number >= self.params.eip98_transition;
Copy link
Collaborator

Choose a reason for hiding this comment

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

also a typo

block_number >= self.ethash_params.eip160_transition,
block_number >= self.ethash_params.eip161abc_transition,
block_number >= self.ethash_params.eip161d_transition,
block_number >= self.params.eip98_transition
Copy link
Collaborator

Choose a reason for hiding this comment

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

also here

@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Mar 9, 2017
fn schedule(&self, _env_info: &EnvInfo) -> Schedule {
Schedule::new_post_eip150(usize::max_value(), true, true, true)
fn schedule(&self, block_number: BlockNumber) -> Schedule {
let eip86 = block_number >= self.params.eip98_transition;
Copy link
Collaborator

Choose a reason for hiding this comment

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

one 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.

ugh, I'm going to replace it with Schedule::from_params for DRY after #4757 is merged. https://github.com/ethcore/parity/pull/4757/files#diff-ef2687136b4e032043a492e9e53dcb7cR177

@debris debris mentioned this pull request Mar 10, 2017
@arkpar arkpar added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 12, 2017
@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Mar 15, 2017
@gavofyork
Copy link
Contributor

what's the rationale behind EnvInfo -> BlockNumber change?

@arkpar
Copy link
Collaborator Author

arkpar commented Mar 31, 2017

There's a number of places now where Schedule is created from the block number. Creating a fake EnvInfo struct seems wasteful.

@arkpar
Copy link
Collaborator Author

arkpar commented Mar 31, 2017

Updated with recent EIP changes. gasprice, noce, value are now required to be zero for unsigned transactions.

@arkpar arkpar force-pushed the eip86 branch 2 times, most recently from 195dbeb to ea479a0 Compare April 5, 2017 13:36
@@ -241,8 +240,9 @@ impl Engine for AuthorityRound {
]
}

fn schedule(&self, _env_info: &EnvInfo) -> Schedule {
Schedule::new_post_eip150(usize::max_value(), true, true, true)
fn schedule(&self, block_number: BlockNumber) -> Schedule {
Copy link
Contributor

Choose a reason for hiding this comment

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

why &EnvInfo -> BlockNumber?

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 a number of places where Schedule is created from the block number number. This PR introduces quite a few more. Creating a fake EnvInfo struct seems wasteful. And furthermore since all of the data except for the block number is unreliable it may introduce subtle bugs if any future implementation will have to rely on any other field.

From::from(stream.as_raw().sha3())
},
CreateContractAddress::FromCodeHash => {
let mut buffer = [0xffu8; 20 + 32];
Copy link
Contributor

Choose a reason for hiding this comment

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

YP & EIP state that this should be the null sender (i.e. zero address). am i missing something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed.

Copy link

@pirapira pirapira Apr 18, 2017

Choose a reason for hiding this comment

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

I think the fix is wrong (and I was mistaken in a PR to YP).

The relevant EIP says the pad is 0xffff..ff. The NULL_SENDER is defined to be 0xffff..ff.

) -> evm::Result<U256> where T: Tracer, V: VMTracer {

// TODO: enable this check once consensus tests are fixed.
//if !overwrite_existing && self.state.exists(&params.address) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should be enabled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

enabled now

@@ -365,16 +383,23 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
params: ActionParams,
substate: &mut Substate,
tracer: &mut T,
vm_tracer: &mut V
vm_tracer: &mut V,
_overwrite_existing: bool
Copy link
Contributor

Choose a reason for hiding this comment

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

overwrite existing what?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed that param since it is already in the schedule.

@@ -234,7 +234,7 @@ impl Parity for ParityClient {
Ok(
txq.ready_transactions(chain_info.best_block_number, chain_info.best_block_timestamp)
.into_iter()
.map(Into::into)
.map(|tx| Transaction::from_pending(tx, chain_info.best_block_number, u64::max_value())) //TODO: enable EIP-86
Copy link
Contributor

Choose a reason for hiding this comment

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

TODOs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved

@@ -245,7 +245,7 @@ impl Parity for ParityClient {
Ok(
txq.future_transactions(chain_info.best_block_number, chain_info.best_block_timestamp)
.into_iter()
.map(Into::into)
.map(|tx| Transaction::from_pending(tx, chain_info.best_block_number, u64::max_value())) //TODO: enable EIP-86
Copy link
Contributor

Choose a reason for hiding this comment

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

TODOs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved

@gavofyork
Copy link
Contributor

gavofyork commented Apr 12, 2017

one probable correction needed and a few clarifications.

@gavofyork gavofyork added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 12, 2017
@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Apr 13, 2017
@gavofyork gavofyork added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 18, 2017
@gavofyork
Copy link
Contributor

needs resolving.

@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Apr 18, 2017
@NikVolf NikVolf added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 19, 2017
@arkpar arkpar merged commit b50fb71 into master Apr 19, 2017
@arkpar arkpar deleted the eip86 branch April 19, 2017 12:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants