Clarify the use of constant values (#200)

* Add comments on constants

* Improve offset comments

* Add bitmask to dictionary

* Renamed to field_at_offset

Author: febo

scripts/setup/solana.dic

@@ -63,4 +63,5 @@ callee
 RPC
 ed25519
 performant
-syscall/S
+syscall/S
+bitmask

sdk/pinocchio/src/account_info.rs

@@ -22,12 +22,16 @@ pub const MAX_PERMITTED_DATA_INCREASE: usize = 1_024 * 10;
 pub enum BorrowState {
     /// Mask to check whether an account is already borrowed.
     ///
-    /// This will test both data and lamports borrow state.
+    /// This will test both data and lamports borrow state. Any position
+    /// in the borrow byte that is not set means that the account
+    /// is borrowed in that state.
     Borrowed = 0b_1111_1111,
 
     /// Mask to check whether an account is already mutably borrowed.
     ///
-    /// This will test both data and lamports mutable borrow state.
+    /// This will test both data and lamports mutable borrow state. If
+    /// one of the mutably borrowed bits is not set, then the account
+    /// is mutably borrowed in that state.
     MutablyBorrowed = 0b_1000_1000,
 }
 
@@ -261,18 +265,19 @@ impl AccountInfo {
         self.can_borrow_lamports()?;
 
         let borrow_state = self.raw as *mut u8;
+        // Use one immutable borrow for lamports by subtracting `1` from the
+        // lamports borrow counter bits; we are guaranteed that there is at
+        // least one immutable borrow available.
+        //
         // SAFETY: The `borrow_state` is a mutable pointer to the borrow state
         // of the account, which is guaranteed to be valid.
-        //
-        // "consumes" one immutable borrow for lamports; we are guaranteed
-        // that there is at least one immutable borrow available.
-        unsafe { *borrow_state -= 1 << LAMPORTS_SHIFT };
+        unsafe { *borrow_state -= 1 << LAMPORTS_BORROW_SHIFT };
 
         // return the reference to lamports
         Ok(Ref {
             value: unsafe { NonNull::from(&(*self.raw).lamports) },
             state: unsafe { NonNull::new_unchecked(borrow_state) },
-            borrow_shift: LAMPORTS_SHIFT,
+            borrow_shift: LAMPORTS_BORROW_SHIFT,
             marker: PhantomData,
         })
     }
@@ -284,18 +289,18 @@ impl AccountInfo {
         self.can_borrow_mut_lamports()?;
 
         let borrow_state = self.raw as *mut u8;
+        // Set the mutable lamports borrow bit to `0`; we are guaranteed
+        // that lamports are not already borrowed in any form.
+        //
         // SAFETY: The `borrow_state` is a mutable pointer to the borrow state
         // of the account, which is guaranteed to be valid.
-        //
-        // "consumes" the mutable borrow for lamports; we are guaranteed
-        // that lamports are not already borrowed in any form
         unsafe { *borrow_state &= 0b_0111_1111 };
 
         // return the mutable reference to lamports
         Ok(RefMut {
             value: unsafe { NonNull::from(&mut (*self.raw).lamports) },
             state: unsafe { NonNull::new_unchecked(borrow_state) },
-            borrow_mask: LAMPORTS_MASK,
+            borrow_bitmask: LAMPORTS_MUTABLE_BORROW_BITMASK,
             marker: PhantomData,
         })
     }
@@ -314,12 +319,14 @@ impl AccountInfo {
     pub fn can_borrow_lamports(&self) -> Result<(), ProgramError> {
         let borrow_state = unsafe { (*self.raw).borrow_state };
 
-        // check if mutable borrow is already taken
-        if borrow_state & 0b_1000_0000 != 0b_1000_0000 {
+        // Check whether the mutable lamports borrow bit is already in
+        // use (value `0`) or not. If it is `0`, then the borrow will fail.
+        if borrow_state & LAMPORTS_MUTABLE_BORROW_BITMASK == 0 {
             return Err(ProgramError::AccountBorrowFailed);
         }
 
-        // check if we have reached the max immutable borrow count
+        // Check whether we have reached the maximum immutable lamports borrow count
+        // or not, i.e., it fails when all immutable lamports borrow bits are `0`.
         if borrow_state & 0b_0111_0000 == 0 {
             return Err(ProgramError::AccountBorrowFailed);
         }
@@ -341,7 +348,8 @@ impl AccountInfo {
     pub fn can_borrow_mut_lamports(&self) -> Result<(), ProgramError> {
         let borrow_state = unsafe { (*self.raw).borrow_state };
 
-        // check if any borrow (mutable or immutable) is already taken for lamports
+        // Check whether any (mutable or immutable) lamports borrow bits are
+        // in use (value `0`) or not.
         if borrow_state & 0b_1111_0000 != 0b_1111_0000 {
             return Err(ProgramError::AccountBorrowFailed);
         }
@@ -356,18 +364,19 @@ impl AccountInfo {
         self.can_borrow_data()?;
 
         let borrow_state = self.raw as *mut u8;
+        // Use one immutable borrow for data by subtracting `1` from the data
+        // borrow counter bits; we are guaranteed that there is at least one
+        // immutable borrow available.
+        //
         // SAFETY: The `borrow_state` is a mutable pointer to the borrow state
         // of the account, which is guaranteed to be valid.
-        //
-        // "consumes" one immutable borrow for data; we are guaranteed
-        // that there is at least one immutable borrow available
         unsafe { *borrow_state -= 1 };
 
         // return the reference to data
         Ok(Ref {
             value: unsafe { NonNull::from(from_raw_parts(self.data_ptr(), self.data_len())) },
             state: unsafe { NonNull::new_unchecked(borrow_state) },
-            borrow_shift: DATA_SHIFT,
+            borrow_shift: DATA_BORROW_SHIFT,
             marker: PhantomData,
         })
     }
@@ -379,18 +388,18 @@ impl AccountInfo {
         self.can_borrow_mut_data()?;
 
         let borrow_state = self.raw as *mut u8;
+        // Set the mutable data borrow bit to `0`; we are guaranteed that account
+        // data is not already borrowed in any form.
+        //
         // SAFETY: The `borrow_state` is a mutable pointer to the borrow state
         // of the account, which is guaranteed to be valid.
-        //
-        // "consumes" the mutable borrow for account data; we are guaranteed
-        // that account data are not already borrowed in any form
         unsafe { *borrow_state &= 0b_1111_0111 };
 
         // return the mutable reference to data
         Ok(RefMut {
             value: unsafe { NonNull::from(from_raw_parts_mut(self.data_ptr(), self.data_len())) },
             state: unsafe { NonNull::new_unchecked(borrow_state) },
-            borrow_mask: DATA_MASK,
+            borrow_bitmask: DATA_MUTABLE_BORROW_BITMASK,
             marker: PhantomData,
         })
     }
@@ -409,13 +418,14 @@ impl AccountInfo {
     pub fn can_borrow_data(&self) -> Result<(), ProgramError> {
         let borrow_state = unsafe { (*self.raw).borrow_state };
 
-        // check if mutable data borrow is already taken (most significant bit
-        // of the data borrow state)
-        if borrow_state & 0b_0000_1000 == 0 {
+        // Check whether the mutable data borrow bit is already in
+        // use (value `0`) or not. If it is `0`, then the borrow will fail.
+        if borrow_state & DATA_MUTABLE_BORROW_BITMASK == 0 {
             return Err(ProgramError::AccountBorrowFailed);
         }
 
-        // check if we have reached the max immutable data borrow count (7)
+        // Check whether we have reached the maximum immutable data borrow count
+        // or not, i.e., it fails when all immutable data borrow bits are `0`.
         if borrow_state & 0b_0000_0111 == 0 {
             return Err(ProgramError::AccountBorrowFailed);
         }
@@ -437,7 +447,8 @@ impl AccountInfo {
     pub fn can_borrow_mut_data(&self) -> Result<(), ProgramError> {
         let borrow_state = unsafe { (*self.raw).borrow_state };
 
-        // check if any borrow (mutable or immutable) is already taken for data
+        // Check whether any (mutable or immutable) data borrow bits are
+        // in use (value `0`) or not.
         if borrow_state & 0b_0000_1111 != 0b_0000_1111 {
             return Err(ProgramError::AccountBorrowFailed);
         }
@@ -610,11 +621,17 @@ impl AccountInfo {
     }
 }
 
-/// Bytes to shift to get to the borrow state of lamports.
-const LAMPORTS_SHIFT: u8 = 4;
+/// Number of bits of the [`Account::borrow_state`] flag to shift to get to
+/// the borrow state bits for lamports.
+///   - `7 6 5 4 3 2 1 0`
+///   - `x x x x . . . .`
+const LAMPORTS_BORROW_SHIFT: u8 = 4;
 
-/// Bytes to shift to get to the borrow state of data.
-const DATA_SHIFT: u8 = 0;
+/// Number of bits of the [`Account::borrow_state`] flag to shift to get to
+/// the borrow state bits for account data.
+///   - `7 6 5 4 3 2 1 0`
+///   - `. . . . x x x x`
+const DATA_BORROW_SHIFT: u8 = 0;
 
 /// Reference to account data or lamports with checked borrow rules.
 pub struct Ref<'a, T: ?Sized> {
@@ -682,18 +699,18 @@ impl<T: ?Sized> Drop for Ref<'_, T> {
 }
 
 /// Mask representing the mutable borrow flag for lamports.
-const LAMPORTS_MASK: u8 = 0b_1000_0000;
+const LAMPORTS_MUTABLE_BORROW_BITMASK: u8 = 0b_1000_0000;
 
 /// Mask representing the mutable borrow flag for data.
-const DATA_MASK: u8 = 0b_0000_1000;
+const DATA_MUTABLE_BORROW_BITMASK: u8 = 0b_0000_1000;
 
 /// Mutable reference to account data or lamports with checked borrow rules.
 pub struct RefMut<'a, T: ?Sized> {
     value: NonNull<T>,
     state: NonNull<u8>,
-    /// Indicates the type of borrow (lamports or data) by representing the
-    /// mutable borrow mask.
-    borrow_mask: u8,
+    /// Indicates borrowed field (lamports or data) by storing the bitmask
+    /// representing the mutable borrow.
+    borrow_bitmask: u8,
     /// The `value` raw pointer is only valid while the `&'a T` lives so we claim
     /// to hold a reference to it.
     marker: PhantomData<&'a mut T>,
@@ -712,7 +729,7 @@ impl<'a, T: ?Sized> RefMut<'a, T> {
         RefMut {
             value: NonNull::from(f(&mut *orig)),
             state: orig.state,
-            borrow_mask: orig.borrow_mask,
+            borrow_bitmask: orig.borrow_bitmask,
             marker: PhantomData,
         }
     }
@@ -732,7 +749,7 @@ impl<'a, T: ?Sized> RefMut<'a, T> {
                 Ok(RefMut {
                     value,
                     state: orig.state,
-                    borrow_mask: orig.borrow_mask,
+                    borrow_bitmask: orig.borrow_bitmask,
                     marker: PhantomData,
                 })
             }
@@ -756,7 +773,7 @@ impl<T: ?Sized> core::ops::DerefMut for RefMut<'_, T> {
 impl<T: ?Sized> Drop for RefMut<'_, T> {
     fn drop(&mut self) {
         // unset the mutable borrow flag
-        unsafe { *self.state.as_mut() |= self.borrow_mask };
+        unsafe { *self.state.as_mut() |= self.borrow_bitmask };
     }
 }
 
@@ -771,31 +788,31 @@ mod tests {
     #[test]
     fn test_data_ref() {
         let data: [u8; 4] = [0, 1, 2, 3];
-        let mut state = NOT_BORROWED - (1 << DATA_SHIFT);
+        let mut state = NOT_BORROWED - (1 << DATA_BORROW_SHIFT);
 
         let ref_data = Ref {
             value: NonNull::from(&data),
-            borrow_shift: DATA_SHIFT,
+            borrow_shift: DATA_BORROW_SHIFT,
             // borrow state must be a mutable reference
             state: NonNull::from(&mut state),
             marker: PhantomData,
         };
 
         let new_ref = Ref::map(ref_data, |data| &data[1]);
 
-        assert_eq!(state, NOT_BORROWED - (1 << DATA_SHIFT));
+        assert_eq!(state, NOT_BORROWED - (1 << DATA_BORROW_SHIFT));
         assert_eq!(*new_ref, 1);
 
         let Ok(new_ref) = Ref::filter_map(new_ref, |_| Some(&3)) else {
             unreachable!()
         };
 
-        assert_eq!(state, NOT_BORROWED - (1 << DATA_SHIFT));
+        assert_eq!(state, NOT_BORROWED - (1 << DATA_BORROW_SHIFT));
         assert_eq!(*new_ref, 3);
 
         let new_ref = Ref::filter_map(new_ref, |_| Option::<&u8>::None);
 
-        assert_eq!(state, NOT_BORROWED - (1 << DATA_SHIFT));
+        assert_eq!(state, NOT_BORROWED - (1 << DATA_BORROW_SHIFT));
         assert!(new_ref.is_err());
 
         drop(new_ref);
@@ -806,31 +823,31 @@ mod tests {
     #[test]
     fn test_lamports_ref() {
         let lamports: u64 = 10000;
-        let mut state = NOT_BORROWED - (1 << LAMPORTS_SHIFT);
+        let mut state = NOT_BORROWED - (1 << LAMPORTS_BORROW_SHIFT);
 
         let ref_lamports = Ref {
             value: NonNull::from(&lamports),
-            borrow_shift: LAMPORTS_SHIFT,
+            borrow_shift: LAMPORTS_BORROW_SHIFT,
             // borrow state must be a mutable reference
             state: NonNull::from(&mut state),
             marker: PhantomData,
         };
 
         let new_ref = Ref::map(ref_lamports, |_| &1000);
 
-        assert_eq!(state, NOT_BORROWED - (1 << LAMPORTS_SHIFT));
+        assert_eq!(state, NOT_BORROWED - (1 << LAMPORTS_BORROW_SHIFT));
         assert_eq!(*new_ref, 1000);
 
         let Ok(new_ref) = Ref::filter_map(new_ref, |_| Some(&2000)) else {
             unreachable!()
         };
 
-        assert_eq!(state, NOT_BORROWED - (1 << LAMPORTS_SHIFT));
+        assert_eq!(state, NOT_BORROWED - (1 << LAMPORTS_BORROW_SHIFT));
         assert_eq!(*new_ref, 2000);
 
         let new_ref = Ref::filter_map(new_ref, |_| Option::<&i32>::None);
 
-        assert_eq!(state, NOT_BORROWED - (1 << LAMPORTS_SHIFT));
+        assert_eq!(state, NOT_BORROWED - (1 << LAMPORTS_BORROW_SHIFT));
         assert!(new_ref.is_err());
 
         drop(new_ref);
@@ -845,7 +862,7 @@ mod tests {
 
         let ref_data = RefMut {
             value: NonNull::from(&mut data),
-            borrow_mask: DATA_MASK,
+            borrow_bitmask: DATA_MUTABLE_BORROW_BITMASK,
             // borrow state must be a mutable reference
             state: NonNull::from(&mut state),
             marker: PhantomData,
@@ -873,7 +890,7 @@ mod tests {
 
         let ref_lamports = RefMut {
             value: NonNull::from(&mut lamports),
-            borrow_mask: LAMPORTS_MASK,
+            borrow_bitmask: LAMPORTS_MUTABLE_BORROW_BITMASK,
             // borrow state must be a mutable reference
             state: NonNull::from(&mut state),
             marker: PhantomData,

sdk/pinocchio/src/instruction.rs

@@ -73,19 +73,35 @@ pub struct Account<'a> {
     _account_info: PhantomData<&'a AccountInfo>,
 }
 
+/// Return a pointer to a type `U` given type `T` has a field of type `U` at the specified
+/// offset (in bytes) from the start of the `T` type.
+///
+/// # Safety
+///
+/// The caller must ensure that the `ptr` is a valid pointer to a type `T`, and `ptr + offset`
+/// points to bytes that are properly aligned for `U` and represent a bit pattern that is a
+/// valid instance of `U`.
+///
+/// If any of this requirements is not valid, this function leads to undefined behavior.
 #[inline(always)]
-const fn offset<T, U>(ptr: *const T, offset: usize) -> *const U {
+const unsafe fn field_at_offset<T, U>(ptr: *const T, offset: usize) -> *const U {
+    // SAFETY: The caller ensures that the offset is valid for the type `T` and that
+    // the resulting pointer is valid for type `U`.
     unsafe { (ptr as *const u8).add(offset) as *const U }
 }
 
 impl<'a> From<&'a AccountInfo> for Account<'a> {
     fn from(account: &'a AccountInfo) -> Self {
         Account {
-            key: offset(account.raw, 8),
-            lamports: offset(account.raw, 72),
+            // SAFETY: offset `8` is the `key` field in the `Account` struct.
+            key: unsafe { field_at_offset(account.raw, 8) },
+            // SAFETY: offset `72` is the `lamports` field in the `Account` struct.
+            lamports: unsafe { field_at_offset(account.raw, 72) },
             data_len: account.data_len() as u64,
-            data: offset(account.raw, 88),
-            owner: offset(account.raw, 40),
+            // SAFETY: offset `88` is the start of the account data in the `Account` struct.
+            data: unsafe { field_at_offset(account.raw, 88) },
+            // SAFETY: offset `40` is the `owner` field in the `Account` struct.
+            owner: unsafe { field_at_offset(account.raw, 40) },
             // The `rent_epoch` field is not present in the `AccountInfo` struct,
             // since the value occurs after the variable data of the account in
             // the runtime input data.