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

Scroll evm executor/v35 #1

Closed
wants to merge 33 commits into from
Closed

Conversation

lispc
Copy link

@lispc lispc commented Apr 22, 2024

No description provided.

lispc and others added 25 commits April 16, 2024 15:59
# Conflicts:
#	Cargo.lock
#	crates/precompile/Cargo.toml
#	crates/primitives/Cargo.toml
#	crates/primitives/src/env.rs
#	crates/primitives/src/state.rs
#	crates/primitives/src/utilities.rs
#	crates/revm/Cargo.toml
#	crates/revm/src/db/in_memory_db.rs
# Conflicts:
#	Cargo.lock
#	crates/precompile/Cargo.toml
#	crates/primitives/src/env.rs
#	crates/primitives/src/state.rs
#	crates/primitives/src/utilities.rs
#	crates/revm/Cargo.toml
#	crates/revm/src/db/states/cache_account.rs
#	crates/revm/src/evm_impl.rs
#	crates/revm/src/handler.rs
#	crates/revm/src/handler/mainnet.rs
#	crates/revm/src/journaled_state.rs
# Conflicts:
#	Cargo.lock
#	crates/interpreter/Cargo.toml
#	crates/precompile/Cargo.toml
#	crates/primitives/Cargo.toml
#	crates/primitives/src/env/handler_cfg.rs
#	crates/revm/Cargo.toml
# Conflicts:
#	Cargo.lock
#	crates/precompile/Cargo.toml
#	crates/primitives/Cargo.toml
#	crates/primitives/src/env.rs
#	crates/revm/Cargo.toml
# Conflicts:
#	crates/primitives/src/env/handler_cfg.rs
# Conflicts:
#	crates/revm/src/context.rs
# Conflicts:
#	crates/revm/src/handler.rs
#[cfg(feature = "scroll")]
pub const BERNOULLI: PrecompileWithAddress = PrecompileWithAddress(
crate::u64_to_address(9),
Precompile::Standard(|_input: &Bytes, _gas_limit: u64| Err(PrecompileError::OutOfGas)),
Copy link
Author

Choose a reason for hiding this comment

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

not sure whether use "PrecompileError::OutOfGas" is good. let us recheck later. Err(PrecompileError::NotImplemented)?

Copy link
Member

Choose a reason for hiding this comment

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

done

Choose a reason for hiding this comment

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

Ok

@@ -41,6 +41,10 @@ serde = { version = "1.0", default-features = false, features = [
"rc",
], optional = true }

# scroll
halo2_proofs = { git = "https://github.com/scroll-tech/halo2.git", branch = "v1.1", optional = true }
hash-circuit = { package = "poseidon-circuit", git = "https://github.com/scroll-tech/poseidon-circuit.git", branch = "scroll-dev-1201", optional = true }
Copy link
Author

Choose a reason for hiding this comment

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

branch = "main"

Copy link
Member

Choose a reason for hiding this comment

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

hash-circuit or halo2_proofs?

Copy link
Author

Choose a reason for hiding this comment

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

hash-circuit

GRAY_GLACIER = 14,
MERGE = 15,
SHANGHAI = 16,
BERNOULLI = 17,
Copy link
Author

Choose a reason for hiding this comment

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

hahaha

#[cfg(all(debug_assertions, feature = "scroll"))]
if eq {
assert_eq!(self.keccak_code_hash, other.keccak_code_hash);
}
Copy link
Author

Choose a reason for hiding this comment

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

great code!

Copy link

Choose a reason for hiding this comment

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

very clear, congrats

Cargo.toml Outdated
@@ -22,3 +22,6 @@ debug = true
[profile.ethtests]
inherits = "test"
opt-level = 3

[patch."https://github.com/privacy-scaling-explorations/halo2.git"]
halo2_proofs = { git = "https://github.com/scroll-tech/halo2.git", branch = "v1.1" }
Copy link
Author

Choose a reason for hiding this comment

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

hahah so we need this scroll-tech/poseidon-circuit#28, then we can only use the poseidon primitive crate, no longer needs halo2_proofs, quite confusing.

Copy link
Member

Choose a reason for hiding this comment

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

re-export peer dependency is a good pattern

Copy link
Author

Choose a reason for hiding this comment

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

i mean we should get rid of halo2_proofs in future. How to do this? Split poseidon-circuit crate into 2, one is focusing on hashing computation, not need of halo2, one is focusing on hahsing circuit, need halo2. Here we only need the former

Copy link

Choose a reason for hiding this comment

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

Re-exporting peer dependencies might seem like a shortcut to a clean and concise codebase. After all, it keeps your imports tidy and avoids users from wrestling with dependency conflicts. But this convenience comes at a hidden cost, potentially leading you down a path of dependency management nightmares.

  1. Loss of Version Control: Peer dependencies are meant to be managed by the consumer, not the provider. By re-exporting, you're taking control over a dependency version that another package might need a specific version of. This can lead to conflicts and unexpected behavior when multiple packages using your library have different version requirements for the peer dependency.
    Imagine this: Your library re-exports a peer dependency, say mathjs, at version 5.0.0. Package A, which uses your library, needs mathjs at version 4.2.1 for specific functionality. Package B, also using your library, needs the latest mathjs (version 6.0.0). This creates a conflict. Package A might break because it relies on features removed in 5.0.0, while Package B might malfunction due to incompatibilities introduced in 6.0.0.
  2. Fragile Ecosystem: Re-exported dependencies become tightly coupled to your library's version. If you update your library and that update requires a newer version of the peer dependency, all your consumers are forced to upgrade as well, even if their code perfectly works with the older version. This creates a domino effect, potentially causing ripple effects throughout your project's ecosystem.
    Think of it this way: You build a house on a foundation designed for a specific weight limit. By re-exporting a peer dependency as part of your house, you're changing the foundation's specifications. If you later decide the house needs a stronger foundation, all the houses built on your design (consumers of your library) would need significant work, even if their original design was perfectly functional.
  3. The Illusion of Clarity: While re-exporting might seem to simplify your code on the surface, it actually obscures the true dependency picture. Consumers have no way of knowing which version of the peer dependency they're actually getting or why it's there. This can make debugging issues and understanding code flow more difficult.
    Consider this analogy: You borrow a toolbox from a friend. They've added their own custom wrench but haven't mentioned it. When you use the wrench on a project and it breaks something, troubleshooting becomes harder because you weren't aware of the extra tool and its potential impact.

Choose a reason for hiding this comment

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

hahahaha so we need this scroll-tech/poseidon-circuit#28, then we can only use the poseidon primitive crate, no longer needs halo2_proofs, quite confusing.

@lispc
Copy link
Author

lispc commented Apr 22, 2024

great! high readable codes, with good docs and tests.

@diamondo6
Copy link

Block hash upgrade....

@lispc
Copy link
Author

lispc commented May 10, 2024

since CI is fixed, maybe we can mark this not draft PR?

Comment on lines 75 to 86
#[inline]
#[cfg(feature = "scroll")]
fn code_hash(&mut self, __address: Address) -> Option<(B256, bool)> {
Some((POSEIDON_EMPTY, false))
}

#[inline]
#[cfg(feature = "scroll")]
fn keccak_code_hash(&mut self, __address: Address) -> Option<(B256, bool)> {
Some((KECCAK_EMPTY, false))
}

Choose a reason for hiding this comment

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

Using both feature and not feature seems like an antipattern. Will be a drag in long term maintainance.

#[inline]
    fn code_hash(&mut self, __address: Address) -> Option<(B256, bool)> {
        #[cfg(feature = "scroll")]
        Some((POSEIDON_EMPTY, false))

        Some((KECCAK_EMPTY, false))
    }
    
    

this will also remove un- needed tags on top of code that uses code_hash function also

* fix extcodesize regression

* fix test
@lightsing lightsing deleted the branch v35-upsteam July 8, 2024 07:30
@lightsing lightsing closed this Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants