Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 3 additions & 16 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 14 additions & 14 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,26 +20,26 @@ Entrypoint implementation currently included in the benchmark:
- [`solana-nostd-entrypoint`](https://github.com/cavemanloverboy/solana-nostd-entrypoint)
- [`solana-program`](https://github.com/anza-xyz/agave/tree/master/sdk/program)

| Benchmark | `pinocchio` | `solana-nostd-entrypoint` | `solana-program` |
| ------------- | ------------ | ------------------------- | ----------------- |
| Benchmark | `pinocchio` | `solana-nostd-entrypoint` | `solana-program` |
| ------------- | -------------- | ------------------------- | ------------------ |
| _Entrypoint_ |
| Ping | 🟩 **14** | 🟩 **14** | 🟧 41 (+27) |
| Log | 🟩 **119** | 🟩 **119** | 🟧 146 (+27) |
| Account (1) | 🟩 **38** | 🟩 39 (+1) | 🟥 235 (+196) |
| Account (3) | 🟩 **66** | 🟩 69 (+3) | 🟥 541 (+475) |
| Account (5) | 🟩 **94** | 🟩 99 (+5) | 🟥 847 (+753) |
| Account (10) | 🟩 **164** | 🟩 174 (+10) | 🟥 1,612 (+1,448) |
| Account (20) | 🟩 **304** | 🟨 324 (+20) | 🟥 3,142 (+2,838) |
| Account (32) | 🟩 **472** | 🟨 504 (+32) | 🟥 4,978 (+4,506) |
| Account (64) | 🟩 **920** | 🟨 985 (+65) | 🟥 9,874 (+8,954) |
| Ping | 🟩 **14** | 🟩 15 (+1) | 🟥 117 (+103) |
| Log | 🟩 **119** | 🟩 120 (+1) | 🟥 222 (+103) |
| Account (1) | 🟩 **38** | 🟩 42 (+4) | 🟥 317 (+279) |
| Account (3) | 🟩 **66** | 🟩 72 (+6) | 🟥 641 (+575) |
| Account (5) | 🟩 **94** | 🟩 102 (+8) | 🟥 965 (+871) |
| Account (10) | 🟩 **164** | 🟩 177 (+13) | 🟥 1,775 (+1,611) |
| Account (20) | 🟩 **304** | 🟨 327 (+23) | 🟥 3,395 (+3,091) |
| Account (32) | 🟩 **472** | 🟨 507 (+35) | 🟥 5,339 (+4,867) |
| Account (64) | 🟩 **920** | 🟨 988 (+68) | 🟥 10,523 (+9,603) |
| _CPI_ |
| CreateAccount | 🟩 **1,449** | 🟨 1,494 (+45) | 🟥 2,786 (+1,337) |
| Transfer | 🟩 **1,439** | 🟨 1,487 (+48) | 🟥 2,379 (+940) |
| CreateAccount | 🟩 **1,311** | 🟩 1,314 (+3) | 🟥 2,866 (+1,555) |
| Transfer | 🟩 **1,307** | 🟩 1,309 (+2) | 🟥 2,459 (+1,152) |

> [!IMPORTANT]
> Values correspond to compute units (CUs) consumed by the entrypoint. The delta in relation to the lowest consumption is shown in brackets.
>
> Solana CLI `v2.2.6` was used in the bench tests.
> Solana CLI `v2.2.13` was used in the bench tests.

## Benchmark

Expand Down
5 changes: 2 additions & 3 deletions programs/pinocchio/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,5 @@ check-cfg = [
crate-type = ["cdylib", "lib"]

[dependencies]
pinocchio = "0.8"
pinocchio-pubkey = "0.2.4"
pinocchio-system = "0.2.3"
pinocchio = { version = "0.8", git = "https://github.com/anza-xyz/pinocchio.git", branch = "febo/cpi-tweaks" }
pinocchio-pubkey = { version = "0.2", git = "https://github.com/anza-xyz/pinocchio.git", branch = "febo/cpi-tweaks" }
58 changes: 58 additions & 0 deletions programs/pinocchio/src/cpi/create_account.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
use super::SYSTEM_PROGRAM_ID;
use pinocchio::{
account_info::AccountInfo,
cpi::invoke_signed_unchecked,
instruction::{AccountMeta, Instruction},
pubkey::Pubkey,
ProgramResult,
};

/// Create a new account.
///
/// This function is a wrapper around the system program's `create_account`
/// instruction.
///
/// # Safety
///
/// This function assumes that accounts are not mutably borrowed.
Comment on lines +10 to +17
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

btw idk if you or anyone on the team know this already but enabling potential UB like this can be extremely dangerous for onchain programs. By right, UB means the following might happen:

  • user has code that forgot to drop a lamports RefMut
  • a similar unsafe CPI helper is used, which modifies the lamports field, resulting in a simultaneous mutable borrow UB
  • there is code later that checks whether the lamports amount has changed after the CPI using the same RefMut from above. Because of the above UB, the rust compiler assumes that the RefMut has exclusive borrow of lamports even across the CPI call, so it optimizes the check to always pass instead of generating the code to actually perform the check

Granted, pinocchio's RefMut contains raw pointers so this is probably not going to happen because rustc is generally very conservative about optimizing anything to do with raw pointers, but still something worth keeping in mind imo.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

That is why the function is marked unsafe. 😉 As the comment says, you need to guarantee that the account is not already borrowed - I probably need to make that comment more clear but most likely I will replace it with a "proper" client once the system crate gets refactored.

In any case, this is not a "generic" helper for the CPI. It is only used for the purpose of this benchmark. But overall the idea is to offer both versions: a safe one with all the necessary checks and an unsafe where you are responsible for the checks. If a developer decides to use the unsafe, then it needs to make sure that it satisfies the constraints. And yes, it enables creating UB, but that is not different than unsafe rust allowing you to create UB, right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The difference is that exposing a public unsafe api, as opposed to encapsulating all the unsafe behaviour behind a safe public api, puts more burden on devs and auditors since now every call site of such a fn needs to be checked thoroughly rather than just the src.

For this case, this is made harder by the fact that Ref and RefMut dont have their scope automatically determined like references. Forgot to call drop() or surround your RefMut in curly braces before unsafe CPI? Instant UB

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Hmmm, I am not sure if I follow the argument. There is a "safe" variant invoke_signed that performs all the required checks. If a dev chooses to use the unsafe invoke_signed_unchecked, then it is an explicit choice that comes with constraints. This is no different than using assume_init for example – it is up to the caller to guarantee that the value is really initialized; calling on an uninitialized value leads to instant UB.

In the particular context of this create_account_unchecked, this is not meant to be a "public" CPI helper for the system program. It is just here for the benchmark and it is being used in a context where there are no borrows to the accounts.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, this is a completely acceptable design decision and standpoint for pinocchio to have. I'm not requesting any changes or anything. The original comment was just a friendly reminder that taking this standpoint can have far-reaching effects on users of this library, and therefore the entire ecosystem, in case these effects weren't considered. By exposing a public unsafe API that can be hard to get right (as demonstrated by the discussion above) and presenting it as part of an '"average" program implementation', pinocchio program devs who don't 100% know what they're doing can get seriously bitten. As library devs, I think that part of our job should be to take away as many of these footguns as possible; ensuring security of onchain programs' business logic is hard enough already. But this is just my own standpoint, pinocchio's standpoint might be to expose low-level unsafe functionality in order to enable program devs to achieve maximum performance, and that's fine too

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is no different than using assume_init for example – it is up to the caller to guarantee that the value is really initialized; calling on an uninitialized value leads to instant UB.

Yep, but there is a difference between making a public library function that uses assume_init internally and verifies that its usage of it is safe in all cases, and making a public unsafe library function that puts the burden of making sure that its safe on the library user

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

This is no different than using assume_init for example – it is up to the caller to guarantee that the value is really initialized; calling on an uninitialized value leads to instant UB.

Yep, but there is a difference between making a public library function that uses assume_init internally and verifies that its usage of it is safe in all cases, and making a public unsafe library function that puts the burden of making sure that its safe on the library user

I was referring to the fact that assume_init is a public library unsafe function in the same way that invoke_signed_uncheckedis a public library unsafe function. From this standpoint, there is no difference. 😉

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Yes, this is a completely acceptable design decision and standpoint for pinocchio to have. I'm not requesting any changes or anything. The original comment was just a friendly reminder that taking this standpoint can have far-reaching effects on users of this library, and therefore the entire ecosystem, in case these effects weren't considered. By exposing a public unsafe API that can be hard to get right (as demonstrated by the discussion above) and presenting it as part of an '"average" program implementation', pinocchio program devs who don't 100% know what they're doing can get seriously bitten. As library devs, I think that part of our job should be to take away as many of these footguns as possible; ensuring security of onchain programs' business logic is hard enough already. But this is just my own standpoint, pinocchio's standpoint might be to expose low-level unsafe functionality in order to enable program devs to achieve maximum performance, and that's fine too

That is precisely the design decision: offer both variants and it is up to the developer to decide which one to use. I would not consider it "hard to get it right" once you understand the implications – for example, for you it is very clear the implication of using it and how to avoid issues. By no means it is being presented as an "average" program implementation – if that is what comes across, it is not intentional and we should definitely improve the documentation to make that clear. Not sure if your comment is generic or specific to the example on this PR – if it is the latter, we should also consider the context of this PR, since this discussion is happening on an unfinished draft PR of a benchmark program.

Developers "who don't 100% know what they're doing" should not be using the unsafe variants, the same way they should not be using unsafe Rust – at the end of the day, no library can prevent you doing the wrong thing if you don't know what you are doing. The fact that you have to wrap unsafe code in an unsafe block is already a good sign that if you don't know what that code is doing, you probably should not be using it. But I think this should not prevent a library to offer low-level unsafe functionality the same way that Rust exposes unsafe functionality – safer "footgun-free" variants are available. It is possible to write a whole program without any unsafe Rust.

I appreciate your comment and agree that going through examples that use unsafe code can lead to people copying the code without understanding its implications. But I think this is a separate issue – it is more about developer education/discipline than whether a library should offer unsafe API or not.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I appreciate your detailed replies too, thank you for clarifying pinocchio's design decisions.

By no means it is being presented as an "average" program implementation – if that is what comes across, it is not intentional and we should definitely improve the documentation to make that clear

The quote is from this part of the README

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Good catch! I think adding the CPI benches did not fit well the original intent of the benchmark, might need to revise the whole thing. Later on the same section there is this text:

The implementation across all different entrypoint programs is as similar as possible. In most cases, the only differences are on the types import, since each entrypoint defines their own AccountInfo and/or Pubkey types.

This is not possible when it comes to CPI clients, since each implementation does things differently.

pub unsafe fn create_account_unchecked(
from: &AccountInfo,
to: &AccountInfo,
lamports: u64,
space: u64,
owner: &Pubkey,
) -> ProgramResult {
// instruction accounts
let account_metas = [
AccountMeta::writable_signer(from.key()),
AccountMeta::writable_signer(to.key()),
];

// instruction data
// - [0..4 ]: instruction discriminator
// - [4..12 ]: lamports
// - [12..20]: account space
// - [20..52]: owner pubkey
let mut instruction_data = [0; 52];
// create account instruction has a '0' discriminator
instruction_data[4..12].copy_from_slice(&lamports.to_le_bytes());
instruction_data[12..20].copy_from_slice(&space.to_le_bytes());
instruction_data[20..52].copy_from_slice(owner);

// SAFETY: Accounts are in the correct order since the helper created
// the instruction accounts array. The caller must guarantee that accounts
// are not mutably borrowed.
unsafe {
invoke_signed_unchecked(
&Instruction {
program_id: &SYSTEM_PROGRAM_ID,
accounts: &account_metas,
data: &instruction_data,
},
&[from.into(), to.into()],
&[],
);
}

Ok(())
}
9 changes: 9 additions & 0 deletions programs/pinocchio/src/cpi/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
use pinocchio::pubkey::Pubkey;

mod create_account;
mod transfer;

pub use create_account::create_account_unchecked;
pub use transfer::transfer_unchecked;

const SYSTEM_PROGRAM_ID: Pubkey = [0; 32];
51 changes: 51 additions & 0 deletions programs/pinocchio/src/cpi/transfer.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
use super::SYSTEM_PROGRAM_ID;
use pinocchio::{
account_info::AccountInfo,
cpi::invoke_signed_unchecked,
instruction::{AccountMeta, Instruction},
ProgramResult,
};

/// Transfer lamports between accounts.
///
/// This function is a wrapper around the system program's `transfer`
/// instruction.
///
/// # Safety
///
/// This function assumes that accounts are not mutably borrowed.
pub unsafe fn transfer_unchecked(
from: &AccountInfo,
to: &AccountInfo,
lamports: u64,
) -> ProgramResult {
// instruction accounts
let account_metas = [
AccountMeta::writable_signer(from.key()),
AccountMeta::writable(to.key()),
];

// instruction data
// - [0..4 ]: instruction discriminator
// - [4..12]: lamports amount
let mut instruction_data = [0; 12];
instruction_data[0] = 2;
instruction_data[4..12].copy_from_slice(&lamports.to_le_bytes());

// SAFETY: Accounts are in the correct order since the helper created
// the instruction accounts array. The caller must guarantee that accounts
// are not mutably borrowed.
unsafe {
invoke_signed_unchecked(
&Instruction {
program_id: &SYSTEM_PROGRAM_ID,
accounts: &account_metas,
data: &instruction_data,
},
&[from.into(), to.into()],
&[],
);
}

Ok(())
}
9 changes: 7 additions & 2 deletions programs/pinocchio/src/entrypoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,15 @@ use {
process_account, process_create_account, process_log, process_ping, process_transfer,
},
},
pinocchio::{account_info::AccountInfo, entrypoint, pubkey::Pubkey, ProgramResult},
pinocchio::{
account_info::AccountInfo, no_allocator, nostd_panic_handler, program_entrypoint,
pubkey::Pubkey, ProgramResult,
},
};

entrypoint!(process_instruction);
program_entrypoint!(process_instruction);
no_allocator!();
nostd_panic_handler!();

#[inline(always)]
pub fn process_instruction(
Expand Down
3 changes: 3 additions & 0 deletions programs/pinocchio/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
#![no_std]

pub mod cpi;
pub mod entrypoint;
pub mod instruction;
pub mod processor;
Expand Down
26 changes: 11 additions & 15 deletions programs/pinocchio/src/processor.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::cpi::{create_account_unchecked, transfer_unchecked};
use pinocchio::{account_info::AccountInfo, msg, program_error::ProgramError, ProgramResult};
use pinocchio_system::instructions::{CreateAccount, Transfer};

#[inline(always)]
pub fn process_ping() -> ProgramResult {
Expand All @@ -23,22 +23,18 @@ pub fn process_account(accounts: &[AccountInfo], expected: u64) -> ProgramResult

#[inline(always)]
pub fn process_create_account(accounts: &[AccountInfo]) -> ProgramResult {
CreateAccount {
from: &accounts[0],
to: &accounts[1],
lamports: 500_000_000,
space: 10,
owner: &crate::ID,
}
.invoke()
let [from, to, _remaining @ ..] = accounts else {
return Err(ProgramError::InvalidArgument);
};

unsafe { create_account_unchecked(from, to, 500_000_000, 10, &crate::ID) }
}

#[inline(always)]
pub fn process_transfer(accounts: &[AccountInfo]) -> ProgramResult {
Transfer {
from: &accounts[0],
to: &accounts[1],
lamports: 1_000_000_000,
}
.invoke()
let [from, to, _remaining @ ..] = accounts else {
return Err(ProgramError::InvalidArgument);
};

unsafe { transfer_unchecked(from, to, 1_000_000_000) }
}
10 changes: 7 additions & 3 deletions programs/solana-nostd-entrypoint/src/cpi/create_account.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
use super::invoke;
use super::invoke_unchecked;
use solana_nostd_entrypoint::{InstructionC, NoStdAccountInfo};
use solana_program::{entrypoint::ProgramResult, pubkey::Pubkey, system_program};

/// Create a new account.
///
/// This function is a wrapper around the system program's `create_account`
/// instruction.
pub fn create_account(
///
/// # Safety
///
/// This function assumes that accounts are not mutably borrowed.
pub unsafe fn create_account_unchecked(
from: &NoStdAccountInfo,
to: &NoStdAccountInfo,
lamports: u64,
Expand All @@ -26,7 +30,7 @@ pub fn create_account(

let instruction_accounts = [from.to_meta_c_signer(), to.to_meta_c_signer()];

invoke(
invoke_unchecked(
&InstructionC {
program_id: &system_program::ID,
accounts: instruction_accounts.as_ptr(),
Expand Down
34 changes: 12 additions & 22 deletions programs/solana-nostd-entrypoint/src/cpi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,12 @@ pub use transfer::*;
///
/// These checks are similar to the checks performed by the default invoke in
/// `solana_program`.
fn invoke<const ACCOUNTS: usize>(
///
/// # Safety
///
/// This function assumes that accounts are not mutably borrowed and passed
/// in the correct order.
unsafe fn invoke_unchecked<const ACCOUNTS: usize>(
instruction: &InstructionC,
accounts: &[&NoStdAccountInfo; ACCOUNTS],
) -> ProgramResult {
Expand All @@ -29,27 +34,12 @@ fn invoke<const ACCOUNTS: usize>(

const UNINIT: MaybeUninit<AccountInfoC> = MaybeUninit::<AccountInfoC>::uninit();
let mut infos = [UNINIT; ACCOUNTS];

let metas = unsafe { core::slice::from_raw_parts(instruction.accounts, ACCOUNTS) };

for index in 0..ACCOUNTS {
let info = &accounts[index];
let meta = &metas[index];

if *info.key() != unsafe { *meta.pubkey } {
return Err(ProgramError::InvalidArgument);
}

if meta.is_writable {
let _ = info.try_borrow_mut_data();
let _ = info.try_borrow_mut_lamports();
} else {
let _ = info.try_borrow_data();
let _ = info.try_borrow_lamports();
}

infos[index].write(info.to_info_c());
}
infos
.iter_mut()
.zip(accounts.iter())
.for_each(|(info, account)| {
info.write(account.to_info_c());
});

let seeds: &[&[&[u8]]] = &[];

Expand Down
14 changes: 11 additions & 3 deletions programs/solana-nostd-entrypoint/src/cpi/transfer.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
use super::invoke;
use super::invoke_unchecked;
use solana_nostd_entrypoint::{InstructionC, NoStdAccountInfo};
use solana_program::{entrypoint::ProgramResult, system_program};

/// Transfer lamports between accounts.
///
/// This function is a wrapper around the system program's `transfer`
/// instruction.
pub fn transfer(from: &NoStdAccountInfo, to: &NoStdAccountInfo, lamports: u64) -> ProgramResult {
///
/// # Safety
///
/// This function assumes that accounts are not mutably borrowed.
pub unsafe fn transfer_unchecked(
from: &NoStdAccountInfo,
to: &NoStdAccountInfo,
lamports: u64,
) -> ProgramResult {
// instruction data
// - [0..4 ]: instruction discriminator
// - [4..12 ]: lamports
Expand All @@ -16,7 +24,7 @@ pub fn transfer(from: &NoStdAccountInfo, to: &NoStdAccountInfo, lamports: u64) -

let instruction_accounts = [from.to_meta_c_signer(), to.to_meta_c_signer()];

invoke(
invoke_unchecked(
&InstructionC {
program_id: &system_program::ID,
accounts: instruction_accounts.as_ptr(),
Expand Down
Loading