-
Notifications
You must be signed in to change notification settings - Fork 100
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-0191: Relax Transaction Constraints - Loading Failures #191
Open
apfitzge
wants to merge
7
commits into
solana-foundation:main
Choose a base branch
from
apfitzge:enable_transaction_loading_failure_fees
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
b2b0c4e
Initial draft
apfitzge 0e3b852
link to feature
apfitzge cc73e72
specify when to check, error variants
apfitzge 41d47a9
reccomendation of check time
apfitzge 30f2741
order
apfitzge 89ccac3
clarifying following
apfitzge ebed71e
linting
apfitzge File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
150 changes: 150 additions & 0 deletions
150
proposals/0191-enable-transaction-loading-failure-fees.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,150 @@ | ||
--- | ||
simd: '0191' | ||
title: Relax Transaction Loading Constraints | ||
authors: | ||
- Andrew Fitzgerald (Anza) | ||
category: Standard | ||
type: Core | ||
status: Review | ||
created: 2024-11-06 | ||
feature: PaymEPK2oqwT9TXAVfadjztH2H6KfLEB9Hhd5Q5frvP (https://github.com/anza-xyz/agave/issues/3244) | ||
supersedes: | ||
superseded-by: | ||
extends: | ||
--- | ||
|
||
## Summary | ||
|
||
This proposal aims to relax certain transaction errors related to loading | ||
transaction accounts, from protocol violations to runtime errors. | ||
Specifically, if a transaction fails to load a valid program account or | ||
exceeds the requested maximum loaded account data size, the transaction | ||
may be included in a block, and the transaction fee will be charged. | ||
|
||
## Motivation | ||
|
||
The current transaction constraints are overly restrictive and adds complexity | ||
in determining whether a block is valid or not. | ||
This proposal aims to relax these loading constraints to simplify the protocol, | ||
and give block-producers more flexibility in determining which transactions | ||
may be included in a block. | ||
The goal is to remove this reliance on account-state in order to validate a | ||
block. | ||
|
||
## New Terminology | ||
|
||
These terms are used elsewhere, but are defined here for clarity: | ||
|
||
- Protocol Violating Transaction Error: A transaction error that violates the | ||
protocol. This class of errors must result in the entire block being rejected | ||
by the network. | ||
- Runtime Transaction Error: A transaction error that results in a failed | ||
transaction, and may be included in the block. These transactions still | ||
incur transaction fees, and nonce advancements. | ||
|
||
## Detailed Design | ||
|
||
Among others, a transaction that fails to load due to violating one of the | ||
following constraints is considered a protocol violation error: | ||
|
||
1. The total loaded data size of the transaction must not exceed | ||
`requested_loaded_accounts_data_size_limit`, or the default limit (64MiB). | ||
2. Any account used as a program in a top-level instruction must: | ||
- be the native loader: `NativeLoader1111111111111111111111111111111` | ||
- OR | ||
- exist | ||
- be executable | ||
- be owned by the native loader: `NativeLoader1111111111111111111111111111111` | ||
- OR | ||
- exist | ||
- be executable | ||
- the owner account be owned by the native loader: `NativeLoader1111111111111111111111111111111` | ||
- the owner account must be executable | ||
|
||
This proposal moves these errors from protocol violations to runtime errors. | ||
A transaction that fails to load due to violating either one of these | ||
constraints may be included in a block, so long as it is otherwise valid. | ||
The transaction must pay transaction fees, and if present, the nonce must be | ||
advanced. | ||
|
||
Constraints SHOULD be checked for each transaction before execution to avoid | ||
unnecessary computation. If a constraint violating transaction is executed, the | ||
constraints MUST be checked BEFORE committing transaction changes. | ||
|
||
The `TransactionError` variants do not need to change from their current | ||
values. This proposal only changes how the validator handles these errors. | ||
|
||
`agave` currently performs the relevant checks in the following order. | ||
This order is not necessary for consensus, and is only provided for clarity. | ||
|
||
For each `Pubkey` included in the transaction message, or loaded from an | ||
address lookup table, the following checks MUST be performed, and SHOULD be | ||
performed in this order for error-consistency: | ||
|
||
- Check if the account exists. If not, assume default account state (empty). | ||
- Accumulate account's `data` field `len`. If the total exceeds the | ||
`requested_loaded_accounts_data_size_limit` (or default if unspecified), | ||
return `MaxLoadedAccountDataSizeExceeded`. | ||
|
||
For each transction-level instruction in the transaction the following | ||
checks MUST be performed, and SHOULD be performed in this order for | ||
error-consistency: | ||
|
||
- If the program account is `native_loader`, continue to next | ||
instruction. | ||
- If the program account does not exist, return `ProgramAccountNotFound` | ||
- If the program account is not executable, return ``InvalidProgramForExecution` | ||
- This only applies until | ||
[SIMD-0162](https://github.com/solana-foundation/solana-improvement-documents/pull/162) | ||
is activated | ||
- If the program account's owner is the native_loader, continue to next | ||
instruction. | ||
- If the program account's owner does not exist, return `ProgramAccountNotFound` | ||
- If the program account's owner is not the native_loader, return ``InvalidProgramForExecution` | ||
- If the program account's owner is not executable, return ``InvalidProgramForExecution` | ||
- This only applies until | ||
[SIMD-0162](https://github.com/solana-foundation/solana-improvement-documents/pull/162) | ||
is activated | ||
- Accumulate the owner account's `data` field `len` and check if the total | ||
exceeds the `requested_loaded_accounts_data_size_limit` (or default if | ||
unspecified), return `MaxLoadedAccountDataSizeExceeded`. | ||
- The owner's data size MUST only be accumulated on the first instruction | ||
that uses the program account. | ||
|
||
## Alternatives Considered | ||
|
||
- Do nothing | ||
- This is the simplest option, as we could leave the protocol as is. | ||
However, this leaves the protocol more complex than it needs to be. | ||
- Relax additional constraints: | ||
- SIMD-0082 sought to relax additional constraints, but has not been | ||
accepted. This proposal is a subset of SIMD-0082, intended to make the | ||
review process simpler and faster. Therefore, we have decided to keep | ||
this proposal focused specifically on certain loading failures. | ||
|
||
## Impact | ||
|
||
- Transactions that would previously have been dropped with a protocol | ||
violation error can now be included and will be charged fees. | ||
- Users must be more careful when constructing transactions to ensure they | ||
are executable if they do not want to waste fees. | ||
- Block-production is simplified as it can be done without needing to load | ||
large program accounts for the initial decision to include a transaction. | ||
|
||
## Security Considerations | ||
|
||
None | ||
|
||
## Drawbacks | ||
|
||
- Users must be more careful about what they sign, as they will be charged fees | ||
for transactions that are included in a block, even if they are not executed. | ||
- This will likely break a lot of tooling, such as explorers, which may expect | ||
all transactions to attempt execution. | ||
|
||
## Backwards Compatibility | ||
|
||
This proposal is backwards compatible with the current protocol, since it only | ||
relaxes constraints, and does not add any new constraints. All previously valid | ||
blocks would still be valid. However, new blocks may not be valid under the old | ||
protocol. |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we specify if these checks are replicated in the runtime, and at what stage? Will the SVM throw these errors? Which errors will be thrown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added: cc73e72