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

Initial refactoring for SMT & Contination support in trace_decoder #197

Merged
merged 10 commits into from
May 3, 2024

Conversation

BGluth
Copy link
Contributor

@BGluth BGluth commented Apr 26, 2024

This PR lays the refactoring groundwork in trace_decoder needed to support both smt and continuations payloads. In effect, this means moving a lot of the existing code around into separate modules.

The biggest changes are:

  • Splitting decoding.rs into separate modules for mpt & smt.
  • Splitting processed_block_trace.rs into separate modules for mpt & smt.
  • Moving logic for processing the "outer" trace protocol structure into it's own module.
  • Slight changes to the trace protocol structure (specifically now the input declares if the format is mpt or smt).
  • Aliasing otherwise identical versions of types between versions of evm_artithmetizationfor mpt & smt usage.

@cffls Would you be able to look at the diff of trace_decoder/src/trace_protocol.rs and make sure this look good from Erigon & CDK's end?

Note that there is still the issue that the global memory allocator in evm_arithmetization currently has a name collision and prevents the test binary from being able to be built (but strangely normal builds are fine). This should be resolved before this PR is merged.

@BGluth BGluth added the crate: trace_decoder Anything related to the trace_decoder crate. label Apr 26, 2024
@github-actions github-actions bot added the crate: mpt_trie Anything related to the mpt_trie crate. label Apr 26, 2024
@BGluth BGluth added this to the Type 2 - Q2 2024 milestone Apr 26, 2024
@cffls
Copy link

cffls commented Apr 27, 2024

trace_protocol.rs looks good to me.

@BGluth BGluth mentioned this pull request May 1, 2024
@BGluth BGluth marked this pull request as ready for review May 1, 2024 14:54
@BGluth BGluth requested review from muursh and Nashtare as code owners May 1, 2024 14:54
trace_decoder/src/decoding_mpt.rs Outdated Show resolved Hide resolved
trace_decoder/src/decoding_mpt.rs Outdated Show resolved Hide resolved
trace_decoder/src/decoding_smt.rs Show resolved Hide resolved
@BGluth BGluth requested a review from wborgeaud as a code owner May 3, 2024 06:19
@github-actions github-actions bot added the crate: evm_arithmetization Anything related to the evm_arithmetization crate. label May 3, 2024
Copy link
Contributor

@wborgeaud wborgeaud left a comment

Choose a reason for hiding this comment

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

LGTM after a first pass

trace_decoder/src/decoding_mpt.rs Outdated Show resolved Hide resolved
@BGluth
Copy link
Contributor Author

BGluth commented May 3, 2024

Just a heads up. I'm going to be doing a rebase to get back up to date with develop, which is going to change history. I think all of the reviews are done, so I'm not too worried about this. Regardless, I'll still make a new commit to address the PR requests.

BGluth added 8 commits May 3, 2024 11:21
- Created type aliases for all `evm_arithmetization` types that we
  need to swap out depending on whether or not we are compiling for
  `mpt` or `smt`.
- Realized that there is no clean way to disable a default feature when
  it absolutely needs to be disabled (eg. when "smt" is enabled).
- Also added a compile time check to enforce that one of these features
  is enabled.
- Prep for `mpt`/`smt` logic feature gating.
Rough draft of SMT & continuation support

- Still need to remove feature gating stuff.
- Realized that we can link again both versions of the library at once
  without conditional compilation.
@BGluth BGluth force-pushed the smt_refactor_prep branch from e9de21d to ac1b15f Compare May 3, 2024 21:13
@github-actions github-actions bot removed the crate: evm_arithmetization Anything related to the evm_arithmetization crate. label May 3, 2024
@BGluth BGluth enabled auto-merge (squash) May 3, 2024 22:40
@BGluth BGluth merged commit 24eb169 into develop May 3, 2024
6 checks passed
@BGluth BGluth deleted the smt_refactor_prep branch May 3, 2024 22:52
@Nashtare
Copy link
Collaborator

Nashtare commented May 4, 2024

Hmm that's my bad on this, but I hadn't realized you had targeted this against develop. Shouldn't all changes related to the type2 be made against feat/type2 until it is feature-complete and stable? (This is what we've been doing for feat/cancun and feat/continuations).

@muursh
Copy link
Contributor

muursh commented May 4, 2024

Hmm that's my bad on this, but I hadn't realized you had targeted this against develop. Shouldn't all changes related to the type2 be made against feat/type2 until it is feature-complete and stable? (This is what we've been doing for feat/cancun and feat/continuations).

Yeah should have been

@BGluth
Copy link
Contributor Author

BGluth commented May 7, 2024

I know we talked about this yesterday, but I'll just do a write-up here as well.

So we could do this, but I would worry that this would hurt overall dev time as we would need to also maintain two separate branches in trace_decoder (which is what we're already doing in evm_arithmetization I guess). If the expected amount of time until we merge in feat/type2 is short, then I think would not be that bad, but if it's expected to be months, then I think it's much better to avoid maintaining two separate branches. This and the upcoming PRs also did a major refactor to trace_decoder, so the longer we waited to merge this back in, the more insane the conflicts probably would get (the upcoming PR already has very bad merge conflicts).

Could we maybe do something different instead? The worst case if there are major breaking changes to evm_arithmetization on feat/type2 is a panic if we receive an SMT payload, so if the user sticks to only working with MPT payloads, that shouldn't be an issue (since the only way feat/type2 is hit is if they receive an SMT payload). We should also lock in the SMT version to a specific commit to avoid compilation breaking.

@Nashtare
Copy link
Collaborator

Nashtare commented May 7, 2024

A fundamental question with the overall approach that I had already shared in the past is why do we even want to maintain two concurrent trace_decoder version, to support MPT and SMT in parallel, as we won't be supporting concurrent type-1 and type-2 in the near/mid-future. It seems to me like the overall workspace should be consistent. trace_decoder and evm_arithemtization should be coupled, such that they both target type-1 or type-2, but having one supporting both concurrently while the other doesn't makes little sense to me.

As for maintenance effort, I agree maintaining branches is a pain. We're currently having:

  • type 1 + shanghai (eg develop)
  • type 2 + shanghai (eg feat/type2)
  • type 1 + cancun (eg feat/cancun)
  • type 1 + continuations (eg feat/continuations, although kind of type X agnostic).

Given that the overall logic of the type2 SMT is dependent on EIP-6780 introduced in Cancun, it would make sense to me to combine both feat/cancun and feat/type2 soon enough. For this, we need to come to a final conclusion as to how to handle it on the evm_arithmetization side. I still feel like from a dev perspective / overall maintenant effort, feature-flagging should be the way to go, though this will be non-negligible and should be handled relatively soon so as to not delay / complicate things further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate: mpt_trie Anything related to the mpt_trie crate. crate: trace_decoder Anything related to the trace_decoder crate.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants