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

simplify program error #80

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

publicqi
Copy link
Contributor

current ProgramError is copied from solana-sdk which is super redundant.

this PR refactors ProgramError. it uses one u8 as the discriminator, and an optional 4 bytes for custom error code.

For u64 to ProgramError conversion, previously the code generated was huge:

image

Now it simplifies to

image

And for ProgramError to u64 conversion (i.e. return of entrypoint), the code reduced from

image

to

image

@febo
Copy link
Collaborator

febo commented Mar 3, 2025

@publicqi Did you check the impact on the program size and CU? I did a quick test and it seems that the "simplified" version increases the program size by ~1kb and CU by 2. Not a lot, but I was expecting the opposite.

@publicqi
Copy link
Contributor Author

publicqi commented Mar 3, 2025

do you have an example program? i checked and the size actually went down. maybe we should add some base examples in the repo to reflect improvements

also i’m digging into some rustc issues. it seems that impl display for u64 occupied a lot of space

@febo
Copy link
Collaborator

febo commented Mar 4, 2025

do you have an example program? i checked and the size actually went down. maybe we should add some base examples in the repo to reflect improvements

I did an ad-hoc test with an existing program that I have. Maybe we could use this one to test: https://github.com/febo/ping

@publicqi
Copy link
Contributor Author

publicqi commented Mar 4, 2025

i am comparing ping program between master(01c1c7a) with this branch (4160b7e).

number of ebpf instruction reduced from 717 to 652. size of the elf reduced from 8424 bytes to 7720 bytes.

no other changes to the ping program except for some dependency updates

diff --git a/program/Cargo.toml b/program/Cargo.toml
index df1c7bf..0cb6ce9 100644
--- a/program/Cargo.toml
+++ b/program/Cargo.toml
@@ -15,4 +15,5 @@ account-dependencies = []
 crate-type = ["cdylib", "lib"]
 
 [dependencies]
-pinocchio = { version="^0", features=["macro"], git="https://github.com/febo/pinocchio.git" }
+pinocchio = { path = "../../pinocchio/sdk/pinocchio" }
+pinocchio-pubkey = { path = "../../pinocchio/sdk/pubkey" }
\ No newline at end of file
diff --git a/program/src/lib.rs b/program/src/lib.rs
index 1b679f7..db2c49a 100644
--- a/program/src/lib.rs
+++ b/program/src/lib.rs
@@ -1,5 +1,7 @@
+use pinocchio_pubkey::declare_id;
+
 pub mod entrypoint;
 pub mod instruction;
 pub mod processor;
 
-pinocchio::declare_id!("Ping111111111111111111111111111111111111111");
+declare_id!("Ping111111111111111111111111111111111111111");

toolchain version:

cargo build-sbf -V
solana-cargo-build-sbf 2.1.14
platform-tools v1.43
rustc 1.79.0

@publicqi
Copy link
Contributor Author

publicqi commented Mar 4, 2025

so the program size both in number of ebpf instruction emitted and the elf size reduced.

how did you compare the CU?

@febo
Copy link
Collaborator

febo commented Mar 7, 2025

so the program size both in number of ebpf instruction emitted and the elf size reduced.

how did you compare the CU?

I executed an instruction that generates an error, so it triggers the conversion to u64.

@publicqi
Copy link
Contributor Author

publicqi commented Mar 7, 2025

emm that's probably because in the old code, the ProgramError -> u64 conversion is just branch matching (see screenshot above). in the new code it's now doing shifting. can you try for all types of program errors what are the diffs? i would expect some conversion will take a bit more, but the size should reduce by a lot.

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.

2 participants