feat(core): adds eip4844 transactions support to eth-signer and eth-sender #976

montekki · 2024-01-31T12:06:39Z

What ❔

Adds the support of EIP4844 transactions to the eth-signer and eth-sender components.

EIP4844 introduces blob transactions that expand the already existing EIP1559 transactions by adding the following information. The main purpose is sending the blobs of data to the network that will only temporarily remain available instead of forever staying in chain history. A single transaction may include multiple blobs.

A new blob gas price field is added to the transaction introducing which introduces a new blob gas market to the protocol
The versioned hashes of the blobs (these first two fields go into tx and remain on-chain forever)
A "sidecar" addition info that includes the blobs themselves, the commitments and proofs of these blobs. This information is only temporarily kept around by CL.

The above mentioned EIP describes signing and network transmission format of these transactions which this PR implements.

Checklist

PR title corresponds to the body of PR (we generate changelog entries from PRs).
Tests for the changes have been added / updated.
Documentation comments have been added / updated.
Code has been formatted via zk fmt and zk lint.
Spellcheck has been run via zk spellcheck.
Linkcheck has been run via zk linkcheck.

core/lib/eth_signer/src/raw_ethereum_tx.rs

montekki · 2024-01-31T12:11:19Z

@koloz193 the testcases in the EIP4844 page are not populated, we need to come up with a strategy to unit test this + integration test

…4-transactions

koloz193 · 2024-01-31T15:00:43Z

@koloz193 the testcases in the EIP4844 page are not populated, we need to come up with a strategy to unit test this + integration test

agreed, for integration tests we can assume that theyll all test using blobs if thats what the server is configured to use. maybe in the long run we may want a new CI step that runs all the tests where pubdata is used as calldata and where pubdata is used as blobs

core/lib/eth_signer/src/raw_ethereum_tx.rs

…4-transactions

core/lib/dal/src/models/storage_eth_tx.rs

core/lib/eth_client/src/clients/http/signing.rs

core/lib/eth_client/src/types.rs

core/lib/zksync_core/src/eth_sender/eth_tx_aggregator.rs

core/lib/zksync_core/src/eth_sender/eth_tx_manager.rs

mm-zk · 2024-02-07T18:37:01Z

sdk/zksync-rs/src/ethereum/mod.rs

                    f.value = Some(value);
                    f.gas_price = Some(gas_price)
                }),
+                None,


maybe consider having a separate method (sign_prepared_tx_with_blobs?)

this way you'll not have to add these None,None in all these places?

it would be a bit less typesafe but a bit more pretty i suppose. i don't have a strong opinion on it

I think this problem would also be solved if we would introduce a custom options type.

mm-zk · 2024-02-07T18:39:18Z

core/lib/eth_client/src/types.rs

+        }
+    }
+
+    pub fn raw_tx(&self, blob_tx_sidecar: Option<&EthTxBlobSidecar>) -> RawTransactionBytes {


can you explain what this method does? Also I'm a little bit surprised, that we are adding this 'blob tx sidecar' from the side (as it seems to really modify the interface).
Maybe have 2 methods instead? (raw_tx) & raw_tx_with_blob ?

(the second one could also check - if we can - that the transaction is really a 'blob type' transaction? )

Before this patch the SignedCallResult contained a public field raw_tx which is essentially a transaction encoded into a byte form. We only supported 1559 transactions and so it made sense to be as it is. Important thing: for 1559 the signed transaction blob prefixed with a byte 0x02 to indicate the tx type is the network form of the transaction, end of story. With the instroduction of 4844 transactions that changes; the signed transaction blob (transaction + signature) + a byte prefix for type is no longer the complete network form for them, there is a new thing ("sidecar") that contains blobs, proofs and commitments + a new way it is all encoded together with the transaction (params + signature).

We want an interface that would leave no space for errors like: you wanted a 4844 tx, but actually went ahead and sent raw_tx field which in itself is not enough for it. All possible solutions to this are not ideal: you may misuse the current interface; you may misuse the interface proposed by you; currently we have no typesafe way of telling one tx from another.

Should blob sidecar be a part of the new and raw_tx will return the correct value depends on it?

the only place where we call this new method is in sign_prepared_tx_for_addr and so we would need to pass the optional sidecar also there although it would be never used there. Which is probably even less ergonomic

@montekki great explanation, and I think it would make an even greater doc comment for future readers; could you please add one?

+1 - let's add this to the comment

…4-transactions

core/lib/dal/migrations/20240201195827_add_4844_specific_tx_fields.up.sql

core/lib/eth_client/src/clients/mock.rs

Deniallugo · 2024-02-08T13:04:06Z

core/lib/eth_client/src/types.rs

+        }
+    }
+
+    pub fn raw_tx(&self, blob_tx_sidecar: Option<&EthTxBlobSidecar>) -> RawTransactionBytes {


Should blob sidecar be a part of the new and raw_tx will return the correct value depends on it?

Deniallugo · 2024-02-08T13:07:38Z

core/lib/eth_client/src/types.rs

+    // itself and the sidecar containing the blobs.
+    async fn test_generating_signed_raw_transaction_with_4844_sidecar_two_blobs() {
+        let private_key =
+            H256::from_str("27593fea79697e947890ecbecce7901b0008345e5d7259710d0dd5e500d040be")


i hope it's newly generated key.

zksync-era/core/lib/env_config/src/eth_sender.rs

Line 79 in 636fcfd

ETH_SENDER_SENDER_OPERATOR_PRIVATE_KEY="0x27593fea79697e947890ecbecce7901b0008345e5d7259710d0dd5e500d040be"

Deniallugo · 2024-02-08T13:12:30Z

core/lib/eth_signer/src/raw_ethereum_tx.rs

+
+        self.rlp_append_access_list(&mut stream);
+
+        stream.append(&self.max_fee_per_blob_gas.unwrap());


should we at least verify that it's the correct tx_type?

core/lib/zksync_core/src/eth_sender/eth_tx_aggregator.rs

…4-transactions

popzxc

Didn't thoroughly check everything, a few surface comments

popzxc · 2024-02-09T07:08:54Z

core/lib/eth_client/src/types.rs

+        }
+    }
+
+    pub fn raw_tx(&self, blob_tx_sidecar: Option<&EthTxBlobSidecar>) -> RawTransactionBytes {


@montekki great explanation, and I think it would make an even greater doc comment for future readers; could you please add one?

popzxc · 2024-02-09T07:12:30Z

core/lib/types/src/eth_sender.rs

+/// A forward-compatible `enum` describing a EIP4844 sidecar
+///
+/// This enum in `bincode`-encoded form is stored in the database
+/// alongside all other transaction-related fields for EIP4844 transactions
+/// in `eth_txs` and `eth_tx_history` tables.
+#[derive(Clone, Deserialize, Serialize)]
+pub enum EthTxBlobSidecar {
+    EthTxBlobSidecarV1(EthTxBlobSidecarV1),
+}


Nice! Could we add some snapshot tests to make sure that we don't break bincode serialization by accident? I'd expect something similar to what we have for prover jobs serialization.

core/lib/eth_client/src/clients/mock.rs

popzxc · 2024-02-09T08:32:16Z

sdk/zksync-rs/src/ethereum/mod.rs

                    f.value = Some(value);
                    f.gas_price = Some(gas_price)
                }),
+                None,


I think this problem would also be solved if we would introduce a custom options type.

…4-transactions

popzxc · 2024-02-12T07:19:00Z

core/lib/dal/src/models/storage_eth_tx.rs

+    // A `EIP_4844_TX_TYPE` transaction blob sidecar.
+    //
+    // Format a `bincode`-encoded `EthTxBlobSidecar` enum.
+    pub blob_sidecar: Option<Vec<u8>>,


Maybe a stupid question, but still. If I'm not mistaken, blobs are taken as whole, even if you use a fraction of it. So we may assume that at times, the blob won't be fully utilized (e.g. each time when pubdata wasn't the criterion to seal the batch), and there will be an "empty" reminder.

Do we want to use any kind of compression when we store/load data? Given that the state size is a problem already, probably we would like to optimize on this front whenever possible.

yes well my current idea here is that as you see Vec<u8> is used for all fields while from the protocol level we need three arrays: [u8; 32] [u8; 48] and [u8; 32*4096]. Two reasons for this:

serde does not implement traits for such const-size arrays

we actually have to store actual data with variable lengths for blob instead of a const array.

So as such there will always be a layer of conversion between a blob expanded to const size and Vec<u8> that is stored in DB and back. We just haven't figured out the ergonomics with @koloz193 but that is the current idea and things will remain as they are now. If we should use some compression for it is another topic; however the versioned blob struct should make it possible to do so in the future if necessary.

we can definitely compress the data at the db level. there are a few things that happen before we store the blob:

we take the raw pubdata and split it into chunks of size 4096*31 lets call these zksync_blobs

for each zksync_blob we then convert it into an ethereum_blob

from the ethereum_blob we compute the other kzg values needed

while the zksync_blob might have trailing zeroes, im not sure if/how many trailing zeroes are in the ethereum_blob. for this we'd want to check if we want to store the zksync_blob or the ethereum_blob. this would introduce a crypto dependency to be able to convert zksync_blob -> ethereum_blob

montekki · 2024-02-27T08:38:05Z

Closing in favor of #1254

initial commit

8b8f2d3

montekki commented Jan 31, 2024

View reviewed changes

core/lib/eth_signer/src/raw_ethereum_tx.rs Show resolved Hide resolved

montekki added 2 commits January 31, 2024 13:25

access list misplaced in encoding procedure

b3e2b44

Merge branch 'main' into fvs-evm-429-update-eth_sender-to-support-484…

776a2d7

…4-transactions

koloz193 reviewed Jan 31, 2024

View reviewed changes

core/lib/eth_signer/src/raw_ethereum_tx.rs Show resolved Hide resolved

montekki added 9 commits February 1, 2024 16:48

Merge branch 'main' into fvs-evm-429-update-eth_sender-to-support-484…

b5a5b03

…4-transactions

create a correct encoding and store it into db

81d08fe

fmt

5fbe97f

cargo sqlx prepare

9765ba0

cargo clippy

a879266

Merge branch 'main' into fvs-evm-429-update-eth_sender-to-support-484…

7f3fd62

…4-transactions

refactorings and lint fix

39e8b13

set type

5f32b8d

set type

3327de2

montekki marked this pull request as ready for review February 6, 2024 07:58

montekki added 5 commits February 6, 2024 16:58

Merge branch 'main' into fvs-evm-429-update-eth_sender-to-support-484…

2cfc5d3

…4-transactions

revert block_dal.rs

204211d

revert blocks_dal.rs again

c3a99cb

Merge branch 'main' into fvs-evm-429-update-eth_sender-to-support-484…

4765efa

…4-transactions

update sqlx

e822257

mm-zk requested changes Feb 7, 2024

View reviewed changes

montekki added 7 commits February 7, 2024 22:35

majority of reviews

e93c394

Merge branch 'main' into fvs-evm-429-update-eth_sender-to-support-484…

d7e6752

…4-transactions

spelling and more comments

031911e

Merge branch 'main' into fvs-evm-429-update-eth_sender-to-support-484…

c43b7e7

…4-transactions

more spelling

42435a7

do not break formatting

de08783

comments and struct rename

07c4605

another spelling

7513690

montekki requested review from popzxc and Deniallugo February 8, 2024 12:48

Deniallugo reviewed Feb 8, 2024

View reviewed changes

montekki added 4 commits February 8, 2024 15:56

Merge branch 'main' into fvs-evm-429-update-eth_sender-to-support-484…

04ea66c

…4-transactions

store the sidecar only once

3449587

create struct for tx and calldata

9935851

Merge branch 'main' into fvs-evm-429-update-eth_sender-to-support-484…

41ba731

…4-transactions

popzxc reviewed Feb 9, 2024

View reviewed changes

montekki added 2 commits February 9, 2024 16:50

create custom options type to expand web3

9daf524

save_eth_tx should have a typed sidecar parameter

837f7e6

RomanBrodetski self-requested a review February 9, 2024 17:19

Merge branch 'main' into fvs-evm-429-update-eth_sender-to-support-484…

95655d2

…4-transactions

popzxc reviewed Feb 12, 2024

View reviewed changes

koloz193 mentioned this pull request Feb 26, 2024

feat(config): Added blobs to commit batches and made da source part of config #1243

Merged

6 tasks

mm-zk approved these changes Feb 26, 2024

View reviewed changes

montekki closed this Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): adds eip4844 transactions support to eth-signer and eth-sender #976

feat(core): adds eip4844 transactions support to eth-signer and eth-sender #976

montekki commented Jan 31, 2024 •

edited

Loading

montekki commented Jan 31, 2024

koloz193 commented Jan 31, 2024

mm-zk Feb 7, 2024

montekki Feb 8, 2024

popzxc Feb 9, 2024

mm-zk Feb 7, 2024

montekki Feb 8, 2024

Deniallugo Feb 8, 2024

montekki Feb 8, 2024

popzxc Feb 9, 2024

mm-zk Feb 26, 2024

Deniallugo Feb 8, 2024

Deniallugo Feb 8, 2024

montekki Feb 8, 2024

Deniallugo Feb 8, 2024

popzxc left a comment

popzxc Feb 9, 2024

popzxc Feb 9, 2024

popzxc Feb 9, 2024

popzxc Feb 12, 2024

montekki Feb 12, 2024

koloz193 Feb 19, 2024

montekki commented Feb 27, 2024


		self.rlp_append_access_list(&mut stream);

		stream.append(&self.max_fee_per_blob_gas.unwrap());

feat(core): adds eip4844 transactions support to eth-signer and eth-sender #976

feat(core): adds eip4844 transactions support to eth-signer and eth-sender #976

Conversation

montekki commented Jan 31, 2024 • edited Loading

What ❔

Checklist

montekki commented Jan 31, 2024

koloz193 commented Jan 31, 2024

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

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

popzxc 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montekki commented Feb 27, 2024

montekki commented Jan 31, 2024 •

edited

Loading