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.

Sign up for GitHub

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
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

SIMD-0191: Relax Transaction Constraints - Loading Failures #191

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b2b0c4e
Initial draft
apfitzge Nov 6, 2024
0e3b852
link to feature
apfitzge Nov 6, 2024
cc73e72
specify when to check, error variants
apfitzge Nov 14, 2024
41d47a9
reccomendation of check time
apfitzge Nov 15, 2024
30f2741
order
apfitzge Nov 15, 2024
89ccac3
clarifying following
apfitzge Nov 15, 2024
ebed71e
linting
apfitzge Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions proposals/0191-enable-transaction-loading-failure-fees.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,30 @@ 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 must be checked before committing transactions and voting.
It is reccomended that the constraints are checked before transaction
execution, in order to avoid unnecessary computation.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about

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.

I don't exactly get why you said "and voting".. is specifying before committing sufficient?


The `TransactionError` variants do not need to change from their current
values. This proposal only changes how the validator handles these errors.
For completeness, the following error variants are affected:

- `MaxLoadedAccountDataSizeExceeded` - breaking constraint 1:
- Loaded acount data exceeds the specified or default limit.
- There are proposed changes to the evaluation of this constraint:
[SIMD-0186](https://github.com/solana-foundation/solana-improvement-documents/pull/186)
- `ProgramAccountNotFound`, `InvalidProgramForExecution` - breaking constraint 2.
Currently checks are performed in this order:
Copy link
Contributor

Choose a reason for hiding this comment

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

Annoyingly, if we want errors to be exactly the same.. We need to prescribe that invoked program accounts are checked in tx instruction order as well. And after we check each invoked program, we need to check for MaxLoadedAccountDataSizeExceeded again after adding the owner account's data size 🤮

Copy link
Contributor Author

Choose a reason for hiding this comment

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

proposal to just wrap these all into a new variant TransactionError::LoadError and give up 😭

I will specify it should be in instruction order as well

- (`ProgramAccountNotFound`) Transaction-level instruction invokes an
account that does not exist
- (`InvalidProgramForExecution`) Transaction-level instruction invokes an
account that is not executable
- (`InvalidProgramForExecution`) Transaction-level instruction invokes an
account which is not owned by an account which is owned by the native
loader (only relevant after [SIMD-0162](https://github.com/solana-foundation/solana-improvement-documents/pull/162))
- (``ProgramAccountNotFound`) Transaction-level instruction invokes an
account whose owner does not exist (only relevant after [SIMD-0162](https://github.com/solana-foundation/solana-improvement-documents/pull/162))
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically these should be flipped around right? And we forgot about checking that the owner account is executable as well.


## Alternatives Considered

- Do nothing
Expand Down
Loading