-
Notifications
You must be signed in to change notification settings - Fork 48
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,4 +17,5 @@ unexpected_cfgs = { level = "warn", check-cfg = [ | |
] } | ||
|
||
[features] | ||
asm = [] | ||
std = [] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
|
@@ -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 | ||
} | ||
|
@@ -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 | ||
} | ||
|
@@ -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()) | ||
} | ||
|
@@ -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()) | ||
} | ||
|
@@ -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 | ||
|
@@ -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); | ||
} | ||
|
||
// 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 lenght, | ||
febo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// 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}", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can try There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per offline conversation, the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
slice::fill
takes58
CUs – same as the current code. Thesol_memset_
is more efficient in this case.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.