Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add get_processed_sibling_instruction syscall #22859

Merged

Conversation

jackcmay
Copy link
Contributor

@jackcmay jackcmay commented Feb 1, 2022

Problem

There are scenarios where instructions (top-level or via CPI) would like to know what the last instruction processed was. Some examples are verification of assert instructions or to check for memo instructions. Programs can already check tx level instructions via the Sysvar1nstructions1111111111111111111111111 but there is no way to get information about what inner instructions have been processed.

Summary of Changes

Add a syscall that allows a program to query what sibling instructions have been successfully processed.

/// Returns a sibling instruction from the processed sibling instruction list.
///
/// The processed sibling instruction list is a reverse-ordered list of
/// successfully processed sibling instructions. For example, given the call flow:
///
/// A
/// B -> C -> D
/// B -> E
/// B -> F
///
/// Then B's processed sibling instruction list is: [A]
/// Then F's processed sibling instruction list is: [E, C]

This PR supersedes #22515

Fixes #22437

@jackcmay jackcmay added the work in progress This isn't quite right yet label Feb 1, 2022
@jackcmay jackcmay force-pushed the sibling-instructions-from-trace branch 4 times, most recently from f214d94 to 3b48112 Compare February 1, 2022 08:50
program-runtime/src/invoke_context.rs Outdated Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
@@ -32,7 +32,8 @@ pub struct TransactionContext {
instruction_context_capacity: usize,
instruction_context_stack: Vec<InstructionContext>,
number_of_instructions_at_transaction_level: usize,
instruction_trace: Vec<Vec<CompiledInstruction>>,
top_level_instruction_trace: Vec<InstructionContext>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is close to what I had in mind. Why not include top_level_instruction_trace as the first entry in each inner_instruction_trace? deconstruct() in bank.rs would then need to skip the first entry, but it might be a bit more compact and easier to understand.

(I can also refactor that later myself)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, check it out

programs/bpf/rust/sibling_inner_instruction/src/lib.rs Outdated Show resolved Hide resolved
programs/bpf/rust/sibling_instruction/src/lib.rs Outdated Show resolved Hide resolved
@jackcmay jackcmay force-pushed the sibling-instructions-from-trace branch from 3b48112 to 7f055f6 Compare February 2, 2022 00:06
@jackcmay jackcmay removed the work in progress This isn't quite right yet label Feb 2, 2022
@codecov
Copy link

codecov bot commented Feb 2, 2022

Codecov Report

Merging #22859 (6bdf7b0) into master (75563f6) will decrease coverage by 0.0%.
The diff coverage is 47.3%.

@@            Coverage Diff            @@
##           master   #22859     +/-   ##
=========================================
- Coverage    81.3%    81.2%   -0.1%     
=========================================
  Files         560      560             
  Lines      152043   152268    +225     
=========================================
+ Hits       123623   123725    +102     
- Misses      28420    28543    +123     

/// Number of accounts in this Instruction (without program accounts)
pub fn get_number_of_instruction_accounts(&self) -> usize {
self.instruction_accounts.len()
}

/// Get the instruction's accounts
pub fn get_instruction_accounts(&self) -> &[InstructionAccount] {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I missed this the last time I reviewed it, but InstructionAccount should not be exposed. It is only meant to be a constructor for the tests. Otherwise access the fields of the InstructionContext separately, because the encoding is still suspect to heavy changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You thinking we replace it with a trace-specific instruction struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm exposing it in the instruction trace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could expose an iterator of a trait that exposes the AccountMeta infos

Copy link
Contributor

Choose a reason for hiding this comment

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

It just occurred to me that you can't know what I refer to as I haven't merged that branch yet.
I meant exposing the individual fields in InstructionAccount separately like this:
Lichtso@c34020d

Copy link
Contributor Author

@jackcmay jackcmay Feb 2, 2022

Choose a reason for hiding this comment

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

Yeah, thats what I thought you meant, but we need a way to discover the account indicies from the InstructiionContext. So instead of get_instruction_accounts() we need a get_instruction_account_indicies_in_transaction()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And make InstructionAccount private

Copy link
Contributor

Choose a reason for hiding this comment

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

We already have a get_instruction_account_indicies_in_transaction()
it is called InstructionContext::get_index_in_transaction()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, way past that :-)

Lichtso
Lichtso previously approved these changes Feb 2, 2022
@jackcmay jackcmay force-pushed the sibling-instructions-from-trace branch from 385d085 to 6bdf7b0 Compare February 2, 2022 21:47
@mergify mergify bot dismissed Lichtso’s stale review February 2, 2022 21:48

Pull request has been modified.

@jackcmay jackcmay merged commit ab02dba into solana-labs:master Feb 3, 2022
@jackcmay jackcmay deleted the sibling-instructions-from-trace branch February 3, 2022 00:46
@jackcmay
Copy link
Contributor Author

jackcmay commented Feb 3, 2022

@CriesofCarrots Try this out and let us know, I'll work on porting it to v1.9

@CriesofCarrots
Copy link
Contributor

@CriesofCarrots Try this out and let us know, I'll work on porting it to v1.9

Awesome, will do!

@CriesofCarrots
Copy link
Contributor

@jackcmay , this is working great. Here's the branch where I have it in use: https://github.com/CriesofCarrots/solana-program-library/tree/finish-memo-extension
Needs patch to local master solana, then cd token/prgoram-2022 && cargo test-bpf realloc

I'll keep my eyes out for your v1.9 backport, and patch to it for testing when it's ready.

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.

Add ability for programs to check the processed instruction stack
3 participants