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

Add a debugger tool to check Kernel (user code) execution against StructLogs #697

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

LindaGuiga
Copy link
Contributor

This PR aims at handling #221 .

It adds a flag --get-struct-logs to get the transaction StructLogs when fetching generation inputs. Then, during interpreter execution, if running in debugger mode and StructLogs are available, it checks at each step the PC, opcode, gas, stack and possible errors against the StructLogs.

(Helped debug failing block 678.)

@github-actions github-actions bot added crate: trace_decoder Anything related to the trace_decoder crate. crate: evm_arithmetization Anything related to the evm_arithmetization crate. crate: zero_bin Anything related to the zero-bin subcrates. labels Oct 3, 2024
Ok(structlogs)
}

pub mod zerostructlog {
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 you should check @einar-polygon's pr for a different way of doing this

evm_arithmetization/src/cpu/kernel/interpreter.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/cpu/kernel/interpreter.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/cpu/kernel/interpreter.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/cpu/kernel/interpreter.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/cpu/kernel/interpreter.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/cpu/kernel/interpreter.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/cpu/kernel/interpreter.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/generation/segments.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/lib.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/cpu/kernel/interpreter.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/cpu/kernel/interpreter.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/cpu/kernel/interpreter.rs Outdated Show resolved Hide resolved
}

// Gets the struct logs with the necessary logs for debugging.
pub async fn get_structlog_for_debug<ProviderT, TransportT>(
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably rebase this and merge with @einar-polygon branch and PR #682. There structlogs retrieval is optimized to be one call per block.

@@ -81,6 +84,7 @@ struct Cli {
pub(crate) async fn fetch_block_prover_inputs<ProviderT, TransportT>(
Copy link
Contributor

Choose a reason for hiding this comment

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

This will conflict with the work in #682, so you probably need to rebase on top of that.

zero/src/bin/trie_diff.rs Outdated Show resolved Hide resolved
zero/src/prover.rs Outdated Show resolved Hide resolved
Comment on lines +50 to +52
/// If true, returns the struct_logs along with the generation data.
#[arg(long, help_heading = HELP_HEADING, default_value_t = false)]
get_struct_logs: bool,
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 @einar-polygon has alternative cli option which will conflict with this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, Einar's PR definitely conflicts with this one, but I wanted to open the PR against develop since the changes are unrelated to Einar's. I will rebase when his is ready, but I think merging now with his PR might create a lot of "noise" for reviewers, don't you think?

Copy link
Contributor

@atanmarko atanmarko Oct 8, 2024

Choose a reason for hiding this comment

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

Maybe you could make the PR against his branch (rebase on top), then merge to develop after his PR is included there?

Copy link
Contributor

Choose a reason for hiding this comment

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

P.S. In the cases like this with major conflicts, I usually start clean (from his branch), then I cherry pick my changes one by one on top.

@Nashtare Nashtare added this to the Testing and Validation milestone Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate: evm_arithmetization Anything related to the evm_arithmetization crate. crate: trace_decoder Anything related to the trace_decoder crate. crate: zero_bin Anything related to the zero-bin subcrates.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants