Skip to content

chore(l1,l2): ordered genesis files #2713

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
Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

chore(l1,l2): ordered genesis files #2713

wants to merge 13 commits into from

Conversation

fkrause98
Copy link
Contributor

@fkrause98 fkrause98 commented May 8, 2025 •
edited
Loading

Motivation

Ordered genesis files make it easy to diff with one another.

Description

  • Add function to write a Genesis json file with its keys ordered.
  • Genesis files are now ordered by key.

Closes #2706.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 8, 2025 •
edited
Loading

Lines of code report

Total lines added: 131
Total lines removed: 1
Total lines changed: 132

Detailed view 
+-----------------------------------------------------------------+-------+------+
| File                                                            | Lines | Diff |
+-----------------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/genesis.rs                                    | 25    | +25  |
+-----------------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/lib.rs                                        | 10    | +1   |
+-----------------------------------------------------------------+-------+------+
| ethrex/crates/common/types/genesis.rs                           | 757   | +105 |
+-----------------------------------------------------------------+-------+------+
| ethrex/crates/l2/contracts/bin/system_contracts_updater/main.rs | 33    | -1   |
+-----------------------------------------------------------------+-------+------+

@fkrause98 fkrause98 changed the title Ordered genesis files chore(l1,l2): ordered genesis files May 8, 2025
@fkrause98 fkrause98 marked this pull request as ready for review May 8, 2025 19:25
@fkrause98 fkrause98 requested a review from a team as a code owner May 8, 2025 19:25
mpaulucci
mpaulucci requested changes May 9, 2025
View reviewed changes
Copy link
Collaborator

@mpaulucci mpaulucci left a comment

Choose a reason for hiding this comment

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

not sure I agree with this approach, now we're mixing fork specific configs like "muirGlacierBlock" with other configs like "depositContractAddress". What is the standard across Ethereum? I have never seen it be alphabetically ordered keys

mpaulucci
mpaulucci reviewed May 12, 2025
View reviewed changes
cmd/ethrex/Cargo.toml
@@ -87,3 +87,7 @@ harness = false

[build-dependencies]
vergen = { version = "9.0.0", features = ["rustc"] }

[[bin]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO, having a binary for this niche usecase goes against the concept of minimalism/simplicity we want in the repo (the same could also be said to other binaries we have).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would probably just keep that in a branch in case we need it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could keep a separate workspace for development tools.

Copy link
Collaborator

Choose a reason for hiding this comment

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

how would that look like?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll discuss this offline, but this seems like a fairly simple addition.

Arkenan
Arkenan approved these changes May 12, 2025
View reviewed changes
crates/common/types/genesis.rs
let genesis_as_map: Map<String, Value> = serde_json::from_str(&genesis_json)
.map_err(|e| format!("Failed to de-serialize genesis file: {}", e))?;
// Keys sorted based off this ethpandaops example:
// https://github.com/ethpandaops/ethereum-genesis-generator/blob/master/apps/el-gen/mainnet/genesis.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we also order the config fields internally as in the example? That is,

- "chainId",
- "homesteadBlock"
- "daoForkBlock"
- "daoForkSupport"
- "eip150Block"
- "eip150Hash"
- "eip155Block"
- "eip158Block"
- "byzantiumBlock"
- "constantinopleBlock"
- "petersburgBlock"
- "istanbulBlock"
- "muirGlacierBlock"
- "berlinBlock"
- "londonBlock"
- "arrowGlacierBlock"
- "grayGlacierBlock"
- "terminalTotalDifficulty"
- "shanghaiTime"
- "cancunTime"
- "pragueTime"
- "ethash"
- "depositContractAddress"
- "blobSchedule"

This fields also have a reasonable order in them (forks are ordered, blob schedule is last)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

cmd/ethrex/Cargo.toml
@@ -87,3 +87,7 @@ harness = false

[build-dependencies]
vergen = { version = "9.0.0", features = ["rustc"] }

[[bin]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll discuss this offline, but this seems like a fairly simple addition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Oppen Oppen Oppen left review comments

@mpaulucci mpaulucci mpaulucci requested changes

@Arkenan Arkenan Arkenan approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make genesis files easy to compare
4 participants
@fkrause98 @Oppen @mpaulucci @Arkenan