Program: add custom error codes for readonly writes #43
Conversation
This file contains hidden or 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
buffalojoec
force-pushed
the
exec-ro-error-codes
branch
from
f6465f3 to
713983f
Compare
This comment was marked as resolved.
This comment was marked as resolved.
buffalojoec
force-pushed
the
exec-ro-error-codes
branch
from
713983f to
24dce76
Compare
buffalojoec
changed the title
buffalojoec
force-pushed
the
exec-ro-error-codes
branch
from
24dce76 to
4b0dbbf
Compare
buffalojoec
force-pushed
the
exec-ro-error-codes
branch
from
4b0dbbf to
85efb5b
Compare
joncinque
approved these changes
Nov 8, 2024
Contributor
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Looks good! Just the one bit about the error code, but that can get changed later if needed
program/src/error.rs
Outdated
| pub enum AddressLookupTableError { | ||
| /// Instruction modified data of a read-only account. | ||
| #[error("Instruction modified data of a read-only account")] | ||
| ReadonlyDataModified = 10, |
Contributor
There was a problem hiding this comment.
Is there a reason this is 10? The other PR didn't set a number
Contributor
Author
There was a problem hiding this comment.
Oops, accidentally removed my comment, but it's to avoid collisions with the custom errors from System program.
Contributor
Author
There was a problem hiding this comment.
Added the comment back!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
Working more with the Firedancer conformance harness, yet another inconsequential mismatch between the BPF version and its original builtin version has popped up.
When a builtin program attempts to write to an executable or read-only account, it will be immediately rejected by the
TransactionContext. However, BPF programs do not query theTransactionContextfor the ability to perform a write. Instead, they perform writes at-will, and the loader will inspect the serialized account memory region for any account update violations after the VM has completed execution.The loader's inspection will catch any unauthorized modifications, however, when the exact same data is written to the account, thus rendering the serialized account state unchanged, the program succeeds.
Summary of Changes
In order to maximize backwards compatibility between the BPF version and its original builtin, we add these checks from
TransactionContextto the program directly, to throw even when the data being written is the same as what's currently in the account.Unfortunately, the two
InstructionErrorvariants thrown do not haveProgramErrorcounterparts, so we mock them out with custom error codes.