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

Soft limit zkApp commands per block #14644

Merged
merged 21 commits into from
Jan 4, 2024

Conversation

Sventimir
Copy link
Contributor

@Sventimir Sventimir commented Nov 30, 2023

Explain your changes:

Explain how you tested your changes:

  • CI
  • The graphql mutation was tested by moving the current code for the ZkApp Limit to the unauthenticated GraphQL endpoint. Here, we tested:
  1. With no limit set the node behaved as usual.
  2. Adding a limit via the unauthenticated GraphQL endpoint did not affect regular payments (sent through GraphQL).
  3. ZkApp transactions sent via a script using o1js were capped at the ZkApp Limit.

Checklist:

  • Dependency versions are unchanged
    • Notify Velocity team if dependencies must change in CI
  • Modified the current draft of release notes with details on what is completed or incomplete within this project
  • Document code purpose, how to use it
    • Mention expected invariants, implicit constraints
  • Tests were added for the new behavior
    • Document test purpose, significance of failures
    • Test names should reflect their purpose
  • All tests pass (CI will check this if you didn't)
  • Serialized types are in stable-versioned modules

@Sventimir
Copy link
Contributor Author

!ci-build-me

@Sventimir Sventimir force-pushed the sventimir/zkapp-cmd-per-block-limit branch from b0d2e0e to 3102d92 Compare November 30, 2023 14:42
@Sventimir
Copy link
Contributor Author

!ci-build-me

@Sventimir Sventimir force-pushed the sventimir/zkapp-cmd-per-block-limit branch from 3102d92 to 4839a88 Compare November 30, 2023 15:22
@Sventimir
Copy link
Contributor Author

!ci-build-me

@bkase
Copy link
Member

bkase commented Nov 30, 2023

we'll wait until discussion on the RFC settles before checking out the PR 👍

@Sventimir Sventimir force-pushed the sventimir/zkapp-cmd-per-block-limit branch from 8e4379e to 83b0b1d Compare December 5, 2023 15:22
Copy link
Member

@nholland94 nholland94 left a comment

Choose a reason for hiding this comment

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

As per the PR, we need some way to make this option dynamically configurable at runtime for the purposes of the authorized GraphQL server for ITN. Given that, it probably makes sense to store a ref somewhere in the application, which gets initialized the the compile time configuration at startup, and can be set dynamically during the daemon's runtime. Then the block producer code can read from that ref when it produces a block.

src/config/fork_at_3757.mlh Outdated Show resolved Hide resolved
src/app/cli/src/cli_entrypoint/mina_cli_entrypoint.ml Outdated Show resolved Hide resolved
src/lib/block_producer/block_producer.ml Outdated Show resolved Hide resolved
src/lib/mina_lib/config.ml Outdated Show resolved Hide resolved
src/lib/staged_ledger/staged_ledger.ml Outdated Show resolved Hide resolved
src/lib/staged_ledger/staged_ledger.ml Outdated Show resolved Hide resolved
src/lib/staged_ledger/staged_ledger.ml Outdated Show resolved Hide resolved
src/lib/staged_ledger/staged_ledger.ml Outdated Show resolved Hide resolved
src/lib/staged_ledger/staged_ledger.ml Show resolved Hide resolved
@Sventimir
Copy link
Contributor Author

Thanks for your comments, @nholland94. I'll incorporate them. Please note, though, that this is still a draft. It's not yet complete or ready for review.

As to configuring the limit dynamically, I'm wary of putting refs at random places in the code. I'd much rather store it in some existing configuration struct so that it's easier to find when looked for. That's why I put it in Mina_lib.Config.t. If there's a better place, I'm happy to move it, although I think making the field mutable will be necessary, I still prefer this to a standalone ref.

@Sventimir
Copy link
Contributor Author

!ci-build-me

@Sventimir Sventimir force-pushed the sventimir/zkapp-cmd-per-block-limit branch from bba43ee to d89299e Compare December 7, 2023 11:58
@ejMina226 ejMina226 marked this pull request as ready for review December 11, 2023 08:29
@ejMina226 ejMina226 requested review from a team as code owners December 11, 2023 08:29
Copy link
Member

@nholland94 nholland94 left a comment

Choose a reason for hiding this comment

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

We should test these changes before landing this work. It doesn't seem the dynamic switching of the soft limit works as written. Also, I would like of you can address the comments I left before about the max size violation on staged ledger diff and the optional arguments.

src/lib/mina_graphql/mina_graphql.ml Outdated Show resolved Hide resolved
src/lib/staged_ledger/staged_ledger.ml Outdated Show resolved Hide resolved
src/lib/staged_ledger/staged_ledger.ml Outdated Show resolved Hide resolved
src/lib/staged_ledger/staged_ledger.ml Outdated Show resolved Hide resolved
src/lib/staged_ledger/staged_ledger.ml Outdated Show resolved Hide resolved
src/lib/staged_ledger/staged_ledger.ml Outdated Show resolved Hide resolved
@deepthiskumar
Copy link
Member

!ci-build-me

@ejMina226
Copy link
Member

!ci-build-me

@ejMina226
Copy link
Member

!ci-build-me

@ejMina226
Copy link
Member

!ci-build-me

@ejMina226
Copy link
Member

!ci-build-me

@mergify mergify bot mentioned this pull request Dec 20, 2023
@ejMina226
Copy link
Member

!ci-build-me

@ejMina226
Copy link
Member

!ci-build-me

@bkase
Copy link
Member

bkase commented Jan 2, 2024

!approved-for-mainnet

@ejMina226
Copy link
Member

!ci-build-me

@ejMina226 ejMina226 merged commit 8c2faea into rampup Jan 4, 2024
36 checks passed
@ejMina226 ejMina226 deleted the sventimir/zkapp-cmd-per-block-limit branch January 4, 2024 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants