Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add close unstable #56

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
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
1 change: 1 addition & 0 deletions sdk/pinocchio/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,5 @@ unexpected_cfgs = { level = "warn", check-cfg = [
] }

[features]
asm = []
std = []
54 changes: 44 additions & 10 deletions sdk/pinocchio/src/account_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ impl AccountInfo {

/// Changes the owner of the account.
#[allow(invalid_reference_casting)]
#[inline]
pub fn assign(&self, new_owner: &Pubkey) {
unsafe {
core::ptr::write_volatile(&(*self.raw).owner as *const _ as *mut Pubkey, *new_owner);
Expand All @@ -169,6 +170,7 @@ impl AccountInfo {
///
/// This method is unsafe because it does not return a `Ref`, thus leaving the borrow
/// flag untouched. Useful when an instruction has verified non-duplicate accounts.
#[inline]
pub unsafe fn borrow_lamports_unchecked(&self) -> &u64 {
&(*self.raw).lamports
}
Expand All @@ -180,6 +182,7 @@ impl AccountInfo {
/// This method is unsafe because it does not return a `Ref`, thus leaving the borrow
/// flag untouched. Useful when an instruction has verified non-duplicate accounts.
#[allow(clippy::mut_from_ref)]
#[inline]
pub unsafe fn borrow_mut_lamports_unchecked(&self) -> &mut u64 {
&mut (*self.raw).lamports
}
Expand All @@ -190,6 +193,7 @@ impl AccountInfo {
///
/// This method is unsafe because it does not return a `Ref`, thus leaving the borrow
/// flag untouched. Useful when an instruction has verified non-duplicate accounts.
#[inline]
pub unsafe fn borrow_data_unchecked(&self) -> &[u8] {
core::slice::from_raw_parts(self.data_ptr(), self.data_len())
}
Expand All @@ -201,6 +205,7 @@ impl AccountInfo {
/// This method is unsafe because it does not return a `Ref`, thus leaving the borrow
/// flag untouched. Useful when an instruction has verified non-duplicate accounts.
#[allow(clippy::mut_from_ref)]
#[inline]
pub unsafe fn borrow_mut_data_unchecked(&self) -> &mut [u8] {
core::slice::from_raw_parts_mut(self.data_ptr(), self.data_len())
}
Expand Down Expand Up @@ -391,6 +396,7 @@ impl AccountInfo {
/// since the account data will need to be zeroed out as well; otherwise the lenght,
/// lamports and owner can be set again before the data is wiped out from
/// the ledger using the keypair of the account being close.
#[inline]
pub fn close(&self) -> ProgramResult {
{
// make sure the account is not borrowed since we are about to
Expand Down Expand Up @@ -430,17 +436,45 @@ impl AccountInfo {
// - 8 bytes for the data_len
//
// So we can zero out them directly.
#[cfg(target_os = "solana")]
sol_memset_(self.data_ptr().sub(48), 0, 48);

Choose a reason for hiding this comment

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

you can use slice::fill and can get rid of the cfg attr

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using slice::fill takes 58 CUs – same as the current code. The sol_memset_ is more efficient in this case.

Choose a reason for hiding this comment

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

yes, but why?

If the compiler is desugaring to stores, CUs should be lower than the syscall - it's the whole point of applying the optimization. It might mean that the optimization is wrong, or the syscall is mispriced.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had the same question. 😊 @LucasSte looked into it and already has a plan to improve it.

Copy link

@LucasSte LucasSte Jan 23, 2025

Choose a reason for hiding this comment

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

Four stores is the threshold to call memset from compiler-builtins. This is what causing the CU bump. Both the threshold to call the library in LLVM and the threshold to call the syscall in compiler-builtins need to be revised.

}

// Zero out the account owner. While the field is a `Pubkey`, it is quicker
// to zero the 32 bytes as 4 x `u64`s.
*(self.data_ptr().sub(48) as *mut u64) = 0u64;
*(self.data_ptr().sub(40) as *mut u64) = 0u64;
*(self.data_ptr().sub(32) as *mut u64) = 0u64;
*(self.data_ptr().sub(24) as *mut u64) = 0u64;
// Zero the account lamports.
(*self.raw).lamports = 0;
// Zero the account data length.
(*self.raw).data_len = 0;
/// Zero out the the account's data length, lamports and owner fields, effectively
/// closing the account.
///
/// This doesn't protect against future reinitialization of the account
/// since the account data will need to be zeroed out as well; otherwise the length,
/// lamports and owner can be set again before the data is wiped out from
/// the ledger using the keypair of the account being close.
///
/// # Safety
///
/// This method is unsafe because it does not check if the account data is already
/// borrowed. It should only be called when the account is not being used.
///
/// It also makes assumptions about the layout and location of memory
/// referenced by `AccountInfo` fields. It should only be called for
/// instances of `AccountInfo` that were created by the runtime and received
/// in the `process_instruction` entrypoint of a program.
#[cfg(feature = "asm")]
#[inline(always)]
pub unsafe fn close_unstable(&self) {
#[cfg(target_os = "solana")]
unsafe {
let zero = 0u64;
core::arch::asm!(
"stxdw [{0}-8], {1}",

Choose a reason for hiding this comment

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

You can try stdw [{0}-8], 0 directly here, without an extra register.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As per offline conversation, the stdw is not yet available to the compiler – only the vm.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this has been disabled since like forever. You literally can't deploy a program that contains any of the st class insns. If your code never updates the value of r0, you can accomplish the same thing with stxdw[offset], r0 but it's not safe to do so in Rust without some kind of a compiler hint to tell it "please don't reassign r0".

Copy link

@LucasSte LucasSte Jan 24, 2025

Choose a reason for hiding this comment

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

You literally can't deploy a program that contains any of the st class insns.

Where did you get this information from? Not only does the class pass the bytecode verification, but also is implemented in the jitter and the interpreter.

The fact that the compiler wasn't emitting code for the store immediate class doesn't mean you can't deploy a program with it.

"stxdw [{0}-16], {1}",
"stxdw [{0}-24], {1}",
"stxdw [{0}-32], {1}",
"stxdw [{0}-40], {1}",
"stxdw [{0}-48], {1}",
in(reg) self.data_ptr(),
in(reg) zero,
options(nostack, preserves_flags)
);
}
}

/// Returns the memory address of the account data.
Expand Down
4 changes: 4 additions & 0 deletions sdk/pinocchio/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@
//! [`solana-program`]: https://docs.rs/solana-program/latest/solana_program/

#![no_std]
#![cfg_attr(
all(feature = "asm", target_os = "solana"),
feature(asm_experimental_arch, asm_const)
)]

#[cfg(feature = "std")]
extern crate std;
Expand Down
Loading