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.

Sign up for GitHub

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

feat(revm): revert EIP-2935 BLOCKHASH opcode changes #1450

Merged
merged 5 commits into from
May 24, 2024
Merged

feat(revm): revert EIP-2935 BLOCKHASH opcode changes #1450

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
974ca9c
feat(revm): revert EIP-2935 BLOCKHASH opcode changes
shekhirin May 24, 2024
b093364
fix lint
shekhirin May 24, 2024
ec6c8fc
bump time
shekhirin May 24, 2024
c772230
return previous impl mod prague branch
shekhirin May 24, 2024
e9f8b65
remove unused imports
shekhirin May 24, 2024
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
8 changes: 4 additions & 4 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

42 changes: 5 additions & 37 deletions crates/revm/src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,11 @@ pub use context_precompiles::{
};
pub use evm_context::EvmContext;
pub use inner_evm_context::InnerEvmContext;
use revm_interpreter::as_usize_saturated;

use crate::{
db::{Database, EmptyDB},
interpreter::{Host, LoadAccountResult, SStoreResult, SelfDestructResult},
primitives::{
Address, Bytecode, EVMError, Env, HandlerCfg, Log, B256, BLOCKHASH_SERVE_WINDOW,
BLOCKHASH_STORAGE_ADDRESS, BLOCK_HASH_HISTORY, PRAGUE, U256,
},
primitives::{Address, Bytecode, Env, HandlerCfg, Log, B256, U256},
};
use std::boxed::Box;

Expand Down Expand Up @@ -110,38 +106,10 @@ impl<EXT, DB: Database> Host for Context<EXT, DB> {
}

fn block_hash(&mut self, number: U256) -> Option<B256> {
let block_number = as_usize_saturated!(self.env().block.number);
let requested_number = as_usize_saturated!(number);

let Some(diff) = block_number.checked_sub(requested_number) else {
return Some(B256::ZERO);
};

// blockhash should push zero if number is same as current block number.
if diff == 0 {
return Some(B256::ZERO);
}

if diff <= BLOCK_HASH_HISTORY {
return self
.evm
.block_hash(number)
.map_err(|e| self.evm.error = Err(e))
.ok();
}

if self.evm.journaled_state.spec.is_enabled_in(PRAGUE) && diff <= BLOCKHASH_SERVE_WINDOW {
Copy link
Collaborator

Choose a reason for hiding this comment

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

only this PRAGUE branch should be removed, the rest is still valid (diff == 0, diff <= HISTORY...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

are you sure? from the EIP:

The BLOCKHASH opcode semantics remains the same as before.

revm/crates/revm/src/context.rs

Lines 108 to 113 in eed27d9

fn block_hash(&mut self, number: U256) -> Option<B256> {
self.evm
.block_hash(number)
.map_err(|e| self.evm.error = Err(e))
.ok()
}

Copy link
Collaborator

@DaniPopes DaniPopes May 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, this is the before:

revm/crates/interpreter/src/instructions/host.rs

Lines 106 to 123 in 688c36c

pub fn blockhash<H: Host + ?Sized>(interpreter: &mut Interpreter, host: &mut H) {
gas!(interpreter, gas::BLOCKHASH);
pop_top!(interpreter, number);
if let Some(diff) = host.env().block.number.checked_sub(*number) {
let diff = as_usize_saturated!(diff);
// blockhash should push zero if number is same as current block number.
if diff <= BLOCK_HASH_HISTORY && diff != 0 {
let Some(hash) = host.block_hash(*number) else {
interpreter.instruction_result = InstructionResult::FatalExternalError;
return;
};
*number = U256::from_be_bytes(hash.0);
return;
}
}
*number = U256::ZERO;
}

This implementation was moved from the blockhash instruction to the host implementation in #1427, and was refactored in #1430

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh, my bad, fixing

let index = number.wrapping_rem(U256::from(BLOCKHASH_SERVE_WINDOW));
return self
.evm
.db
.storage(BLOCKHASH_STORAGE_ADDRESS, index)
.map_err(|e| self.evm.error = Err(EVMError::Database(e)))
.ok()
.map(|v| v.into());
}

Some(B256::ZERO)
self.evm
.block_hash(number)
.map_err(|e| self.evm.error = Err(e))
.ok()
}

fn load_account(&mut self, address: Address) -> Option<LoadAccountResult> {
Expand Down
Loading