Skip to content
Open
Show file tree
Hide file tree
Changes from 7 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
12 changes: 6 additions & 6 deletions define-syscall/src/definitions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,12 @@ define_syscall!(fn sol_get_return_data(data: *mut u8, length: u64, program_id: *
// - `is_writable` (`u8`): `true` if the instruction requires the account to be writable
define_syscall!(fn sol_get_processed_sibling_instruction(index: u64, meta: *mut u8, program_id: *mut u8, data: *mut u8, accounts: *mut u8) -> u64);

// these are to be deprecated once they are superceded by sol_get_sysvar
define_syscall!(fn sol_get_clock_sysvar(addr: *mut u8) -> u64);
define_syscall!(fn sol_get_epoch_schedule_sysvar(addr: *mut u8) -> u64);
define_syscall!(fn sol_get_rent_sysvar(addr: *mut u8) -> u64);
define_syscall!(fn sol_get_last_restart_slot(addr: *mut u8) -> u64);
define_syscall!(fn sol_get_epoch_rewards_sysvar(addr: *mut u8) -> u64);
// these are deprecated - use sol_get_sysvar instead
define_syscall!(#[deprecated(since = "3.0.0", note = "Use `sol_get_sysvar` with `Clock` sysvar address instead")] fn sol_get_clock_sysvar(addr: *mut u8) -> u64);
define_syscall!(#[deprecated(since = "3.0.0", note = "Use `sol_get_sysvar` with `EpochSchedule` sysvar address instead")] fn sol_get_epoch_schedule_sysvar(addr: *mut u8) -> u64);
define_syscall!(#[deprecated(since = "3.0.0", note = "Use `sol_get_sysvar` with `Rent` sysvar address instead")] fn sol_get_rent_sysvar(addr: *mut u8) -> u64);
define_syscall!(#[deprecated(since = "3.0.0", note = "Use `sol_get_sysvar` with `LastRestartSlot` sysvar address instead")] fn sol_get_last_restart_slot(addr: *mut u8) -> u64);
define_syscall!(#[deprecated(since = "3.0.0", note = "Use `sol_get_sysvar` with `EpochRewards` sysvar address instead")] fn sol_get_epoch_rewards_sysvar(addr: *mut u8) -> u64);

// this cannot go through sol_get_sysvar but can be removed once no longer in use
define_syscall!(fn sol_get_fees_sysvar(addr: *mut u8) -> u64);
14 changes: 8 additions & 6 deletions define-syscall/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ pub mod definitions;
))]
#[macro_export]
macro_rules! define_syscall {
(fn $name:ident($($arg:ident: $typ:ty),*) -> $ret:ty) => {
($(#[$attr:meta])* fn $name:ident($($arg:ident: $typ:ty),*) -> $ret:ty) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enabling deprecated to be passed through (as in the original sol_get_sysvar PR)

$(#[$attr])*
#[inline]
pub unsafe fn $name($($arg: $typ),*) -> $ret {
// this enum is used to force the hash to be computed in a const context
Expand All @@ -23,8 +24,8 @@ macro_rules! define_syscall {
}

};
(fn $name:ident($($arg:ident: $typ:ty),*)) => {
define_syscall!(fn $name($($arg: $typ),*) -> ());
($(#[$attr:meta])* fn $name:ident($($arg:ident: $typ:ty),*)) => {
define_syscall!($(#[$attr])* fn $name($($arg: $typ),*) -> ());
}
}

Expand All @@ -34,13 +35,14 @@ macro_rules! define_syscall {
)))]
#[macro_export]
macro_rules! define_syscall {
(fn $name:ident($($arg:ident: $typ:ty),*) -> $ret:ty) => {
($(#[$attr:meta])* fn $name:ident($($arg:ident: $typ:ty),*) -> $ret:ty) => {
extern "C" {
$(#[$attr])*
pub fn $name($($arg: $typ),*) -> $ret;
}
};
(fn $name:ident($($arg:ident: $typ:ty),*)) => {
define_syscall!(fn $name($($arg: $typ),*) -> ());
($(#[$attr:meta])* fn $name:ident($($arg:ident: $typ:ty),*)) => {
define_syscall!($(#[$attr])* fn $name($($arg: $typ),*) -> ());
}
}

Expand Down
59 changes: 58 additions & 1 deletion sysvar/src/clock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,65 @@ pub use {
};

impl Sysvar for Clock {
impl_sysvar_get!(sol_get_clock_sysvar);
impl_sysvar_get!(id());
}

#[cfg(feature = "bincode")]
impl SysvarSerialize for Clock {}

#[cfg(test)]
mod tests {
use {super::*, crate::tests::to_bytes, serial_test::serial};

#[test]
#[cfg(feature = "bincode")]
fn test_clock_size_matches_bincode() {
// Prove that Clock's in-memory layout matches its bincode serialization,
// so we do not need to use sysvar_packed_struct.
let clock = Clock::default();
let in_memory_size = core::mem::size_of::<Clock>();
let bincode_size = bincode::serialized_size(&clock).unwrap() as usize;

assert_eq!(
in_memory_size, bincode_size,
"Clock in-memory size ({in_memory_size}) must match bincode size ({bincode_size})",
);
}

#[test]
#[serial]
fn test_clock_get_uses_sysvar_syscall() {
let expected = Clock {
slot: 1,
epoch_start_timestamp: 2,
epoch: 3,
leader_schedule_epoch: 4,
unix_timestamp: 5,
};

let data = to_bytes(&expected);
crate::tests::mock_get_sysvar_syscall(&data);

let got = Clock::get().unwrap();
assert_eq!(got, expected);
}

#[test]
#[serial]
fn test_clock_get_passes_correct_sysvar_id() {
let expected = Clock {
slot: 11,
epoch_start_timestamp: 22,
epoch: 33,
leader_schedule_epoch: 44,
unix_timestamp: 55,
};
let data = to_bytes(&expected);
let prev = crate::tests::mock_get_sysvar_syscall_with_id(&data, &id());

let got = Clock::get().unwrap();
assert_eq!(got, expected);

let _ = crate::program_stubs::set_syscall_stubs(prev);
}
}
72 changes: 70 additions & 2 deletions sysvar/src/epoch_rewards.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,15 +156,83 @@

#[cfg(feature = "bincode")]
use crate::SysvarSerialize;
use crate::{impl_sysvar_get, Sysvar};
use crate::{get_sysvar_via_packed, Sysvar};
pub use {
solana_epoch_rewards::EpochRewards,
solana_sdk_ids::sysvar::epoch_rewards::{check_id, id, ID},
};

#[repr(C, packed)]
#[derive(Clone, Copy)]
struct EpochRewardsPacked {
distribution_starting_block_height: u64,
num_partitions: u64,
parent_blockhash: [u8; 32],
total_points: u128,
total_rewards: u64,
distributed_rewards: u64,
active: u8, // bool as u8
}

const _: () = assert!(core::mem::size_of::<EpochRewardsPacked>() == 81);

impl From<EpochRewardsPacked> for EpochRewards {
fn from(p: EpochRewardsPacked) -> Self {
Copy link
Contributor Author

@rustopian rustopian Nov 25, 2025

Choose a reason for hiding this comment

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

@febo please lmk what you think of this approach.

Originally I did a whole macro setup, but it was overkill given that we're only worried about 3 padded sysvars here.

Compile-time checks just below ensure 1) field parity between the two structs (repr(C) and packed) and 2) size

Copy link
Contributor

Choose a reason for hiding this comment

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

Left a comment below about the use of packed.

// Ensure field parity at compile time
let EpochRewardsPacked {
distribution_starting_block_height,
num_partitions,
parent_blockhash,
total_points,
total_rewards,
distributed_rewards,
active,
} = p;

Self {
distribution_starting_block_height,
num_partitions,
parent_blockhash: solana_hash::Hash::new_from_array(parent_blockhash),
total_points,
total_rewards,
distributed_rewards,
active: active != 0,
}
}
}

impl Sysvar for EpochRewards {
impl_sysvar_get!(sol_get_epoch_rewards_sysvar);
fn get() -> Result<Self, solana_program_error::ProgramError> {
get_sysvar_via_packed::<Self, EpochRewardsPacked>(&id())
}
}

#[cfg(feature = "bincode")]
impl SysvarSerialize for EpochRewards {}

#[cfg(test)]
mod tests {
use {super::*, crate::Sysvar, serial_test::serial};

#[test]
#[serial]
#[cfg(feature = "bincode")]
fn test_epoch_rewards_get() {
let expected = EpochRewards {
distribution_starting_block_height: 42,
num_partitions: 7,
parent_blockhash: solana_hash::Hash::new_unique(),
total_points: 1234567890,
total_rewards: 100,
distributed_rewards: 10,
active: true,
};

let data = bincode::serialize(&expected).unwrap();
assert_eq!(data.len(), 81);

crate::tests::mock_get_sysvar_syscall(&data);
let got = EpochRewards::get().unwrap();
assert_eq!(got, expected);
}
}
57 changes: 55 additions & 2 deletions sysvar/src/epoch_schedule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,15 +121,68 @@
//! ```
#[cfg(feature = "bincode")]
use crate::SysvarSerialize;
use crate::{impl_sysvar_get, Sysvar};
use crate::{get_sysvar_via_packed, Sysvar};
pub use {
solana_epoch_schedule::EpochSchedule,
solana_sdk_ids::sysvar::epoch_schedule::{check_id, id, ID},
};

#[repr(C, packed)]
#[derive(Clone, Copy)]
struct EpochSchedulePacked {
slots_per_epoch: u64,
leader_schedule_slot_offset: u64,
warmup: u8, // bool as u8
first_normal_epoch: u64,
first_normal_slot: u64,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Using packed is problematic when there are unaligned fields – both first_normal_epoch and first_normal_slot are unaligned so can't be read directly.

What do you think if we change the definition of all sysvars to be like?

pub struct EpochSchedule {
    /// The maximum number of slots in each epoch.
    slots_per_epoch: [u8; 8],

    /// A number of slots before beginning of an epoch to calculate
    /// a leader schedule for that epoch.
    leader_schedule_slot_offset: [u8; 8],

    /// Whether epochs start short and grow.
    warmup: u8,

    /// The first epoch after the warmup period.
    ///
    /// Basically: `log2(slots_per_epoch) - log2(MINIMUM_SLOTS_PER_EPOCH)`.
    first_normal_epoch: [u8; 8],

    /// The first slot after the warmup period.
    ///
    /// Basically: `MINIMUM_SLOTS_PER_EPOCH * (2.pow(first_normal_epoch) - 1)`.
    first_normal_slot: [u8; 8],
}

impl EpochSchedule {
    pub fn slots_per_epoch(&self) -> u64 {
        u64::from_le_bytes(self.slots_per_epoch)
    }

    pub fn leader_schedule_slot_offset(&self) -> u64 {
        u64::from_le_bytes(self.leader_schedule_slot_offset)
    }

    // Could also return the `u8` value directly.
    pub fn warmup(&self) -> Result<bool, ProgramError> {
        match self.warmup {
            0 => Ok(false),
            1 => Ok(true),
            _ => Err(ProgramError::InvalidAccountData),
        }
    }

     // other fields...

    pub fn first_normal_slot(&self) -> u64 {
        u64::from_le_bytes(self.first_normal_slot)
    }
}

There is no "penalty" for using u64::from_le_bytes, although the implementation is a bit more verbose.

Copy link
Contributor Author

@rustopian rustopian Nov 25, 2025

Choose a reason for hiding this comment

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

Ah, of course. The packed struct works currently since it’s just used for From, but that of course copies everything out.

I’ll implement your approach, thanks.

(Despite appearances here, I did in fact attend your optimization talk which mentioned this. exact. pattern. Ahem.)

Choose a reason for hiding this comment

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

Doesn't this constitute a breaking change (public fields removed)? I believe the internal-only packed version is better here - an unaligned read from a packed field produces the same IR/code as reading it as an array.

Copy link
Contributor Author

@rustopian rustopian Nov 26, 2025

Choose a reason for hiding this comment

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

@febo I've implemented your suggestion in this PR, and it's much nicer looking (and zero copy). However, as @jamie-osec mentions, the accessors are a breaking change. What formerly was e.g. .slots_per_epoch becomes .slots_per_epoch()

Note: I've only implemented it for the problematic Rent, EpochRewards, and EpochSchedule for now, pending our decision here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this constitute a breaking change (public fields removed)?

Yes, that is a drawback of the approach. We should be probably ok since crates are versioned individually.

I believe the internal-only packed version is better here - an unaligned read from a packed field produces the same IR/code as reading it as an array.

The issue is that we do an extra copy to return the "non-packed" version, which is what we would like to avoid. Returning the "packed" version is problematic – easy to end up in UB by taking a reference to an unaligned field.

Copy link
Contributor Author

@rustopian rustopian Nov 26, 2025

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this break would be annoying for Agave and some downstream programs that directly access any of these fields. We've introduced Pod* types for other sysvars, so I would recommend doing the same thing here


const _: () = assert!(core::mem::size_of::<EpochSchedulePacked>() == 33);

impl From<EpochSchedulePacked> for EpochSchedule {
fn from(p: EpochSchedulePacked) -> Self {
// Ensure field parity at compile time
let EpochSchedulePacked {
slots_per_epoch,
leader_schedule_slot_offset,
warmup,
first_normal_epoch,
first_normal_slot,
} = p;

Self {
slots_per_epoch,
leader_schedule_slot_offset,
warmup: warmup != 0,
first_normal_epoch,
first_normal_slot,
}
}
}

impl Sysvar for EpochSchedule {
impl_sysvar_get!(sol_get_epoch_schedule_sysvar);
fn get() -> Result<Self, solana_program_error::ProgramError> {
get_sysvar_via_packed::<Self, EpochSchedulePacked>(&id())
}
}

#[cfg(feature = "bincode")]
impl SysvarSerialize for EpochSchedule {}

#[cfg(test)]
mod tests {
use {super::*, crate::Sysvar, serial_test::serial};

#[test]
#[serial]
#[cfg(feature = "bincode")]
fn test_epoch_schedule_get() {
let expected = EpochSchedule::custom(1234, 5678, false);
let data = bincode::serialize(&expected).unwrap();
assert_eq!(data.len(), 33);

crate::tests::mock_get_sysvar_syscall(&data);
let got = EpochSchedule::get().unwrap();
assert_eq!(got, expected);
}
}
48 changes: 47 additions & 1 deletion sysvar/src/fees.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ impl SysvarSerialize for Fees {}

#[cfg(test)]
mod tests {
use super::*;
use {super::*, serial_test::serial};

#[test]
fn test_clone() {
Expand All @@ -76,4 +76,50 @@ mod tests {
let cloned_fees = fees.clone();
assert_eq!(cloned_fees, fees);
}

struct MockFeesSyscall;
impl crate::program_stubs::SyscallStubs for MockFeesSyscall {
fn sol_get_fees_sysvar(&self, var_addr: *mut u8) -> u64 {
let fees = Fees {
fee_calculator: FeeCalculator {
lamports_per_signature: 42,
},
};
unsafe {
std::ptr::copy_nonoverlapping(
&fees as *const _ as *const u8,
var_addr,
core::mem::size_of::<Fees>(),
);
}
solana_program_entrypoint::SUCCESS
}
}

#[test]
#[serial]
fn test_fees_get_deprecated_syscall_path() {
let _ = crate::program_stubs::set_syscall_stubs(Box::new(MockFeesSyscall));
let got = Fees::get().unwrap();
assert_eq!(got.fee_calculator.lamports_per_signature, 42);
}

struct FailFeesSyscall;
impl crate::program_stubs::SyscallStubs for FailFeesSyscall {
fn sol_get_fees_sysvar(&self, _var_addr: *mut u8) -> u64 {
9999
}
}

#[test]
#[serial]
fn test_fees_get_deprecated_non_success_maps_to_unsupported() {
let prev = crate::program_stubs::set_syscall_stubs(Box::new(FailFeesSyscall));
let got = Fees::get();
assert_eq!(
got,
Err(solana_program_error::ProgramError::UnsupportedSysvar)
);
let _ = crate::program_stubs::set_syscall_stubs(prev);
}
}
35 changes: 34 additions & 1 deletion sysvar/src/last_restart_slot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,41 @@ pub use {
};

impl Sysvar for LastRestartSlot {
impl_sysvar_get!(sol_get_last_restart_slot);
impl_sysvar_get!(id());
}

#[cfg(feature = "bincode")]
impl SysvarSerialize for LastRestartSlot {}

#[cfg(test)]
mod tests {
use {super::*, crate::tests::to_bytes, serial_test::serial};

#[test]
#[cfg(feature = "bincode")]
fn test_last_restart_slot_size_matches_bincode() {
// Prove that LastRestartSlot's in-memory layout matches its bincode serialization,
// so we do not need to use sysvar_packed_struct.
let slot = LastRestartSlot::default();
let in_memory_size = core::mem::size_of::<LastRestartSlot>();
let bincode_size = bincode::serialized_size(&slot).unwrap() as usize;

assert_eq!(
in_memory_size, bincode_size,
"LastRestartSlot in-memory size ({in_memory_size}) must match bincode size ({bincode_size})",
);
}

#[test]
#[serial]
fn test_last_restart_slot_get_uses_sysvar_syscall() {
let expected = LastRestartSlot {
last_restart_slot: 9999,
};
let data = to_bytes(&expected);
crate::tests::mock_get_sysvar_syscall(&data);

let got = LastRestartSlot::get().unwrap();
assert_eq!(got, expected);
}
}
Loading