Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Berlin support #40

Merged
merged 42 commits into from
Aug 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
d7ea767
Initial stab at EIP-2929 support
birchmd Jun 30, 2021
8c24c13
Revert "Initial stab at EIP-2929 support"
birchmd Jun 30, 2021
b471064
Another try at EIP-2929 (I think this one is better)
birchmd Jun 30, 2021
5ee05ae
Cleanup
joshuajbouw Jul 1, 2021
7654641
Clean up costs
joshuajbouw Jul 1, 2021
a08f255
Add `Precompiles` trait
joshuajbouw Jul 2, 2021
4d7c084
Update other costs
joshuajbouw Jul 2, 2021
7705151
General fixes
joshuajbouw Jul 2, 2021
238f262
Avoid unnecessary Vec allocation
birchmd Jul 5, 2021
d5c078a
Ensure precompile addresses are added to storage access in create cal…
birchmd Jul 5, 2021
a75b46f
Fix benches compilation
birchmd Jul 5, 2021
3674356
Initialize accessed_addressed with caller too
birchmd Jul 5, 2021
712e7c3
Export Precompile at root
joshuajbouw Jul 6, 2021
224a471
Enable EIP-2929 for Berlin
joshuajbouw Jul 6, 2021
45de2a4
Update accessed_addresses and accessed_storage_keys on read/write; sc…
birchmd Jul 6, 2021
a2410a5
Fix sload cost
birchmd Jul 6, 2021
40c1023
Fix sstore refund
birchmd Jul 8, 2021
3e081dc
Fix random state test 649 (see https://github.com/ethereum/tests/comm…
birchmd Jul 9, 2021
0b07823
EIP-2930 support
birchmd Jul 9, 2021
7a74753
Fix bug in EIP-1706 implementation. Note the spec says _less than or …
birchmd Jul 13, 2021
bf2fb77
Import alloc types
birchmd Jul 14, 2021
09d4fe0
Precompiles trait refactor
birchmd Jul 14, 2021
0bd05b1
Merge branch 'master' into berlin-support
joshuajbouw Jul 14, 2021
f230e7d
Update `transact_call` documentation
joshuajbouw Jul 14, 2021
49aefb2
Merge remote-tracking branch 'aurora-is-near/master' into berlin-support
joshuajbouw Jul 27, 2021
384ff22
cargo fmt
joshuajbouw Jul 27, 2021
d3adea7
Merge remote-tracking branch 'origin/master' into berlin-support
joshuajbouw Jul 28, 2021
335e2d9
Clippy fix
birchmd Jul 28, 2021
9bee750
Add comment to memory.rs change
birchmd Jul 28, 2021
79d12b1
get_accessed_addresses -> accessed_addresses
joshuajbouw Jul 30, 2021
d16bdec
Don't imply use of a single state for precompiles
joshuajbouw Jul 30, 2021
c2af451
Move access lists to metadata
joshuajbouw Aug 2, 2021
6b46fa8
Handle all clippy errors
joshuajbouw Aug 2, 2021
56a7ee0
Remove debug asserts (they do not passs Istanbul tests)
birchmd Aug 2, 2021
2c92477
Remove commented out code
birchmd Aug 2, 2021
62c1c52
Resore immutability of gas_cost calls
birchmd Aug 2, 2021
0d70657
Remove obsolete additions made to gasometer
birchmd Aug 2, 2021
2a08e99
Fix storage access tracking and is_cold functionality
birchmd Aug 2, 2021
a252024
Move, do not copy, access struct in swallow_commit
birchmd Aug 2, 2021
51d6876
Merge branch 'master' into berlin-support
joshuajbouw Aug 9, 2021
5bad029
Use precompile type over trait
joshuajbouw Aug 9, 2021
865af75
Fix min version of ethereum dependency
joshuajbouw Aug 10, 2021
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
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ rlp = { version = "0.5", default-features = false }
primitive-types = { version = "0.10", default-features = false, features = ["rlp"] }
serde = { version = "1.0", default-features = false, features = ["derive"], optional = true }
codec = { package = "parity-scale-codec", version = "2.0", default-features = false, features = ["derive"], optional = true }
ethereum = { version = ">= 0.8, <= 0.9", default-features = false }
ethereum = { version = "~0.8", default-features = false }
environmental = { version = "1.1.2", default-features = false, optional = true }

[dev-dependencies]
Expand Down
1 change: 1 addition & 0 deletions benches/loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ fn run_loop_contract() {
.unwrap(),
// hex::decode("0f14a4060000000000000000000000000000000000000000000000000000000000002ee0").unwrap(),
u64::MAX,
Vec::new(),
);
}

Expand Down
9 changes: 9 additions & 0 deletions core/src/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,15 @@ impl Memory {
len: U256,
data: &[u8],
) -> Result<(), ExitFatal> {
// Needed to pass ethereum test defined in
// https://github.com/ethereum/tests/commit/17f7e7a6c64bb878c1b6af9dc8371b46c133e46d
// (regardless of other inputs, a zero-length copy is defined to be a no-op).
// TODO: refactor `set` and `copy_large` (see
// https://github.com/rust-blockchain/evm/pull/40#discussion_r677180794)
if len.is_zero() {
Copy link
Member

Choose a reason for hiding this comment

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

This is actually not necessary for semantics, since below we call set, and within that function it checked the len is zero case.

I understand the concern of a short-cut here since we do not need to do additional checks below, but there are indeed several things:

  • If we add this check, it should be below the ErrorFatal::NotSupported check but not above. The ErrorFatal type is a unique error type not covered in spec but useful in practice for non-metered cases -- it ensures we can always handle the addressing spaces since EVM supports 256-bit memory, but in reality all current EVM executors can only support up to 64-bit memory running on x86_64.
  • The len zero condition should be triggered extremely rarely since it's basically a no-op. And a well-optimized EVM contract should not have triggered it.
  • Our memory code set and copy_large is already quiet complex and it happened that some trivial changes in the past caused compliance issue. We should really not add additional "patches" unless necessary and instead prefer to fully refactor those two functions.

With all the above said, my current recommendation is to remove this change. We then spend some time refactor set and copy_large to make it less error-prone and then consider to add those sugar-sized optimizations.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added this change to pass this eth json test (you can see it linked from the commit where the change was introduced).

That eth json test was added before Berlin tests so when we updated the evm tests repo to point at a later version of ethereum tests this change was needed to have them all pass.

I agree it's a little hacky, but I do not think we should block merging Berlin changes to refactor the memory functions. We could create an issue to do that refactoring and come back to it after Berlin changes are merged.

Copy link
Member

Choose a reason for hiding this comment

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

We don't need that now though -- till the moment we update jsontests.

Alternatively, you can add comments to this line explaining why this is introduced now, and that this is a temporary solution and will eventually be refactored. The priority here should be to avoid more confusions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added a comment in 9bee750

till the moment we update jsontests.

Are we not going to update the jsontests along side this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I am confused about, as jsontests will be updated immediately after.

return Ok(());
}

let memory_offset = if memory_offset > U256::from(usize::MAX) {
return Err(ExitFatal::NotSupported);
} else {
Expand Down
102 changes: 79 additions & 23 deletions gasometer/src/costs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,18 @@ pub fn sstore_refund(original: H256, current: H256, new: H256, config: &Config)
}

if original == new {
let (gas_sstore_reset, gas_sload) = if config.increase_state_access_gas {
(
config.gas_sstore_reset - config.gas_sload_cold,
config.gas_storage_read_warm,
)
} else {
(config.gas_sstore_reset, config.gas_sload)
};
if original == H256::default() {
refund += (config.gas_sstore_set - config.gas_sload) as i64;
refund += (config.gas_sstore_set - gas_sload) as i64;
} else {
refund += (config.gas_sstore_reset - config.gas_sload) as i64;
refund += (gas_sstore_reset - gas_sload) as i64;
}
}

Expand Down Expand Up @@ -122,11 +130,10 @@ pub fn verylowcopy_cost(len: U256) -> Result<u64, ExitError> {
Ok(gas.as_u64())
}

pub fn extcodecopy_cost(len: U256, config: &Config) -> Result<u64, ExitError> {
pub fn extcodecopy_cost(len: U256, is_cold: bool, config: &Config) -> Result<u64, ExitError> {
let wordd = len / U256::from(32);
let wordr = len % U256::from(32);

let gas = U256::from(config.gas_ext_code)
let gas = U256::from(address_access_cost(is_cold, config.gas_ext_code, config))
.checked_add(
U256::from(G_COPY)
.checked_mul(if wordr == U256::zero() {
Expand Down Expand Up @@ -186,39 +193,71 @@ pub fn sha3_cost(len: U256) -> Result<u64, ExitError> {
Ok(gas.as_u64())
}

pub fn sload_cost(is_cold: bool, config: &Config) -> u64 {
if config.increase_state_access_gas {
if is_cold {
config.gas_sload_cold
} else {
config.gas_storage_read_warm
}
} else {
config.gas_sload
}
}

#[allow(clippy::collapsible_else_if)]
pub fn sstore_cost(
original: H256,
current: H256,
new: H256,
gas: u64,
is_cold: bool,
config: &Config,
) -> Result<u64, ExitError> {
if config.sstore_gas_metering {
if config.sstore_revert_under_stipend && gas < config.call_stipend {
let (gas_sload, gas_sstore_reset) = if config.increase_state_access_gas {
(
config.gas_storage_read_warm,
config.gas_sstore_reset - config.gas_sload_cold,
)
} else {
(config.gas_sload, config.gas_sstore_reset)
};
let gas_cost = if config.sstore_gas_metering {
if config.sstore_revert_under_stipend && gas <= config.call_stipend {
return Err(ExitError::OutOfGas);
}

Ok(if new == current {
config.gas_sload
} else if original == current {
if original == H256::zero() {
config.gas_sstore_set
if new == current {
gas_sload
} else {
if original == current {
if original == H256::zero() {
config.gas_sstore_set
} else {
gas_sstore_reset
}
} else {
config.gas_sstore_reset
gas_sload
}
} else {
config.gas_sload
})
}
} else {
Ok(if current == H256::zero() && new != H256::zero() {
if current == H256::zero() && new != H256::zero() {
config.gas_sstore_set
} else {
config.gas_sstore_reset
})
}
gas_sstore_reset
}
};
Ok(
// In EIP-2929 we charge extra if the slot has not been used yet in this transaction
if is_cold {
gas_cost + config.gas_sload_cold
} else {
gas_cost
},
)
}

pub fn suicide_cost(value: U256, target_exists: bool, config: &Config) -> u64 {
pub fn suicide_cost(value: U256, is_cold: bool, target_exists: bool, config: &Config) -> u64 {
let eip161 = !config.empty_considered_exists;
let should_charge_topup = if eip161 {
value != U256::zero() && !target_exists
Expand All @@ -232,22 +271,39 @@ pub fn suicide_cost(value: U256, target_exists: bool, config: &Config) -> u64 {
0
};

config.gas_suicide + suicide_gas_topup
let mut gas = config.gas_suicide + suicide_gas_topup;
if config.increase_state_access_gas && is_cold {
gas += config.gas_account_access_cold
}
gas
}

pub fn call_cost(
value: U256,
is_cold: bool,
is_call_or_callcode: bool,
is_call_or_staticcall: bool,
new_account: bool,
config: &Config,
) -> u64 {
let transfers_value = value != U256::default();
config.gas_call
address_access_cost(is_cold, config.gas_call, config)
+ xfer_cost(is_call_or_callcode, transfers_value)
+ new_cost(is_call_or_staticcall, new_account, transfers_value, config)
}

pub fn address_access_cost(is_cold: bool, regular_value: u64, config: &Config) -> u64 {
if config.increase_state_access_gas {
if is_cold {
config.gas_account_access_cold
} else {
config.gas_storage_read_warm
}
} else {
regular_value
}
}

fn xfer_cost(is_call_or_callcode: bool, transfers_value: bool) -> u64 {
if is_call_or_callcode && transfers_value {
G_CALLVALUE
Expand Down
Loading