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

feat: Validium mode #1017

Closed
wants to merge 11 commits into from
Closed

feat: Validium mode #1017

wants to merge 11 commits into from

Conversation

ilitteri
Copy link
Member

@ilitteri ilitteri commented Feb 5, 2024

NOTE: This PR is the Validium base working branch.

Changes

The commits are self-explanatory, so I'll list them and briefly describe what has been made on them.

  • Add Validium mode example. This is a binary that implements an example that will be used for testing things out in both modes.
  • Use Ethereum's L1 gas price. To have more precise results, we're altering the gas costs for mainnet ones, Ethereum's L1 gas cost is between ~35-47 Gwei, and in zkSync mainnet the L2 fair gas price si 0.1 Gwei (the latter is untouched as it is already set).
  • Add --validium-mode flag to zk init command. This changes allow the system to run in Validium mode, spreading out the flag thought the codebase where it is necessary, and also setting the environmental variable for later era-contracts and zksync_server use.
  • Do not construct the pubdata in Validium mode. In Validium mode we do not want to send the pubdata to L1, our workaround on this for the PoC is to not construct it. For that, we read the VALIDIUM_MODE env set by the init.ts step.
  • Do not charge for pubdata in Validium mode. In Validium mode we also do not want to charge the user for pubdata, for that, thanks to the new fee model (V2) most of the changes can be done in the chain.toml file but one, this is l1_pubdata_price, which when set to 0 gives a huge gas use improvement in addition to the said config (yes, said config impacts the gas used by its own, setting l1_pubdata_price to 0 lowers more the gas usage).

Testing scenario

For comparing results between Rollup and Validium mode we are running the validium_mode_example binary with the system initialized both with zk init and zk init --validium-mode and running the server with zk server.

The validium_mode_example binary deposits some ETH to an account which will be used to deploy an ERC20 contract that is going to be called twice, for minting and transferring. For every mentioned action, some useful data like the transaction hash gas used will be printed to the stdout for later analysis.

In parallel, we are also using a debugging tool developed by Marcin that displays batch info. It is very useful to see data like L2 -> L1 messages, Large L2 -> L1 messages, Published bytecodes, and writes to the storage. As a TLDR, it is a server from which you can query batches by number and analyze said data. It's easy to know in which batch a transaction has been added with the eth_getTransactionByHash method.

In the section below you'll find a step-by-step guide to run said test scenario and see the results by yourself

Step-by-step

For running the example in Rollup mode

  • Run zk && zk clean --all && zk init to initialize the system in Rollup mode in a clear environment.
  • Run zk server to run the server in one console.
  • Run cargo run --release --bin validium_mode_example to run the Validium mode example in another console.
  • To run the Batch Status app

For running the example in Validium mode

  • Run zk && zk clean --all && zk init --validium-mode to initialize the system in Validium mode in a clear environment.
  • Run zk server to run the server in one console.
  • Run cargo run --release --bin validium_mode_example to run the Validium mode example in another console.
  • To run the Batch Status app

Results

Config:

Rollup Validium
max_pubdata_per_batch 120.000 1.000.000.000.000
pubdata_overhead_part 0.7 0
compute_overhead_part 0.5 1
batch_overhead_l1_gas 1.000.000 1.000.000
internal_enforced_l1_gas_price 45.000.000.000 45.000.000.000
Fee Model Gas Used Ratio Gas used Improvement
V1 Transaction Rollup Mode Validium Mode Validium gasUsed / Rollup gasUsed Gas Difference / Rollup gasUsed
ERC20 Transfer 581.716 130.366
ERC20 Mint 810.134 129.284
ERC20 Second Deploy 1.884.480 163.230
ERC20 First Deploy 71.551.780 490.930
V2
ERC20 Transfer 794.116 140.431
ERC20 Mint 1.130.534 139.349
ERC20 Second Deploy 2.705.730 173.295
ERC20 First Deploy TODO 500.995

@@ -130,7 +129,7 @@ impl<E: EthInterface> L1GasPriceProvider for GasAdjuster<E> {

fn estimate_effective_pubdata_price(&self) -> u64 {
// For now, pubdata is only sent via calldata, so its price is pegged to the L1 gas price.
self.estimate_effective_gas_price() * L1_GAS_PER_PUBDATA_BYTE as u64
self.estimate_effective_gas_price() * self.config.l1_gas_per_pubdata_byte
Copy link
Collaborator

Choose a reason for hiding this comment

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

The L1_GAS_PER_PUBDATA_BYTE variable is used in many other parts of the code, so we risk inconsistency (if config value changes, this component will consider a different value than others). Additionally, it doesn't feel right that in config when we set the mode (rollup/validium) we also need to set this gas per pubdata byte to zero.

What if we have a trait GasAdjuster with all its logic identical to this file, and two structs that implement it:

struct RollupGasAdjuster {
    l1_gas_per_pubdata_byte: L1_GAS_PER_PUBDATA_BYTE
}

struct ValidiumGasAdjuster {
    l1_gas_per_pubdata_byte: 0
}

This also allows for different DA modes to have slightly different behaviour when predicting L1/DA cost of a transaction

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please also consider this comment for other places where you add/change configs. Would the same reasoning apply?

Copy link
Member Author

@ilitteri ilitteri Feb 7, 2024

Choose a reason for hiding this comment

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

The L1_GAS_PER_PUBDATA_BYTE variable is used in many other parts of the code, so we risk inconsistency (if config value changes, this component will consider a different value than others). Additionally, it doesn't feel right that in config when we set the mode (rollup/validium) we also need to set this gas per pubdata byte to zero.

What if we have a trait GasAdjuster with all its logic identical to this file, and two structs that implement it:

struct RollupGasAdjuster {
    l1_gas_per_pubdata_byte: L1_GAS_PER_PUBDATA_BYTE
}

struct ValidiumGasAdjuster {
    l1_gas_per_pubdata_byte: 0
}

This also allows for different DA modes to have slightly different behaviour when predicting L1/DA cost of a transaction

I completely agree with this (actually, this design passed through my mind but I was not sure because of the complexity it implies and because of it comes with big changes), we'll move forward with this request.

Please also consider this comment for other places where you add/change configs. Would the same reasoning apply?

For sure! We'll take a look at the configs we've added/changed and take a similar approach for that (in the case the conclusion is that it'd be right for us as it is, I'll also let you know so you can see if it's ok or if you have some other idea).

Copy link
Member Author

Choose a reason for hiding this comment

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

The L1_GAS_PER_PUBDATA_BYTE variable is used in many other parts of the code, so we risk inconsistency (if config value changes, this component will consider a different value than others). Additionally, it doesn't feel right that in config when we set the mode (rollup/validium) we also need to set this gas per pubdata byte to zero.

What if we have a trait GasAdjuster with all its logic identical to this file, and two structs that implement it:

struct RollupGasAdjuster {
    l1_gas_per_pubdata_byte: L1_GAS_PER_PUBDATA_BYTE
}

struct ValidiumGasAdjuster {
    l1_gas_per_pubdata_byte: 0
}

This also allows for different DA modes to have slightly different behaviour when predicting L1/DA cost of a transaction

A note on this, making this change will imply changing the actual [eth_sender.gas_adjuster] section for [eth_sender.rollup_gas_adjuster] and adding a [eth_sender.validium_gas_adjuster] one (in addition with another match on the l1_bach_commit_data_generator_mode config to instantiate either RollupGasAdjuster or ValidiumGasAdjuster (I think that now that this config is being used in more than one case we should think in another name for it, if you have any ideas on generalizing Rollup and Validium modes it'd be helpful).

Copy link
Collaborator

Choose a reason for hiding this comment

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

A note on this, making this change will imply changing the actual [eth_sender.gas_adjuster] section for [eth_sender.rollup_gas_adjuster] and adding a [eth_sender.validium_gas_adjuster] one

Hmmm why is it needed though?

Copy link
Member Author

@ilitteri ilitteri Feb 7, 2024

Choose a reason for hiding this comment

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

I'd also suggest setting another constant/config for the Validium's l1_pubdata_price given that as said here: https://github.com/matter-labs/zksync-era/blob/2fa6a7369b0f4fbd94e0dda3cec06ceeda090d15/docs/guides/advanced/fee_model.md#validium--data-availability-configurations

If you're running an alternative DA, you should adjust the l1_pubdata_price to roughly cover the cost of writing one byte to the DA, and set

In the mean time I'll come back with something.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd also suggest setting another constant/config for the Validium's l1_pubdata_price given that as said here:

ah, makes sense. It's indeed possible, but given we don't have alternative DAs (and we can run the first testnet version without one I think), we can add this config later

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it! I'll add a comment in the line and file an issue internally (and in this repo after the code is merged) to track this TODO.

Copy link
Member Author

@ilitteri ilitteri Feb 16, 2024

Choose a reason for hiding this comment

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

Hi @RomanBrodetski, we created a PR to test how this change would look, and we think it adds too much complexity, making the codebase messier just to avoid adding one more configuration parameter. We suggest keeping it simpler, but if there's no way for the current solution to fit (or the new one), we can think of other solutions.

Copy link
Member Author

@ilitteri ilitteri Feb 16, 2024

Choose a reason for hiding this comment

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

As an addition to the above comment, feel free to comment and ask any questions on the linked PR.

@@ -0,0 +1,275 @@
use std::{str::FromStr, time::Duration};
Copy link
Member

@popzxc popzxc Feb 20, 2024

Choose a reason for hiding this comment

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

The core monorepo follows a certain hierarchical structure where all the Rust code is located in the core directory, and adding a crate to the root seems to be inconsistent.

I'm not sure what the right place for this would be, but I guess it can be explained by the fact that this crate has no concrete purpose. I assume that it's useful during the development for debugging/testing, and serves a good example of usage, but what would the use cases be after this PR is merged?

I'm not sure if people would often try to run this example. At the same time, the developers of the core monorepo would have to maintain it to avoid having dead/outdated code.

Overall, I think that it'd be better to either remove this crate or repurpose it to be an integration test (then it can be put to core/tests), so that it's run continously to check the validium mode logic.

[[package]]
name = "actix-codec"
version = "0.5.1"
version = "0.5.2"
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you've run cargo update.
Unfortunately, I have to ask you to roll this change back, as we have several crypto dependencies specified via git, and cargo update may pin them to newer commits, which may be breaking (e.g. the node can start producing different state diffs or have other unexpected consequences).

@ilitteri
Copy link
Member Author

Closing this PR as it is outdated. The new base will be #1015.

@RomanBrodetski we can the discussion on the GasAdjuster trait in lambdaclass#145

@popzxc your assessment is correct, validium_mode_example/ was only meant for development and it wasn't a plan to take it to main. We're removing it in lambdaclass#161 and made a similar integration test in lambdaclass#144.
And regarding the Cargo.lock, I think it is now fixed in lambdaclass:feat_validium_pubdata_abstraction, but if not we'll fix it.

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.

None yet

5 participants