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

Update default l1 gas price value, add CLI arg #26

Closed
wants to merge 1 commit into from

Conversation

AntonD3
Copy link
Collaborator

@AntonD3 AntonD3 commented Aug 4, 2023

What πŸ’»

  • The default l1 gas price value was changed from 50 gwei to 1 gwei
  • CLI argument to set the l1 gas price was added

Why βœ‹

With the l1 gas price of 50 gwei, the gas price per pubdata is 17 * 5010^9 / 250000000 = 3400. It means that we can publish only 8010^6 / 3400 ~ 23529 bytes of the pubdata in one tx(with 80*10^6 gas limit). That's definitely not enough for the tests, usually, contracts are bigger. Also, the possible solution was to increase the gas limit instead, as I know, that's allowed to have a bigger gas limit when the gas is spent on the pubdata, but it can cause issues when the transaction wants to spend more on computation. I think it makes sense to think about it when the gas limit estimation will be implemented.

#[arg(long)]
/// Whether to override the l1 gas price.
/// If not set - fork or default value(`1 gwei`) will be used.
override_l1_gas_price: Option<u64>,
Copy link
Collaborator

@MexicanAce MexicanAce Aug 4, 2023

Choose a reason for hiding this comment

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

Can we avoid having another CLI argument in favor of a new RPC endpoint in the CONFIG namespace? e.g. config_setL1GasPrice(gasPrice: u64)

The idea would be to have the test node configuration editable without restarting the node.

Example (Would go in https://github.com/matter-labs/era-test-node/blob/main/src/configuration_api.rs):

#[rpc(name = "config_setL1GasPrice", returns = "u64")]
fn config_set_l1_gas_price(&self, gas_price: u64) -> Result<u64>;

// ...

fn config_set_l1_gas_price(&self, gas_price: u64) -> Result<u64> {
    let mut inner = self.node.write().unwrap(); // We're cleaning up `.unwrap()` use, just leaving here for simplicity
    inner.l1_gas_price = gas_price;
    Ok(inner.l1_gas_price)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have not seen it, but sounds good. Will take a look, thanks

@MexicanAce
Copy link
Collaborator

Closing stale PR

@MexicanAce MexicanAce closed this Oct 13, 2023
@AntonD3 AntonD3 deleted the ad-l1-gas-price-arg branch October 13, 2023 10:42
IAvecilla pushed a commit to lambdaclass/era-test-node that referenced this pull request Feb 29, 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.

2 participants