Skip to content

Conversation

@rustopian
Copy link
Contributor

@rustopian rustopian commented Nov 25, 2025

[WIP]

This addresses Issue #235, fixing two issues with the original (reverted) PR:

  1. The contract of from_raw_parts_mut was being violated, in a way that miri considered safe, but which could have theoretically caused trouble in the future.
  2. The implementation assumed that the number of bytes returned by the runtime was exactly
    mem::size_of::<Self>(), which was not always true. Rent, for example, contains a u64, an f64 and a u8. In memory the compiler pads this struct to 24 bytes, but the runtime stores its contents as a 17‑byte bincode serialization. The get_sysvar call thus returns only 17 bytes. Passing a 24‑byte buffer results in an OFFSET_LENGTH_EXCEEDS_SYSVAR error and undefined behavior when uninitialized bytes are interpreted as f64.

In order to address this second issue, this PR provides byte‑array representations of the sysvars with zero-copy accessors.

This is a breaking change since sysvar fields are now private, but updating is easy: dot fields are now dot accessors.

Note: this only updates the three sysvars Rent, EpochSchedule, and EpochRewards. The others will follow in a subsequent PR.

Thanks to jamie-osec for investigating the cause here and noticing the from_raw_parts_mut violation. I'll add you as contributor to the final.

@github-actions
Copy link
Contributor

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

@github-actions
Copy link
Contributor

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

@github-actions
Copy link
Contributor

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

@github-actions
Copy link
Contributor

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

Comment on lines 165 to 180
#[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.

@rustopian rustopian requested a review from febo November 25, 2025 15:36
@rustopian rustopian marked this pull request as ready for review November 25, 2025 15:36
#[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)

@github-actions
Copy link
Contributor

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

1 similar comment
@github-actions
Copy link
Contributor

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

@rustopian rustopian force-pushed the sysvar-get-packed-reprs branch from 6c827fd to a359358 Compare November 25, 2025 15:49
@github-actions
Copy link
Contributor

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

@github-actions
Copy link
Contributor

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

Comment on lines 130 to 138
#[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

@rustopian rustopian marked this pull request as draft November 25, 2025 18:31
@rustopian rustopian changed the title Use sol_get_sysvar instead of sol_get_<NAME>_sysvar, now with deserialization Use sol_get_sysvar instead of sol_get_<NAME>_sysvar, fixed Nov 26, 2025
@github-actions
Copy link
Contributor

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

2 similar comments
@github-actions
Copy link
Contributor

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

@github-actions
Copy link
Contributor

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

@rustopian rustopian force-pushed the sysvar-get-packed-reprs branch from 36610a1 to d558879 Compare November 26, 2025 10:59
@github-actions
Copy link
Contributor

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

@rustopian rustopian force-pushed the sysvar-get-packed-reprs branch from d558879 to e1ef624 Compare November 26, 2025 11:02
@github-actions
Copy link
Contributor

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

@github-actions
Copy link
Contributor

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

1 similar comment
@github-actions
Copy link
Contributor

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

@rustopian rustopian force-pushed the sysvar-get-packed-reprs branch from b2c4dd2 to 201ffb1 Compare November 26, 2025 12:21
@github-actions
Copy link
Contributor

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

@rustopian rustopian marked this pull request as ready for review November 26, 2025 12:31
@rustopian rustopian requested a review from febo November 26, 2025 12:31
@github-actions
Copy link
Contributor

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

@rustopian rustopian requested a review from joncinque November 26, 2025 16:21
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.

4 participants