Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Add runtime feature set #1636

Closed
ChihChengLiang opened this issue Sep 28, 2023 · 1 comment · Fixed by #1728
Closed

Add runtime feature set #1636

ChihChengLiang opened this issue Sep 28, 2023 · 1 comment · Fixed by #1728
Assignees
Labels
T-feature Type: new features

Comments

@ChihChengLiang
Copy link
Collaborator

Describe the feature you would like

The Scroll's and Taiko's forks have diverged significantly from this codebase. The cause for the divergence is our codebase has some L1-specific features, and the collaborating teams need to customize for L2 use cases.

We plan to increase the modularity of the codebase so that we can minimize the difference between different forks.

A while ago @Brechtpd proposed the idea of having runtime configs. Teams can include the features they want by configuring the feature set.

@ed255 proposed that we introduce a feature set that looks like

enum FeatureSet {
  EthereumL1,
  Scroll,
  Taiko,
}

However, the team-specific flag might be confusing because it doesn't convey what are the functionalities to include and why they are included. For example, Scroll skips TestBalanceOverflow and TestDifficulty in Ethereum tests. But it is hard to expect those tests to be skipped by seeing a feature named "scroll". When we see tests are skipped with if match!(feature_set, FeatureSet::Scroll), it is hard to guess why they are skipped.

I propose we name the FeatureSet with its functionalities.

enum FeatureSet {
  EthereumL1,
  ZeroDifficulty,
  L2Balance,
}

And we define variables to include those features.

const ScrollFeatures = [ZeroDifficulty, L2Balance]

if match!(feature_set, FeatureSet::ZeroDifficulty) would tell the exact reason why a test is skipped.

Additional context

Before we work on an actual PR, we need some input on these.

  • What feature flags should we include?
  • How do we structure the flags? What module should we put the feature set in and where to configure them?
@Brechtpd
Copy link
Collaborator

Brechtpd commented Oct 6, 2023

Feature set with specific features sounds good to me!

On the Taiko side I can think of these features:

  • No fee payment for 1st transaction. The 1st transaction in Taiko is a generated transaction to help sync data between L1 and L2. It also does some additional checks so we don't have to do them directly in a circuit. Maybe in the future this will a be a dedicated transaction type though.
  • Disable standard EIP-1559 calculation to calculate the basefee
  • Invalid tx support

I think that's it, will add later if there are more.

@ed255 ed255 moved this to Milestone Tasks in zkEVM Community Edition Nov 30, 2023
@ed255 ed255 added this to the Feature Completeness milestone Nov 30, 2023
@ed255 ed255 moved this from Milestone Tasks to 📋 Sprint Focus in zkEVM Community Edition Nov 30, 2023
@ChihChengLiang ChihChengLiang mentioned this issue Jan 10, 2024
2 tasks
@ChihChengLiang ChihChengLiang moved this from 📋 Sprint Focus to 🏗 In progress in zkEVM Community Edition Jan 12, 2024
@ed255 ed255 linked a pull request Jan 18, 2024 that will close this issue
2 tasks
@ed255 ed255 moved this from 🏗 In progress to 👀 In review in zkEVM Community Edition Jan 25, 2024
github-merge-queue bot pushed a commit that referenced this issue Jan 31, 2024
### Description

We add an initial structure for runtime config. In this PR, we plan to
add only the invalid tx configuration for the starter.

### Issue Link

#1636 

### Type of change

New feature (non-breaking change which adds functionality)


### Decision

- Allow Geth to take invalid tx. The config doesn't affect geth util


### TODO

- [x] Fix invalid tx test
- [ ] Add tx validity check. (Will continue #1740)
@ChihChengLiang ChihChengLiang moved this from 👀 In review to ✅ Done in zkEVM Community Edition Jan 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-feature Type: new features
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants