From 61125855c45223972eac09608e69badfa8c9d03f Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:40:45 +0200 Subject: [PATCH] Add instruction to remove a solver --- bench-report.json | 28 +- client/src/instructions.rs | 23 ++ client/src/parse.rs | 16 +- interface/src/data/state.rs | 104 ++++++- interface/src/instruction/mod.rs | 1 + interface/src/instruction/remove_solver.rs | 243 +++++++++++++++++ interface/src/lib.rs | 14 +- programs/settlement/src/add_solver.rs | 4 +- programs/settlement/src/lib.rs | 10 +- programs/settlement/src/remove_solver.rs | 165 ++++++++++++ programs/settlement/tests/common/benchmark.rs | 2 + programs/settlement/tests/remove_solvers.rs | 255 ++++++++++++++++++ 12 files changed, 842 insertions(+), 23 deletions(-) create mode 100644 interface/src/instruction/remove_solver.rs create mode 100644 programs/settlement/src/remove_solver.rs create mode 100644 programs/settlement/tests/remove_solvers.rs diff --git a/bench-report.json b/bench-report.json index 738f6ee..1d0e2ce 100644 --- a/bench-report.json +++ b/bench-report.json @@ -12,6 +12,8 @@ "reclaim_buffer/max_buffers_in_one_instruction": 64, "reclaim_buffer/reclaims_multiple_buffers_skipping_funded": 9, "reclaim_order/happy_path_returns_lamports_and_closes_pda": 4, + "remove_solver/remove_with_many_existing_solvers": 5, + "remove_solver/removes_a_solver": 5, "settle/finalizes_with_no_pushes": 5, "settle/pulls_from_multiple_orders": 15, "settle/pulls_funds_to_destination": 10, @@ -28,16 +30,18 @@ "compute_units": { "add_solver/add_with_many_existing_solvers": 5074, "add_solver/adds_a_solver": 4622, - "create_buffers/happy_path_creates_initialized_buffer_token_account": 10345, - "create_buffers/happy_path_creates_multiple_buffers_in_one_instruction": 21743, - "create_buffers/max_buffers_in_one_instruction": 177040, + "create_buffers/happy_path_creates_initialized_buffer_token_account": 10347, + "create_buffers/happy_path_creates_multiple_buffers_in_one_instruction": 21747, + "create_buffers/max_buffers_in_one_instruction": 177071, "create_order/happy_path_creates_order_pda_with_expected_body": 4978, - "initialize/happy_path_initializes_state_pda_with_expected_data": 4529, + "initialize/happy_path_initializes_state_pda_with_expected_data": 4530, "reclaim_buffer/funded_buffer_is_skipped": 6335, - "reclaim_buffer/happy_path_reclaims_empty_buffer_to_the_authority_itself": 7483, - "reclaim_buffer/max_buffers_in_one_instruction": 136652, - "reclaim_buffer/reclaims_multiple_buffers_skipping_funded": 18082, - "reclaim_order/happy_path_returns_lamports_and_closes_pda": 2183, + "reclaim_buffer/happy_path_reclaims_empty_buffer_to_the_authority_itself": 7482, + "reclaim_buffer/max_buffers_in_one_instruction": 136622, + "reclaim_buffer/reclaims_multiple_buffers_skipping_funded": 18081, + "reclaim_order/happy_path_returns_lamports_and_closes_pda": 2182, + "remove_solver/remove_with_many_existing_solvers": 3763, + "remove_solver/removes_a_solver": 3498, "settle/finalizes_with_no_pushes": 7154, "settle/pulls_from_multiple_orders": 20043, "settle/pulls_funds_to_destination": 13632, @@ -47,9 +51,9 @@ "settle/pushes_several_orders_from_one_buffer": 17750, "settle/settles_a_single_order": 12505, "settle/settles_multiple_orders": 23062, - "transfer_authority/manager_can_transfer_manager": 3174, - "transfer_authority/manager_can_transfer_reclaim_authority": 3176, - "transfer_authority/reclaim_authority_can_transfer_itself": 3180 + "transfer_authority/manager_can_transfer_manager": 3173, + "transfer_authority/manager_can_transfer_reclaim_authority": 3175, + "transfer_authority/reclaim_authority_can_transfer_itself": 3179 }, "transaction_bytes": { "add_solver/add_with_many_existing_solvers": 366, @@ -64,6 +68,8 @@ "reclaim_buffer/max_buffers_in_one_instruction": 332, "reclaim_buffer/reclaims_multiple_buffers_skipping_funded": 466, "reclaim_order/happy_path_returns_lamports_and_closes_pda": 236, + "remove_solver/remove_with_many_existing_solvers": 365, + "remove_solver/removes_a_solver": 365, "settle/finalizes_with_no_pushes": 290, "settle/pulls_from_multiple_orders": 656, "settle/pulls_funds_to_destination": 473, diff --git a/client/src/instructions.rs b/client/src/instructions.rs index e1c7976..207016e 100644 --- a/client/src/instructions.rs +++ b/client/src/instructions.rs @@ -271,6 +271,29 @@ impl From for Instruction { } } +/// Removes `solver` from the state PDA's solver list. Authorized by `manager`; +/// the freed rent is paid to `rent_recipient`. +pub struct RemoveSolver { + pub program_id: Pubkey, + pub manager: Pubkey, + pub rent_recipient: Pubkey, + pub solver: Pubkey, +} + +impl From for Instruction { + fn from(builder: RemoveSolver) -> Self { + let (state_pda, _bump) = find_state_pda(&builder.program_id); + cow_settlement_interface::instruction::remove_solver::RemoveSolver { + program_id: builder.program_id, + manager: builder.manager, + rent_recipient: builder.rent_recipient, + state_pda, + solver: builder.solver, + } + .into() + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/client/src/parse.rs b/client/src/parse.rs index 6088546..7f0c235 100644 --- a/client/src/parse.rs +++ b/client/src/parse.rs @@ -11,6 +11,7 @@ use cow_settlement_interface::{ initialize::InitializeInput, reclaim_buffer::ReclaimBufferInput, reclaim_order::ReclaimOrderInput, + remove_solver::RemoveSolverInput, settle::{BeginSettleInput, FinalizeSettleInput}, transfer_authority::TransferAuthorityInput, InstructionInputParsing, @@ -30,6 +31,7 @@ pub enum ParsedInstruction<'a, A> { ReclaimBuffer(ReclaimBufferInput<'a, A>), TransferAuthority(TransferAuthorityInput<'a, A>), AddSolver(AddSolverInput<'a, A>), + RemoveSolver(RemoveSolverInput<'a, A>), } /// Parses any settlement instruction by its discriminator. @@ -66,6 +68,9 @@ pub fn parse_instruction<'a, A>( SettlementInstruction::AddSolver => { ParsedInstruction::AddSolver(AddSolverInput::parse_body(remaining_data, accounts)?) } + SettlementInstruction::RemoveSolver => ParsedInstruction::RemoveSolver( + RemoveSolverInput::parse_body(remaining_data, accounts)?, + ), }) } @@ -74,7 +79,7 @@ mod tests { use super::*; use crate::instructions::{ AddSolver, BeginSettle, CreateBuffers, CreateOrder, FinalizeSettle, Initialize, - InitializedIntent, + InitializedIntent, RemoveSolver, }; use cow_settlement_interface::{ data::intent::fixtures::sample_intent, @@ -159,6 +164,13 @@ mod tests { solver: pubkey_from_seed("solver"), } .into(), + SettlementInstruction::RemoveSolver => RemoveSolver { + program_id, + manager: payer, + rent_recipient: payer, + solver: pubkey_from_seed("solver"), + } + .into(), } } @@ -177,6 +189,7 @@ mod tests { SettlementInstruction::ReclaimBuffer, SettlementInstruction::TransferAuthority, SettlementInstruction::AddSolver, + SettlementInstruction::RemoveSolver, ] { let ix = build(expected); let accounts: Vec<_> = ix @@ -196,6 +209,7 @@ mod tests { ParsedInstruction::ReclaimBuffer(_) => SettlementInstruction::ReclaimBuffer, ParsedInstruction::TransferAuthority(_) => SettlementInstruction::TransferAuthority, ParsedInstruction::AddSolver(_) => SettlementInstruction::AddSolver, + ParsedInstruction::RemoveSolver(_) => SettlementInstruction::RemoveSolver, }; assert_eq!(actual, expected); } diff --git a/interface/src/data/state.rs b/interface/src/data/state.rs index 3850384..c21809e 100644 --- a/interface/src/data/state.rs +++ b/interface/src/data/state.rs @@ -172,6 +172,19 @@ impl> StateAccount { .checked_add(WIDTH_PUBKEY) .ok_or(ProgramError::ArithmeticOverflow) } + + /// The account's data length after shrinking it by one solver slot: the size + /// it must be resized to once [`remove_solver`](Self::remove_solver) has + /// shifted the tail over the removed slot. + /// + /// Returns [`ProgramError::ArithmeticOverflow`] if that length underflows, + /// which a caller that located a solver to remove can treat as unreachable. + pub fn shrunk_len(&self) -> Result { + self.0 + .len() + .checked_sub(WIDTH_PUBKEY) + .ok_or(ProgramError::ArithmeticOverflow) + } } impl<'a> StateAccount> { @@ -258,6 +271,37 @@ impl> StateAccount { data[gap..gap_end].copy_from_slice(&solver.to_bytes()); Ok(()) } + + /// Remove `solver` from the sorted solver list, or fail with + /// [`SettlementError::SolverNotFound`] if it isn't stored. + /// + /// The entries after the removed one are shifted one slot left to close the + /// gap; the now-stale trailing slot is left in place for the caller to drop + /// by resizing the account down to [`shrunk_len`](Self::shrunk_len). + pub fn remove_solver(&mut self, solver: &Pubkey) -> Result<(), ProgramError> { + let index = match self.solver_region().binary_search(&solver.to_bytes()) { + Ok(index) => index, + Err(_) => return Err(SettlementError::SolverNotFound.into()), + }; + + // Shift the entries after `index` down one slot; the trailing slot is + // left unchanged. + let data: &mut [u8] = &mut self.0; + let len = data.len(); + let offset = WIDTH_HEADER + .checked_add( + index + .checked_mul(WIDTH_PUBKEY) + .expect("removal index bound by data length"), + ) + .expect("removal offset bound by data length"); + let slot_end = offset + .checked_add(WIDTH_PUBKEY) + .expect("removal slot bound by data length"); + + data.copy_within(slot_end..len, offset); + Ok(()) + } } /// Test scaffolding for building state-account bytes, shared by this crate's @@ -555,8 +599,6 @@ mod tests { prop_assert_eq!(state.solvers().collect::>(), expected); } - /// `insert_solver` rejects a solver that is already stored and leaves - /// the live list untouched. #[test] fn insert_solver_rejects_an_existing_solver( header in fixtures::arb_init_params(), @@ -583,6 +625,64 @@ mod tests { let state = StateAccount::attach(&bytes[..]).expect("valid header"); prop_assert_eq!(state.solvers().take(stored.len()).collect::>(), stored); } + + #[test] + fn remove_solver_drops_a_present_solver( + header in fixtures::arb_init_params(), + // Unique and already sorted, being a `BTreeSet`. + raw_solvers in prop::collection::btree_set(any::<[u8; 32]>(), 1..50), + pick in any::(), + ) { + let stored: Vec = + raw_solvers.into_iter().map(Pubkey::new_from_array).collect(); + let index = pick.index(stored.len()); + let removed = stored[index]; + + // Remove the solver, then shrink to the length `shrunk_len` + // reports, exactly as the handler resizes the account. + let mut bytes = fixtures::state_account_bytes(&header, &stored); + let shrunk_len = StateAccount::attach(&bytes[..]) + .expect("valid header") + .shrunk_len() + .expect("shrunk length fits"); + prop_assert_eq!(shrunk_len, bytes.len().strict_sub(WIDTH_PUBKEY)); + StateAccount::attach(&mut bytes[..]) + .expect("valid header") + .remove_solver(&removed) + .expect("a present solver is removed"); + bytes.truncate(shrunk_len); + + let mut expected = stored; + expected.remove(index); + let state = StateAccount::attach(&bytes[..]).expect("valid header"); + prop_assert_eq!(state.solvers().collect::>(), expected); + prop_assert_eq!(state.solver_search(&removed), Err(index)); + } + + #[test] + fn remove_solver_rejects_an_absent_solver( + header in fixtures::arb_init_params(), + // Unique and already sorted, being a `BTreeSet`. + raw_solvers in prop::collection::btree_set(any::<[u8; 32]>(), 0..50), + raw_absent in any::<[u8; 32]>(), + ) { + prop_assume!(!raw_solvers.contains(&raw_absent)); + let stored: Vec = + raw_solvers.into_iter().map(Pubkey::new_from_array).collect(); + let absent = Pubkey::new_from_array(raw_absent); + + let mut bytes = fixtures::state_account_bytes(&header, &stored); + prop_assert_eq!( + StateAccount::attach(&mut bytes[..]) + .expect("valid header") + .remove_solver(&absent), + Err(SettlementError::SolverNotFound.into()), + ); + + // Nothing was removed: the stored solvers still read back in order. + let state = StateAccount::attach(&bytes[..]).expect("valid header"); + prop_assert_eq!(state.solvers().collect::>(), stored); + } } } } diff --git a/interface/src/instruction/mod.rs b/interface/src/instruction/mod.rs index 6da3a24..2ff4200 100644 --- a/interface/src/instruction/mod.rs +++ b/interface/src/instruction/mod.rs @@ -14,6 +14,7 @@ pub mod create_order; pub mod initialize; pub mod reclaim_buffer; pub mod reclaim_order; +pub mod remove_solver; pub mod settle; pub mod transfer_authority; diff --git a/interface/src/instruction/remove_solver.rs b/interface/src/instruction/remove_solver.rs new file mode 100644 index 0000000..233fa8c --- /dev/null +++ b/interface/src/instruction/remove_solver.rs @@ -0,0 +1,243 @@ +//! `RemoveSolver` instruction builder and parser. +//! +//! It removes a solver from the sorted solver list stored in the state PDA (see +//! [`crate::data::state`]). Only the manager may authorize it. The state PDA +//! shrinks by one solver and the freed rent is paid to `rent_recipient`. + +use core::mem::size_of; + +use solana_instruction::{AccountMeta, Instruction}; +use solana_program_error::ProgramError; +use solana_pubkey::Pubkey; + +use crate::instruction::InstructionInputParsing; +use crate::SettlementInstruction; + +/// Builder for a `RemoveSolver` instruction. +/// +/// `manager` authorizes the change and must be the state PDA's current manager; +/// it signs but doesn't receive anything. `rent_recipient` receives the freed +/// rent. `solver` is removed from the sorted solver list; removing one that +/// isn't present fails. +/// +/// Wire format: `[discriminator=9, solver (32 bytes)]`. +/// Required accounts: `[manager (S), rent_recipient (W), state_pda (W)]`. +pub struct RemoveSolver { + pub program_id: Pubkey, + pub manager: Pubkey, + pub rent_recipient: Pubkey, + pub state_pda: Pubkey, + pub solver: Pubkey, +} + +impl From for Instruction { + fn from(builder: RemoveSolver) -> Self { + let mut data = vec![SettlementInstruction::RemoveSolver.discriminator()]; + data.extend_from_slice(&builder.solver.to_bytes()); + Instruction { + program_id: builder.program_id, + accounts: vec![ + AccountMeta::new_readonly(builder.manager, true), + AccountMeta::new(builder.rent_recipient, false), + AccountMeta::new(builder.state_pda, false), + ], + data, + } + } +} + +/// Parsed inputs of a `RemoveSolver` instruction. +pub struct RemoveSolverInput<'a, A> { + pub manager: &'a A, + pub rent_recipient: &'a A, + pub state_pda: &'a A, + pub solver: Pubkey, +} + +impl<'a, A> InstructionInputParsing<'a, A> for RemoveSolverInput<'a, A> { + const DISCRIMINATOR: SettlementInstruction = SettlementInstruction::RemoveSolver; + + fn parse_body(instruction_data: &[u8], accounts: &'a [A]) -> Result { + let solver: &[u8; size_of::()] = instruction_data + .try_into() + .map_err(|_| ProgramError::InvalidInstructionData)?; + let solver = Pubkey::new_from_array(*solver); + + // Accounts: [manager (S), rent_recipient (W), state_pda (W)]. + let [manager, rent_recipient, state_pda, ..] = accounts else { + return Err(ProgramError::NotEnoughAccountKeys); + }; + + Ok(Self { + manager, + rent_recipient, + state_pda, + solver, + }) + } +} + +/// Test scaffolding for `RemoveSolver` parsing and handling, shared by this +/// crate's tests and the settlement program's via the `test-fixtures` feature. +#[cfg(any(test, feature = "test-fixtures"))] +pub mod fixtures { + use solana_address::Address; + + use super::{Instruction, RemoveSolver}; + + /// Number of accounts `RemoveSolver` expects: manager, rent recipient, and + /// state PDA. + pub const NUM_ACCOUNTS: usize = 3; + + /// `RemoveSolver` instruction data with placeholder addresses, for failure + /// cases where the actual addresses don't matter. + pub fn remove_solver_data() -> Vec { + let zero = Address::new_from_array([0; 32]); + Instruction::from(RemoveSolver { + program_id: zero, + manager: zero, + rent_recipient: zero, + state_pda: zero, + solver: zero, + }) + .data + } +} + +#[cfg(test)] +mod tests { + use super::fixtures::{remove_solver_data, NUM_ACCOUNTS}; + use super::*; + use crate::fixtures::pubkey_from_seed; + use crate::instruction::fixtures::{fake_account, fake_sequential_accounts}; + use crate::instruction::tests::{assert_readonly_signer, assert_writable_nonsigner}; + use solana_account_view::AccountView; + + #[test] + fn remove_solver_input_parses_valid_input() { + let program_id = pubkey_from_seed("program id"); + let manager = fake_account(pubkey_from_seed("manager")); + let rent_recipient = fake_account(pubkey_from_seed("rent recipient")); + let state_pda = fake_account(pubkey_from_seed("state pda")); + let solver = pubkey_from_seed("solver"); + + let data = Instruction::from(RemoveSolver { + program_id, + manager: *manager.address(), + rent_recipient: *rent_recipient.address(), + state_pda: *state_pda.address(), + solver, + }) + .data; + let accounts = [manager, rent_recipient, state_pda]; + + let RemoveSolverInput { + manager: parsed_manager, + rent_recipient: parsed_rent_recipient, + state_pda: parsed_state_pda, + solver: parsed_solver, + } = RemoveSolverInput::parse(&data, &accounts).expect("parse should succeed"); + + assert_eq!(parsed_manager.address(), accounts[0].address()); + assert_eq!(parsed_rent_recipient.address(), accounts[1].address()); + assert_eq!(parsed_state_pda.address(), accounts[2].address()); + assert_eq!(parsed_solver, solver); + } + + #[test] + fn remove_solver_input_rejects_long_data() { + let mut data = remove_solver_data(); + data.push(0); // trailing byte + let accounts = fake_sequential_accounts::(); + assert_eq!( + RemoveSolverInput::parse(&data, &accounts).err(), + Some(ProgramError::InvalidInstructionData), + ); + } + + #[test] + fn remove_solver_input_rejects_short_data() { + let mut data = remove_solver_data(); + data.pop(); // one byte short + let accounts = fake_sequential_accounts::(); + assert_eq!( + RemoveSolverInput::parse(&data, &accounts).err(), + Some(ProgramError::InvalidInstructionData), + ); + } + + #[test] + fn remove_solver_input_rejects_missing_accounts() { + let data = remove_solver_data(); + let mut accounts: Vec = fake_sequential_accounts::().into(); + accounts.pop(); + assert_eq!( + RemoveSolverInput::parse(&data, &accounts).err(), + Some(ProgramError::NotEnoughAccountKeys), + ); + } + + #[test] + fn instruction_data_has_expected_layout() { + let solver = pubkey_from_seed("solver"); + let Instruction { data, .. } = RemoveSolver { + program_id: pubkey_from_seed("program id"), + manager: pubkey_from_seed("manager"), + rent_recipient: pubkey_from_seed("rent recipient"), + state_pda: pubkey_from_seed("state pda"), + solver, + } + .into(); + + assert_eq!(data.len(), 1 + size_of::()); + assert_eq!(data[0], SettlementInstruction::RemoveSolver.discriminator()); + assert_eq!(&data[1..], &solver.to_bytes()); + } + + #[test] + fn instruction_data_regression() { + let solver = Pubkey::new_from_array([0x11; 32]); + let Instruction { data, .. } = RemoveSolver { + program_id: pubkey_from_seed("program id"), + manager: pubkey_from_seed("manager"), + rent_recipient: pubkey_from_seed("rent recipient"), + state_pda: pubkey_from_seed("state pda"), + solver, + } + .into(); + + #[rustfmt::skip] + let expected: [u8; 1 + size_of::()] = [ + // discriminator (RemoveSolver = 9) + 0x09, + // solver + 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, + 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, + 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, + 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, + ]; + assert_eq!(data, expected); + } + + #[test] + fn instruction_has_expected_accounts() { + let manager = pubkey_from_seed("manager"); + let rent_recipient = pubkey_from_seed("rent recipient"); + let state_pda = pubkey_from_seed("state pda"); + let Instruction { accounts, .. } = RemoveSolver { + program_id: pubkey_from_seed("program id"), + manager, + rent_recipient, + state_pda, + solver: pubkey_from_seed("solver"), + } + .into(); + + assert_eq!(accounts.len(), 3); + // The manager authorizes the change; the freed rent is paid to the + // recipient; the state PDA is shrunk and written. + assert_readonly_signer(&accounts[0], manager); + assert_writable_nonsigner(&accounts[1], rent_recipient); + assert_writable_nonsigner(&accounts[2], state_pda); + } +} diff --git a/interface/src/lib.rs b/interface/src/lib.rs index 9a294c8..e8867fa 100644 --- a/interface/src/lib.rs +++ b/interface/src/lib.rs @@ -26,6 +26,7 @@ pub enum SettlementInstruction { ReclaimBuffer = 6, TransferAuthority = 7, AddSolver = 8, + RemoveSolver = 9, } impl SettlementInstruction { @@ -222,14 +223,17 @@ pub enum SettlementError { /// `TransferAuthority`'s signer is neither the manager nor the current /// holder of the role being transferred, so it may not transfer it. UnauthorizedAuthorityTransfer = 34, - /// `AddSolver`'s manager account isn't a signer, or doesn't match the - /// `manager` recorded in the settlement state PDA. + /// `AddSolver`/`RemoveSolver`'s manager account isn't a signer, or doesn't + /// match the `manager` recorded in the settlement state PDA, so it may not + /// change the solver list. UnauthorizedSolverManagement = 35, /// `AddSolver`'s solver is already in the state PDA's solver list. SolverAlreadyExists = 36, - /// `BeginSettle`/`FinalizeSettle`'s solver account isn't a signer or isn't - /// in the state PDA's solver list, so it may not settle. - UnauthorizedSolver = 37, + /// `RemoveSolver`'s solver isn't in the state PDA's solver list. + SolverNotFound = 37, + /// `BeginSettle`'s solver account isn't a signer or isn't in the state PDA's + /// solver list, so it may not settle. + UnauthorizedSolver = 38, } impl From for u32 { diff --git a/programs/settlement/src/add_solver.rs b/programs/settlement/src/add_solver.rs index 244364b..2a51bff 100644 --- a/programs/settlement/src/add_solver.rs +++ b/programs/settlement/src/add_solver.rs @@ -93,8 +93,8 @@ mod tests { #[test] fn process_add_solver_rejects_non_canonical_state_pda() { - // `fake_sequential_accounts` puts the state PDA at `[3; 32]`, which is - // not the canonical state PDA for this program. + // `fake_sequential_accounts` puts the state PDA at some arbitrary + // address, which is not the canonical state PDA for this program. let data = add_solver_data(); let mut accounts = fake_sequential_accounts::(); assert_eq!( diff --git a/programs/settlement/src/lib.rs b/programs/settlement/src/lib.rs index d60fa23..1fc2f41 100644 --- a/programs/settlement/src/lib.rs +++ b/programs/settlement/src/lib.rs @@ -1,5 +1,8 @@ //! On-chain CoW Protocol settlement program. +use cow_settlement_interface::{recover_discriminator, SettlementInstruction}; +use pinocchio::{entrypoint, AccountView, Address, ProgramResult}; + mod add_solver; mod create_buffer; mod create_order; @@ -7,17 +10,17 @@ mod initialize; mod processor; mod reclaim_buffer; mod reclaim_order; +mod remove_solver; mod settle; mod transfer_authority; use add_solver::process_add_solver; -use cow_settlement_interface::{recover_discriminator, SettlementInstruction}; use create_buffer::process_create_buffer; use create_order::process_create_order; use initialize::process_initialize; -use pinocchio::{entrypoint, AccountView, Address, ProgramResult}; use reclaim_buffer::process_reclaim_buffer; use reclaim_order::process_reclaim_order; +use remove_solver::process_remove_solver; use settle::{process_begin_settle, process_finalize_settle}; use transfer_authority::process_transfer_authority; @@ -57,5 +60,8 @@ pub fn process_instruction( SettlementInstruction::AddSolver => { process_add_solver(program_id, accounts, instruction_data) } + SettlementInstruction::RemoveSolver => { + process_remove_solver(program_id, accounts, instruction_data) + } } } diff --git a/programs/settlement/src/remove_solver.rs b/programs/settlement/src/remove_solver.rs new file mode 100644 index 0000000..c393e6f --- /dev/null +++ b/programs/settlement/src/remove_solver.rs @@ -0,0 +1,165 @@ +//! `RemoveSolver` instruction handler. +//! +//! Removes a solver from the sorted solver list that follows the state PDA +//! header, shifting the tail left to close the gap and shrinking the account. +//! Only the manager may authorize it, and the freed rent is paid to +//! `rent_recipient`. The refund is a direct lamport move out of the +//! program-owned state PDA, so no system program is involved. + +use cow_settlement_interface::{ + data::state::StateAccount, + instruction::{remove_solver::RemoveSolverInput, InstructionInputParsing}, + Role, SettlementError, +}; +use pinocchio::{ + error::ProgramError, + sysvars::{rent::Rent, Sysvar}, + AccountView, Address, ProgramResult, Resize, +}; + +use crate::processor::check_state_pda; + +pub fn process_remove_solver( + program_id: &Address, + accounts: &mut [AccountView], + instruction_data: &[u8], +) -> ProgramResult { + let RemoveSolverInput { + manager, + rent_recipient, + state_pda, + solver, + } = RemoveSolverInput::parse(instruction_data, accounts)?; + + check_state_pda(program_id, state_pda)?; + + let mut state_pda = *state_pda; + let new_len = { + let mut state = StateAccount::attach(state_pda.try_borrow_mut()?)?; + if !manager.is_signer() || *manager.address() != state.authority(Role::Manager) { + return Err(SettlementError::UnauthorizedSolverManagement.into()); + } + state.remove_solver(&solver)?; + state + .shrunk_len() + .expect("a solver has been removed, so the length doesn't underflow") + }; + state_pda.resize(new_len)?; + + // Refund the rent the smaller account no longer needs to `rent_recipient`. + // The state PDA is program-owned, so the program may debit it directly. + let surplus = state_pda + .lamports() + .checked_sub(Rent::get()?.try_minimum_balance(new_len)?) + // The failure case is basically unreachable unless there are some + // protocol changes to the rent mechanism. + .ok_or(ProgramError::AccountNotRentExempt)?; + let mut rent_recipient = *rent_recipient; + let refunded = rent_recipient + .lamports() + .checked_add(surplus) + .ok_or(ProgramError::ArithmeticOverflow)?; + let retained = state_pda + .lamports() + .checked_sub(surplus) + .ok_or(ProgramError::ArithmeticOverflow)?; + state_pda.set_lamports(retained); + rent_recipient.set_lamports(refunded); + + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use cow_settlement_interface::instruction::fixtures::fake_sequential_accounts; + use cow_settlement_interface::instruction::remove_solver::fixtures::{ + remove_solver_data, NUM_ACCOUNTS, + }; + use pinocchio::error::ProgramError; + + const PROGRAM_ID: Address = Address::new_from_array([0xc0; 32]); + + #[test] + fn process_remove_solver_propagates_parse_error() { + let mut data = remove_solver_data(); + data.push(0); // trailing byte triggers a parse error + let mut accounts = fake_sequential_accounts::(); + assert_eq!( + process_remove_solver(&PROGRAM_ID, &mut accounts, &data), + Err(ProgramError::InvalidInstructionData), + ); + } + + #[test] + fn process_remove_solver_rejects_non_canonical_state_pda() { + // `fake_sequential_accounts` puts the state PDA at some arbitrary + // address, which is not the canonical state PDA for this program. + let data = remove_solver_data(); + let mut accounts = fake_sequential_accounts::(); + assert_eq!( + process_remove_solver(&PROGRAM_ID, &mut accounts, &data), + Err(SettlementError::StateAccountMismatch.into()), + ); + } + + mod proptest { + use ::proptest::prelude::*; + + use super::*; + use cow_settlement_interface::data::state::fixtures::{ + arb_init_params, state_account_bytes, + }; + use cow_settlement_interface::fixtures::pubkey_from_seed; + use cow_settlement_interface::instruction::fixtures::{ + fake_account, fake_account_owned_by, fake_signer, + }; + use cow_settlement_interface::instruction::remove_solver::RemoveSolver; + use cow_settlement_interface::pda::state::find_state_pda; + use cow_settlement_interface::{Instruction, Pubkey}; + + proptest! { + #[test] + fn process_remove_solver_rejects_an_absent_solver( + header in arb_init_params(), + // Unique and already sorted, being a `BTreeSet`. + raw_solvers in ::proptest::collection::btree_set(any::<[u8; 32]>(), 0..50), + raw_absent in any::<[u8; 32]>(), + ) { + prop_assume!(!raw_solvers.contains(&raw_absent)); + let manager = header.manager; + let stored: Vec = + raw_solvers.into_iter().map(Pubkey::new_from_array).collect(); + let absent = Pubkey::new_from_array(raw_absent); + + // Mock the three accounts the handler parses. Only the manager + // signer and the state PDA carry meaning here; the rent recipient + // is never touched, since the reject happens before the refund. + let (state_pda_address, _bump) = find_state_pda(&PROGRAM_ID); + let mut accounts = [ + fake_signer(manager), + fake_account(pubkey_from_seed("rent recipient")), + fake_account_owned_by( + state_pda_address, + PROGRAM_ID, + &state_account_bytes(&header, &stored), + ), + ]; + + let data = Instruction::from(RemoveSolver { + program_id: PROGRAM_ID, + manager, + rent_recipient: pubkey_from_seed("rent recipient"), + state_pda: state_pda_address, + solver: absent, + }) + .data; + + prop_assert_eq!( + process_remove_solver(&PROGRAM_ID, &mut accounts, &data), + Err(SettlementError::SolverNotFound.into()), + ); + } + } + } +} diff --git a/programs/settlement/tests/common/benchmark.rs b/programs/settlement/tests/common/benchmark.rs index 23f0ee3..ced249d 100644 --- a/programs/settlement/tests/common/benchmark.rs +++ b/programs/settlement/tests/common/benchmark.rs @@ -25,6 +25,7 @@ pub enum BenchLabel { Settle, TransferAuthority, AddSolver, + RemoveSolver, } impl fmt::Display for BenchLabel { @@ -40,6 +41,7 @@ impl fmt::Display for BenchLabel { Self::Settle => "settle", Self::TransferAuthority => "transfer_authority", Self::AddSolver => "add_solver", + Self::RemoveSolver => "remove_solver", }) } } diff --git a/programs/settlement/tests/remove_solvers.rs b/programs/settlement/tests/remove_solvers.rs new file mode 100644 index 0000000..1de58e2 --- /dev/null +++ b/programs/settlement/tests/remove_solvers.rs @@ -0,0 +1,255 @@ +//! Integration tests for removing solvers from the state PDA's list (shrinking +//! the account and refunding rent) and the manager gate on removal. Adding +//! solvers is covered by `add_solvers.rs`; the solver gate on settling by +//! `settle_solver_auth.rs`. + +use cow_settlement_client::cow_settlement_interface::{ + data::state::{StateAccount, WIDTH_HEADER, WIDTH_PUBKEY}, + Instruction, SettlementError, +}; +use cow_settlement_client::instructions::RemoveSolver; +use litesvm::LiteSVM; +use solana_sdk::{ + instruction::InstructionError, + pubkey::Pubkey, + signature::Signer, + transaction::{Transaction, TransactionError}, +}; + +use crate::common::{ + assert_instruction_error, + benchmark::{send_transaction_metered, BenchLabel}, + lamports, setup_init, to_instruction_error, unique_keypair, unique_pubkey, InitializedParams, +}; + +mod common; + +/// [`setup_init`] plus a funded, dedicated `rent_recipient` for removals. A +/// removal refunds the rent to this account, and the recipient of a lamport +/// credit must itself end up rent-exempt, so it's airdropped here. +fn setup() -> (LiteSVM, InitializedParams, Pubkey) { + let (mut svm, params) = setup_init(); + let rent_recipient = unique_pubkey(); + svm.airdrop(&rent_recipient, 1_000_000_000) + .expect("airdrop to rent recipient should succeed"); + (svm, params, rent_recipient) +} + +#[track_caller] +fn assert_solver_invariant(solvers: &[Pubkey]) { + assert!( + solvers.is_sorted_by(|a, b| a < b), + "invariant violated: solver list must be strictly ascending by address: {solvers:?}", + ); +} + +/// The solver list currently stored in the state PDA, in stored order. Reading +/// it also re-checks the storage invariant (see [`assert_solver_invariant`]), so +/// every test that inspects the list enforces it, not just the ones that compare +/// against a sorted expectation. +#[track_caller] +fn solvers(svm: &LiteSVM, state_pda: &Pubkey) -> Vec { + let data = svm + .get_account(state_pda) + .expect("state PDA should exist") + .data; + let solvers: Vec = StateAccount::attach(&data[..]) + .expect("state PDA should be a valid state account") + .solvers() + .collect(); + assert_solver_invariant(&solvers); + solvers +} + +/// Build a `RemoveSolver` transaction authorized by the manager, refunding the +/// freed rent to `rent_recipient`. Signed by the payer and the manager. Split +/// from [`remove_solver`] so the happy-path test can meter the same transaction. +fn remove_solver_tx( + svm: &LiteSVM, + params: &InitializedParams, + rent_recipient: &Pubkey, + solver: &Pubkey, +) -> Transaction { + let ix = RemoveSolver { + program_id: params.program_id, + manager: params.manager.pubkey(), + rent_recipient: *rent_recipient, + solver: *solver, + }; + common::signed_tx(svm, ¶ms.payer, ¶ms.manager, ix) +} + +/// Send a [`remove_solver_tx`]. +fn remove_solver( + svm: &mut LiteSVM, + params: &InitializedParams, + rent_recipient: &Pubkey, + solver: &Pubkey, +) -> Result<(), TransactionError> { + let tx = remove_solver_tx(svm, params, rent_recipient, solver); + svm.send_transaction(tx).map(|_| ()).map_err(|e| e.err) +} + +#[test] +fn removes_a_solver() { + let (mut svm, params, rent_recipient) = setup(); + let keep = unique_keypair().pubkey(); + let drop = unique_keypair().pubkey(); + common::register_solver(&mut svm, ¶ms, &keep); + common::register_solver(&mut svm, ¶ms, &drop); + + let recipient_before = lamports(&svm, &rent_recipient); + let tx = remove_solver_tx(&svm, ¶ms, &rent_recipient, &drop); + send_transaction_metered(&mut svm, tx, BenchLabel::RemoveSolver) + .expect("removing a solver should succeed"); + + // Only `keep` remains, the account shrank by one solver and stayed exactly + // rent-exempt, and the freed rent went to the rent recipient. + assert_eq!(solvers(&svm, ¶ms.state_pda), vec![keep]); + let account = svm + .get_account(¶ms.state_pda) + .expect("state PDA exists"); + assert_eq!(account.data.len(), WIDTH_HEADER + WIDTH_PUBKEY); + assert_eq!( + account.lamports, + svm.minimum_balance_for_rent_exemption(account.data.len()), + ); + assert!( + lamports(&svm, &rent_recipient) > recipient_before, + "the rent recipient received the freed rent", + ); +} + +#[test] +fn rejects_removing_absent_solver() { + let (mut svm, params, rent_recipient) = setup(); + let absent = unique_keypair().pubkey(); + + assert_instruction_error( + remove_solver(&mut svm, ¶ms, &rent_recipient, &absent), + to_instruction_error(SettlementError::SolverNotFound), + ); +} + +#[test] +fn rejects_removing_solver_by_non_manager() { + let (mut svm, params, rent_recipient) = setup(); + let solver = unique_keypair().pubkey(); + common::register_solver(&mut svm, ¶ms, &solver); + + let stranger = unique_keypair(); + let ix = RemoveSolver { + program_id: params.program_id, + manager: stranger.pubkey(), + rent_recipient, + solver, + }; + let tx = common::signed_tx(&svm, ¶ms.payer, &stranger, ix); + assert_instruction_error( + svm.send_transaction(tx).map(|_| ()).map_err(|e| e.err), + to_instruction_error(SettlementError::UnauthorizedSolverManagement), + ); +} + +#[test] +fn rejects_removing_solver_if_manager_is_not_signer() { + let (mut svm, params, rent_recipient) = setup(); + let solver = unique_keypair().pubkey(); + common::register_solver(&mut svm, ¶ms, &solver); + + // The correct manager, but with its signer flag cleared: authorization must + // require the manager to actually sign, not just be named. + let mut ix: Instruction = RemoveSolver { + program_id: params.program_id, + manager: params.manager.pubkey(), + rent_recipient, + solver, + } + .into(); + + /// Index of the manager account in a `RemoveSolver` instruction. + const MANAGER_INDEX: usize = 0; + assert!( + ix.accounts[MANAGER_INDEX].is_signer + && ix.accounts[MANAGER_INDEX].pubkey == params.manager.pubkey(), + "sanity check: MANAGER_INDEX should point to the manager signer" + ); + ix.accounts[MANAGER_INDEX].is_signer = false; + + let res = common::send(&mut svm, ¶ms.payer, vec![ix]); + assert_instruction_error( + res, + to_instruction_error(SettlementError::UnauthorizedSolverManagement), + ); +} + +/// A state PDA holding less than its shrunk rent minimum is rejected with +/// [`InstructionError::AccountNotRentExempt`], not refunded. +/// The flow in this test isn't expected to be reachable unless there are +/// changes to how rent is handled. Still, if it does, this will be less of an +/// issue than it could be. +#[test] +fn rejects_removing_from_a_below_rent_state_pda() { + let (mut svm, params, rent_recipient) = setup(); + let solver = unique_keypair().pubkey(); + common::register_solver(&mut svm, ¶ms, &solver); + + // Reduce the state PDA to one lamport below the rent minimum for zero + // solvers (its size after the removal), so it can't hold the rent it needs to + // exist and the refund's `checked_sub` underflows. + let below_rent = svm + .minimum_balance_for_rent_exemption(WIDTH_HEADER) + .strict_sub(1); + let mut account = svm + .get_account(¶ms.state_pda) + .expect("state PDA exists"); + account.lamports = below_rent; + svm.set_account(params.state_pda, account) + .expect("set_account should succeed"); + + assert_instruction_error( + remove_solver(&mut svm, ¶ms, &rent_recipient, &solver), + InstructionError::AccountNotRentExempt, + ); +} + +/// Removing a solver still works, and stays sorted, when the list is already +/// large. This test also benchmarks moving a lot of account data. +#[test] +fn remove_with_many_existing_solvers() { + let (mut svm, params, rent_recipient) = setup(); + + /// A deterministic solver address holding `index` big-endian in its leading + /// two bytes and the rest zero, so their relative order is their index's + /// order. + fn indexed_solver(index: u16) -> Pubkey { + let mut bytes = [0u8; 32]; + let leading = index.to_be_bytes(); + bytes[0] = leading[0]; + bytes[1] = leading[1]; + Pubkey::new_from_array(bytes) + } + + // Existing solvers 0x0000, 0x0001, …, written straight into the state PDA after + // its header rather than added one transaction at a time. + const EXISTING: u16 = 1_000; + const REMOVE_INDEX: u16 = 42; + let mut expected: Vec = (0..=EXISTING).map(indexed_solver).collect(); + let mut account = svm + .get_account(¶ms.state_pda) + .expect("state PDA exists"); + for solver in &expected { + account.data.extend_from_slice(&solver.to_bytes()); + } + account.lamports = svm.minimum_balance_for_rent_exemption(account.data.len()); + svm.set_account(params.state_pda, account) + .expect("set_account should succeed"); + + let sacrifice = indexed_solver(REMOVE_INDEX); + let tx = remove_solver_tx(&svm, ¶ms, &rent_recipient, &sacrifice); + send_transaction_metered(&mut svm, tx, BenchLabel::RemoveSolver) + .expect("removing from a large list should succeed"); + + expected.retain(|&s| s != sacrifice); + assert_eq!(solvers(&svm, ¶ms.state_pda), expected); +}