-
Notifications
You must be signed in to change notification settings - Fork 36
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
refactor: remove the trace_decoder
crate
#659
base: develop
Are you sure you want to change the base?
Conversation
9b34032
to
2c7652b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the README needs an update as well (it's mentioning the trace_decoder + includes it in the mermaid graph)
cafa2d9
to
c439a5e
Compare
f7b5823
to
15fb792
Compare
Will update them once this lands (@muursh and I got rights to bypass PR requirements to merge this)
Good question, I'm not even sure this ever got used as it aims to be, probably best to simply remove. I think this came from an initial (long ago) refactoring of the CI by Leo if I'm correct, from which it got kept around. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't try to argue against this, but I'll just reiterate once that I don't believe merging literally everything in a single crate is an ideal approach.
I think it is meaningful to keep the distinction between:
- higher application level
- mid level library (
trace_decoder
) parsing payloads into prover inputs - backend library (
evm_arithmetization
) in charge of the actual proving behind the scene
There are pros and cons in both approaches, none is perfect, but for the time being, this feels to bring more downsides than upsides. This being said, if @muursh and @atanmarko want to approve this, then let's merge it.
Also as a side note, as it looks like this may be the following target, I want to clarify that I am strongly against merging evm_arithmetization
into zero
crate and as codeowner, will reject any attempt to do so for the time being.
I believe we're on similar pages since our extensive discussion in To be clear, my motivation for this change is summarized in this excerpt from our conversation:
This unblocks my forward progress on #93.
Out of curiosity, could you elaborate on these? I think
Thank you for your clarification. |
On "zero knows more about the latter, so the wire parsing should live there":
Sure, my main interests with respect to separate crate with the layout I mentioned above are:
|
@@ -34,7 +32,7 @@ fn criterion_benchmark(c: &mut Criterion) { | |||
block_trace, | |||
other_data, | |||
}| { | |||
trace_decoder::entrypoint( | |||
zero::intra_block_tries::entrypoint( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repeating the point from some previous PR that entrypoint
is not a good API function name.
@@ -1,3 +1,42 @@ | |||
//! An _Ethereum Node_ executes _transactions_ in _blocks_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me intra_block_tries
is a bit misleading and out of context name for a module for somebody looking at the project for the first time. entrypoint
is not just about intermediate tries, it processes node tracer input and metadata and prepares the whole input for the prover. Not sure what would be the good alternative - e.g. witness_processor
, prover_input
, preprocessor
or something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, for somebody looking at what the leader does, crate::prover_input::generate_inputs(block_trace, block_data)
would be much more meaningful than crate::intra_block_tries::entrypoint()
|
||
/// Like `#[serde(with = "::hex")]`, but tolerates and emits leading `0x` | ||
/// prefixes | ||
mod hex { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel there is a lot of potential for some zero/src/common
folder/submodule. Looking at the moment at the 20 files (submodules) in the zero crate it is not obvious what is important, what is the core of the zero
logic and purpose. I think we have gone too far in the "un-cratezation".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minimum fs.rs
, env.rs
, parsing.rs
, tracing.rs
, debug_utils.rs
and this hex
module should be moved to the zero/src/common/
.
Btw. to somehow make the compromise between the @Nashtare and @0xaatif viewpoint, I would keep the ex trace decoder a bit separated from the existing zero code, having all ex trace decoder related code in the Literally, existing |
100% agreed.
I agree with Marko here. I'm not sure I see the argument right now either. |
mostly file renames or plain transplants except:
::trace_decoder::observer
has just been folded into::zero::intra_block_tries
::trace_decoder::{type1,type2}
have been made submodules of::zero::wire_tries
trace_decoder
from the READMERUSTFLAGS
in ci jobs so we have one place where they're defined, and I don't waste a day debugging esoteric linker issues