Space efficient custom panic

[LucasSte]

Problem

Our implementation of custom_panic can consume up to 25kb in contracts. This happens because it relies on the format! macro and, consequently, on std::fmt::write. They include many more functions in the contract and utilize dynamic dispatch, a technique that hinders compiler and link side optimizations for size reduction.

Summary of Changes

I implemented a new custom_panic that functions independently with only two syscalls. It requires Rust 1.84, which ships with platform-tools v1.47. The new tools are available on Agave's master branch and have been back ported to Agave v2.2.

Size comparison

Take this simple contract as an example:

entrypoint!(process_instruction);

fn process_instruction(
    _program_id: &Pubkey,
    accounts: &[AccountInfo],
    instruction_data: &[u8],
) -> ProgramResult {
    Ok(())
}

The binary size has whooping 18888 bytes (18kb).
The contract with an empty custom_panic function has 11536 bytes (11kb), so panic is consuming 7352 bytes.
The contract with my new implementation has 11800 bytes (11kb), so my implementation has 264 bytes.

New error messages

The members of fmt::Arguments are all private, so I cannot build custom panic messages during runtime as Rust does (think about the error you get when you access an invalid index from a vector: we can only know the index and the vector length during execution time). These messages will be elided in the new panic implementation (see examples below). Error messages whose content is known at compile time will still be shown normally.

The formatting is also different. It is more efficient to syscall twice than to format a string.

Accessing an invalid index from a vector:

OLD:

'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S invoke [1]'
'Program log: panicked at src/lib.rs:19:13:\nindex out of bounds: the len is 44 but the index is 85034'
'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S consumed 2495 of 200000 compute units'
'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S failed: SBF program panicked'

NEW:

'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S invoke [1]'
'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S consumed 546 of 200000 compute units'
'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S failed: SBF program Panicked in src/lib.rs at 19:13'

Calling unwrap on a None:

OLD:

'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S invoke [1]'
'Program log: panicked at src/lib.rs:21:15:\ncalled `Option::unwrap()` on a `None` value'
'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S consumed 2118 of 200000 compute units'
'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S failed: SBF program panicked'

NEW:

'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S invoke [1]'
'Program log: called `Option::unwrap()` on a `None` value'
'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S consumed 643 of 200000 compute units'
'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S failed: SBF program Panicked in src/lib.rs at 21:15'

[joncinque]

I still like the change! Just a few nits.

Also, since this change requires a new version of the platform tools, we need to make that clear somehow, since rust-version may not be respected.

On an older version, people will immediately see:

error[E0658]: use of unstable library feature 'panic_info_message'
 --> helloworld/src/lib.rs:1:1
  |
1 | solana_program_entrypoint::entrypoint!(process_instruction);

We can add a Solana Stack Exchange question and answer for it, and call it out in the release notes for this crate, but other than that, I'm not too sure how to make it clear.

cc @jacobcreech for more ideas

[jacobcreech]

Also, since this change requires a new version of the platform tools, we need to make that clear somehow, since rust-version may not be respected.

Does this mean we're starting the version floor for platform tools, or the expectation is that platform tools vX is required for this version and higher?

[joncinque]

Does this mean we're starting the version floor for platform tools, or the expectation is that platform tools vX is required for this version and higher?

I'm not sure I understand the first part. We did set a floor version in solana-program to match platform-tools for a time, but to be honest, I don't think we should set a minimum version unless it's needed:

solana-sdk/program/Cargo.toml

Line 12 in 691d306

rust-version = "1.79.0" # solana platform-tools rust version

Either way, to use this code, you'll need platform tools v1.47

[joncinque]

As another option, we could opt for a model like bytemuck, which adds features for changes that require a higher rust version: https://github.com/Lokathor/bytemuck/blob/028ff3bec68ab9c123dc578b74a7af4a72a37609/Cargo.toml#L70

[LucasSte]

The bytemuck model seems interesting, but I believe we should maintain a minimum Rust version to build the SDK. We should aim for compatibility with the last two (or three?) Rust versions we released support for. I know my patch would break that assumption, but we also shouldn't be blocked by supporting two-year-old Rust versions.

[LucasSte]

I applied @joncinque's suggestions, but it looks like we must wait for a v2.3 tag on the Agave's monorepo to be able to run the build-sbf CI test.

[LucasSte]

I applied @joncinque's suggestions, but it looks like we must wait for a v2.3 tag on the Agave's monorepo to be able to run the build-sbf CI test.

I had forgotten that I backported the new platform tools version to v2.2, so I bumped the Solana CLI version on Cargo.toml.

[LucasSte]

The PR is ready for another review. I happy to implement any method for versioning the Rust version, if necessary.

[ilya-bobyr]

It is common convention to hide symbols that are re-exported for use in macros from the documentation with #[doc(hidden)].
https://doc.rust-lang.org/rustdoc/write-documentation/the-doc-attribute.html#hidden

I personally also put them in a separate submodule.
Both for the documentation purposes and to avoid namespace pollution/name clashes.
If you have a lot of functionality in the macros, you might need to re-export a lot of symbols.

Suggested change

	pub use {
	solana_account_info::AccountInfo as __AccountInfo,
	solana_account_info::MAX_PERMITTED_DATA_INCREASE, solana_msg::msg as __msg,
	solana_program_error::ProgramResult, solana_pubkey::Pubkey as __Pubkey,
	solana_account_info::MAX_PERMITTED_DATA_INCREASE,
	// Re-exporting for custom_panic
	solana_define_syscall::definitions::{sol_log_ as __log, sol_panic_ as __panic},
	solana_msg::msg as __msg,
	solana_program_error::ProgramResult,
	solana_pubkey::Pubkey as __Pubkey,
	};
	pub use {
	solana_account_info::AccountInfo as __AccountInfo,
	solana_account_info::MAX_PERMITTED_DATA_INCREASE,
	solana_program_error::ProgramResult,
	};
	#[doc(hidden)]
	pub mod re_export {
	solana_define_syscall::definitions::{sol_log_, sol_panic_},
	solana_msg::msg,
	solana_pubkey::Pubkey,
	};

The downside here is that the fully qualified usage name gets longer.
But you can use local imports, to address that:

#[macro_export]
macro_rules! custom_panic_default {
    () => {
        #[cfg(all(not(feature = "custom-panic"), target_os = "solana"))]
        #[no_mangle]
        fn custom_panic(info: &core::panic::PanicInfo<'_>) {
            use $crate::re_export::{sol_log_, sol_panic_};

            // Full panic reporting
            if let Some(mm) = info.message().as_str() {
                unsafe {
                    sol_log_(mm.as_ptr(), mm.len() as u64);
                }
            }

            if let Some(loc) = info.location() {
                unsafe {
                    sol_panic_(
                        loc.file().as_ptr(),
                        loc.file().len() as u64,
                        loc.line() as u64,
                        loc.column() as u64,
                    )
                }
            }
        }
    };
}

[LucasSte]

I used the export with alias, following what was done for the other macros in this crate. I happy to change to whatever method works better here.

[LucasSte]

Since these imports are only used in the macros, the goal is for the them not be visible when writing a Solana programs, since developers may also import the same names as we. Creating a new module to wrap the macro specific imports has basically the same effect as aliasing the, doesn't it?

[ilya-bobyr]

Since these imports are only used in the macros, the goal is for the them not be visible when writing a Solana programs, since developers may also import the same names as we.

I think the problem is the other way around.
If you do not reexport the necessary symbols, at the spot of the macro call one needs to get all the same symbols available. Which is both inconvenient and fragile.
Though, maybe you have something else in mind here.

There is no problem in exporting an extra name, I think.
People should only import what they need.

The problem is that you do not know what is the name of the needed crates in the calling code. Nor can you specify their versions.
The only thing a macro can rely on is the special $crate name.

Creating a new module to wrap the macro specific imports has basically the same effect as aliasing them, doesn't it?

Yes, prefixing everything with a double underscore is another option.
Not sure, they are drastically different from the standpoint of the namespace pollution.

But, combining re-exports in a single module seems cleaner in this crate code itself.
You do not need to rename things.
I just remembered that for complex macros I've used to re-export the whole dependency like this:

#[doc(hidden)]
pub mod re_export {
    solana_define_syscall,
    solana_msg,
    solana_pubkey,
};

This makes it clear in the macro code, where each symbol is coming from:

#[macro_export]
macro_rules! custom_panic_default {
    () => {
        #[cfg(all(not(feature = "custom-panic"), target_os = "solana"))]
        #[no_mangle]
        fn custom_panic(info: &core::panic::PanicInfo<'_>) {
            use $crate::re_export::{
                solana_define_syscall::definitions::{sol_log_, sol_panic_},
                solana_msg::msg,
                solana_pubkey::Pubkey,
            };

[ilya-bobyr]

You can see this pattern user, for example, by the tokio macros.
Except that instead of re_export they call the helper module support.

Here is the support module is defined:

https://docs.rs/tokio/latest/src/tokio/macros/mod.rs.html#34-36

And here it is used:

https://docs.rs/tokio/latest/src/tokio/macros/join.rs.html#82-83

[LucasSte]

You've convinced me. Since this change also touches on the code of other macros, do you mind if I do it in another PR?

[ilya-bobyr]

Glad you found my comments useful.
If you think it is better done in a separate PR - you can absolutely do it this way.

[joncinque]

For versioning, since we're planning on publishing breaking changes soon, how about we just include this as a breaking change? I know that MSRV changes are not technically breaking, but given how slow people are to upgrade their Solana toolchains, I think it would be safer to just do a major version bump. Any thoughts?

[LucasSte]

For versioning, since we're planning on publishing breaking changes soon, how about we just include this as a breaking change? I know that MSRV changes are not technically breaking, but given how slow people are to upgrade their Solana toolchains, I think it would be safer to just do a major version bump. Any thoughts?

Fine for me.