Skip to content

Make InstructionAccount compatible with #[repr(C)]#6829

Merged
LucasSte merged 5 commits intoanza-xyz:masterfrom
LucasSte:ix-acc-1
Jul 11, 2025
Merged

Make InstructionAccount compatible with #[repr(C)]#6829
LucasSte merged 5 commits intoanza-xyz:masterfrom
LucasSte:ix-acc-1

Conversation

@LucasSte
Copy link
Copy Markdown

@LucasSte LucasSte commented Jul 3, 2025

Problem

In runtime ABIv2, we want to share the same data structures between programs and the validator. In SIMD-0177, we propose the usage of InstructionAccount to hold the information about the accounts an instruction needs in its own memory region.

Exposing the data structure to programs means it must be #[repr(C)] in Rust so that it can have a stable layout between Rust versions.

Rust does not support bool types in #[repr(C)], so InstructionAccount is not stable.

Summary of Changes

  1. Make is_signer and is_writable private.
  2. Create accessor and modifier functions for the is_signer and is_writable members.
  3. Create a constructor for InstructionAccount, since two of its members are now private.
  4. Change the inner type of is_signer and is_writable to u8.
  5. Adapt the impacted code.

@mergify
Copy link
Copy Markdown

mergify Bot commented Jul 3, 2025

The Firedancer team maintains a line-for-line reimplementation of the
native programs, and until native programs are moved to BPF, those
implementations must exactly match their Agave counterparts.
If this PR represents a change to a native program implementation (not
tests), please include a reviewer from the Firedancer team. And please
keep refactors to a minimum.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 3, 2025

Codecov Report

❌ Patch coverage is 99.18033% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.2%. Comparing base (a056aac) to head (24e4dec).
⚠️ Report is 2888 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6829   +/-   ##
=======================================
  Coverage    83.2%    83.2%           
=======================================
  Files         856      856           
  Lines      376863   376858    -5     
=======================================
+ Hits       313749   313774   +25     
+ Misses      63114    63084   -30     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@LucasSte LucasSte requested a review from Lichtso July 3, 2025 21:08
@LucasSte LucasSte marked this pull request as ready for review July 3, 2025 21:08
@LucasSte LucasSte requested a review from a team as a code owner July 3, 2025 21:08
Comment thread transaction-context/src/lib.rs
buffalojoec
buffalojoec previously approved these changes Jul 8, 2025
Copy link
Copy Markdown

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Lgtm. Do we want to actually add #[repr(C)] in this PR, or not yet?

@Lichtso
Copy link
Copy Markdown

Lichtso commented Jul 8, 2025

To keep track we could mark the things which are now runtime-VM-sharable as #[repr(C)].

@LucasSte
Copy link
Copy Markdown
Author

LucasSte commented Jul 8, 2025

Do we want to actually add #[repr(C)] in this PR, or not yet?

I haven't added the marker, because the struct isn't in the format suggested in the SIMD yet: https://github.com/solana-foundation/solana-improvement-documents/pull/177/files#diff-57951771b76e6c68725097214b9c8db27ace5e34dc1ca67b423e1eb441e9caa0R116-R120

There is still one more refactor to remove the index_in_caller and index_in_callee. Once this is ready, I'll add the marker.

@buffalojoec
Copy link
Copy Markdown

There is still one more refactor to remove the index_in_caller and index_in_callee. Once this is ready, I'll add the marker.

This next refactoring step has nothing to do with #[repr(C)] though. It's ready to be runtime-VM-sharable now, just not according to the spec. I wouldn't use an attribute that means one thing to signal another.

@LucasSte
Copy link
Copy Markdown
Author

LucasSte commented Jul 8, 2025

There is still one more refactor to remove the index_in_caller and index_in_callee. Once this is ready, I'll add the marker.

This next refactoring step has nothing to do with #[repr(C)] though. It's ready to be runtime-VM-sharable now, just not according to the spec. I wouldn't use an attribute that means one thing to signal another.

Added the marker in 4482181

@LucasSte LucasSte requested a review from buffalojoec July 8, 2025 17:22
buffalojoec
buffalojoec previously approved these changes Jul 9, 2025
Copy link
Copy Markdown

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Lgtm 🫡

@LucasSte
Copy link
Copy Markdown
Author

I had to rebase and fix conflicts after #5871 was merged. Can someone approve again? @Lichtso or @buffalojoec ?

@LucasSte LucasSte merged commit a99e440 into anza-xyz:master Jul 11, 2025
41 checks passed
@LucasSte LucasSte deleted the ix-acc-1 branch July 11, 2025 23:04
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.

4 participants