Make ProgramError compatible with pinocchio

[kevinheavey]

Problem: solana-program-error and pinocchio define ProgramError enums that are identical except for the BorshIoError variant, which looks like BorshIoError(String) in solana-program-error and is fieldless (just BorshIoError) in pinocchio.

It is going to be a significant source of grief for users to have two almost-identical ProgramError types in their dependency tree. People will accidentally import the wrong one a lot

Solution: make solana_program_error::ProgramError the same as the pinocchio one so pinocchio can replace its definition with a re-export.

Breaking changes:

• the BorshIoError variants of ProgramError and InstructionError become fieldless. This will require a major version bump for solana-program-error, solana-instruction-error, solana-program and solana-sdk. I haven't included the version bump in the PR as it breaks compilation because of the solana-system-interface dep. This may or may not be curable with patching but is ultimately not a real problem.
• add a num-traits feature to solana-program-error so that all functionality requiring num-traits becomes optional (pinocchio isn't using this functionality)
• make std optional in solana-program-error as pinocchio is no-std. The only thing this affects is impl std::error::Error for ProgramError {}

Other changes:

• Extract solana-pubkey-error and solana-instruction-error crates and use them in solana-program-error. This is so pinocchio's deps can be as light as possible
• Make num-traits optional

[kevinheavey]

CI error is as expected given the breaking change

[kevinheavey]

any outstanding issues with this?

[joncinque]

Really great work! I'm a big fan of the change overall, mostly small things and questions to consider

[joncinque]

Confirming that this change is good from the runtime side too -- the error text is never used or stored anywhere

[joncinque]

I looked around to see if there was a standard feature name for num-traits. The highest downloaded crate that has it optional uses num-traits https://github.com/starkat99/half-rs, and the second just uses num https://github.com/jhpratt/deranged.

All that to say, I'm good with num-traits

[joncinque]

I don't want to cause too much whiplash, but I'm not sure this feature adds too much value, especially considering solana-pubkey-error is such a light crate.

My vote would be to move the From<PubkeyError> for ProgramError implementation to solana-pubkey-error instead.

@febo thoughts?

[febo]

Makes sense to me. I think it is simpler to import solana-pubkey-error directly if you want to use it.

[kevinheavey]

that would add an indirect solana-program-error dep to solana-pubkey unless we add a program-error feature to solana-pubkey-error

[joncinque]

Sorry, I think I'm missing something about the dependency graph -- is there an issue with having solana-pubkey depend on solana-program-error? Or is it the problem of adding any dependencies to solana-pubkey?

[kevinheavey]

It's more bloaty than having program-error depend on pubkey-error, since if you have to choose a superfluous dep, pubkey-error is the smaller one. I would just have program-error depend on pubkey-error without a feature flag if @febo is ok with Pinocchio depending unnecessarily on pubkey-error

[febo]

Hmmm, I think it will be a bit strange to have it that way. In a way ProgramError is the "root" error type generated by programs, so the direction of the dependency looks more natural if PubkeyError depends on ProgramError for cases where it will be wrapped as a ProgramError. This is what usually will happen to any other custom error type defined that can be wrapped as a ProgramError.

[kevinheavey]

ok i'm moving PubkeyError back to solana-pubkey

[joncinque]

We can figure this out in follow-up work, but since this isn't actually doing any string formatting, we can probably get by with using the syscall directly and completely avoid pulling in solana-msg and format!, which is typically the source of bloat.

And that way, we don't need the solana-msg feature.

What do you think @febo ?

[febo]

Agree – another option is to use a trait that just converts a ProgramError to a &str, like here.

[kevinheavey]

If @febo is fine with it I suggest we just remove the feature and make solana-msg a regular dependency. It causes no measurable increase in build time and I doubt the solana-msg crate is going to have any major version bumps

[joncinque]

This looks good to me, but let's see what @febo thinks before merging

[febo]

This works as a drop in replacement for pinocchio, which is great! We could pull in solana-msg but we need to make that crate no_std first, otherwise it will bring std because of the format! – something we can address in a separate PR.

Ideally I would deprecate the PrintProgramError trait in favour of one that just returns a static &str. Feels more useful than "bundling" the msg! inside the trait implementation. Thoughts?

[kevinheavey]

Does anyone use PrintProgramError outside of SPL? We could move it somewhere else.

What's the big deal with having unused std code in a dependency? #![no_std] still works even if a dependency uses std somewhere

[febo]

What's the big deal with having unused std code in a dependency? #![no_std] still works even if a dependency uses std somewhere

Messes around with the panic handler implementation when std is indirectly available to the linker: anza-xyz/pinocchio#84

[febo]

Does anyone use PrintProgramError outside of SPL? We could move it somewhere else.

I think programs tend to use it since SPL programs do. We could add a ToStr trait and deprecate PrintProgramError behind the "std" feature.

[kevinheavey]

I'm not following - isn't std always available to the linker if the platform supports it? And #[no_std] just takes it out of scope? Putting #![no_std] in a crate doesn't affect that crate's dependencies, and vice versa

[febo]

I'm not following - isn't std always available to the linker if the platform supports it? And #[no_std] just takes it out of scope? Putting #![no_std] in a crate doesn't affect that crate's dependencies, and vice versa

It might be a specific behaviour of platform-tools toolchain, but this is the difference (full details here):

std:
when a panic happens, rust runtime will call

rust_begin_unwind (std generted)
    std::sys::pal::sbf::panic (sbf compiler generated)
        custom_panic (pinocchio)
        abort syscall (sbf compiler generated)

no_std:

rust_begin_unwind (pinocchio)

This changes which panic handler to include in your program, since for std it is more like a "hook", but for no_std it is a "full" handler. This is not controlled by whether your program is no_std or std – having an indirect dependency to the std changes the behaviour.

[kevinheavey]

If that's true that's a bug in platform-tools rust right? In normal Rust adding a dependency on a crate that uses std doesn't change the behaviour of other crates.

Anyway I've now made it so the std feature is enabled if the solana-msg feature is enabled

[febo]

If that's true that's a bug in platform-tools rust right? In normal Rust adding a dependency on a crate that uses std doesn't change the behaviour of other crates.

Anyway I've now made it so the std feature is enabled if the solana-msg feature is enabled

I think this is the "normal" behaviour. When the std is available, either because your crate depends on it or any of your crate's dependencies, it automatically sets a panic handler. When all crates are no_std, there is no panic handler attached and you need to set your one.

You can quickly test this in Rust. Create a new crate (cargo init) with type "cdylib"; add #![no_std] to the lib.rs and try to compile (standard cargo build). You will get an error that there is no panic handler set.

error: `#[panic_handler]` function required, but not found

error: unwinding panics are not supported without std

Now add solana-msg as a dependency and add a solana_msg::msg!("I am std") to a function. If you try to compile now, it works.

All that to say that it is better to "mark" crates as no_std when they do not dependent on std. 😉

EDIT: A better example is to use five8_const = "=0.1.3" as a dependency – the version before being "marked" as no_std.

[kevinheavey]

Interesting, that's really annoying! Not to go on a tangent but is there a downside to removing the root #![no_std] from pinocchio so that you get predictable panic behaviour? All the other modules in pinocchio could still have #[no_std].

AFAICT Rust tooling provides no good ways of ensuring that all a project's dependencies are no-std, so this is going to be a footgun for users

[febo]

Interesting, that's really annoying! Not to go on a tangent but is there a downside to removing the root #![no_std] from pinocchio so that you get predictable panic behaviour? All the other modules in pinocchio could still have #[no_std].

AFAICT Rust tooling provides no good ways of ensuring that all a project's dependencies are no-std, so this is going to be a footgun for users

Not a lot of downside, but it would prevent someone to completely replace the panic handler with a custom implementation if they wanted to. I imagine most users won't see this issue, since you need to be explicitly writing a no_std program and not use the "standard" entrypoint! macro – at that point, you probably will be able to handle the conflict.

I think this will eventually go away when we bump to Rust 1.84 – more here (Discord).

[febo]

Looks great and works great as well! Here it is how this is integrated in pinocchio: anza-xyz/pinocchio#112

[joncinque]

I'm thinking we should land the deprecation of PrintProgramError first with #102, publish a patch release of this crate, then we can land this. How does that sound?