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.

Sign up for GitHub

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

Jump to bottom

Record inner instruction stack height #28430

Merged
merged 3 commits into from
Oct 26, 2022
Merged

Record inner instruction stack height #28430

merged 3 commits into from
Oct 26, 2022

Conversation

jstarry
Copy link
Member

@jstarry jstarry commented Oct 17, 2022

Problem

The inner instruction stack height isn't recorded after transaction processing so clients can't determine if an inner instruction was invoked by another inner instruction.

Summary of Changes

Builds on #28427

  • Record stack_height in a new InnerInstruction protobuf message type for each inner instruction
  • Add a new stack_height field to instruction data types returned by the RPC API

Related to #27736

@jstarry jstarry added the v1.14 label Oct 17, 2022
@jstarry jstarry force-pushed the feat/inner-instruction-stack-height branch from 546b579 to 476b0f0 Compare October 17, 2022 17:58
@jstarry jstarry removed the v1.14 label Oct 17, 2022
@jstarry jstarry requested a review from Lichtso October 17, 2022 17:59
@Lichtso
Copy link
Contributor

Lichtso commented Oct 17, 2022

We can't change CompiledInstruction and thus have to add a wrapper InnerInstruction, or what is it for?

@jstarry jstarry force-pushed the feat/inner-instruction-stack-height branch from 476b0f0 to 08616e8 Compare October 18, 2022 08:37
@jstarry
Copy link
Member Author

jstarry commented Oct 18, 2022

We can't change CompiledInstruction and thus have to add a wrapper InnerInstruction, or what is it for?

Yes, exactly. Can't change CompiledInstruction in the sdk since it's used for transaction serialization. InnerInstruction is just a wrapper type.. it doesn't have to be specific to inner instructions but it's not necessary to record the stack height for transaction level instructions.

Lichtso
Lichtso reviewed Oct 21, 2022
View reviewed changes
storage-proto/proto/confirmed_block.proto
// Invocation stack height of an inner instruction.
// Available since Solana v1.14.6
// Set to `None` for txs executed on earlier versions.
optional uint32 stack_height = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

uint32 is that really necessary, the runtime itself only used 8 bits for the stack height.

Copy link
Member Author

Choose a reason for hiding this comment

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

Smallest size in protobuf is u32

Lichtso
Lichtso reviewed Oct 21, 2022
View reviewed changes
runtime/src/bank.rs Outdated Show resolved Hide resolved
@jstarry jstarry force-pushed the feat/inner-instruction-stack-height branch from 80f9937 to d043a95 Compare October 24, 2022 02:22
Lichtso
Lichtso previously approved these changes Oct 24, 2022
View reviewed changes
@jstarry jstarry added the automerge Merge this Pull Request automatically once CI passes label Oct 24, 2022
@mergify mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Oct 24, 2022
@mergify
Copy link
Contributor

mergify bot commented Oct 24, 2022

automerge label removed due to a CI failure

@jstarry jstarry mentioned this pull request Oct 24, 2022
Closed
@jstarry jstarry force-pushed the feat/inner-instruction-stack-height branch from d043a95 to a64e016 Compare October 25, 2022 04:04
@mergify mergify bot dismissed Lichtso’s stale review October 25, 2022 04:04

Pull request has been modified.

@jstarry jstarry added the automerge Merge this Pull Request automatically once CI passes label Oct 25, 2022
@mergify mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Oct 25, 2022
@mergify
Copy link
Contributor

mergify bot commented Oct 25, 2022

automerge label removed due to a CI failure

@jstarry jstarry force-pushed the feat/inner-instruction-stack-height branch from a64e016 to e2a9e43 Compare October 25, 2022 15:43
@jstarry jstarry merged commit 2d8665d into solana-labs:master Oct 26, 2022
@austbot
Copy link

austbot commented Nov 7, 2022

@jstarry thank you for this !

@jstarry jstarry deleted the feat/inner-instruction-stack-height branch November 9, 2022 04:25
gnapoli23 pushed a commit to gnapoli23/solana that referenced this pull request Dec 16, 2022
@jstarry @gnapoli23
Record inner instruction stack height (solana-labs#28430)
93fd8b2 
* Record inner instruction stack height

* fix sbf tests

* feedback
nickfrosty pushed a commit to nickfrosty/solana that referenced this pull request Jan 4, 2023
@jstarry @nickfrosty
Record inner instruction stack height (solana-labs#28430)
fe376e6 
* Record inner instruction stack height

* fix sbf tests

* feedback
@steveluscher steveluscher mentioned this pull request Jun 27, 2024
Merged
steveluscher added a commit to solana-labs/solana-web3.js that referenced this pull request Jun 28, 2024
@steveluscher
Add stackHeight to the instruction type (#2866)
73bd5a9 
See solana-labs/solana#28430
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lichtso Lichtso Lichtso left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jstarry @Lichtso @austbot