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

Fairly price storage costs for transactions #2456

Open
wants to merge 6 commits into
base: mainnet-staging
Choose a base branch
from

Conversation

iamalwaysuncomfortable
Copy link
Contributor

@iamalwaysuncomfortable iamalwaysuncomfortable commented May 16, 2024

Motivation

Transactions are currently limited to being 128kb or less in size.

Most execution transactions are small (for instance, transfer_public is 2880 bytes in size). However, transaction size can be greatly increased if the function being executed has very large inputs or very large outputs in the form of nested structs or arrays.

SnarkOS nodes must allocate enough memory to verify all transactions and carry out speculate operations on them. If many executions near the 128kb limit are sent to a network in a short timeframe, it can lead to a large amount of memory growth, long block times, long sync times and fast ledger growth.

Below is an example of memory growth on a validator observed from sending 3 TPS of 124kb transactions over 2 hours.
Screen Shot 2024-05-10 at 3 38 13 PM

Currently a 128kb transaction would only cost .128 credits to execute, which makes it relatively cheap to flood the network with them. Given the deleterious effects of large transactions to network health, it makes sense to impose higher storage costs on larger transactions to ensure users of them are paying the appropriate costs for the effort it takes to process them.

This PR proposes that the storage cost of execution transactions above 5kb in size be penalized with a quadratic function.

$$\text{fee\_microcredits} = \begin{cases} \text{num\_bytes} & \text{if } 0 < \text{num\_bytes} \leq 5 \\\ \frac{{num\_bytes}^2}{1000} & \text{otherwise} & \text{if } \text{num\_bytes} > 5 \\\ \end{cases}$$

An exact function to price storage remains open for debate, and alternatives can certainly be proposed, but it is strongly encouraged storage costs be increased to properly pay for the extra effort the network must perform verify and store them.

NOTE: This would be a breaking change for Testnet, so this should be a pre-mainnet change and integrated into mainnet-staging.

@iamalwaysuncomfortable iamalwaysuncomfortable changed the base branch from testnet-beta to mainnet-staging May 16, 2024 20:31
@raychu86
Copy link
Contributor

When a decision is made, can you post the before and after fees amounts for the credits.aleo functions?

@iamalwaysuncomfortable
Copy link
Contributor Author

When a decision is made, can you post the before and after fees amounts for the credits.aleo functions?

The decision on the function has been made and posted above on the PR description:

The costs of major credits.aleo functions are as follows:

bond_public: 306229 (storage: 1519, finalize: 304710)
join: 2087 (storage: 2087, finalize: 0)
split: 2131 (storage: 2131, finalize: 0)
transfer_private: 2242 (storage: 2242, finalize: 0)
transfer_private_to_public: 26961 (storage: 2141, finalize: 24820)
transfer_public: 51060 (storage: 1420, finalize: 49640)
transfer_public_to_private: 26439 (storage: 1619, finalize: 24820)
unbond_public: 325149 (storage: 1309, finalize: 323840)

There haven't been any changes in storage costs in credits.aleo functions under this pricing model.

@iamalwaysuncomfortable iamalwaysuncomfortable marked this pull request as ready for review May 25, 2024 02:26
console/network/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: vicsn <victor.s.nicolaas@protonmail.com>
Copy link
Contributor

@vicsn vicsn left a comment

Choose a reason for hiding this comment

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

Await merge until Howard approved and CI passes

@iamalwaysuncomfortable iamalwaysuncomfortable added the requires-ledger-reset Merging this PR will require a ledger reset label May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires-ledger-reset Merging this PR will require a ledger reset
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants