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

EIP 1283: Net gas metering for SSTORE without dirty maps #9319

Merged
merged 42 commits into from
Sep 7, 2018
Merged

Conversation

sorpaas
Copy link
Collaborator

@sorpaas sorpaas commented Aug 8, 2018

This implements EIP-1283.

This implementation only modifies EVM gas costs, wasm gas costs is unaffected by eip1283Transition flag.

@sorpaas sorpaas added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Aug 8, 2018
@sorpaas sorpaas added this to the 2.1 milestone Aug 8, 2018
@5chdn 5chdn added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Aug 18, 2018
ethcore/evm/src/tests.rs Outdated Show resolved Hide resolved
@@ -184,18 +185,20 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
schedule: schedule,
depth: 0,
static_flag: false,
transaction_checkpoint_index: None,
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a bit of comment I think. From what I understand it is to manage reentrant call by addressing the first checkpoint initial values; if it is the case renaming to first_transaction_checkpoint_index also makes sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This value actually just means to be transaction_checkpoint_index -- the checkpoint where if the transaction gets reverted. We set it at the first toplevel call/create executive because that checkpoint is the same as the "transaction checkpoint". I think the understanding that it's the "first checkpoint initial values" is correct, but I think it's not first_transaction_checkpoint_index.

@@ -539,8 +542,49 @@ impl<B: Backend> State<B> {
|a| a.as_ref().and_then(|account| account.storage_root().cloned()))
}

/// Mutate storage of account `address` so that it is `value` for `key`.
pub fn storage_at(&self, address: &Address, key: &H256) -> TrieResult<H256> {
/// Get the value of storage at last checkpoint.
Copy link
Contributor

Choose a reason for hiding this comment

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

is 'at last' compatible with checkpoint_index parameter.

Some(entry) => {
match entry.account {
Some(ref account) => {
if let Some(value) = account.cached_storage_at(key) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get this, the function is only used in reverted_storage_at to initiate origin value of the storage key, so why are we looking for the new value value of the key (cached_storage_at query first the storage changes). Maybe it is cached_original_storage_at that was meant to be called?

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 checkpoint stores an account entry. When a reversion happens, it gets swapped with the current cache (or cause the current cache to get invalidated). So here, we first check whether we can get the storage key value in the checkpoint account entry, if not, then this value has not been changed, so we fetch the original value since last state commit.

},
}
} else {
// This key does not have a checkpoint entry. We use the current value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible here that the checkpoint do not have the value because of the indexed checkpoint mechanism (I am unsure about the indexed checkpoint mechanism seeing that it was added in a later commit, I am wondering), leading to issues?

Copy link
Collaborator Author

@sorpaas sorpaas Aug 28, 2018

Choose a reason for hiding this comment

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

Yeah the logic was flawed there. My current fix is recursive, and I can't think of a better way right now using the current checkpointing method.

I think we need to finish the state mod refactoring before this PR lands.

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 realized that I was doing the index change in wrong direction. Just fixed this. Current solution is still recursive, but it has an upper bound. So I think this would work fine without #9427. However, #9427 would still be a really nice optimization.

@cheme
Copy link
Contributor

cheme commented Aug 27, 2018

Hello sorpaas, I started to review your code but I'am a bit to unsure about the current_checkpoint_index. So I just left you a few comments.

ext.inc_sstore_refund(sstore_clears_schedule);
}
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

The code looks is similar to your eip description (great eip by the way), but it is not that easy to ensure that all state transition are covered just by reading it.

So I find it useful to write a few additional comments for checking the algorithm:

pub fn handle_eip1283_sstore_clears_refund(ext: &mut vm::Ext, original: &U256, current: &U256, new: &U256) {
	let sstore_clears_schedule = U256::from(ext.schedule().sstore_refund_gas);
 	if current == new {
		// No refund
	} else {
		if original == current {
			if original.is_zero() {
				// No refund
			} else {
				if new.is_zero() {
					ext.inc_sstore_refund(sstore_clears_schedule);
				}
			}
		} else {
      // put in a `dirty_refund` function??
			if !original.is_zero() {
        // refund case
				if current.is_zero() {
          // a refund happened, revert refund
					ext.dec_sstore_refund(sstore_clears_schedule);
				} else if new.is_zero() {
          // refund
					ext.inc_sstore_refund(sstore_clears_schedule);
				}
			}
			if original == new {
        // reverting to init state
				if original.is_zero() {
          // revert sstore full cost (minus sload amount)
					let refund = U256::from(ext.schedule().sstore_set_gas - ext.schedule().sload_gas);
					ext.inc_sstore_refund(refund);
				} else {
          // revert sstore change cost (revert of refund done in previous conditions)
					let refund = U256::from(ext.schedule().sstore_reset_gas - ext.schedule().sload_gas);
					ext.inc_sstore_refund(refund);
				}
			}
		}
	}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps pattern matching here as well? Not sure if it would be more readable though.

@5chdn 5chdn mentioned this pull request Aug 31, 2018
6 tasks
@sorpaas sorpaas removed the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Aug 31, 2018
@sorpaas sorpaas added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Sep 6, 2018
@sorpaas
Copy link
Collaborator Author

sorpaas commented Sep 7, 2018

Addressed all current round of grumbles. This PR is again ready for review!

Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Great work @sorpaas! LGTM! 👍

.gitlab-ci.yml Outdated
@@ -85,6 +85,7 @@ test-coverage-kcov:
stage: test
only:
- master
- pr-9319
Copy link
Collaborator

Choose a reason for hiding this comment

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

To be removed before merge

@@ -124,13 +124,18 @@ impl<Gas: evm::CostType> Gasometer<Gas> {
let address = H256::from(stack.peek(0));
let newval = stack.peek(1);
let val = U256::from(&*ext.storage_at(&address)?);
let orig = U256::from(&*ext.initial_storage_at(&address)?);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still believe it's better to move it inside the if banch

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry I missed this. Moved!

Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

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

I do a fresh pass yesterday, some minor comments.

let gas = if val.is_zero() && !newval.is_zero() {
schedule.sstore_set_gas
let gas = if schedule.eip1283 {
calculate_eip1283_sstore_gas(schedule, &orig, &val, &newval)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could orig be initiated in this conditional branch ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need Ext handle for that, but right now we only need to pass schedule to the function, so I think it may not be necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

You address it in your last commit (my comment was unclear)

(res, substate.sstore_clears_refund, gas)
};
let gas_used = gas - gas_left;
// sstore: 1 -> (1) -> () -> (0 -> 1 -> 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the first number in this comments (others are ok)? It is nice to have this test.

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 first number is the original value. Added the tests!

// LRU Cache of the trie-backed storage for original value.
// This is only used when the initial storage root is different compared to
// what is in the database. That is, it is only used for new contracts.
original_storage_cache: Option<(H256, RefCell<LruCache<H256, H256>>)>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This original_storage_cache behave a lot like storage_root + storage_cache. It creates some additional code, sometime a bit redundant. Maybe it is possible to create a new struct and associated method to try to reduce the size of the additional code. This comment is only a suggestion (question).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, original_storage_cache is a lot like storage_root + storage_cache. However, the usage for them are really different so I'm not sure if refactoring would save code.

And TBH, after we reviewed and merged this PR, I think the next reasonable step would be to speed up finishing #9427 -- the current checkpoint logic is not easy to follow, so moving the code would only help a little. If we make it so that account/storage changes struct are cheap to clone (through PDS), then I think checkpoint logic can be simplified a lot.

@@ -154,12 +162,17 @@ impl Account {

/// Create a new contract account.
/// NOTE: make sure you use `init_code` on this before `commit`ing.
pub fn new_contract(balance: U256, nonce: U256) -> Account {
pub fn new_contract(balance: U256, nonce: U256, original_storage_root: H256) -> Account {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and in the next PR code, we use original_storage_root as a H256 with KECCAK_NULL_RLP as a special value. In existing code, storage_root seems to prefer use of Option<H256> (in function prototype). Personally I also prefer the use of option. For the codebase I think it would be good if both mechanism use the same thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I may have a wrong impression about the existing code (I noted that yesterday evening), still likes Option version better, but it is only a suggestion.

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 think it may actually be more confusing. In account model, we currently distinguish:

  • Account that does not exist. (None)
  • Account exists, and storage is empty. (Some(KECCAK_NULL_RLP))
  • Account exists, and storage is not empty. (Some(x) where x != KECCAK_NULL_RLP)

For the purpose of original_storage_root, we don't need to distinguish None and Some(KECCAK_NULL_RLP). So without Option it may work better here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it is good to keep it in mind, could be a point to consider in #9427 (Option<Option< is ugly but there is other ways).

Downward,
}

let (checkpoints_len, kind) = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Putting this block it in its own function could be possible (since you take time to separate concerns with an explicit enum). Personally I got nothing against big function, but generally code quality tools like short function with low cyclomatic complexity.

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 block is actually small and mostly consists of comments. :)

I think it would be better to keep ReturnKind to be inside checkpoint_storage_at, but putting another function would mean that's not possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine with me.

// commit, then it can only be created from a new contract, where the base storage root
// would always be empty. Note that this branch is actually never called, because
// `cached_storage_at` handled this case.
return Ok(Some(H256::new()));
Copy link
Contributor

Choose a reason for hiding this comment

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

unreachable macro ? Not sure it is a good idea, personnaly I like when unexpected behavior panics but in this context it may not be the proper choice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should indeed be unreachable! if we're absolutely certain our code contains no bug. I think we try to make sure the logic is correct as best as we can, but if we made some mistakes elsewhere and it accidentally reached this cause, this reasoning logic provided in this cause is still correct, and we would avoid a client panic.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is what I thought, as I say I prefer to fail than having an unexpected behavior, but I understand that in a production context it is not really possible. Ideally we would have a test build with panics and a production build without. A log alert trace could be a good option for such case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently codecov shows that this cause is indeed not triggered by any tests. I would actually consider it more like a failsafe (it does not necessarily break consensus) -- if this cause is ever reached, returning H256::new() is the only possible correct value.

Added a warn! so that if it ever happens, we'll (hopefully) get reports!

let y2 = Address::from(0x2002);

let mut state = get_temp_state_with_factory(factory.clone());
state.new_contract(&x1, U256::zero(), U256::from(1)).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

The new_contract does not seems to be mandatory, it contradicts the comment of init_code, but was use this way in other test. Let's keep it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this is just to test whether checkpointing is working with new_contract. In reality we would not encounter new_contract called on an existing account.

Copy link
Collaborator Author

@sorpaas sorpaas Sep 7, 2018

Choose a reason for hiding this comment

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

Ah sorry, I was confused on this and the other test: In this case, we want to create new contracts and init code for x1, x2, y1, y2. new_contract must be called before init_code.

(Reading the code it seems indeed the case that calling new_contract is not necessary before calling init_code, but we've already done this in many other places.)

cache.insert(k, v);
}
} else {
self.storage_cache = other.storage_cache;
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense to me (the replacement logic for new contracts and cache), but I do not have the background to be confident with this new behavior in the context of the evm. (I am not requesting anything, jut pointing out that my review is a bit incomplete on this part of the code).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case, self.storage_cache does not equal other.storage_cache. This means one of them is KECCAK_NULL_RLP. We cannot merge the storage cache here. Because this function is to "overwrite" self with other, we just move other's storage cache to self.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes the logic is sound, thanks for confirming it, I was just worried about some 'assertion' that could have been made on existing code.

Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

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

Approved.

@sorpaas sorpaas merged commit 915c366 into master Sep 7, 2018
@sorpaas sorpaas deleted the sp-eip1283 branch September 7, 2018 10:51
@cheme cheme mentioned this pull request Sep 19, 2018
17 tasks
andresilva pushed a commit that referenced this pull request Oct 9, 2018
* Implement last_checkpoint_storage_at

* Add reverted_storage_at for externalities

* sstore_clears_count -> sstore_clears_refund

* Implement eip1283 for evm

* Add eip1283Transition params

* evm: fix tests

* jsontests: fix test

* Return checkpoint index when creating

* Comply with spec Version II

* Fix docs

* Fix jsontests feature compile

* Address grumbles

* Fix no-checkpoint-entry case

* Remove unnecessary expect

* Add test for State::checkpoint_storage_at

* Add executive level test for eip1283

* Hard-code transaction_checkpoint_index to 0

* Fix jsontests

* Add tests for checkpoint discard/revert

* Require checkpoint to be empty for kill_account and commit

* Get code coverage

* Use saturating_add/saturating_sub

* Fix issues in insert_cache

* Clear the state again

* Fix original_storage_at

* Early return for empty RLP trie storage

* Update comments

* Fix borrow_mut issue

* Simplify checkpoint_storage_at if branches

* Better commenting for gas handling code

* Address naming grumbles

* More tests

* Fix an issue in overwrite_with

* Add another test

* Fix comment

* Remove unnecessary bracket

* Move orig to inner if

* Remove test coverage for this PR

* Add tests for executive original value

* Add warn! for an unreachable cause
andresilva pushed a commit that referenced this pull request Oct 9, 2018
* Implement last_checkpoint_storage_at

* Add reverted_storage_at for externalities

* sstore_clears_count -> sstore_clears_refund

* Implement eip1283 for evm

* Add eip1283Transition params

* evm: fix tests

* jsontests: fix test

* Return checkpoint index when creating

* Comply with spec Version II

* Fix docs

* Fix jsontests feature compile

* Address grumbles

* Fix no-checkpoint-entry case

* Remove unnecessary expect

* Add test for State::checkpoint_storage_at

* Add executive level test for eip1283

* Hard-code transaction_checkpoint_index to 0

* Fix jsontests

* Add tests for checkpoint discard/revert

* Require checkpoint to be empty for kill_account and commit

* Get code coverage

* Use saturating_add/saturating_sub

* Fix issues in insert_cache

* Clear the state again

* Fix original_storage_at

* Early return for empty RLP trie storage

* Update comments

* Fix borrow_mut issue

* Simplify checkpoint_storage_at if branches

* Better commenting for gas handling code

* Address naming grumbles

* More tests

* Fix an issue in overwrite_with

* Add another test

* Fix comment

* Remove unnecessary bracket

* Move orig to inner if

* Remove test coverage for this PR

* Add tests for executive original value

* Add warn! for an unreachable cause
andresilva pushed a commit that referenced this pull request Oct 9, 2018
* Implement last_checkpoint_storage_at

* Add reverted_storage_at for externalities

* sstore_clears_count -> sstore_clears_refund

* Implement eip1283 for evm

* Add eip1283Transition params

* evm: fix tests

* jsontests: fix test

* Return checkpoint index when creating

* Comply with spec Version II

* Fix docs

* Fix jsontests feature compile

* Address grumbles

* Fix no-checkpoint-entry case

* Remove unnecessary expect

* Add test for State::checkpoint_storage_at

* Add executive level test for eip1283

* Hard-code transaction_checkpoint_index to 0

* Fix jsontests

* Add tests for checkpoint discard/revert

* Require checkpoint to be empty for kill_account and commit

* Get code coverage

* Use saturating_add/saturating_sub

* Fix issues in insert_cache

* Clear the state again

* Fix original_storage_at

* Early return for empty RLP trie storage

* Update comments

* Fix borrow_mut issue

* Simplify checkpoint_storage_at if branches

* Better commenting for gas handling code

* Address naming grumbles

* More tests

* Fix an issue in overwrite_with

* Add another test

* Fix comment

* Remove unnecessary bracket

* Move orig to inner if

* Remove test coverage for this PR

* Add tests for executive original value

* Add warn! for an unreachable cause
andresilva pushed a commit that referenced this pull request Oct 9, 2018
* Implement last_checkpoint_storage_at

* Add reverted_storage_at for externalities

* sstore_clears_count -> sstore_clears_refund

* Implement eip1283 for evm

* Add eip1283Transition params

* evm: fix tests

* jsontests: fix test

* Return checkpoint index when creating

* Comply with spec Version II

* Fix docs

* Fix jsontests feature compile

* Address grumbles

* Fix no-checkpoint-entry case

* Remove unnecessary expect

* Add test for State::checkpoint_storage_at

* Add executive level test for eip1283

* Hard-code transaction_checkpoint_index to 0

* Fix jsontests

* Add tests for checkpoint discard/revert

* Require checkpoint to be empty for kill_account and commit

* Get code coverage

* Use saturating_add/saturating_sub

* Fix issues in insert_cache

* Clear the state again

* Fix original_storage_at

* Early return for empty RLP trie storage

* Update comments

* Fix borrow_mut issue

* Simplify checkpoint_storage_at if branches

* Better commenting for gas handling code

* Address naming grumbles

* More tests

* Fix an issue in overwrite_with

* Add another test

* Fix comment

* Remove unnecessary bracket

* Move orig to inner if

* Remove test coverage for this PR

* Add tests for executive original value

* Add warn! for an unreachable cause
andresilva pushed a commit that referenced this pull request Oct 9, 2018
* Implement last_checkpoint_storage_at

* Add reverted_storage_at for externalities

* sstore_clears_count -> sstore_clears_refund

* Implement eip1283 for evm

* Add eip1283Transition params

* evm: fix tests

* jsontests: fix test

* Return checkpoint index when creating

* Comply with spec Version II

* Fix docs

* Fix jsontests feature compile

* Address grumbles

* Fix no-checkpoint-entry case

* Remove unnecessary expect

* Add test for State::checkpoint_storage_at

* Add executive level test for eip1283

* Hard-code transaction_checkpoint_index to 0

* Fix jsontests

* Add tests for checkpoint discard/revert

* Require checkpoint to be empty for kill_account and commit

* Get code coverage

* Use saturating_add/saturating_sub

* Fix issues in insert_cache

* Clear the state again

* Fix original_storage_at

* Early return for empty RLP trie storage

* Update comments

* Fix borrow_mut issue

* Simplify checkpoint_storage_at if branches

* Better commenting for gas handling code

* Address naming grumbles

* More tests

* Fix an issue in overwrite_with

* Add another test

* Fix comment

* Remove unnecessary bracket

* Move orig to inner if

* Remove test coverage for this PR

* Add tests for executive original value

* Add warn! for an unreachable cause
andresilva pushed a commit that referenced this pull request Oct 10, 2018
* Implement last_checkpoint_storage_at

* Add reverted_storage_at for externalities

* sstore_clears_count -> sstore_clears_refund

* Implement eip1283 for evm

* Add eip1283Transition params

* evm: fix tests

* jsontests: fix test

* Return checkpoint index when creating

* Comply with spec Version II

* Fix docs

* Fix jsontests feature compile

* Address grumbles

* Fix no-checkpoint-entry case

* Remove unnecessary expect

* Add test for State::checkpoint_storage_at

* Add executive level test for eip1283

* Hard-code transaction_checkpoint_index to 0

* Fix jsontests

* Add tests for checkpoint discard/revert

* Require checkpoint to be empty for kill_account and commit

* Get code coverage

* Use saturating_add/saturating_sub

* Fix issues in insert_cache

* Clear the state again

* Fix original_storage_at

* Early return for empty RLP trie storage

* Update comments

* Fix borrow_mut issue

* Simplify checkpoint_storage_at if branches

* Better commenting for gas handling code

* Address naming grumbles

* More tests

* Fix an issue in overwrite_with

* Add another test

* Fix comment

* Remove unnecessary bracket

* Move orig to inner if

* Remove test coverage for this PR

* Add tests for executive original value

* Add warn! for an unreachable cause
5chdn pushed a commit that referenced this pull request Oct 10, 2018
* ethash: implement EIP-1234 (#9187)

* Implement EIP-1052 (EXTCODEHASH) and fix several issues in state account cache (#9234)

* Implement EIP-1052 and fix several issues related to account cache

* Fix jsontests

* Merge two matches together

* Avoid making unnecessary Arc<Vec>

* Address grumbles

* Comply EIP-86 with the new definition (#9140)

* Comply EIP-86 with the new CREATE2 opcode

* Fix rpc compile

* Fix interpreter CREATE/CREATE2 stack pop difference

* Add unreachable! to fix compile

* Fix instruction_info

* Fix gas check due to new stack item

* Add new tests in executive

* Fix have_create2 comment

* Remove all unused references of eip86_transition and block_number

* Implement KIP4: create2 for wasm (#9277)

* Basic implementation for kip4

* Add KIP-4 config flags

* typo: docs fix

* Fix args offset

* Add tests for create2

* tests: evm

* Update wasm-tests and fix all gas costs

* Update wasm-tests

* Update wasm-tests and fix gas costs

* `gasleft` extern implemented for WASM runtime (kip-6) (#9357)

* Wasm gasleft extern added

* wasm_gasleft_activation_transition -> kip4_transition

* use kip-6 switch

* gasleft_panic -> gasleft_fail rename

* call_msg_gasleft test added and gas_left agustments because this openethereum/wasm-tests#52

* change .. to _

* fix comment for the have_gasleft param

* update tests (openethereum/wasm-tests@0edbf86)

* Add EIP-1014 transition config flag (#9268)

* Add EIP-1014 transition config flag

* Remove EIP-86 configs

* Change CREATE2 opcode index to 0xf5

* Move salt to the last item in the stack

* Change sendersaltandaddress scheme to comply with current EIP-1014

* Fix json configs

* Fix create2 test

* Fix deprecated comments

* EIP 1283: Net gas metering for SSTORE without dirty maps (#9319)

* Implement last_checkpoint_storage_at

* Add reverted_storage_at for externalities

* sstore_clears_count -> sstore_clears_refund

* Implement eip1283 for evm

* Add eip1283Transition params

* evm: fix tests

* jsontests: fix test

* Return checkpoint index when creating

* Comply with spec Version II

* Fix docs

* Fix jsontests feature compile

* Address grumbles

* Fix no-checkpoint-entry case

* Remove unnecessary expect

* Add test for State::checkpoint_storage_at

* Add executive level test for eip1283

* Hard-code transaction_checkpoint_index to 0

* Fix jsontests

* Add tests for checkpoint discard/revert

* Require checkpoint to be empty for kill_account and commit

* Get code coverage

* Use saturating_add/saturating_sub

* Fix issues in insert_cache

* Clear the state again

* Fix original_storage_at

* Early return for empty RLP trie storage

* Update comments

* Fix borrow_mut issue

* Simplify checkpoint_storage_at if branches

* Better commenting for gas handling code

* Address naming grumbles

* More tests

* Fix an issue in overwrite_with

* Add another test

* Fix comment

* Remove unnecessary bracket

* Move orig to inner if

* Remove test coverage for this PR

* Add tests for executive original value

* Add warn! for an unreachable cause

* Update state tests execution model (#9440)

* Update & fix JSON state tests.

* Update tests to be able to run ethtest at
021fe3d410773024cd5f0387e62db6e6ec800f32.

- Touch user in state
- Adjust transaction tests to new json format

* Switch to same commit for submodule ethereum/test as geth (next includes constantinople changes).
Added test `json_tests::trie::generic::TrieTests_trieanyorder` and a few
difficulty tests.

* Remove trietestnextprev as it would require to parse differently and
implement it.

* Support new (shitty) format of transaction tests.

* Ignore junk in ethereum/tests repo.

* Ignore incorrect test.

* Update to a later commit

* Move block number to a constant.

* Fix ZK2 test - touched account should also be cleared.

* Fix conflict resolution

* Fix checkpointing when creating contract failed (#9514)

* In create memory calculation is the same for create2 because the additional parameter was popped before. (#9522)

* Enable all Constantinople hard fork changes in constantinople_test.json (#9505)

* Enable all Constantinople hard fork changes in constantinople_test.json

* Address grumbles

* Remove EIP-210 activation

* 8m -> 5m

* Temporarily add back eip210 transition so we can get test passed

* Add eip210_test and remove eip210 transition from const_test

* Add constantinople conf to EvmTestClient. (#9570)

* Add constantinople conf to EvmTestClient.

* Skip some test to update submodule etheureum/tests submodule to latest.

* Put skipping 'under issue' test behind a feature.

* Change blockReward for const-test to pass ethereum/tests

* Update tests to new constantinple definition (change of reward at block
5).
Switch 'reference' to string, that way we can include issues from others
repo (more flexible)Update tests to new constantinple definition (change
of reward at block 5).
Switch 'reference' to string, that way we can include issues from others
repo (more flexible).

* Fix modexp and bn128_mul gas prices in chain config

* Changes `run_test_path` method to append its directory results (without
that it stop testing at the first file failure).
Add some missing tests.
Add skip for those (block create2 is one hundred percent false but on
hive we can see that geth and aleth got similar issue for this item).

* retab current.json

* Update reference to parity issue for failing tests.

* Hardfork the testnets (#9562)

* ethcore: propose hardfork block number 4230000 for ropsten

* ethcore: propose hardfork block number 9000000 for kovan

* ethcore: enable kip-4 and kip-6 on kovan

* etcore: bump kovan hardfork to block 9.2M

* ethcore: fix ropsten constantinople block number to 4.2M

* ethcore: disable difficulty_test_ropsten until ethereum/tests are updated upstream

* Don't hash the init_code of CREATE. (#9688)

* Implement CREATE2 gas changes and fix some potential overflowing (#9694)

* Implement CREATE2 gas changes and fix some potential overflowing

* Ignore create2 state tests

* Split CREATE and CREATE2 in gasometer

* Generalize rounding (x + 31) / 32 to to_word_size

* ethcore: delay ropsten hardfork (#9704)

* Add hardcoded headers (#9730)

* add foundation hardcoded header #6486017

* add ropsten hardcoded headers #4202497

* add kovan hardcoded headers #9023489

* gitlab ci: releasable_branches: change variables condition to schedule (#9729)

* HF in POA Core (2018-10-22) (#9724)

poanetwork/poa-chain-spec#87
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. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants