Rearrange fields of InstructionFrame#9714
Conversation
LucasSte
changed the title
LucasSte
force-pushed
the
match-fields
branch
from
6c40296 to
b60eb20
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9714 +/- ##
=======================================
Coverage 82.7% 82.7%
=======================================
Files 901 901
Lines 324841 324852 +11
=======================================
+ Hits 268717 268762 +45
+ Misses 56124 56090 -34 🚀 New features to boost your workflow:
|
| pub nesting_level: u32, | ||
| /// This is the index of the parent instruction if this is a CPI and u32::MAX if this is a | ||
| /// top-level instruction | ||
| pub index_of_parent_instruction: u32, |
There was a problem hiding this comment.
I think they are all double of what they need to be. If we go with u32, u16, u16 then we reach u64 alignment as well.
There was a problem hiding this comment.
Fixed in 88bf1de. If you like this version, I'll need to update the SIMD.
| pub struct InstructionFrame { | ||
| pub nesting_level: usize, | ||
| pub program_account_index_in_tx: IndexOfAccount, | ||
| pub program_account_index_in_tx: u32, |
There was a problem hiding this comment.
Hm, now we have to cast to IndexOfAccount, which is also somewhat ugly. Maybe split it in two u16 by adding an explicit reserved: u16?
| pub nesting_level: u16, | ||
| /// This is the index of the parent instruction if this is a CPI and u16::MAX if this is a | ||
| /// top-level instruction | ||
| pub index_of_parent_instruction: u16, |
There was a problem hiding this comment.
To me, "parent" reads like "previous" (ie. Bank, etc.). It would be clearer to use something like this:
| pub index_of_parent_instruction: u16, | |
| pub index_of_root_instruction: u16, |
There was a problem hiding this comment.
The parent instruction is the one that originated the instruction. Imagine the following CPI scenario:
Program A ==CPI==> Program B ==CPI==> Program C.
The parent instruction for program C is that of program B. If you use root instruction, I'd understand it as the instruction for program A.
There was a problem hiding this comment.
Ahh gotcha, thanks! It's going to be tricky tracking which dimension "parent" means throughout the program-runtime though.
There was a problem hiding this comment.
That matches the convention used in CPI, so makes sense to me.
Program A ==CPI==> Program B ==CPI==> Program C.
Is the index the index in the transaction or the stack height? If it's the former, then what does this value represent during Program C's execution?
There was a problem hiding this comment.
Maybe rename "parent" => "caller"?
Will do.
Is the index the index in the transaction or the stack height? If it's the former, then what does this value represent during Program C's execution?
It is the index of the instruction in the instruction trace. Since it is updated at each CPI entry, for Program C, it is the index of the instruction executing program B.
agave/transaction-context/src/lib.rs
Line 90 in 6bd0cd8
| instruction_accounts.len(), | ||
| instruction_data.len() as u64, | ||
| ); | ||
| instruction.index_of_parent_instruction = parent_index.unwrap_or(u16::MAX); |
There was a problem hiding this comment.
Just curious - where did the need for this field come from? I didn't see it in the SIMD discussions.
There was a problem hiding this comment.
One of the ideas behind ABIv2 is to eliminate the syscall sol_get_processed_sibling_instruction, by exposing all executing instructions to programs. Having the index of the parent instruction would help one navigate the list of instructions.
4 participants
Problem
The layout of
InstructionFramestill differs from what we propose in SIMD-0177. This PR changes field types and include new members to match with the proposal.Summary of Changes
program_account_index_in_txau64.nesting_levelau32.index_of_parent_instructionin the structure.