Merge pull request #12 from mikemaccana-edwardbot/fix-code-inconsistencies-2026-04-22

Fix code inconsistencies across various programs

Author: mikemaccana

basics/counter/native/program/src/lib.rs

@@ -20,24 +20,34 @@ use solana_program::entrypoint;
 entrypoint!(process_instruction);
 
 pub fn process_instruction(
-    _program_id: &Pubkey,
+    program_id: &Pubkey,
     accounts: &[AccountInfo],
     instruction_data: &[u8],
 ) -> ProgramResult {
+    // Reject empty instruction data explicitly rather than panicking via split_at.
+    if instruction_data.is_empty() {
+        msg!("Error: instruction data is empty");
+        return Err(ProgramError::InvalidInstructionData);
+    }
     let (instruction_discriminant, instruction_data_inner) = instruction_data.split_at(1);
     match instruction_discriminant[0] {
         0 => {
             msg!("Instruction: Increment");
-            process_increment_counter(accounts, instruction_data_inner)?;
+            process_increment_counter(program_id, accounts, instruction_data_inner)?;
         }
         _ => {
-            msg!("Error: unknown instruction")
+            // Previously this branch logged a message and returned Ok(()), which let
+            // unknown discriminants succeed silently. Return InvalidInstructionData
+            // so callers get a real failure.
+            msg!("Error: unknown instruction");
+            return Err(ProgramError::InvalidInstructionData);
         }
     }
     Ok(())
 }
 
 pub fn process_increment_counter(
+    program_id: &Pubkey,
     accounts: &[AccountInfo],
     _instruction_data: &[u8],
 ) -> Result<(), ProgramError> {
@@ -49,6 +59,13 @@ pub fn process_increment_counter(
         "Counter account must be writable"
     );
 
+    // Owner check: without this the program would happily decode any 8 bytes of
+    // data as a Counter, even when the account belongs to a different program.
+    if counter_account.owner != program_id {
+        msg!("Error: counter account is not owned by this program");
+        return Err(ProgramError::IncorrectProgramId);
+    }
+
     let mut counter = Counter::try_from_slice(&counter_account.try_borrow_mut_data()?)?;
     counter.count += 1;
     counter.serialize(&mut *counter_account.data.borrow_mut())?;

basics/counter/native/program/tests/test.rs

@@ -9,13 +9,47 @@ use solana_rent::Rent;
 use solana_system_interface::instruction::create_account;
 use solana_transaction::Transaction;
 
+// The .so is built into ../../tests/fixtures by `pnpm build-and-test` (which runs
+// `cargo build-sbf --sbf-out-dir=./tests/fixtures` from the package root). Run
+// that script (or `cargo build-sbf` with --sbf-out-dir set accordingly) before
+// `cargo test`.
+const PROGRAM_SO: &[u8] = include_bytes!("../../tests/fixtures/counter_solana_native.so");
+
+fn setup_with_counter() -> (LiteSVM, Pubkey, Keypair, Keypair) {
+    let program_id = Pubkey::new_unique();
+
+    let mut svm = LiteSVM::new();
+    svm.add_program(program_id, PROGRAM_SO).unwrap();
+
+    let payer = Keypair::new();
+    let counter_account = Keypair::new();
+
+    svm.airdrop(&payer.pubkey(), LAMPORTS_PER_SOL * 10).unwrap();
+
+    let counter_account_size = std::mem::size_of::<Counter>();
+    let create_ix = create_account(
+        &payer.pubkey(),
+        &counter_account.pubkey(),
+        Rent::default().minimum_balance(counter_account_size),
+        counter_account_size as u64,
+        &program_id,
+    );
+    let tx = Transaction::new_signed_with_payer(
+        &[create_ix],
+        Some(&payer.pubkey()),
+        &[&payer, &counter_account],
+        svm.latest_blockhash(),
+    );
+    svm.send_transaction(tx).unwrap();
+    (svm, program_id, payer, counter_account)
+}
+
 #[test]
 fn test_counter() {
     let program_id = Pubkey::new_unique();
-    let program_bytes = include_bytes!("../../tests/fixtures/counter_solana_native.so");
 
     let mut svm = LiteSVM::new();
-    svm.add_program(program_id, program_bytes).unwrap();
+    svm.add_program(program_id, PROGRAM_SO).unwrap();
 
     let payer = Keypair::new();
     let counter_account = Keypair::new();
@@ -38,7 +72,7 @@ fn test_counter() {
         &[&payer, &counter_account],
         svm.latest_blockhash(),
     );
-    assert!(svm.send_transaction(tx).is_ok());
+    svm.send_transaction(tx).unwrap();
 
     let ix = Instruction {
         program_id,
@@ -53,9 +87,79 @@ fn test_counter() {
         svm.latest_blockhash(),
     );
 
-    let _ = svm.send_transaction(tx).is_ok();
+    // Actually assert the transaction succeeded - the previous shape used
+    // `let _ = ...is_ok();` which discarded the result.
+    svm.send_transaction(tx).unwrap();
 
     let counter_account_data = svm.get_account(&counter_account.pubkey()).unwrap().data;
     let counter = Counter::try_from_slice(&counter_account_data).unwrap();
     assert_eq!(counter.count, 1);
 }
+
+#[test]
+fn test_unknown_instruction_fails() {
+    let (mut svm, program_id, payer, counter_account) = setup_with_counter();
+
+    // Discriminant 9 is not handled and must now return an error rather than Ok(()).
+    let ix = Instruction {
+        program_id,
+        accounts: vec![AccountMeta::new(counter_account.pubkey(), false)],
+        data: vec![9],
+    };
+    let tx = Transaction::new_signed_with_payer(
+        &[ix],
+        Some(&payer.pubkey()),
+        &[&payer],
+        svm.latest_blockhash(),
+    );
+    assert!(
+        svm.send_transaction(tx).is_err(),
+        "unknown instruction discriminant must fail"
+    );
+}
+
+#[test]
+fn test_wrong_owner_fails() {
+    let program_id = Pubkey::new_unique();
+
+    let mut svm = LiteSVM::new();
+    svm.add_program(program_id, PROGRAM_SO).unwrap();
+
+    let payer = Keypair::new();
+    let counter_account = Keypair::new();
+    svm.airdrop(&payer.pubkey(), LAMPORTS_PER_SOL * 10).unwrap();
+
+    // Create the counter account but owned by a different (random) program.
+    let counter_account_size = std::mem::size_of::<Counter>();
+    let wrong_owner = Pubkey::new_unique();
+    let create_ix = create_account(
+        &payer.pubkey(),
+        &counter_account.pubkey(),
+        Rent::default().minimum_balance(counter_account_size),
+        counter_account_size as u64,
+        &wrong_owner,
+    );
+    let tx = Transaction::new_signed_with_payer(
+        &[create_ix],
+        Some(&payer.pubkey()),
+        &[&payer, &counter_account],
+        svm.latest_blockhash(),
+    );
+    svm.send_transaction(tx).unwrap();
+
+    let ix = Instruction {
+        program_id,
+        accounts: vec![AccountMeta::new(counter_account.pubkey(), false)],
+        data: vec![0],
+    };
+    let tx = Transaction::new_signed_with_payer(
+        &[ix],
+        Some(&payer.pubkey()),
+        &[&payer],
+        svm.latest_blockhash(),
+    );
+    assert!(
+        svm.send_transaction(tx).is_err(),
+        "counter owned by a different program must be rejected"
+    );
+}

basics/counter/pinocchio/program/src/lib.rs

@@ -21,14 +21,22 @@ pub fn process_instruction(
     accounts: &[AccountView],
     instruction_data: &[u8],
 ) -> ProgramResult {
+    if instruction_data.is_empty() {
+        log!("Error: instruction data is empty");
+        return Err(ProgramError::InvalidInstructionData);
+    }
     let (instruction_discriminant, instruction_data_inner) = instruction_data.split_at(1);
     match instruction_discriminant[0] {
         0 => {
             log!("Instruction: Increment");
             process_increment_counter(accounts, instruction_data_inner)?;
         }
         _ => {
+            // Previously this branch logged a message and returned Ok(()), which let
+            // unknown discriminants succeed silently. Return InvalidInstructionData
+            // so callers get a real failure.
             log!("Error: unknown instruction");
+            return Err(ProgramError::InvalidInstructionData);
         }
     }
     Ok(())

basics/counter/pinocchio/program/tests/test.rs

@@ -8,13 +8,18 @@ use solana_rent::Rent;
 use solana_system_interface::instruction::create_account;
 use solana_transaction::Transaction;
 
+// The .so is built into ../../tests/fixtures by `pnpm build-and-test` (which runs
+// `cargo build-sbf --sbf-out-dir=./tests/fixtures` from the package root). Run
+// that script (or `cargo build-sbf` with --sbf-out-dir set accordingly) before
+// `cargo test`.
+const PROGRAM_SO: &[u8] = include_bytes!("../../tests/fixtures/counter_solana_pinocchio.so");
+
 #[test]
 fn test_counter() {
     let program_id = Pubkey::new_unique();
-    let program_bytes = include_bytes!("../../tests/fixtures/counter_solana_pinocchio.so");
 
     let mut svm = LiteSVM::new();
-    svm.add_program(program_id, program_bytes).unwrap();
+    svm.add_program(program_id, PROGRAM_SO).unwrap();
 
     let payer = Keypair::new();
     let counter_account = Keypair::new();
@@ -38,8 +43,7 @@ fn test_counter() {
         svm.latest_blockhash(),
     );
 
-    let res = svm.send_transaction(tx);
-    assert!(res.is_ok());
+    svm.send_transaction(tx).unwrap();
 
     let ix = Instruction {
         program_id,
@@ -54,11 +58,55 @@ fn test_counter() {
         svm.latest_blockhash(),
     );
 
-    let res = svm.send_transaction(tx);
-    assert!(res.is_ok());
+    svm.send_transaction(tx).unwrap();
 
     let counter_account_data = svm.get_account(&counter_account.pubkey()).unwrap().data;
     let counter_bytes: [u8; 8] = counter_account_data[0..8].try_into().unwrap();
     let count = u64::from_le_bytes(counter_bytes);
     assert_eq!(count, 1);
 }
+
+#[test]
+fn test_unknown_instruction_fails() {
+    let program_id = Pubkey::new_unique();
+
+    let mut svm = LiteSVM::new();
+    svm.add_program(program_id, PROGRAM_SO).unwrap();
+
+    let payer = Keypair::new();
+    let counter_account = Keypair::new();
+    svm.airdrop(&payer.pubkey(), LAMPORTS_PER_SOL * 10).unwrap();
+
+    let counter_account_size = std::mem::size_of::<Counter>();
+    let create_ix = create_account(
+        &payer.pubkey(),
+        &counter_account.pubkey(),
+        Rent::default().minimum_balance(counter_account_size),
+        counter_account_size as u64,
+        &program_id,
+    );
+    let tx = Transaction::new_signed_with_payer(
+        &[create_ix],
+        Some(&payer.pubkey()),
+        &[&payer, &counter_account],
+        svm.latest_blockhash(),
+    );
+    svm.send_transaction(tx).unwrap();
+
+    // Discriminant 9 is not handled and must now return an error rather than Ok(()).
+    let ix = Instruction {
+        program_id,
+        accounts: vec![AccountMeta::new(counter_account.pubkey(), false)],
+        data: vec![9],
+    };
+    let tx = Transaction::new_signed_with_payer(
+        &[ix],
+        Some(&payer.pubkey()),
+        &[&payer],
+        svm.latest_blockhash(),
+    );
+    assert!(
+        svm.send_transaction(tx).is_err(),
+        "unknown instruction discriminant must fail"
+    );
+}

basics/repository-layout/anchor/programs/carnival/src/error.rs

@@ -1 +1,17 @@
-// For any custom errors
+use anchor_lang::prelude::*;
+
+// Real failure modes for the carnival examples. Previously this file was empty
+// and the rejection branches in the instruction handlers just logged a message
+// and returned Ok(()), which made it impossible for callers (or tests) to
+// distinguish "the ride happened" from "the ride refused service".
+#[error_code]
+pub enum CarnivalError {
+    // The rider/gamer/eater did not bring enough tickets for the requested
+    // attraction.
+    #[msg("Not enough tickets for the requested attraction")]
+    NotEnoughTickets,
+
+    // The rider is below the minimum height required for the ride.
+    #[msg("Rider is below the minimum height for this ride")]
+    RiderTooShort,
+}

basics/repository-layout/anchor/programs/carnival/src/instructions/eat_food.rs

@@ -1,6 +1,6 @@
 use anchor_lang::prelude::*;
 
-use crate::state::food;
+use crate::{error::CarnivalError, state::food};
 
 // Instruction Data
 
@@ -24,10 +24,12 @@ pub fn eat_food(ix: EatFoodInstructionData) -> Result<()> {
                     food_stand.food_type,
                     food_stand.tickets
                 );
-            } else {
-                msg!("  Enjoy your {}!", food_stand.food_type);
-            };
+                // Refuse service rather than logging and returning Ok(()), so
+                // callers can distinguish a sale from a refusal.
+                return Err(CarnivalError::NotEnoughTickets.into());
+            }
 
+            msg!("  Enjoy your {}!", food_stand.food_type);
             return Ok(());
         }
     }

basics/repository-layout/anchor/programs/carnival/src/instructions/get_on_ride.rs

@@ -1,6 +1,6 @@
 use anchor_lang::prelude::*;
 
-use crate::state::ride;
+use crate::{error::CarnivalError, state::ride};
 
 // Instruction Data
 
@@ -18,14 +18,17 @@ pub fn get_on_ride(ix: GetOnRideInstructionData) -> Result<()> {
         if ix.ride.eq(&ride.name) {
             msg!("You're about to ride the {}!", ride.name);
 
+            // Refuse service: failures used to log + return Ok(()), which made
+            // them indistinguishable from a successful ride for callers and
+            // tests. Return a real error instead.
             if ix.rider_ticket_count < ride.tickets {
                 msg!(
                     "  Sorry {}, you need {} tickets to ride the {}!",
                     ix.rider_name,
                     ride.tickets,
                     ride.name
                 );
-                return Ok(());
+                return Err(CarnivalError::NotEnoughTickets.into());
             };
 
             if ix.rider_height < ride.min_height {
@@ -35,7 +38,7 @@ pub fn get_on_ride(ix: GetOnRideInstructionData) -> Result<()> {
                     ride.min_height,
                     ride.name
                 );
-                return Ok(());
+                return Err(CarnivalError::RiderTooShort.into());
             };
 
             msg!("  Welcome aboard the {}!", ride.name);