Extract instruction crate #2405

kevinheavey · 2024-08-02T12:35:33Z

Problem

solana_program::instruction needs to be made available outside solana_program

This branches off #2394 so that needs to be merged first

Summary of Changes

Pull out a solana_instruction crate and re-export the relevant contents in solana_program::instruction for backwards compatibility
Move the numeric consts (ACCOUNT_ALREADY_INITIALIZED etc) and impl<T> From<T> for InstructionError from program_error.rs to solana-instruction. The compiler requires this.
Re-export the numeric consts in program_error.rs
Make bincode and serde optional in solana-instruction

Fun fact: a release build of the new crate without default features takes <1 second on my machine

mergify · 2024-08-09T12:17:08Z

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.

joncinque

Sorry for the lateness, this one was pretty chunky, and I wanted to figure out a future for syscall stubs. Looks great! Just some tiny things.

sdk/program/src/program_error.rs

sdk/instruction/Cargo.toml

sdk/instruction/src/lib.rs

joncinque · 2024-09-11T23:18:15Z

sdk/instruction/src/error.rs

+// Warning: Any new error codes added here must also be:
+// - Added to the below conversions
+// - Added as an equivalent to ProgramError and InstructionError
+// - Be featureized in the BPF loader to return `InstructionError::InvalidError`


nit typo (I know you didn't do this, but now that it's being moved, we can fix it)

Suggested change

// - Be featureized in the BPF loader to return `InstructionError::InvalidError`

// - Be featurized in the BPF loader to return `InstructionError::InvalidError`

joncinque · 2024-09-12T13:07:19Z

sdk/instruction/src/lib.rs

+/// Addition that returns [`InstructionError::InsufficientFunds`] on overflow.
+///
+/// This is an internal utility function.
+#[doc(hidden)]
+pub fn checked_add(a: u64, b: u64) -> Result<u64, InstructionError> {
+    a.checked_add(b).ok_or(InstructionError::InsufficientFunds)
+}


This is so strange, but it looks like it has uses in the stake and system program. Can you leave it in solana_program with a note saying to remove it?

joncinque · 2024-09-12T13:08:30Z

sdk/instruction/src/lib.rs

+/// Describes a single account read or written by a program during instruction
+/// execution.
+///
+/// When constructing an [`Instruction`], a list of all accounts that may be
+/// read or written during the execution of that instruction must be supplied.
+/// Any account that may be mutated by the program during execution, either its
+/// data or metadata such as held lamports, must be writable.
+///
+/// Note that because the Solana runtime schedules parallel transaction
+/// execution around which accounts are writable, care should be taken that only
+/// accounts which actually may be mutated are specified as writable. As the
+/// default [`AccountMeta::new`] constructor creates writable accounts, this is
+/// a minor hazard: use [`AccountMeta::new_readonly`] to specify that an account
+/// is not writable.
+#[repr(C)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[derive(Debug, Default, PartialEq, Eq, Clone)]
+pub struct AccountMeta {
+    /// An account's public key.
+    pub pubkey: Pubkey,
+    /// True if an `Instruction` requires a `Transaction` signature matching `pubkey`.
+    pub is_signer: bool,
+    /// True if the account data or metadata may be mutated during program execution.
+    pub is_writable: bool,
+}


Can you move AccountMeta into a separate file? It'll make things neater when adding a new instruction type

joncinque · 2024-09-12T13:11:33Z

sdk/instruction/src/lib.rs

+    serde(rename_all = "camelCase")
+)]
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub struct CompiledInstruction {


Man, I really think this belongs more in Message than Instruction, but it's currently exported from instruction... what do you think about leaving this in solana_program for now, and then moving it to solana_message whenever that gets broken out? Without breaking the re-exports from solana_program, of course.

That'll also remove the dep on solana-sanitize

sdk/instruction/src/lib.rs

…olana-program to avoid breaking change

joncinque

One last question around AccountMeta, otherwise this is looking good!

joncinque · 2024-09-12T21:29:51Z

sdk/instruction/Cargo.toml

+    "dep:rustc_version",
+    "dep:solana-frozen-abi",
+    "dep:solana-frozen-abi-macro",
+    "serde",


Oh yeah, I noticed this was needed while testing, thanks for fixing!

joncinque · 2024-09-12T21:37:15Z

sdk/instruction/src/account_meta.rs

@@ -0,0 +1,105 @@
+#[cfg(any(feature = "std", target_arch = "wasm32"))]


I might be missing something -- why is this gated on the std feature / wasm32 arch? It should be usable regardless of features and all that, no?

Yeah I took another look and realised this can be much simpler, had been assuming the wasm32 code needed std because we have other places where that's the case but it's not the case here

joncinque · 2024-09-12T21:37:35Z

sdk/instruction/src/account_meta.rs

+/// default [`AccountMeta::new`] constructor creates writable accounts, this is
+/// a minor hazard: use [`AccountMeta::new_readonly`] to specify that an account
+/// is not writable.
+#[cfg(any(feature = "std", target_arch = "wasm32"))]


Same with this, is the featurization / arch cfg needed?

joncinque · 2024-09-12T22:03:43Z

sdk/instruction/src/error.rs

+/// an error be consistent across software versions.  For example, it is
+/// dangerous to include error strings from 3rd party crates because they could
+/// change at any time and changes to them are difficult to detect.
+#[cfg(feature = "std")]


I'm guessing this is no-std because of the String in one of the errors, correct? I hope we can make a breaking change to make this no-std in the future...

joncinque · 2024-09-12T22:11:48Z

@yihau can you accept the ownership for solana-instruction once it gets passed over? This is very close to ready

joncinque

Good to go from my side -- thanks for responding to all the feedback so quickly!

mergify · 2024-09-13T01:18:55Z

automerge label removed due to a CI failure

yihau · 2024-09-13T15:24:55Z

all green!

* extract instruction crate * remove thiserror * make bincode optional in solana-instruction * make serde optional in solana-instruction * fix doc tests * fmt * harmonize features * fmt Cargo.toml * clippy * fix syscalls.rs imports * update digests * update digest * unused imports * post-rebase fmt * fix doc links * use workspace lints * fmt * make rustc_version dep optional * update digest * update digest * update import * fmt * fix dup import * remove dev-context-only-utils (no longer needed) * fmt * remove unnecessary allow() * fix overwriting instruction syscall stubs * fmt * move get_processed_sibling_instruction and get_stack_height back to solana-program to avoid breaking change * fix path * fix typo * move the checked_add utility function back to solana-program * move AccountMeta to its own file * fix bad import * move ProcessedSiblingInstruction to syscalls.rs * move CompiledInstruction back to solana-program * move ProcessedSiblingInstruction back into lib.rs * make std optional in solana-instruction * fix required features for frozen-abi * update digest * update digest * missing import * update digest * don't assume std is imported in frozen-abi macros * fix import warnings * simplify cfg usage

mergify bot added the need:merge-assist label Aug 2, 2024

kevinheavey mentioned this pull request Aug 2, 2024

Extract program-error crate #2413

Merged

kevinheavey force-pushed the extract-instruction branch from 68b1078 to b3d20cd Compare August 9, 2024 14:11

kevinheavey force-pushed the extract-instruction branch from b3d20cd to df7249d Compare August 19, 2024 08:12

kevinheavey force-pushed the extract-instruction branch 7 times, most recently from 3da2871 to 461914f Compare September 10, 2024 18:50

kevinheavey marked this pull request as ready for review September 10, 2024 19:44

kevinheavey requested a review from joncinque September 10, 2024 19:44

kevinheavey force-pushed the extract-instruction branch from d446961 to 7c9d7dc Compare September 10, 2024 22:26

joncinque reviewed Sep 12, 2024

View reviewed changes

kevinheavey added 14 commits September 12, 2024 21:15

extract instruction crate

03f7de0

remove thiserror

269af1f

make bincode optional in solana-instruction

022bc50

make serde optional in solana-instruction

ed76f8d

fix doc tests

8c0e5ef

fmt

cbe3cc2

harmonize features

fc9e019

fmt Cargo.toml

50cd0f3

clippy

d9f5335

fix syscalls.rs imports

55dca67

update digests

629bfdb

update digest

afd03a0

unused imports

cf3212b

post-rebase fmt

12f9a7a

kevinheavey added 15 commits September 12, 2024 21:15

move get_processed_sibling_instruction and get_stack_height back to s…

7bc0fae

…olana-program to avoid breaking change

fix path

f8acbfb

fix typo

bf9a4a4

move the checked_add utility function back to solana-program

cdb0a48

move AccountMeta to its own file

0c43f15

fix bad import

1127181

move ProcessedSiblingInstruction to syscalls.rs

9ed5c74

move CompiledInstruction back to solana-program

e7db89a

move ProcessedSiblingInstruction back into lib.rs

0504374

make std optional in solana-instruction

5d62dff

fix required features for frozen-abi

10f1c82

update digest

3d77935

update digest

edfd466

missing import

0d4dd64

update digest

d2564f3

kevinheavey force-pushed the extract-instruction branch from a338945 to d2564f3 Compare September 12, 2024 17:15

kevinheavey added 2 commits September 13, 2024 00:35

don't assume std is imported in frozen-abi macros

c0e6ca0

fix import warnings

4b20996

joncinque reviewed Sep 12, 2024

View reviewed changes

simplify cfg usage

5727e9f

joncinque reviewed Sep 12, 2024

View reviewed changes

kevinheavey added the automerge automerge Merge this Pull Request automatically once CI passes label Sep 13, 2024

mergify bot removed the automerge automerge Merge this Pull Request automatically once CI passes label Sep 13, 2024

kevinheavey added the automerge automerge Merge this Pull Request automatically once CI passes label Sep 13, 2024

joncinque approved these changes Sep 13, 2024

View reviewed changes

joncinque merged commit b4ed7a4 into anza-xyz:master Sep 13, 2024
55 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract instruction crate #2405

Extract instruction crate #2405

kevinheavey commented Aug 2, 2024

mergify bot commented Aug 9, 2024

joncinque left a comment

joncinque Sep 11, 2024

joncinque Sep 12, 2024

joncinque Sep 12, 2024

joncinque Sep 12, 2024

joncinque left a comment

joncinque Sep 12, 2024

joncinque Sep 12, 2024

kevinheavey Sep 12, 2024

joncinque Sep 12, 2024

joncinque Sep 12, 2024

kevinheavey Sep 12, 2024

joncinque commented Sep 12, 2024

joncinque left a comment

mergify bot commented Sep 13, 2024

yihau commented Sep 13, 2024

	// - Be featureized in the BPF loader to return `InstructionError::InvalidError`
	// - Be featurized in the BPF loader to return `InstructionError::InvalidError`

		@@ -0,0 +1,105 @@
		#[cfg(any(feature = "std", target_arch = "wasm32"))]

Extract instruction crate #2405

Extract instruction crate #2405

Conversation

kevinheavey commented Aug 2, 2024

Problem

Summary of Changes

mergify bot commented Aug 9, 2024

joncinque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque commented Sep 12, 2024

joncinque left a comment

Choose a reason for hiding this comment

mergify bot commented Sep 13, 2024

yihau commented Sep 13, 2024