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 instruction to close violation report #19

Merged
merged 6 commits into from
Feb 25, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
9 changes: 9 additions & 0 deletions program/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ use {
/// Errors that may be returned by the program.
#[derive(Clone, Copy, Debug, Eq, Error, FromPrimitive, PartialEq)]
pub enum SlashingError {
/// Attempting to close a violation report before 3
/// epochs have passed
#[error("Closing violation report too soon")]
CloseViolationReportTooSoon,

/// Violation has already been reported
#[error("Duplicate report")]
DuplicateReport,
Expand All @@ -17,6 +22,10 @@ pub enum SlashingError {
#[error("Exceeds statute of limitations")]
ExceedsStatuteOfLimitations,

/// Destination account does not match the key on the report
#[error("Invalid destination account")]
InvalidDestinationAccount,

/// Invalid shred variant
#[error("Invalid shred variant")]
InvalidShredVariant,
Expand Down
49 changes: 47 additions & 2 deletions program/src/instruction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,30 @@ use {
#[repr(u8)]
#[derive(Clone, Copy, Debug, PartialEq, TryFromPrimitive, IntoPrimitive)]
pub enum SlashingInstruction {
/// Close the report account that was created after successfully submitting
/// a slashing proof. To ensure that the runtime and indexers have seen the
/// report, we require that at least 3 epochs have passed since creation.
///
/// After closing the account, we credit the lamports to the destination
/// address denoted in the report.
///
/// Accounts expected by this instruction:
/// 0. `[WRITE]` PDA where the violation report is stored, see
/// `[get_violation_report_address]` for the address derivation.
/// 1. `[WRITE]` Destination account which will be credited the lamports
/// from the PDA.
/// 2. `[]` System program
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed it here 754ad31

CloseViolationReport,

/// Submit a slashable violation proof for `node_pubkey`, which indicates
/// that they submitted a duplicate block to the network
///
///
/// Accounts expected by this instruction:
/// 0. `[]` Proof account, must be previously initialized with the proof
/// data.
/// 1. `[WRITE]` PDA to store the violation report
/// 1. `[WRITE]` PDA to store the violation report, see
/// `[get_violation_report_address]` for the address derivation.
/// 2. `[]` Instructions sysvar
/// 3. `[]` System program
///
Expand Down Expand Up @@ -141,6 +157,25 @@ pub(crate) fn decode_instruction_data<T: Pod>(input_with_type: &[u8]) -> Result<
}
}

/// Create a `SlashingInstruction::CloseViolationReport` instruction
/// Callers can use `[get_violation_report_address]` to derive the report
/// account address
pub fn close_violation_report(
report_account: &Pubkey,
destination_account: &Pubkey,
) -> Instruction {
let accounts = vec![
AccountMeta::new(*report_account, false),
AccountMeta::new(*destination_account, false),
AccountMeta::new_readonly(system_program::id(), false),
];
encode_instruction(
accounts,
SlashingInstruction::CloseViolationReport,
&[0u8; 0],
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is it possible to use some form of () instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, used &() instead 68e2d6a

)
}

/// Create a `SlashingInstruction::DuplicateBlockProof` instruction
pub fn duplicate_block_proof(
proof_account: &Pubkey,
Expand Down Expand Up @@ -286,7 +321,7 @@ mod tests {
shred_2_signature,
};
let instruction = duplicate_block_proof(&Pubkey::new_unique(), &instruction_data);
let mut expected = vec![0];
let mut expected = vec![1];
expected.extend_from_slice(&offset.to_le_bytes());
expected.extend_from_slice(&slot.to_le_bytes());
expected.extend_from_slice(&node_pubkey.to_bytes());
Expand Down Expand Up @@ -316,6 +351,16 @@ mod tests {
assert_eq!(instruction_data.shred_2_signature, shred_2_signature);
}

#[test]
fn serialize_close_violation_report() {
let instruction = close_violation_report(&Pubkey::new_unique(), &Pubkey::new_unique());

assert_eq!(
SlashingInstruction::CloseViolationReport,
decode_instruction_type(&instruction.data).unwrap()
);
}

#[test]
fn deserialize_invalid_instruction() {
let mut expected = vec![12];
Expand Down
50 changes: 49 additions & 1 deletion program/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ pub mod state;
pub use solana_program;
use {
solana_program::{clock::Slot, pubkey::Pubkey},
state::ProofType,
state::{ProofType, ViolationReport},
};

solana_program::declare_id!("S1ashing11111111111111111111111111111111111");
Expand All @@ -37,3 +37,51 @@ pub fn get_violation_report_address(
&id(),
)
}

struct ViolationReportAddress<'a> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if me trying to reuse some code made this better or worse, happy to delete this if you think worse.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good actually! My only comment would be to move it to another file like state.rs or create address.rs since it's only used internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it to a separate file here f9ab512
After removing the system program from CloseViolationReport, we actually don't need it there, so there's only one consumer. I've kept it here in case we need it in the future.

address: Pubkey,
pubkey_seed: &'a [u8],
slot_seed: &'a [u8; 8],
violation_seed: [u8; 1],
bump_seed: [u8; 1],
}

impl<'a> ViolationReportAddress<'a> {
pub(crate) fn new(report: &'a ViolationReport) -> ViolationReportAddress<'a> {
let pubkey_seed = report.pubkey.as_ref();
let slot_seed = &report.slot.0;
let violation_seed = [report.violation_type];
let (pda, bump) =
Pubkey::find_program_address(&[pubkey_seed, slot_seed, &violation_seed], &id());
Copy link
Contributor

Choose a reason for hiding this comment

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

The one annoying feature of this type is that it always derives the pda, which is a costly syscall, and it isn't used during close.

It might better to do the derivation in a separate function -- ie rename key() to find_program_address() and do the derivation there. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out we don't need to use it in close. Also we do need to find the bump since it's part of the seeds so I think we'll need to have it for anyone that uses the seeds fn as well.

let bump_seed = [bump];
Self {
address: pda,
pubkey_seed,
slot_seed,
violation_seed,
bump_seed,
}
}

pub(crate) fn key(&self) -> &Pubkey {
&self.address
}

pub(crate) fn seeds(&self) -> [&[u8]; 4] {
[
self.pubkey_seed,
self.slot_seed,
&self.violation_seed,
&self.bump_seed,
]
}

pub(crate) fn seeds_owned(&self) -> [Vec<u8>; 4] {
[
self.pubkey_seed.to_owned(),
Vec::from(self.slot_seed),
Vec::from(self.violation_seed),
Vec::from(self.bump_seed),
]
}
}
30 changes: 25 additions & 5 deletions program/src/processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,25 @@

use {
crate::{
check_id,
duplicate_block_proof::DuplicateBlockProofData,
error::SlashingError,
instruction::{
decode_instruction_data, decode_instruction_type, DuplicateBlockProofInstructionData,
SlashingInstruction,
},
state::{
store_violation_report, PodEpoch, ProofType, SlashingAccounts, SlashingProofData,
ViolationReport,
close_violation_report, store_violation_report, PodEpoch, ProofType, SlashingAccounts,
SlashingProofData, ViolationReport,
},
},
solana_program::{
account_info::AccountInfo,
account_info::{next_account_info, AccountInfo},
entrypoint::ProgramResult,
msg,
program_error::ProgramError,
pubkey::Pubkey,
system_program,
sysvar::{clock::Clock, epoch_schedule::EpochSchedule, Sysvar},
},
};
Expand Down Expand Up @@ -65,9 +67,27 @@ pub fn process_instruction(
) -> ProgramResult {
let instruction_type = decode_instruction_type(input)?;
let account_info_iter = &mut accounts.iter();
let accounts = SlashingAccounts::new(account_info_iter)?;
match instruction_type {
SlashingInstruction::CloseViolationReport => {
let report_account = next_account_info(account_info_iter)?;
let destination_account = next_account_info(account_info_iter)?;
let system_program_account = next_account_info(account_info_iter)?;

if !check_id(report_account.owner) || report_account.data_is_empty() {
return Err(ProgramError::from(
SlashingError::InvalidViolationReportAcccount,
));
}
if !system_program::check_id(system_program_account.key) {
return Err(ProgramError::from(
SlashingError::MissingSystemProgramAccount,
));
}

close_violation_report(report_account, destination_account, system_program_account)?;
}
SlashingInstruction::DuplicateBlockProof => {
let accounts = SlashingAccounts::new(account_info_iter)?;
let data = decode_instruction_data::<DuplicateBlockProofInstructionData>(input)?;
let proof_data = &accounts.proof_account.try_borrow_data()?[data.offset()?..];
let violation_report = ViolationReport {
Expand All @@ -86,9 +106,9 @@ pub fn process_instruction(
proof_data,
input,
)?;
Ok(())
}
}
Ok(())
}

#[cfg(test)]
Expand Down
96 changes: 73 additions & 23 deletions program/src/state.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
//! Program state
use {
crate::{check_id, duplicate_block_proof::DuplicateBlockProofData, error::SlashingError, id},
crate::{
check_id, duplicate_block_proof::DuplicateBlockProofData, error::SlashingError, id,
ViolationReportAddress,
},
bytemuck::{Pod, Zeroable},
solana_program::{
account_info::{next_account_info, AccountInfo},
clock::Slot,
clock::{Epoch, Slot},
msg,
program::invoke_signed,
program_error::ProgramError,
Expand All @@ -13,7 +16,7 @@ use {
system_instruction, system_program,
sysvar::{self, Sysvar},
},
spl_pod::primitives::PodU64,
spl_pod::{bytemuck::pod_from_bytes, primitives::PodU64},
};

const PACKET_DATA_SIZE: usize = 1232;
Expand Down Expand Up @@ -222,16 +225,15 @@ pub(crate) fn store_violation_report<'a, 'b, T>(
where
T: SlashingProofData<'a>,
{
let slot = report.slot;
let pubkey_seed = report.pubkey.as_ref();
let slot_seed = slot.0;
let violation_seed = [report.violation_type];
let mut seeds: Vec<&[u8]> = vec![&pubkey_seed, &slot_seed, &violation_seed];
let (pda, bump) = Pubkey::find_program_address(&seeds, &id());
let bump_seed = [bump];
seeds.push(&bump_seed);

if pda != *accounts.violation_account() {
let report_address = ViolationReportAddress::new(&report);
let report_key = report_address.key();
let seeds = report_address.seeds();
let cpi_accounts = [
accounts.violation_pda_account.clone(),
accounts.system_program_account.clone(),
];

if *report_key != *accounts.violation_account() {
return Err(ProgramError::from(
SlashingError::InvalidViolationReportAcccount,
));
Expand All @@ -242,7 +244,7 @@ where
msg!(
"{} violation verified in slot {} however the violation has already been reported",
T::PROOF_TYPE.violation_str(),
u64::from(slot),
u64::from(report.slot),
);
return Err(ProgramError::from(SlashingError::DuplicateReport));
}
Expand All @@ -255,15 +257,8 @@ where
}

// Assign the slashing program as the owner
let assign_instruction = system_instruction::assign(&pda, &id());
invoke_signed(
&assign_instruction,
&[
accounts.violation_pda_account.clone(),
accounts.system_program_account.clone(),
],
&[&seeds],
)?;
let assign_instruction = system_instruction::assign(report_key, &id());
invoke_signed(&assign_instruction, &cpi_accounts, &[&seeds])?;

// Allocate enough space for the report
accounts.violation_pda_account.realloc(data_len, false)?;
Expand All @@ -282,6 +277,61 @@ where
accounts.write_violation_report(report, proof_data)
}

pub(crate) fn close_violation_report<'a, 'b>(
report_account: &'a AccountInfo<'b>,
destination_account: &'a AccountInfo<'b>,
system_program_account: &'a AccountInfo<'b>,
) -> Result<(), ProgramError> {
let report_data = report_account.try_borrow_data()?;
let report: &ViolationReport =
pod_from_bytes(&report_data[0..std::mem::size_of::<ViolationReport>()])?;
let report_address = ViolationReportAddress::new(report);
let report_key = *report_address.key();
let destination = report.destination;

debug_assert_eq!(*report_account.key, report_key);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for exactly? Does the key for the violation report matter for closing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it doesn't actually you're right, and with the change to not use transfer here we don't even need to get the address.


if Epoch::from(report.epoch).saturating_add(3) > sysvar::clock::Clock::get()?.epoch {
return Err(ProgramError::from(
SlashingError::CloseViolationReportTooSoon,
));
}

if destination != *destination_account.key {
return Err(ProgramError::from(SlashingError::InvalidDestinationAccount));
}

// Copy seeds and drop as we will modify the report account
let seeds = report_address.seeds_owned();
let seeds: Vec<&[u8]> = seeds.iter().map(|seed| &seed[..]).collect();
drop(report_data);

// Reallocate the account to 0 bytes
report_account.realloc(0, false)?;

// Assign the system program as the owner
report_account.assign(&system_program::id());

// Transfer the lamports to the destination address
let lamports = report_account.lamports();
let transfer_instruction = system_instruction::transfer(&report_key, &destination, lamports);
invoke_signed(
&transfer_instruction,
&[
report_account.clone(),
destination_account.clone(),
system_program_account.clone(),
],
&[&seeds],
)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

It'll be much easier to just subtract the lamports directly from report_account and then realloc / reassign, and that way there's no need to CPI to the system program.

Check out how the token program does this: https://github.com/solana-program/token/blob/08aa3ccecb30692bca18d6f927804337de82d5ff/program/src/processor.rs#L702-L708

We'll just need some special logic to do nothing if the destination is the report account itself, so that accounts are always closable. Alternatively, we can make sure the destination isn't the report account during creation

Copy link
Contributor Author

@AshwinSekar AshwinSekar Feb 15, 2025

Choose a reason for hiding this comment

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

That's much easier! Removed it here 754ad31

And good point, i decided to return an error if your destination is equal to the report address on report creation and added a test 3dfef7d


msg!(
"Closed violation report and credited {} lamports to the destination address",
lamports
);
Ok(())
}

#[cfg(test)]
mod tests {
use crate::state::PACKET_DATA_SIZE;
Expand Down
Loading