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

Implement Simple Subroutines for the EVM (EIP 2315) #11629

Merged
merged 12 commits into from
May 15, 2020
Merged

Implement Simple Subroutines for the EVM (EIP 2315) #11629

merged 12 commits into from
May 15, 2020

Conversation

vorot93
Copy link

@vorot93 vorot93 commented Apr 13, 2020

This PR implements Simple Subroutines for the EVM (EIP 2315).

The code was written by @adria0 with my assistance and also guidance by @sorpaas during a live coding session on Monday. It should be functional but may require some cleanup.

@vorot93 vorot93 requested a review from sorpaas April 13, 2020 23:42
ordian
ordian reviewed Apr 14, 2020
View reviewed changes
Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

I think some parts are missing like
https://github.com/openethereum/openethereum/pull/11612/files#diff-62a9081304c809673d1a737d1a3d469eR605-R607

ethcore/evm/src/interpreter/shared_cache.rs Outdated Show resolved Hide resolved
ethcore/evm/src/interpreter/mod.rs Outdated Show resolved Hide resolved
ethcore/evm/src/interpreter/mod.rs Outdated Show resolved Hide resolved
@ordian ordian added A0-pleasereview 🤓 Pull request needs code review. A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Apr 14, 2020
@adria0
Copy link

adria0 commented Apr 16, 2020

@ordian, @dvdplm, @vorot93 this needs to be activated via chain params, like EIP2046, isn't it?

@ordian
Copy link
Collaborator

ordian commented Apr 16, 2020

@ordian, @dvdplm, @vorot93 this needs to be activated via chain params, like EIP2046, isn't it?

I believe so

@adria0 adria0 marked this pull request as draft April 17, 2020 09:01
ordian
ordian reviewed Apr 17, 2020
View reviewed changes
ethcore/evm/src/instructions.rs Outdated Show resolved Hide resolved
ethcore/evm/src/interpreter/shared_cache.rs Outdated Show resolved Hide resolved
ethcore/trace/src/types/error.rs Outdated Show resolved Hide resolved
ethcore/trace/src/types/error.rs Outdated Show resolved Hide resolved
ethcore/types/src/engines/params.rs Outdated Show resolved Hide resolved
ethcore/vm/src/error.rs Outdated Show resolved Hide resolved
@ordian ordian removed the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Apr 17, 2020
@adria0 adria0 marked this pull request as ready for review April 17, 2020 11:56
@adria0 adria0 requested a review from ordian April 17, 2020 11:56
ordian
ordian approved these changes Apr 17, 2020
View reviewed changes
Copy link
Collaborator

@ordian ordian left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM modulo one unresolved question.

ethcore/evm/src/interpreter/mod.rs Show resolved Hide resolved
dvdplm
dvdplm reviewed Apr 17, 2020
View reviewed changes
ethcore/evm/src/interpreter/shared_cache.rs
/// Global cache for EVM interpreter
pub struct SharedCache {
jump_destinations: Mutex<MemoryLruCache<H256, Bits>>,
jump_destinations: Mutex<MemoryLruCache<H256, CacheItem>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably not in scope for this PR but I don't think the Mutex is necessary here.

ethcore/vm/src/error.rs Outdated Show resolved Hide resolved
@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 20, 2020
@niklasad1 niklasad1 mentioned this pull request Apr 22, 2020
Open
3 tasks
sorpaas
sorpaas approved these changes May 15, 2020
View reviewed changes
Copy link
Collaborator

@sorpaas sorpaas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM just a slight thing that I think can be improved in the future. Mark this as mustn'tgrumble!

ethcore/evm/src/interpreter/mod.rs
@@ -412,6 +422,21 @@ impl<Cost: CostType> Interpreter<Cost> {
};
self.reader.position = pos;
},
InstructionResult::JumpToSubroutine(position) => {
if self.valid_subroutine_destinations.is_none() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

When we fetch valid_subrountine_destinations, we can set valid_jump_destinations at the same time, and vice versa, because those two values are always computed at the same time. Should slightly improve performance.

@sorpaas sorpaas added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels May 15, 2020
@sorpaas sorpaas merged commit 234a287 into master May 15, 2020
@sorpaas sorpaas deleted the eip-2315 branch May 15, 2020 11:52
@gcolvin gcolvin mentioned this pull request May 15, 2020
Closed
dvdplm added a commit that referenced this pull request May 25, 2020
@dvdplm
Merge branch 'master' into dp/chore/upgrade-to-rocksdb-0.14
9845679 
* master:
  Implement Simple Subroutines for the EVM (EIP 2315) (#11629)
  Adria0/fix/tests (#11597)
holiman
holiman reviewed May 25, 2020
View reviewed changes
ethcore/evm/src/instructions.rs
@@ -586,6 +592,9 @@ lazy_static! {
arr[LOG2 as usize] = Some(InstructionInfo::new("LOG2", 4, 0, GasPriceTier::Special));
arr[LOG3 as usize] = Some(InstructionInfo::new("LOG3", 5, 0, GasPriceTier::Special));
arr[LOG4 as usize] = Some(InstructionInfo::new("LOG4", 6, 0, GasPriceTier::Special));
arr[BEGINSUB as usize] = Some(InstructionInfo::new("BEGINSUB", 0, 0, GasPriceTier::Base));
arr[JUMPSUB as usize] = Some(InstructionInfo::new("JUMPSUB", 1, 0, GasPriceTier::Low));
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm mistaken, both geth and besu used mid (8) for JUMPSUB.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We followed the suggestion from the spec

We suggest that the cost of JUMPSUB be low, and RETURNSUB be verylow. Measurement will tell.

Is there any reason why geth and besu chose mid?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the original geth-PR from Greg used that value, and then we didn't change it when the EIP was updated at some point. IMO it makes zero sense to have JUMPSUB be cheaper than JUMP, considering that JUMPSUB causes a memory allocation in the returnstack.

Copy link
Contributor

Choose a reason for hiding this comment

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

PTAL: ethereum/EIPs#2669

@adria0
Copy link

adria0 commented May 25, 2020

Created this PR https://github.com/openethereum/openethereum/pull/11731 for updates to the EIP

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@adria0 adria0 adria0 left review comments

@ordian ordian ordian approved these changes

@holiman holiman holiman left review comments

@dvdplm dvdplm dvdplm left review comments

@sorpaas sorpaas sorpaas approved these changes

Assignees
No one assigned
Labels
A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vorot93 @adria0 @ordian @dvdplm @holiman @sorpaas