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.

Sign up for GitHub

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

Jump to bottom

feat: Server 10k gwei limit on gas price and 1M limit on pubdata price #2460

Merged
merged 15 commits into from
Jul 30, 2024
Merged

feat: Server 10k gwei limit on gas price and 1M limit on pubdata price #2460

merged 15 commits into from
Jul 30, 2024

Conversation

cytadela8
Copy link
Member

@cytadela8 cytadela8 commented Jul 22, 2024 •
edited
Loading

What ❔

  • Limit l2 gas price on server to 10k gwei.
  • Limit pubdata price on server to 1M gwei.
  • Fixes in docs and comments.

Why ❔

Bootloader imposes 10k gwei limit on gas price and 1M limit on pubdata price right now. This limits are not respected by server, what may result in batches being rejected by bootloader.

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.

@cytadela8 cytadela8 requested a review from shahar4 July 22, 2024 13:57
@cytadela8 cytadela8 changed the title Server 10k gwei limit on gas price feat: Server 10k gwei limit on gas price Jul 22, 2024
@cytadela8 cytadela8 marked this pull request as ready for review July 23, 2024 14:29
shahar4
shahar4 reviewed Jul 24, 2024
View reviewed changes
docs/guides/advanced/fee_model.md Outdated Show resolved Hide resolved
core/lib/types/src/fee_model.rs Outdated Show resolved Hide resolved
core/node/fee_model/src/lib.rs Outdated Show resolved Hide resolved
core/node/fee_model/src/lib.rs Show resolved Hide resolved
@cytadela8 cytadela8 requested a review from popzxc July 24, 2024 11:25
@cytadela8 cytadela8 requested a review from shahar4 July 24, 2024 14:47
@cytadela8 cytadela8 changed the title feat: Server 10k gwei limit on gas price feat: Server 10k gwei limit on gas price and 1M limit on pubdata price Jul 24, 2024
@cytadela8 cytadela8 removed the request for review from popzxc July 24, 2024 14:49
mm-zk
mm-zk requested changes Jul 25, 2024
View reviewed changes
core/lib/types/src/fee_model.rs Outdated Show resolved Hide resolved
core/lib/types/src/fee_model.rs Outdated Show resolved Hide resolved
core/lib/types/src/fee_model.rs Outdated Show resolved Hide resolved
core/lib/types/src/fee_model.rs Outdated Show resolved Hide resolved
docs/guides/external-node/09_decentralization.md Outdated Show resolved Hide resolved
core/node/fee_model/src/lib.rs Outdated Show resolved Hide resolved
@cytadela8 cytadela8 requested a review from mm-zk July 25, 2024 10:50
mm-zk
mm-zk previously approved these changes Jul 25, 2024
View reviewed changes
perekopskiy
perekopskiy reviewed Jul 26, 2024
View reviewed changes
Copy link
Contributor

@perekopskiy perekopskiy left a comment

Choose a reason for hiding this comment

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

I think we should somehow track events when parameters are clipped. I assume someone needs to make a decision if we should subsidize or stop operations. We should definitely add a metric and a log to the code. And maybe add an alert for mainnet chains that would page protocol team if @mm-zk is ok.

core/node/fee_model/src/lib.rs Outdated Show resolved Hide resolved
core/node/fee_model/src/lib.rs Outdated Show resolved Hide resolved
docs/guides/advanced/07_fee_model.md Show resolved Hide resolved
@mm-zk
Copy link
Collaborator

mm-zk commented Jul 27, 2024

I think we should somehow track events when parameters are clipped. I assume someone needs to make a decision if we should subsidize or stop operations. We should definitely add a metric and a log to the code. And maybe add an alert for mainnet chains that would page protocol team if @mm-zk is ok.

Adding a metric and log makes sense - but I wouldn't add an alert on our mainnets (with current settings, this would mean that L1 gas price is also over 10'000 Gwei - and we'd be hitting a LOT of different problems before that happens :-)

@cytadela8 cytadela8 requested a review from mm-zk July 30, 2024 08:50
@cytadela8
Copy link
Member Author

I think we should somehow track events when parameters are clipped. I assume someone needs to make a decision if we should subsidize or stop operations. We should definitely add a metric and a log to the code. And maybe add an alert for mainnet chains that would page protocol team if @mm-zk is ok.

Adding a metric and log makes sense - but I wouldn't add an alert on our mainnets (with current settings, this would mean that L1 gas price is also over 10'000 Gwei - and we'd be hitting a LOT of different problems before that happens :-)

  • We have planned to add observability in PE-129. I added a specific acceptance criterion there to monitor l2 gas price for exceeding bootloader limits.
  • Important note is that this is not expected to be an issue in mainnet that we operate because the bootloader limitation itself should be lifted before the first mainnet is live

@perekopskiy
Copy link
Contributor

@cytadela8 But this change is not base-token specific. Same code is executed and gas prices are capped if there is no base token. I want to add logs and metric so that if smth goes wrong (either bug in code or huge l1 gas price on sepolia, etc) we have a way to trace it

@cytadela8
Copy link
Member Author

@cytadela8 But this change is not base-token specific. Same code is executed and gas prices are capped if there is no base token. I want to add logs and metric so that if smth goes wrong (either bug in code or huge l1 gas price on sepolia, etc) we have a way to trace it

I will add a warning.

@cytadela8
Copy link
Member Author

@cytadela8 But this change is not base-token specific. Same code is executed and gas prices are capped if there is no base token. I want to add logs and metric so that if smth goes wrong (either bug in code or huge l1 gas price on sepolia, etc) we have a way to trace it

I will add a warning.

Or do we need a metric?

@perekopskiy
Copy link
Contributor

Log is enough

perekopskiy
perekopskiy previously approved these changes Jul 30, 2024
View reviewed changes
@cytadela8 cytadela8 enabled auto-merge July 30, 2024 12:01
@cytadela8 cytadela8 added this pull request to the merge queue Jul 30, 2024
Merged via the queue into main with commit be238cc Jul 30, 2024
47 checks passed
@cytadela8 cytadela8 deleted the apu-l2-max-gas-price branch July 30, 2024 13:14
@zksync-era-bot zksync-era-bot mentioned this pull request Jul 30, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Aug 1, 2024
@zksync-era-bot
chore(main): release core 24.13.0 (#2502)
4729435 
🤖 I have created a release *beep* *boop*
---


##
[24.13.0](core-v24.12.0...core-v24.13.0)
(2024-07-31)


### Features

* Add recovery tests to zk_supervisor
([#2444](#2444))
([0c0d10a](0c0d10a))
* Added a JSON RPC to simulating L1 for consensus attestation
([#2480](#2480))
([c6b3adf](c6b3adf))
* added dropping all attester certificates when doing hard fork
([#2529](#2529))
([5acd686](5acd686))
* **configs:** Do not panic if config is only partially filled
([#2545](#2545))
([db13fe3](db13fe3))
* **eth-sender:** Make eth-sender tests use blob txs + refactor of
eth-sender tests
([#2316](#2316))
([c8c8334](c8c8334))
* Introduce more tracing instrumentation
([#2523](#2523))
([79d407a](79d407a))
* Remove unused VKs, add docs for BWG
([#2468](#2468))
([2fa6bf0](2fa6bf0))
* Revisit base config values
([#2532](#2532))
([3fac8ac](3fac8ac))
* Server 10k gwei limit on gas price and 1M limit on pubdata price
([#2460](#2460))
([be238cc](be238cc))
* **vlog:** Implement otlp guard with force flush on drop
([#2536](#2536))
([c9f76e5](c9f76e5))
* **vlog:** New vlog interface + opentelemtry improvements
([#2472](#2472))
([c0815cd](c0815cd))
* **zk_toolbox:** add test upgrade subcommand to zk_toolbox
([#2515](#2515))
([1a12f5f](1a12f5f))
* **zk_toolbox:** use configs from the main repo
([#2470](#2470))
([4222d13](4222d13))


### Bug Fixes

* **contract verifier:** Fix config values
([#2510](#2510))
([3729468](3729468))
* fixed panic propagation
([#2525](#2525))
([e0fc58b](e0fc58b))
* **proof_data_handler:** Unlock jobs on transient errors
([#2486](#2486))
([7c336b1](7c336b1))
* **prover:** Parallelize circuit metadata uploading for BWG
([#2520](#2520))
([f49720f](f49720f))
* VM performance diff: don't show 0 as N/A
([#2276](#2276))
([2fa2249](2fa2249))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: zksync-era-bot <zksync-era-bot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@perekopskiy perekopskiy perekopskiy approved these changes

@shahar4 shahar4 Awaiting requested review from shahar4

@tomg10 tomg10 Awaiting requested review from tomg10

@mm-zk mm-zk Awaiting requested review from mm-zk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cytadela8 @mm-zk @perekopskiy @shahar4