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(contracts)!: integrate protocol defense changes #2737

Merged
merged 115 commits into from
Oct 15, 2024
Merged

feat(contracts)!: integrate protocol defense changes #2737

merged 115 commits into from
Oct 15, 2024

Conversation

koloz193
Copy link
Contributor

What ❔

The work done in the protocol defense project introduced a number of changes, namely custom errors in our solidity contracts. We need to update the server code to handle these new errors.

Why ❔

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.

Deniallugo and others added 25 commits June 25, 2024 10:44
@Deniallugo
use forge for l2 contracts
463d27b 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Fix ignores
0d267a4 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Fix libs
c9dc879 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Fix
1a58ac6 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Add support for new path of sol files
42bdff8 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Merge branch 'main' into deniallugo-forge-l2-contracts
64a01c2
@Deniallugo
Some changes
c619472 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Merge branch 'main' into deniallugo-forge-l2-contracts
3d97914
@Deniallugo
Forge
1e0f6d4 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Fix
f1f9ab5 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Merge branch 'main' into deniallugo-forge-l2-contracts
7bd4637
@Deniallugo
Load correct yul files
512b2e0 
Signed-off-by: Danil <[email protected]>
@Deniallugo
use foundry-zksync
ec6662e 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Remove cludges
4727923 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Add preprocessing
f617cc3 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Merge branch 'main' into deniallugo-forge-l2-contracts
5b59255
@Deniallugo
Remove garbage
2cc02d2 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Add prerequisite for forge zksync
5d49f3a 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Fix
770999f 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Try to use main
accf1cf 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Use main
ddcebe1 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Use main
4044fcc 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Update contracts
839504a 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Merge branch 'main' into deniallugo-forge-l2-contracts
a2906b1
@koloz193
feat(contracts: integrate protocol defense changes
650a63a
@koloz193 koloz193 changed the title feat(contracts: integrate protocol defense changes feat(contracts): integrate protocol defense changes Aug 26, 2024
@Deniallugo Deniallugo changed the title feat(contracts): integrate protocol defense changes feat(contracts)!: integrate protocol defense changes Oct 11, 2024
Deniallugo
Deniallugo previously approved these changes Oct 11, 2024
View reviewed changes
.github/workflows/new-build-contract-verifier-template.yml Outdated Show resolved Hide resolved
@koloz193
Copy link
Contributor Author

@perekopskiy For the server side increases of those values, given it will take a non-trivial amount of work I think we should do it in a follow up PR, the important this here is that it will be enabled from a protocol perspective. For performance, I ran the performance test locally on main and on this branch.

Here are the results:
Screenshot 2024-10-11 at 11 33 51 AM

currently im trying to investigate the degraded performance for access_memory

perekopskiy
perekopskiy previously approved these changes Oct 14, 2024
View reviewed changes
@Deniallugo Deniallugo dismissed stale reviews from perekopskiy and themself via 45ec323 October 14, 2024 10:48
hatemosphere
hatemosphere approved these changes Oct 14, 2024
View reviewed changes
Copy link
Member

@hatemosphere hatemosphere left a comment

Choose a reason for hiding this comment

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

CI changes LGTM

@github-actions GitHub Actions
Copy link
Contributor

Detected VM performance changes

Benchmark name change in estimated runtime change in number of opcodes executed
event_spam +10.1% +0 (NaN%)
deploy_simple_contract +69.7% +0 (NaN%)
finish_eventful_frames_legacy +4.3% N/A
call_far +15.9% +0 (NaN%)
deploy_simple_contract_legacy +61.6% N/A
write_and_decode +12.4% +0 (NaN%)
call_far_legacy +5.7% N/A
finish_eventful_frames +4.5% +0 (NaN%)
decode_shl_sub +14.7% +0 (NaN%)
slot_hash_collision +13.1% +0 (NaN%)
access_memory +9.9% +0 (NaN%)

Changes in number of opcodes executed indicate that the gas price of the benchmark has changed, which causes it run out of gas at a different time. Or that it is behaving completely differently.

@koloz193 koloz193 enabled auto-merge October 15, 2024 12:17
@koloz193 koloz193 added this pull request to the merge queue Oct 15, 2024
Merged via the queue into main with commit c60a348 Oct 15, 2024
42 checks passed
@koloz193 koloz193 deleted the zk-protocol-defense branch October 15, 2024 12:53
This was referenced Oct 15, 2024
Open
Merged
@cytadela8
Copy link
Member

  • Do you plan to increase MAXIMUM_L2_GAS_PRICE, MAXIMUM_PUBDATA_PRICE in server? It won't be easy if go over i32::MAX because db columns are of type bigint, so non-trivial migration will be required. Also, no matter what the increase will be (if any) it should be covered with tests

@perekopskiy bigint in postgresql is 8bytes

@perekopskiy
Copy link
Contributor

@cytadela8 yep, but signed, replace i32 with i64 in my message

github-merge-queue bot pushed a commit that referenced this pull request Oct 23, 2024
@zksync-era-bot
chore(main): release core 25.0.0 (#3094)
0d78228 
🤖 I have created a release *beep* *boop*
---


##
[25.0.0](core-v24.29.0...core-v25.0.0)
(2024-10-23)


### ⚠ BREAKING CHANGES

* **contracts:** integrate protocol defense changes
([#2737](#2737))

### Features

* Add CoinMarketCap external API
([#2971](#2971))
([c1cb30e](c1cb30e))
* **api:** Implement eth_maxPriorityFeePerGas
([#3135](#3135))
([35e84cc](35e84cc))
* **api:** Make acceptable values cache lag configurable
([#3028](#3028))
([6747529](6747529))
* **contracts:** integrate protocol defense changes
([#2737](#2737))
([c60a348](c60a348))
* **external-node:** save protocol version before opening a batch
([#3136](#3136))
([d6de4f4](d6de4f4))
* Prover e2e test
([#2975](#2975))
([0edd796](0edd796))
* **prover:** Add min_provers and dry_run features. Improve metrics and
test. ([#3129](#3129))
([7c28964](7c28964))
* **tee_verifier:** speedup SQL query for new jobs
([#3133](#3133))
([30ceee8](30ceee8))
* vm2 tracers can access storage
([#3114](#3114))
([e466b52](e466b52))
* **vm:** Return compressed bytecodes from `push_transaction()`
([#3126](#3126))
([37f209f](37f209f))


### Bug Fixes

* **call_tracer:** Flat call tracer fixes for blocks
([#3095](#3095))
([30ddb29](30ddb29))
* **consensus:** preventing config update reverts
([#3148](#3148))
([caee55f](caee55f))
* **en:** Return `SyncState` health check
([#3142](#3142))
([abeee81](abeee81))
* **external-node:** delete empty unsealed batch on EN initialization
([#3125](#3125))
([5d5214b](5d5214b))
* Fix counter metric type to be Counter.
([#3153](#3153))
([08a3fe7](08a3fe7))
* **mempool:** minor mempool improvements
([#3113](#3113))
([cd16083](cd16083))
* **prover:** Run for zero queue to allow scaling down to 0
([#3115](#3115))
([bbe1919](bbe1919))
* restore instruction count functionality
([#3081](#3081))
([6159f75](6159f75))
* **state-keeper:** save call trace for upgrade txs
([#3132](#3132))
([e1c363f](e1c363f))
* **tee_prover:** add zstd compression
([#3144](#3144))
([7241ae1](7241ae1))
* **tee_verifier:** correctly initialize storage for re-execution
([#3017](#3017))
([9d88373](9d88373))

---
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StanislavBreadless StanislavBreadless StanislavBreadless left review comments

@hatemosphere hatemosphere hatemosphere approved these changes

@Deniallugo Deniallugo Deniallugo approved these changes

@perekopskiy perekopskiy perekopskiy left review comments

@yorik yorik Awaiting requested review from yorik yorik is a code owner automatically assigned from matter-labs/devops

@alexandrst88 alexandrst88 Awaiting requested review from alexandrst88 alexandrst88 is a code owner automatically assigned from matter-labs/devops

@artmakh artmakh Awaiting requested review from artmakh artmakh is a code owner automatically assigned from matter-labs/devops

@onyxet onyxet Awaiting requested review from onyxet onyxet is a code owner automatically assigned from matter-labs/devops

@otani88 otani88 Awaiting requested review from otani88 otani88 is a code owner automatically assigned from matter-labs/devops

@iluwaa iluwaa Awaiting requested review from iluwaa iluwaa is a code owner automatically assigned from matter-labs/devops

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

Successfully merging this pull request may close these issues.
6 participants
@koloz193 @cytadela8 @perekopskiy @Deniallugo @hatemosphere @StanislavBreadless