feat: implement EIP-4844 #668

DaniPopes · 2023-08-30T20:20:41Z

Implements EIP-4844.

Closes #526.
Supersedes #536.

Rjected

just adding some comments on how we might be able to configure the trusted setup from reth

Rjected · 2023-08-30T23:19:11Z

crates/precompile/build.rs

+fn generate_kzg_settings() {
+    let in_path = "src/kzg/trusted_setup.txt";
+    let out_path = "src/kzg/generated_settings.rs";
+
+    let in_path = Path::new(in_path);
+    println!("cargo:rerun-if-changed={}", in_path.display());
+    assert!(in_path.exists());
+    let contents = format_kzg_settings(in_path);
+    fs::write(out_path, contents).unwrap();
+}


makes sense - generating consts here makes creating the default kzgsettings easy, and does not require reading the file on startup etc

Rjected · 2023-08-30T23:19:21Z

crates/precompile/src/kzg/mod.rs

+fn get_kzg_settings() -> &'static KzgSettings {
+    static SETTINGS: OnceCell<KzgSettings> = OnceCell::new();
+    SETTINGS.get_or_init(|| {
+        c_kzg::KzgSettings::load_trusted_setup(
+            generated_settings::G1_POINTS,
+            generated_settings::G2_POINTS,
+        )
+        .expect("failed to load trusted setup")
+    })
+}


This makes sense given how the precompiles have to access the kzgsettings, but we need to be able to configure this from reth as well somehow.

Maybe we could have a static mut and use a pattern similar to this:
https://github.com/tokio-rs/tracing/blob/bf05c61b7234eb1a196ef8d30f773d3ea6b60ce9/tracing-core/src/dispatch.rs#L329-L366

To provide a way to set the KzgSettings in reth?

I believe they use static mut and manual atomic operations because of MSRV restrictions, once_cell is probably better

mattsse · 2023-08-31T00:38:39Z

just flagging that it probably should be Arc because KzgSettings is not clone

ref

https://github.com/paradigmxyz/reth/blob/893f4cf2a27034f4550d3f4443a0fa7c2cd2444b/bin/reth/src/node/mod.rs#L575-L585

mattsse · 2023-08-31T00:45:36Z

crates/precompile/src/lib.rs

+        static INSTANCE: OnceCell<Precompiles> = OnceCell::new();
+        INSTANCE.get_or_init(|| {
+            let mut precompiles = Self::berlin().clone();


for precompiles, we should consider either an enum

enum Precompile { Fun(fn(&[u8], u64) -> PrecompileResult), Fn(Arc<dyn Send + Sync + Fn(&[u8], u64) -> PrecompileResult) }

or always use the Arc variant,

and a static mut Precompile list that supports configuring global defaults

This is in my TODO list :)

hm we can simplify a lot of things about initialization by adding function signature that has CfgEnv input
No global initialization at all, and we set KzgSettings inside CfgEnv.

@DaniPopes @mattsse @Rjected wdyt?

No global initialization at all, and we set KzgSettings inside CfgEnv.

I am open to that, as long as we can set a default that's hard-coded to Cancun/Sepolia/.. mainnet?

No global initialization at all, and we set KzgSettings inside CfgEnv.

I am open to that, as long as we can set a default that's hard-coded to Cancun/Sepolia/.. mainnet?

yeah this seems much nicer than messing around with globals

DaniPopes · 2023-08-31T00:46:50Z

@mattsse
What's clone needed for? AFAIK you just need to pass the instance pointer into C, so I'm thinking just OnceCell<KzgSettings>

mattsse · 2023-08-31T00:49:06Z

What's clone needed for?

we need the KzgSettings in multiple locations (for example tx pool validation) and ideally, we just want to load it once.

mattsse · 2023-08-31T00:53:53Z

hmm, the way precompiles are currently used

let mut precompiles = Self::berlin().clone();

it's only really useful to allow custom default precompiles starting with cancun hardfork, pre-cancun this doesn't really make sense.

so actually not sure yet what the ideal static mut should look like.
perhaps just a single static mut specifically for the kzg precompile?

or perhaps just use the kzg settings in a static mut

DaniPopes · 2023-08-31T01:06:48Z

we need the KzgSettings in multiple locations (for example tx pool validation) and ideally, we just want to load it once.

You can store a local &'static KzgSettings or get it when needed with a global getter function. I don't think Arcs are needed here.

See #668 (comment) for static muts.

mattsse · 2023-08-31T01:17:35Z

okay @Rjected I figured out how the hardfork precompiles are currently instantiated, and since we only need to access the KzgSettings once, a static Lazy<Mutex<Arc<KzgSettings>> should be enough to support custom KzgSettings if set at the start of the program

DaniPopes · 2023-08-31T03:19:39Z

crates/precompile/src/blob/kzg_settings/mod.rs

@@ -0,0 +1,33 @@
+use c_kzg::KzgSettings;


This module is exported, lmk wyt @Rjected @mattsse

I tend not to usemod inside revm

I think it makes sense here for now, kzg_settings::set_global like tracing::subscriber::set_global_default. It's equivalent of a ZST struct with the module functions as static methods on it. In the future we can think of a better solution.

With EnvCfg path of setting kzg setting, this is unnecessary.

crates/interpreter/src/instructions/host_env.rs

crates/precompile/build.rs

gakonst · 2023-08-31T16:51:13Z

crates/precompile/build.rs

+    // TODO: We can use `hex_literal` to reduce file size
+    // load g1 points
+    let mut g1_points = Vec::with_capacity(n_g1);
+    for _ in 0..n_g1 {
+        let line = lines.next().unwrap();
+        let mut bytes = [0; BYTES_PER_G1_POINT];
+        hex::decode_to_slice(line, &mut bytes).unwrap();
+        g1_points.push(bytes);
+    }
+
+    // load g2 points
+    let mut g2_points = Vec::with_capacity(n_g2);
+    for _ in 0..n_g2 {
+        let line = lines.next().unwrap();
+        let mut bytes = [0; BYTES_PER_G2_POINT];
+        hex::decode_to_slice(line, &mut bytes).unwrap();
+        g2_points.push(bytes);
+    }


Nit but as discussed you don't need to read all the points right?

It's done at compile-time. There's only 2 methods exposed to init a KzgSettings, with the points bytes, or with a file path.

gakonst · 2023-08-31T16:51:51Z

crates/precompile/build.rs

+/*
+/// Pros:
+/// - no need to call `load_trusted_setup` at runtime
+///
+/// Cons:
+/// - larger runtime static size (722K = `4096*32 + 4096*144 + 65*288`)
+/// - larger generated file size (2M)
+/// - possibly unsafe? `as_ptr() as *mut _`
+/// - depends on `c_kzg`; this might not matter much
+fn format_kzg_settings_large(in_path: &Path) -> String {


We should check about this w/ the c-kzg folks.

I think I'll delete the second option, it's way too big to have at runtime

crates/precompile/src/blob/mod.rs

gakonst · 2023-08-31T16:54:35Z

crates/precompile/src/lib.rs

+        static INSTANCE: OnceCell<Precompiles> = OnceCell::new();
+        INSTANCE.get_or_init(|| {
+            let mut precompiles = Self::berlin().clone();


No global initialization at all, and we set KzgSettings inside CfgEnv.

I am open to that, as long as we can set a default that's hard-coded to Cancun/Sepolia/.. mainnet?

bins/revme/src/statetest/runner.rs

mattsse · 2023-09-11T16:39:16Z

so all precompiles need to be configured for every CfgEnv?

that would be super inconvenient and would most likely result in lazy statics anyway,

but the CfgEnv could take an Option of Precompiles if None use the existing defaults?

Rjected · 2023-09-11T19:45:43Z

so all precompiles need to be configured for every CfgEnv?

that would be super inconvenient and would most likely result in lazy statics anyway,

but the CfgEnv could take an Option of Precompiles if None use the existing defaults?

wouldn't it be something like:

enum StandardPrecompile {
  Fun(fn(&[u8], u64) -> PrecompileResult),
  ConfiguredFun(fn(&CfgEnv, &[u8], u64) -> PrecompileResult)
}

and then in call_precompile:

let out = match precompile {
    Precompile::Standard(standard) => match standard {
        StandardPrecompile::Fun(fun) => fun(input_data, gas.limit()),
        StandardPrecompile::ConfiguredFun(fun) => fun(self.env().cfg, input_data, gas.limit())
    },
    Precompile::Custom(fun) => fun(input_data, gas.limit()),
};

The point eval precompile would be:

pub const POINT_EVALUATION: PrecompileAddress =
    PrecompileAddress(ADDRESS, Precompile::Standard(StandardPrecompile::Configured(run)));

fn run(cfg: &CfgEnv, input: &[u8], gas_limit: u64) -> PrecompileResult {
    // ... setup

    // Verify KZG proof
    let commitment = as_bytes48(commitment);
    let z = as_bytes32(&input[32..64]);
    let y = as_bytes32(&input[64..96]);
    let proof = as_bytes48(&input[144..192]);
    // ommitting match for simplicity
    if !KzgProof::verify_kzg_proof(commitment, z, y, proof, cfg.kzg_settings) {
        return Err(Error::BlobVerifyKzgProofFailed);
    }

    Ok((GAS_COST, RETURN_VALUE.to_vec()))
}

The current solution seems to work, ~~if we wanted to go static-less for the kzg settings then I think the above would also work.~~ I guess we would still want a static for the default, which would be the initial value of KzgSettings in the Env. IMO this should be for a follow up, if at all

mattsse · 2023-09-11T22:05:14Z

which would be the initial value of KzgSettings in the Env

this would require non-optional KzgSettings config, which would also make the static default for the cancun precompile redundant, I'd prefer this. Similar to how the default chain id is set to mainnet.

I guess we would still want a static for the default, which would be the initial value of KzgSettings in the Env

ah yes, as @Rjected already mentioned

sorry, for derailing this, I overlooked some of @rakita previous comments here. To which I agree, having reread them.

So we can get rid of the get_global_or_default etc., by adding kzg_settings: Arc<KzgSettings> to CfgEnv which is set to some default settings in Default for CfgEnv?

having non configurable default for the settings is fine I think, even if it requires more work in reth.

the issue with the precompiles remains, either we support Arc< dyn Fn> or as @Rjected suggested support a &CfgEnv argument

rakita · 2023-09-12T11:52:56Z

Unresolved things:

Remove unsafe: https://github.com/bluealloy/revm/pull/668/files#r1313344511
use EnvCfg for setting, feat: implement EIP-4844 #668 (comment) and another explanation here: feat: implement EIP-4844 #668 (comment)
Use Arc for easier cloning: feat: implement EIP-4844 #668 (comment)

rakita · 2023-09-14T12:59:32Z

~~Remove unsafe: https://github.com/bluealloy/revm/pull/668/files#r1313344511~~
Added here: ea2321f
~~use EnvCfg for setting, feat: implement EIP-4844 #668 (comment) and another explanation here: feat: implement EIP-4844 #668 (comment)~~
Moved here: ce19006
~~Use Arc for easier cloning: feat: implement EIP-4844 #668 (comment)~~
Arc was introduced when KzgSetting is moved to Env here: ce19006

Added c-kzg behind std flag, maybe this is just happening on macos as llvm by default does not include wasm32 targets, and because of this, the build is failing for wasm32-wasi, wasm32-unknown-unknown and wasm32-unknown-ecmascript, we can investigate this later.

@DaniPopes @mattsse can you take a look at the changes

mattsse

only doc nits

README.md

crates/precompile/src/kzg_point_evaluation.rs

crates/primitives/src/kzg/env_settings.rs

crates/primitives/src/kzg/generated.rs

crates/primitives/src/utilities.rs

crates/primitives/src/kzg/generated.rs

onbjerg

small qs/nits, overall looks good to me i think

crates/primitives/src/utilities.rs

crates/primitives/src/precompile.rs

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>

mattsse

lgtm

mattsse · 2023-09-15T09:12:57Z

crates/precompile/src/kzg_point_evaluation.rs

+/// The input is encoded as follows:
+/// | versioned_hash |  z  |  y  | commitment | proof |
+/// |     32         | 32  | 32  |     48     |   48  |


DaniPopes

lgtm

rakita · 2023-09-15T11:57:49Z

Just to mention here, we have disabled some of ethereum/tests related to eip4844 that have expectException field set. As the state root is different from test expected value that is generated.

Those tests are:

"emptyBlobhashList.json" // '>=Cancun': TR_EMPTYBLOB
"wrongBlobhashVersion.json"  // '>=Cancun': TR_BLOBVERSION_INVALID
"createBlobhashTx.json" // '>=Cancun': TR_BLOBCREATE
"blobhashListBounds7.json" // ">=Cancun": "TR_BLOBLIST_OVERSIZE"

blobhashListBounds7 and blobhashListBounds6 are very similar but I am not sure why ListBound7 is marked as TR_BLOBLIST_OVERSIZE and it has different behaviour from ListBounds6.

Will treat this as tests not being aligned on what evm should do, and will check later. Additionally asked on Eth R&D discord here: https://discord.com/channels/595666850260713488/753271902520213625/1152206478816583770

crates/precompile/src/kzg_point_evaluation.rs

* feat: point evaluation precompile * feat: BLOBHASH opcode * refactor: revme runner * renames * export global kzg settings * feat: include kzg settings bytes with `include_bytes!` * build.rs: remove second option, update docs * revme: remove unused files and dead code * feat: implement remaining block and tx env fields * Add tests for helper functions, update constants * Add EIP-4844 EF tests to CI, skip outdated ones * chore: make skip_test more readable * Fix tests * Fix fmt * Fix lints, review * fix: validate new tx, block fields; add to balance check * Restore `load_access_list` * chore: drop c-kzg fork * test: update tests from Geth See: <ethereum/go-ethereum#28026> * chore: revert `is_create` change * chore: fmt toml * chore: unnecessary import * remove unsafe from fake_exponential * Remove kzg global settings, and move it to CfgEnv * enable kzg only in std. main README updated * fmt and clippy * Update README.md Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de> * nits and docs * disable exception eip4844 tests, small refactor * revert back last commit refactor --------- Co-authored-by: rakita <dragan0rakita@gmail.com> Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de> Co-authored-by: Waylon Jepsen <57912727+0xJepsen@users.noreply.github.com>

DaniPopes added 4 commits August 30, 2023 22:12

feat: point evaluation precompile

aefca60

feat: BLOBHASH opcode

3342e99

refactor: revme runner

92e627b

renames

37feea7

Rjected reviewed Aug 30, 2023

View reviewed changes

mattsse reviewed Aug 31, 2023

View reviewed changes

export global kzg settings

9c048b4

DaniPopes force-pushed the eip-4844 branch from dc2094a to 9c048b4 Compare August 31, 2023 03:17

DaniPopes commented Aug 31, 2023

View reviewed changes

crates/interpreter/src/instructions/host_env.rs Show resolved Hide resolved

rakita mentioned this pull request Aug 31, 2023

Waylon/eip4844 #536

Closed

feat: include kzg settings bytes with include_bytes!

bb0a0e6

gakonst reviewed Aug 31, 2023

View reviewed changes

DaniPopes added 3 commits August 31, 2023 19:38

build.rs: remove second option, update docs

f4dbaf9

revme: remove unused files and dead code

7792a91

feat: implement remaining block and tx env fields

71a29cb

mattsse mentioned this pull request Aug 31, 2023

Reuse revm eip-4844 impls and constants paradigmxyz/reth#4444

Closed

DaniPopes added 6 commits September 1, 2023 18:29

Add tests for helper functions, update constants

16ff92c

Add EIP-4844 EF tests to CI, skip outdated ones

8d94cc5

chore: make skip_test more readable

44e5bea

Merge branch 'main' into eip-4844

25f0b83

Fix tests

85d25bc

Fix fmt

8c02e4a

rakita reviewed Sep 1, 2023

View reviewed changes

bins/revme/src/statetest/runner.rs Outdated Show resolved Hide resolved

Merge branch 'main' into eip-4844

7bf0578

thedevbirb mentioned this pull request Sep 14, 2023

EIP-6780: SELFDESTRUCT only in same transaction #719

Merged

rakita added 6 commits September 14, 2023 09:49

Merge remote-tracking branch 'origin/main' into eip4844

31807d2

remove unsafe from fake_exponential

ea2321f

Remove kzg global settings, and move it to CfgEnv

ce19006

enable kzg only in std. main README updated

3e6bf70

fmt and clippy

798bc90

Merge remote-tracking branch 'origin/main' into eip4844

bbfa7de

mattsse requested changes Sep 14, 2023

View reviewed changes

mattsse reviewed Sep 14, 2023

View reviewed changes

crates/primitives/src/kzg/generated.rs Show resolved Hide resolved

onbjerg reviewed Sep 14, 2023

View reviewed changes

crates/primitives/src/utilities.rs Outdated Show resolved Hide resolved

crates/primitives/src/precompile.rs Show resolved Hide resolved

rakita and others added 2 commits September 15, 2023 00:40

Update README.md

54aad80

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>

nits and docs

8820a34

mattsse approved these changes Sep 15, 2023

View reviewed changes

DaniPopes commented Sep 15, 2023

View reviewed changes

disable exception eip4844 tests, small refactor

ffda4fd

DaniPopes commented Sep 15, 2023

View reviewed changes

crates/precompile/src/kzg_point_evaluation.rs Outdated Show resolved Hide resolved

revert back last commit refactor

eb8b223

rakita approved these changes Sep 15, 2023

View reviewed changes

rakita merged commit fa13fea into bluealloy:main Sep 15, 2023
8 checks passed

DaniPopes deleted the eip-4844 branch September 28, 2023 15:43

github-actions bot mentioned this pull request Jan 12, 2024

chore: release #973

Closed

shekhirin mentioned this pull request Apr 18, 2024

feat(membership): add DaniPopes from Reth protocolguild/documentation#159

Merged

feat: implement EIP-4844 #668

feat: implement EIP-4844 #668

Conversation

DaniPopes commented Aug 30, 2023

Rjected left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse commented Aug 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaniPopes commented Aug 31, 2023 • edited

mattsse commented Aug 31, 2023

mattsse commented Aug 31, 2023 • edited

DaniPopes commented Aug 31, 2023 • edited

mattsse commented Aug 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse commented Sep 11, 2023

Rjected commented Sep 11, 2023 • edited

mattsse commented Sep 11, 2023 • edited

rakita commented Sep 12, 2023 • edited

rakita commented Sep 14, 2023

mattsse left a comment

Choose a reason for hiding this comment

onbjerg left a comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaniPopes left a comment

Choose a reason for hiding this comment

rakita commented Sep 15, 2023 • edited

DaniPopes commented Aug 31, 2023 •

edited

mattsse commented Aug 31, 2023 •

edited

DaniPopes commented Aug 31, 2023 •

edited

mattsse commented Aug 31, 2023 •

edited

Rjected commented Sep 11, 2023 •

edited

mattsse commented Sep 11, 2023 •

edited

rakita commented Sep 12, 2023 •

edited

rakita commented Sep 15, 2023 •

edited