Skip to content

Conversation

@buffalojoec
Copy link
Contributor

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 the TransactionContext for 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 TransactionContext to the program directly, to throw even when the data being written is the same as what's currently in the account.

Unfortunately, the two InstructionError variants thrown do not have ProgramError counterparts, so we mock them out with custom error codes.

Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks good overall, just a question on the executable check

@buffalojoec buffalojoec changed the title Program: add custom error codes for executable, readonly writes Program: add custom error codes for readonly writes Nov 8, 2024
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks great!

@buffalojoec buffalojoec merged commit 4979b95 into main Nov 9, 2024
9 checks passed
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.

3 participants