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

Jump to bottom

feat: TracerEip3155 optionally traces memory #1234

Merged
merged 4 commits into from
Mar 29, 2024
Merged

feat: TracerEip3155 optionally traces memory #1234

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
138ad5b
feat: TracerEip3155 optionally traces memory
Otto-AA Mar 27, 2024
1279c02
perf: use hex::encode_prefixed for TracerEip3155
Otto-AA Mar 28, 2024
24a90c8
refactor: use builder pattern for TracerEip3155
Otto-AA Mar 28, 2024
8a9194f
refactor: use String for TracerEip3155 memory
Otto-AA Mar 29, 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
3 changes: 2 additions & 1 deletion bins/revme/src/cmd/statetest/runner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ pub fn execute_test_suite(
.reset_handler_with_external_context(TracerEip3155::new(
Box::new(stderr()),
false,
false,
))
.append_handler_register(inspector_handle_register)
.build();
Expand Down Expand Up @@ -421,7 +422,7 @@ pub fn execute_test_suite(
let mut evm = Evm::builder()
.with_spec_id(spec_id)
.with_db(state)
.with_external_context(TracerEip3155::new(Box::new(stdout()), false))
.with_external_context(TracerEip3155::new(Box::new(stdout()), false, false))
.append_handler_register(inspector_handle_register)
.build();
let _ = evm.transact_commit();
Expand Down
21 changes: 18 additions & 3 deletions crates/revm/src/inspector/eip3155.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ pub struct TracerEip3155 {
refunded: i64,
mem_size: usize,
skip: bool,
include_memory: bool,
memory: Option<Vec<u8>>,
Copy link
Member

Choose a reason for hiding this comment

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

Onee more thing, it would be good to have String here so we don't copy memory multiple times.

Now we are doing clone in step and clone into String in step_end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it to String now, however my change did not improve the performance. I guess the self.memory.take() method also copies it internally. Without the .take() I've got the error "cannot move out of self.memory which is behind a mutable reference" and didn't manage to fix it in a better way.

Copy link
Member

Choose a reason for hiding this comment

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

take() does not copy full string, it is the right approach. Copying memory on every step is a heavy operation, not a lot we could do there. There are few optimisation that we could do but they require introspection of opcodes that change memory, definitely out of scope for this PR.

}

// # Output
Expand Down Expand Up @@ -58,7 +60,7 @@ struct Output {
error: Option<String>,
/// Array of all allocated values
#[serde(default, skip_serializing_if = "Option::is_none")]
memory: Option<Vec<String>>,
memory: Option<String>,
rakita marked this conversation as resolved.
Show resolved Hide resolved
/// Array of all stored values
#[serde(default, skip_serializing_if = "Option::is_none")]
storage: Option<HashMap<String, String>>,
Expand Down Expand Up @@ -98,12 +100,14 @@ impl TracerEip3155 {
}

impl TracerEip3155 {
pub fn new(output: Box<dyn Write>, print_summary: bool) -> Self {
pub fn new(output: Box<dyn Write>, print_summary: bool, include_memory: bool) -> Self {
Copy link
Member

@rakita rakita Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Would not add include_memor flag here but a new function with_memory(self) -> Self

This would allow setting TracerEip3155 as TracerEip3155::new(Box::new(std::io::stdout()), true).with_memory()`

Would be good to add same for print_summary but your choice if you want it in 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.

Thanks for the suggestion, I've implemented it for both now.
For the summary I've defaulted to true, as it is a MUST for the (current) EIP-3155. This implicitly adds it to the revme execute_test_suite stderr and stdout. I could change that if that's not wanted.

For the memory I've defaulted to false, as it's not mandatory and often not wanted due to performance issues.

Self {
output,
gas_inspector: GasInspector::default(),
print_summary,
include_memory,
stack: Default::default(),
memory: Default::default(),
pc: 0,
opcode: 0,
gas: 0,
Expand All @@ -128,6 +132,11 @@ impl<DB: Database> Inspector<DB> for TracerEip3155 {
fn step(&mut self, interp: &mut Interpreter, context: &mut EvmContext<DB>) {
self.gas_inspector.step(interp, context);
self.stack = interp.stack.data().clone();
self.memory = if self.include_memory {
Some(interp.shared_memory.context_memory().to_owned())
} else {
None
};
self.pc = interp.program_counter();
self.opcode = interp.current_opcode();
self.mem_size = interp.shared_memory.len();
Expand Down Expand Up @@ -159,7 +168,13 @@ impl<DB: Database> Inspector<DB> for TracerEip3155 {
} else {
None
},
memory: None,
memory: match &self.memory {
Some(memory) => {
let hex_memory = hex::encode(memory);
Some(format!("0x{hex_memory}"))
}
None => None,
},
storage: None,
return_stack: None,
};
Expand Down
2 changes: 1 addition & 1 deletion examples/db_by_ref.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ fn run_transaction_and_commit(db: &mut CacheDB<EmptyDB>) -> anyhow::Result<()> {
fn main() -> anyhow::Result<()> {
let mut cache_db = CacheDB::new(EmptyDB::default());

let mut tracer = TracerEip3155::new(Box::new(std::io::stdout()), true);
let mut tracer = TracerEip3155::new(Box::new(std::io::stdout()), true, false);

run_transaction_and_commit_with_ext(&mut cache_db, &mut tracer, inspector_handle_register)?;
run_transaction_and_commit(&mut cache_db)?;
Expand Down
2 changes: 1 addition & 1 deletion examples/generate_block_traces.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ async fn main() -> anyhow::Result<()> {
let mut state = StateBuilder::new_with_database(cache_db).build();
let mut evm = Evm::builder()
.with_db(&mut state)
.with_external_context(TracerEip3155::new(Box::new(std::io::stdout()), true))
.with_external_context(TracerEip3155::new(Box::new(std::io::stdout()), true, false))
.modify_block_env(|b| {
if let Some(number) = block.number {
let nn = number.0[0];
Expand Down
Loading