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

[Feature] Introduce BLOCK_SPEND_LIMIT #2565

Draft
wants to merge 11 commits into
base: staging
Choose a base branch
from

Conversation

vicsn
Copy link
Contributor

@vicsn vicsn commented Oct 25, 2024

Motivation

This PR introduces BLOCK_SPEND_LIMIT, the maximum amount in microcredits that can be spent on finalizing executions and synthesizing deployments in a block. This gives much greater certainty of maximum blocktimes. The advantages of this limit, analogous to a gas limit, are:

  • more consistent blocktimes, so improved chain UX and consensus scalability
  • fees are no longer required to be high to prevent against DoS attacks
  • more accurate (validator/mining) rewards if they're assuming a particular blocktime No longer relevant

The new limit is only enabled from CONSENSUS_V2_HEIGHT onwards.

A limit of 950 credits was chosen because:

  • BLOCKTIME is supposed to be 10 seconds.
  • Given the assumption 100 credits == 1 second of runtime, 950 credits translates to 9.5 seconds
  • Which leaves space for Solution verification, this incurs significant free compute. 4 solutions take 180ms to verify on macbook M1 max.
  • To account for execution processing, EXECUTION_FIXED_COST of 2 credits is introduced. This is not reflected in fees, as it is subsidized, but it does count towards the BLOCK_SPEND_LIMIT. Benchmarks on M2 max imply between 15ms and 25ms verification time for executions.
  • The biggest block seen on mainnet so far had 137 executions, which would still remain below this BLOCK_SPEND_LIMIT.

Open questions

  • Flexible limits: could we have a ROUND_SPEND_LIMIT*num_rounds or PROPOSAL_SPEND_LIMIT*num_proposals? Or better: could we impose a spend limit per individual proposal?
  • Only snarkOS-level change or only snarkVM-level change?
  • Should we have to verify transactions/fees before enforcing the spend limit?
  • Note for migration that existing programs will not retroactively be rejected.

Out of scope / future work

  • Ensuring taking fees: We should review and ensure we always take fees from aborted/invalid transactions. Because any block limit effectively lowers the available blockspace by a well targeted attacker who knows how to make their transactions be aborted. 950 credits can be filled through 9 executions or 19 deployments.
  • Reinserting transactions: if transactions ever surpass the spend limit, they will be aborted. Ideally, they are reinserted into batch proposals so that users don't have to retransmit. However, this would require a significant reworking all over snarkOS to ensure duplicate transmission ids are accepted under this circumstance.

Test Plan

  • Local network succeeded, surviving the transition into the new consensus rules.
  • Deployed canary network succeeded, surviving the transition, surviving the transition into the new consensus rules.

Related PRs

Replaces #2371

Comment on lines 868 to 870
if current_block_height >= N::CONSENSUS_V2_HEIGHT {
// If the transaction is an execution, ensure that the total finalize cost does not exceed the block spend limit.
if let Transaction::Execute(_, execution, _) = transaction {
Copy link
Collaborator

@ljedrz ljedrz Oct 25, 2024

Choose a reason for hiding this comment

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

nit: it might be a good idea to swap these conditions in order to not suggest that the code block below contains the only CONSENSUS_V2 logic (since there is also extra logic for deploy txs); alternatively, that extra logic applicable to deploy txs could be moved under this code block (without altering the order), keeping all the V2 logic in one place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree I like the clarity: fcbbb68

@@ -86,6 +89,12 @@ fn execution_storage_cost<N: Network>(size_in_bytes: u64) -> u64 {
}
}

/// Returns the fixed cost for an execution.
/// NOTE: this constant reflects the compute cost of an execution, but is not required to be paid by the user.
pub fn execution_fixed_cost<N: Network>() -> u64 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

are there plans to extend this logic (in which case an alternative name might be more fitting)? I'm wondering about the rationale for the existence of a (non-const) function wrapper for a const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I swear this was in my git staging area already: 3443c3e

are there plans to extend this logic (in which case an alternative name might be more fitting)?

Unclear at the moment. Only other name I can imagine is execution_compute_cost?

@vicsn
Copy link
Contributor Author

vicsn commented Oct 28, 2024

@zkxuerb @zosorock do you have permission to draft this PR? Otherwise I'll close it. Still needs significant thinking.

@alzger alzger marked this pull request as draft November 7, 2024 14:52
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.

3 participants