SIMD-0191: Relax Transaction Constraints - Loading Failures #191
Conversation
apfitzge
force-pushed
the
enable_transaction_loading_failure_fees
branch
from
085412a to
b9a67a7
Compare
apfitzge
changed the title
apfitzge
force-pushed
the
enable_transaction_loading_failure_fees
branch
from
b9a67a7 to
b2b0c4e
Compare
| type: Core | ||
| status: Review | ||
| created: 2024-11-06 | ||
| feature: PaymEPK2oqwT9TXAVfadjztH2H6KfLEB9Hhd5Q5frvP |
There was a problem hiding this comment.
Let's link to the github issue instead: anza-xyz/agave#3244
There was a problem hiding this comment.
Added link with feature key as well
| - 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. |
There was a problem hiding this comment.
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?
| 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. |
There was a problem hiding this comment.
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?
| - (`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)) |
There was a problem hiding this comment.
Technically these should be flipped around right? And we forgot about checking that the owner account is executable as well.
| - `ProgramAccountNotFound`, `InvalidProgramForExecution` - breaking constraint 2. | ||
| Currently checks are performed in this order: |
There was a problem hiding this comment.
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 🤮
There was a problem hiding this comment.
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
|
@topointon-jump I think I addressed your previous review. Could you re-review this? |
This proposal was split out of #82 in order to simplify the review process and have a more tightly coupled SIMD-feature activation relationship.