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

SIMD-0172: Reduce default CU per instruction #172

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

Conversation

tao-stones
Copy link
Contributor

No description provided.

@tao-stones tao-stones marked this pull request as ready for review September 4, 2024 22:40
@cavemanloverboy
Copy link

cavemanloverboy commented Sep 5, 2024

  1. need to reduce CUs of compute budget instructions to zero if you're going to require them imo. enables hyperoptimization of programs as well. Some programs use less cus than a compute budget instruction

  2. if CB ixs are going to be required, just make a new transaction format where you add 12 bytes to the header for u64 limit and u32 price. No need to pass in 32 byte key for the compute budget program + ix discriminators.

@cavemanloverboy
Copy link

cavemanloverboy commented Sep 5, 2024

on 2: perhaps another two u32 for the heap limit + loaded data limit. All of this together is less than the 32 byte CB pubkey

@tao-stones
Copy link
Contributor Author

Once this is implemented, we could eventually move all limits set by compute-budget instructions into the Transaction Header, allowing us to retire the compute-budget program entirely. However, that's outside the scope of this proposal.

`set-compute-unit-limit` for their transactions to ensure accurate compute unit
allocation. However, adding compute-budget instructions reduces the available
space within the 1232-byte transaction limit, which could pose challenges for
some developers.
Copy link
Contributor Author

@tao-stones tao-stones Sep 5, 2024

Choose a reason for hiding this comment

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

add concerns of compute-budget instructions would further reduce tx space. (thanks @cavemanloverboy for bring it up).

Both CB instructions take up tx space and consume CU budget, can be addressed, in longer term, but replacing CB program with Transaction Header, or similar approaches. In immediate term, if absolutely necessary, could take Alternative #1 approach (eg lower to a value that reduce over-estimation yet not force devs to include CB ixs), only necessary.

@Benhawkins18 Benhawkins18 changed the title Reduce default CU per instruction SIMD-0172: Reduce default CU per instruction Sep 10, 2024
@loopcreativeandy
Copy link

overall I like this proposal!

My recommendation would be to set the default to the minimum required by an instruction. As far as I can tell that would be 150CU per instruction.
This would still guarantee that blocks are filled efficiently while also guaranteeing that small transactions (like SOL transfers) still work without calling the CB program (which would also double the minimum CU from 150 to 300 since a CB instruction also requires CU).

You write in 3. that simple transactions are not affected. Is that an explicit feature of this SIMD? Or how would that work exactly?

@steveluscher
Copy link

I have spent zero seconds fully thinking through the implications of this suggestion, but I’ll present it anyway. In order to avoid the mandatory cost of including a compute budget instruction we could advance to a world with v1 transactions that include the budget as part of the byte schema. One nice side effect of this is that once v1 transactions become mandatory, as specified in this SIMD, apps can completely drop support for v0 and legacy transactions.

@cloakd
Copy link

cloakd commented Sep 22, 2024

If your going to make such sweeping changes it needs to be focused on the long term

Adding an interim fix just adds more pain for developers

Priority fee & cu budget should be in the headers in a new txn version to take up minimal space rather than further reducing the available byte space for instructions, it's already hard enough fitting more complex instructions without loosing an additional 42 bytes for an "interim" fix

@cryptopapi997
Copy link

cryptopapi997 commented Sep 22, 2024

Agree with all of the above, I don't think I've seen a SIMD more fitting for a new tx version since versioned txs were introduced:

  • affects nearly every type of tx
  • requires changes in nearly every tx building code
  • can be solved way more efficiently by slightly changing the structure of txs as mentioned by others before me

This change is focused on the long term anyway, why make it a short-term devex pain when this can be avoided?

@alessandrod
Copy link

Some random notes from twitter convos so they don't get lost:

  • Is 20k per epoch perhaps a little aggressive time wise?
  • Regarding the tx data bloat concern: do we have numbers on how many txs today are already loading the ComputeBudget program? The key is the largest offender wrt space - if it's already there, it's not as bad IMO.
  • A new TX version needs its own SIMD. Someone needs to champion it, think through pros and cons, and do the work. While/if that happens, does it make sense to go ahead with this SIMD? (IMO yes, but people may want to argue otherwise).

@alessandrod
Copy link

Also: have we measured/estimated what kind of impact is this going to have?

@zfedoran
Copy link

This SIMD would mean that any durable nonced transactions created before this change would probably not be valid anymore. Durable nonced transactions don’t currently have an expected expiration date.

We make use of nonced transactions to allow us to close dormant accounts, along with a number of other use cases. Often it is not possible to get a new signature.

@tao-stones
Copy link
Contributor Author

You write in 3. that simple transactions are not affected. Is that an explicit feature of this SIMD? Or how would that work exactly?

SIMD-0170 makes all builtin instructions consume exact CUs without the need of explicit set-compute-unit-limit.

@jacobcreech
Copy link
Contributor

Also: have we measured/estimated what kind of impact is this going to have?

Did some data analysis on this, it'd impact roughly ~11% of all transactions in the past 30 days in current state.

@tao-stones
Copy link
Contributor Author

it'd impact roughly ~11% of all transactions in the past 30 days in current state.

Some additional datapoints to make case for rather quicker pace of reduction to 20K (even 10K) as DEFAULT, then slowly reduce to 0:

  • Amount those transactions could be impacted (eg transactions without requesting cu-limit), nearly half (45+%) actually consumed less than 5K CUs. These are likely simple builtin transactions. Lowering default instruction CUs would have less, if not none, impact on builtin-only transactions.
  • another 40% impacted transactions actually consumed 20K - 80K Cus.

Further, if DEFAULT is lowered to 20K, 4% transactions would fail; if lowered to 10K, 10% would fail. (blue line in this chart. Red line is percentage over impacted txs - txs without cu-limit)

image
  • note: all transactions referred here are non-vote transactions.

@cavemanloverboy
Copy link

semilog plots are your friend

@tao-stones
Copy link
Contributor Author

I analyzed transaction size limitations to better understand the fail rate mentioned above from a user perspective. Transactions with enough space to include a compute-unit-limit instruction can be updated to continue functioning under the reduced DEFAULT environment. However, for transactions with less than 32 bytes of available space, more extensive work will be required for upgrades.

To determine how many transactions in mainnet-beta fall into this second category, I examined 654,431,955 'non-vote and no CU-limit' transactions. The table below shows the number of these transactions as the DEFAULT limit is lowered.

set DEFAULT to number of would-fail txs have no space for CB ix percentage
20,000 343,220 0.05%
10,000 692,732 0.11%
5,000 1,285,490 0.20%

So, if we lower DEFAULT to 10,000 CUs, 219,826,448 transactions (about 33%) need and can add compute-unit-limit instruction, and 692,732 transactions (about 0.11%) need more work to be able to make room for compute-unit-limit.

@sakridge sakridge changed the title SIMD-0172: Reduce default CU per instruction SIMD-0172: Reduce default CU per instruction to 0 Nov 18, 2024
@sakridge sakridge changed the title SIMD-0172: Reduce default CU per instruction to 0 SIMD-0172: Reduce default CU per instruction Nov 18, 2024
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.

9 participants