counter(native): avoid panic, validate owner, checked_add, fail unknown instructions#531
counter(native): avoid panic, validate owner, checked_add, fail unknown instructions#531yukikm wants to merge 1 commit intosolana-developers:mainfrom
Conversation
…ct unknown instructions
There was a problem hiding this comment.
Pull request overview
This PR hardens the Solana-native counter example program to fail safely on malformed instructions and unsafe account inputs, and adds regression tests intended to prevent reintroducing these footguns.
Changes:
- Avoids panics on-chain by replacing
assert!with explicit error returns for readonly accounts. - Validates instruction discriminant (including empty data) and rejects unknown instructions.
- Adds ownership validation and overflow-safe increment (
checked_add) in the counter update path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| basics/counter/native/program/src/lib.rs | Adds instruction-data validation, unknown-instruction failures, owner + writable checks, and overflow-safe increment. |
| basics/counter/native/program/tests/test.rs | Strengthens the happy-path test and adds new failure-mode tests (unknown instruction + readonly counter). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Mark counter account as readonly; program should return an error (not panic). | ||
| let ix = Instruction { | ||
| program_id, | ||
| accounts: vec![AccountMeta::new_readonly(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()); |
There was a problem hiding this comment.
test_readonly_counter_fails currently only asserts that the transaction errors. If the program panics (the pre-change assert! behavior), LiteSVM may still surface that as an Err, so this test may not actually guard against panics as intended. Consider asserting on the specific failure reason (e.g., expected ProgramError/InstructionError) or checking the returned logs/error kind to ensure it's a graceful error rather than a panic/trap.
| let (instruction_discriminant, instruction_data_inner) = instruction_data | ||
| .split_first() | ||
| .ok_or(ProgramError::InvalidInstructionData)?; | ||
|
|
There was a problem hiding this comment.
The new split_first().ok_or(InvalidInstructionData)? behavior rejects empty instruction_data, but there is no regression test covering the empty-instruction case. Adding a test that sends an instruction with data: vec![] and asserts the expected error would help prevent this from regressing.
| counter.count = counter | ||
| .count | ||
| .checked_add(1) | ||
| .ok_or(ProgramError::ArithmeticOverflow)?; |
There was a problem hiding this comment.
The checked_add(1) overflow handling is new behavior, but there’s no test that demonstrates the overflow path returns ArithmeticOverflow. Consider adding a regression test that initializes counter.count to u64::MAX and asserts the transaction fails with the expected error.
1 participant
Fixes several footguns in basics/counter/native/program that are easy to copy into real programs: remove assert! panic on readonly account, add owner check, use checked_add, and fail unknown/empty instruction discriminants. Includes regression tests: test_unknown_instruction_fails, test_readonly_counter_fails.