-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat: Validium mode #1017
Conversation
* add variable to .toml * zk fmt
…m_mode_new_fee_model_final
…class/zksync-era into validium_mode_new_fee_model_final
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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}; |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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).
Closing this PR as it is outdated. The new base will be #1015. @RomanBrodetski we can the discussion on the @popzxc your assessment is correct, |
Changes
The commits are self-explanatory, so I'll list them and briefly describe what has been made on them.
era-contracts
andzksync_server
use.VALIDIUM_MODE
env set by theinit.ts
step.chain.toml
file but one, this isl1_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, settingl1_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 withzk init
andzk init --validium-mode
and running the server withzk 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
zk && zk clean --all && zk init
to initialize the system in Rollup mode in a clear environment.zk server
to run the server in one console.cargo run --release --bin validium_mode_example
to run the Validium mode example in another console.cd
into it.python3 operator/app.py
.http://127.0.0.1:5000
in your browser./batch/<batch_number>
For running the example in Validium mode
zk && zk clean --all && zk init --validium-mode
to initialize the system in Validium mode in a clear environment.zk server
to run the server in one console.cargo run --release --bin validium_mode_example
to run the Validium mode example in another console.cd
into it.python3 operator/app.py
.http://127.0.0.1:5000
in your browser./batch/<batch_number>
Results
Config:
max_pubdata_per_batch
pubdata_overhead_part
compute_overhead_part
batch_overhead_l1_gas
internal_enforced_l1_gas_price