refactor: rework adress definitions & introduce region reservation#7
refactor: rework adress definitions & introduce region reservation#7nagisa wants to merge 26 commits into
Conversation
Having host addresses represented by VmSlice seems not right? I switched those functions to return a regular raw pointer. Which in turn simplified the memory mapping creation somewhat.
* Create tests * @nagisa's review
* Write function to update account permissions * Add unit test * Add integration test * Use all accounts as mutable
Co-authored-by: Lucas <lucas.tnagel@gmail.com>
* Fix account mapping * Add test and fix edge case * Verify account owner for permissions * Fix rebase problems * Remove optimization * Change condition * @nagisa's review * Update program-runtime/src/memory_context.rs Co-authored-by: Simonas Kazlauskas <github@kazlauskas.me> * Format file --------- Co-authored-by: Simonas Kazlauskas <github@kazlauskas.me>
* Implement syscall * Write a test * Amend function with writability change * Finish implementation * Specify index in transction * @nagisa's review
* Implement syscall * Write integration test * @nagisa's review * Change name
In order to introduce sysvar regions, it is becoming necessary to make our address space allocation somewhat more future proof. One of such future proofing example is allocating enough reserved space for account payloads if we were to increase the number of accounts that can be specified in a transaction. This, however, introduces a concept of a "gap" between regions which did not exist before and complicates the conversions between region indices and guest addresses. After a few experiments I found that adding a structure and some methods to describe the expected memory layout is the least ugly option.
There was a problem hiding this comment.
Pull request overview
This PR refactors ABIv2 guest memory address definitions by introducing a GuestMemorySection abstraction, and extends the memory layout to reserve space for future “sysvar regions” via an address-space gap.
Changes:
- Introduce
GuestMemorySectionto describe guest memory layout in a structured way and compute per-section region indices/guest address ranges. - Add a reserved “gap” to support sysvar regions while keeping region indices contiguous.
- Update transaction context, syscalls, and memory mapping code to use section-based address/range calculations instead of hard-coded base constants.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| transaction-context/src/vm_addresses.rs | Adds GuestMemorySection, defines the new ABIv2 section layout, introduces sysvar reservation, and computes NUM_REGIONS. |
| transaction-context/src/transaction.rs | Updates scratchpad and region computations to use GuestMemorySection; adjusts region resize dispatch. |
| transaction-context/src/transaction_accounts.rs | Switches account payload region addressing to section-based ranges. |
| transaction-context/src/instruction.rs | Updates instruction frame VM slice addressing to section-based ranges. |
| transaction-context/src/instruction_accounts.rs | Updates resize payload VM address computation to section-based ranges. |
| syscalls/src/lib.rs | Updates region index/address handling and adjusts tests to iterate the new section layout. |
| program-runtime/src/memory_context.rs | Reworks ABIv2 region construction using the new section model (but currently contains debug leftovers). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return_data_scratchpad: VmSlice::new( | ||
| vm_addresses::RETURN_DATA_SCRATCHPAD_SECTION | ||
| .guest_address_range() | ||
| .start, | ||
| 0, | ||
| ), | ||
| cpi_scratchpad: VmSlice::new( | ||
| GUEST_INSTRUCTION_DATA_BASE_ADDRESS.saturating_add( | ||
| GUEST_REGION_SIZE.saturating_mul(number_of_top_level_instructions as u64), | ||
| ), | ||
| vm_addresses::INSTRUCTION_DATA_SECTION | ||
| .guest_address_range() | ||
| .start, | ||
| 0, | ||
| ), |
| if let Some(caller_index) = caller_index { | ||
| self.transaction_frame.total_number_of_instructions_in_trace = self | ||
| .transaction_frame | ||
| .total_number_of_instructions_in_trace | ||
| .saturating_add(1); | ||
| instruction.index_of_caller_instruction = caller_index; | ||
| let next_ptr = self | ||
| .transaction_frame | ||
| .cpi_scratchpad | ||
| .ptr() | ||
| .saturating_add(GUEST_REGION_SIZE); | ||
| self.transaction_frame.cpi_scratchpad = VmSlice::new(next_ptr, 0); | ||
| let section = vm_addresses::INSTRUCTION_DATA_SECTION; | ||
| let scratchpad_addrs = | ||
| section.guest_address_range_for(caller_index.saturating_add(1) as usize); | ||
| self.transaction_frame.cpi_scratchpad = VmSlice::new(scratchpad_addrs.start, 0); | ||
| } |
| a if vm_addresses::INSTRUCTION_DATA_SECTION.contains_guest_ptr(a) => { | ||
| if !old_region.writable { | ||
| return Err(InstructionError::InvalidArgument); | ||
| } | ||
| let ix_address = vm_address | ||
| .checked_sub(GUEST_INSTRUCTION_DATA_BASE_ADDRESS) | ||
| let ix_idx = vm_addresses::INSTRUCTION_ACCOUNTS_SECTION | ||
| .region_index_containing(a) | ||
| .ok_or(InstructionError::InvalidArgument)?; | ||
| let ix_idx = abiv2_region_index_from_vm_address(ix_address); | ||
| let ix = self.instruction_data.get_mut(ix_idx); |
| assert_eq!( | ||
| transaction_context.transaction_frame.cpi_scratchpad.ptr(), | ||
| GUEST_INSTRUCTION_DATA_BASE_ADDRESS.saturating_add(GUEST_REGION_SIZE.saturating_mul(3)) | ||
| vm_addresses::INSTRUCTION_ACCOUNTS_SECTION | ||
| .guest_address_range_for(3) | ||
| .start | ||
| ); |
| let section = INSTRUCTION_ACCOUNTS_SECTION; | ||
| let regions = v2_regions | ||
| .get_mut(dbg!(section.region_index_range())) | ||
| .unwrap(); | ||
| transaction_context.instruction_accounts_regions(regions); |
| pub const INSTRUCTION_DATA_SECTION: GuestMemorySection = | ||
| GuestMemorySection::new_following(SYSVAR_ACCOUNT_SECTION, MAX_INSTRUCTION_TRACE_LENGTH); | ||
| pub const INSTRUCTION_ACCOUNTS_SECTION: GuestMemorySection = | ||
| GuestMemorySection::new_following(INSTRUCTION_DATA_SECTION, MAX_INSTRUCTION_TRACE_LENGTH); | ||
|
|
|
Looks like I'll need to make modifications to sbpf before this can land – AlignedMemoryMapping has code internally to create all the intermediate mappings that are now making things go wrong, or maybe this idea is bust actually... will find out. |
|
What things go wrong? How is this different from what we had before when |
Well, I guess its not And then we still have some code that directly deal with There's point to be made that we might be fine with those filler regions being present, as it how we achieve efficient address translation. When we start reusing the But either way I need to rewrite some code 😓 |
The merge-base changed after approval.
* Fix account mapping * Add test and fix edge case * Verify account owner for permissions * Fix rebase problems * Remove optimization * Change condition * @nagisa's review * Update program-runtime/src/memory_context.rs Co-authored-by: Simonas Kazlauskas <github@kazlauskas.me> * Format file --------- Co-authored-by: Simonas Kazlauskas <github@kazlauskas.me>
In order to introduce sysvar regions, it is becoming necessary to make our address space allocation somewhat more future proof. One of such future proofing example is allocating enough reserved space for account payloads if we were to increase the number of accounts that can be specified in a transaction.
This, however, introduces a concept of a "gap" between regions which did not exist before and complicates the conversions between region indices and guest addresses.
After a few experiments I found that adding a structure and some methods to describe the expected memory layout is the least ugly option. As we only have one gap currently, the conversion in either direction is still relatively straightforward.