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: gateway preparation #3006

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

feat: gateway preparation #3006

wants to merge 30 commits into from
+4,818 −1,207

Conversation

perekopskiy
Copy link
Contributor

What ❔

  • adds new fields to DB tables and rust structs
  • adds new config variables
  • update commitment generator to work with post-gateway
  • adds new vm subversion (vm fast is not changed yet)

Why ❔

prepare for gateway, reduce sync-layer-stable diff

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_supervisor fmt and zk_supervisor lint.

@perekopskiy perekopskiy marked this pull request as ready for review October 3, 2024 09:59
slowli
slowli reviewed Oct 4, 2024
View reviewed changes
core/lib/multivm/src/versions/vm_latest/types/internals/pubdata.rs Outdated Show resolved Hide resolved
core/lib/multivm/src/versions/vm_latest/types/internals/pubdata.rs Outdated Show resolved Hide resolved
core/lib/multivm/src/versions/vm_latest/bootloader_state/utils.rs Outdated Show resolved Hide resolved
core/lib/multivm/src/versions/vm_latest/types/internals/pubdata.rs Outdated Show resolved Hide resolved
core/lib/multivm/src/versions/vm_latest/types/internals/pubdata.rs Outdated Show resolved Hide resolved
core/node/commitment_generator/src/utils.rs Outdated Show resolved Hide resolved
core/node/node_framework/src/implementations/layers/state_keeper/output_handler.rs Outdated Show resolved Hide resolved
core/node/node_framework/src/implementations/layers/state_keeper/output_handler.rs Outdated Show resolved Hide resolved
core/node/proof_data_handler/src/request_processor.rs Outdated Show resolved Hide resolved
core/lib/types/src/block.rs Show resolved Hide resolved
StanislavBreadless
StanislavBreadless previously approved these changes Oct 4, 2024
View reviewed changes
core/lib/multivm/src/versions/vm_latest/types/internals/pubdata.rs Outdated Show resolved Hide resolved
core/lib/multivm/src/versions/vm_latest/types/internals/pubdata.rs Outdated Show resolved Hide resolved
core/lib/types/src/block.rs Show resolved Hide resolved
core/lib/basic_types/src/protocol_version.rs Outdated Show resolved Hide resolved
slowli
slowli reviewed Oct 14, 2024
View reviewed changes
Copy link
Contributor

@slowli slowli left a comment

Choose a reason for hiding this comment

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

Not a full review, just some quick feedback. Will try to look at the PR more exhaustively tomorrow.

Not going to lie, the current PubdataBuilder design looks overly complicated, and in some aspects worse than the ad-hoc design during the last review. I think it could be salvaged though; see line comments.

core/lib/dal/src/consensus/mod.rs Outdated Show resolved Hide resolved
core/lib/vm_interface/src/executor.rs Show resolved Hide resolved
core/lib/vm_interface/src/pubdata/mod.rs Outdated Show resolved Hide resolved
core/lib/vm_interface/src/pubdata/mod.rs Outdated Show resolved Hide resolved
core/lib/vm_interface/src/vm.rs Outdated Show resolved Hide resolved
core/lib/multivm/src/versions/vm_fast/vm.rs Outdated Show resolved Hide resolved
core/lib/vm_executor/src/storage.rs Outdated Show resolved Hide resolved
core/lib/vm_interface/Cargo.toml Outdated Show resolved Hide resolved
core/lib/vm_interface/src/pubdata/mod.rs Outdated Show resolved Hide resolved
@perekopskiy
Copy link
Contributor Author

Not going to lie, the current PubdataBuilder design looks overly complicated, and in some aspects worse than the ad-hoc design during the last review. I think it could be salvaged though; see line comments.

I agree but the previous version was rather naive. I'm trying to find some spot between "keep it as simple as possible and delay dealing with complexity till we need first custom builder" and "implement complex solution that fully allows custom pubdata"

slowli
slowli reviewed Oct 21, 2024
View reviewed changes
Copy link
Contributor

@slowli slowli left a comment

Choose a reason for hiding this comment

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

Sorry for merging #3109 which requires to fix edit conflicts in multivm unit tests (seems quite straightforward – you should move the changes to the test logic in versions::testonly submodules.

core/lib/multivm/src/dump.rs Outdated Show resolved Hide resolved
core/lib/vm_interface/src/vm.rs Show resolved Hide resolved
core/node/state_keeper/src/io/mempool.rs Outdated Show resolved Hide resolved
core/node/state_keeper/src/io/mempool.rs Outdated Show resolved Hide resolved
core/node/state_keeper/src/io/persistence.rs Outdated Show resolved Hide resolved
core/lib/vm_interface/src/utils/dump.rs Outdated Show resolved Hide resolved
core/lib/vm_interface/src/vm.rs Outdated Show resolved Hide resolved
core/node/commitment_generator/src/lib.rs Outdated Show resolved Hide resolved
core/node/commitment_generator/src/lib.rs Outdated Show resolved Hide resolved
core/node/consensus/src/storage/store.rs Outdated Show resolved Hide resolved
slowli
slowli previously approved these changes Oct 22, 2024
View reviewed changes
Copy link
Contributor

@slowli slowli left a comment

Choose a reason for hiding this comment

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

Can't say I've looked thoroughly at 100% of the code, but it looks good in general.

core/lib/vm_interface/src/types/inputs/execution_mode.rs Show resolved Hide resolved
core/lib/vm_interface/src/types/inputs/execution_mode.rs Outdated Show resolved Hide resolved
core/lib/vm_interface/src/utils/shadow.rs Outdated Show resolved Hide resolved
core/node/commitment_generator/src/lib.rs Outdated Show resolved Hide resolved
core/node/commitment_generator/src/utils.rs Show resolved Hide resolved
core/lib/vm_interface/src/pubdata/mod.rs Show resolved Hide resolved
slowli
slowli previously approved these changes Oct 23, 2024
View reviewed changes
@perekopskiy perekopskiy added this pull request to the merge queue Oct 23, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 23, 2024
@github-actions GitHub Actions
Copy link
Contributor

No performance difference detected (anymore)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xVolosnikov 0xVolosnikov 0xVolosnikov left review comments

@slowli slowli slowli left review comments

@StanislavBreadless StanislavBreadless StanislavBreadless left review comments

@RomanBrodetski RomanBrodetski Awaiting requested review from RomanBrodetski

At least 1 approving review is required to merge this pull request.

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
@perekopskiy @slowli @0xVolosnikov @StanislavBreadless