diff --git a/minter/src/monitor/mod.rs b/minter/src/monitor/mod.rs index 691acb4c..a054cc03 100644 --- a/minter/src/monitor/mod.rs +++ b/minter/src/monitor/mod.rs @@ -71,7 +71,7 @@ async fn check_submitted_transactions(runtime: &R) -> bool { state .submitted_transactions() .iter() - .map(|(sig, tx)| (*sig, tx.block_height)) + .map(|(sig, tx)| (*sig, tx.block_height())) .collect() }); if all_transactions.is_empty() { @@ -162,8 +162,8 @@ pub async fn resubmit_transactions(runtime: R) { .map(|(sig, tx)| { ( *sig, - tx.message.clone(), - tx.signers + tx.message().clone(), + tx.signers() .iter() .map(Signer::derivation_path) .collect::>(), diff --git a/minter/src/monitor/tests.rs b/minter/src/monitor/tests.rs index a956c6ef..60270107 100644 --- a/minter/src/monitor/tests.rs +++ b/minter/src/monitor/tests.rs @@ -400,7 +400,7 @@ mod resubmission { read_state(|s| { assert_eq!(s.submitted_transactions().len(), 1); let resubmitted = s.submitted_transactions().get(&new_signature).unwrap(); - assert_eq!(resubmitted.block_height, RESUBMISSION_BLOCK_HEIGHT); + assert_eq!(resubmitted.block_height(), RESUBMISSION_BLOCK_HEIGHT); }); } diff --git a/minter/src/sol_transfer/mod.rs b/minter/src/sol_transfer/mod.rs index 66c74310..4a8793c3 100644 --- a/minter/src/sol_transfer/mod.rs +++ b/minter/src/sol_transfer/mod.rs @@ -116,17 +116,21 @@ async fn sign_transaction( signer_derivation_paths: impl IntoIterator, signer: &impl SchnorrSigner, ) -> Result<(), CreateTransferError> { - let message_bytes = transaction.message_data(); - let message_len = message_bytes.len(); - transaction.signatures = sign_bytes(signer_derivation_paths, signer, message_bytes).await?; + ensure_within_transaction_size(&transaction.message)?; + transaction.signatures = + sign_bytes(signer_derivation_paths, signer, transaction.message_data()).await?; + Ok(()) +} - let tx_size = 1 + message_len + transaction.signatures.len() * BYTES_PER_SIGNATURE; +fn ensure_within_transaction_size(message: &Message) -> Result<(), CreateTransferError> { + let tx_size = 1 + + message.serialize().len() + + message.header.num_required_signatures as usize * BYTES_PER_SIGNATURE; if tx_size > MAX_TX_SIZE { return Err(CreateTransferError::TransactionTooLarge { max: MAX_TX_SIZE, got: tx_size, }); } - Ok(()) } diff --git a/minter/src/sol_transfer/tests.rs b/minter/src/sol_transfer/tests.rs index 10c9def4..e9a6217c 100644 --- a/minter/src/sol_transfer/tests.rs +++ b/minter/src/sol_transfer/tests.rs @@ -276,9 +276,12 @@ mod batch_withdrawal_tests { }) .collect(); - let result = - create_signed_batch_withdrawal_transaction(&minter_signing_once(), &targets, blockhash) - .await; + let result = create_signed_batch_withdrawal_transaction( + &TestCanisterRuntime::new(), + &targets, + blockhash, + ) + .await; assert_matches!( result, diff --git a/minter/src/state/mod.rs b/minter/src/state/mod.rs index 95966ad2..818c0b40 100644 --- a/minter/src/state/mod.rs +++ b/minter/src/state/mod.rs @@ -111,10 +111,10 @@ pub struct State { sent_withdrawal_requests: BTreeMap, successful_withdrawal_requests: BTreeMap, failed_withdrawal_requests: BTreeMap, - submitted_transactions: InsertionOrderedMap, - transactions_to_resubmit: InsertionOrderedMap, + submitted_transactions: InsertionOrderedMap, + transactions_to_resubmit: InsertionOrderedMap, succeeded_transactions: BTreeSet, - failed_transactions: InsertionOrderedMap, + failed_transactions: InsertionOrderedMap, nonce_pool: DurableNoncePool, active_tasks: BTreeSet, balance: Lamport, @@ -196,11 +196,11 @@ impl State { &self.failed_withdrawal_requests } - pub fn submitted_transactions(&self) -> &InsertionOrderedMap { + pub fn submitted_transactions(&self) -> &InsertionOrderedMap { &self.submitted_transactions } - pub fn transactions_to_resubmit(&self) -> &InsertionOrderedMap { + pub fn transactions_to_resubmit(&self) -> &InsertionOrderedMap { &self.transactions_to_resubmit } @@ -219,23 +219,22 @@ impl State { .unwrap_or_else(|| { panic!("BUG: cannot mark non-submitted transaction {signature} for resubmission") }); - if let TransactionPurpose::SweepDeposits { .. } = &transaction.purpose { - self.deposits.drop_swept(signature); - return; + match transaction { + MinterTransaction::SweepDeposit { .. } => self.deposits.drop_swept(signature), + MinterTransaction::Withdrawal { .. } => assert!( + self.transactions_to_resubmit + .insert(*signature, transaction) + .is_none(), + "BUG: transaction {signature} is already queued for resubmission" + ), } - assert!( - self.transactions_to_resubmit - .insert(*signature, transaction) - .is_none(), - "BUG: transaction {signature} is already queued for resubmission" - ); } pub fn succeeded_transactions(&self) -> &BTreeSet { &self.succeeded_transactions } - pub fn failed_transactions(&self) -> &InsertionOrderedMap { + pub fn failed_transactions(&self) -> &InsertionOrderedMap { &self.failed_transactions } @@ -532,7 +531,9 @@ impl State { !self.failed_transactions.contains_key(signature), "Attempted to submit already failed transaction {signature:?}" ); - let amount = match purpose { + let message = transaction.clone(); + let signers = signers.to_vec(); + let submitted_transaction = match purpose { TransactionPurpose::WithdrawSol { burn_indices } => { let mut total: Lamport = 0; for burn_index in burn_indices { @@ -561,27 +562,28 @@ impl State { .balance .checked_sub(total + tx_fee) .expect("BUG: insufficient minter balance for withdrawal"); - total + MinterTransaction::Withdrawal { + message, + signers, + block_height, + } } TransactionPurpose::SweepDeposits { deposit_ids } => { let sweep_destination = minter_address(self.minter_public_key.as_ref().expect( "BUG: a sweep was submitted before the minter public key was recorded", )); self.deposits - .sweep(deposit_ids, sweep_destination, transaction, signature) + .sweep(deposit_ids, sweep_destination, transaction, signature); + MinterTransaction::SweepDeposit { + message, + signers, + block_height, + } } }; assert_eq!( - self.submitted_transactions.insert( - *signature, - SolanaTransaction { - message: transaction.clone(), - signers: signers.to_vec(), - block_height, - purpose: purpose.clone(), - amount, - } - ), + self.submitted_transactions + .insert(*signature, submitted_transaction), None, "Attempted to submit transaction with signature {signature:?} twice" ); @@ -599,13 +601,20 @@ impl State { .unwrap_or_else(|| { panic!("Attempted to resubmit unknown transaction with signature {old_signature:?}") }); - assert!( - !matches!( - old_transaction.purpose, - TransactionPurpose::SweepDeposits { .. } + let new_transaction = match old_transaction { + MinterTransaction::SweepDeposit { .. } => panic!( + "BUG: sweep transaction {old_signature} must be dropped instead of resubmitted" ), - "BUG: sweep transaction {old_signature} must be dropped instead of resubmitted" - ); + MinterTransaction::Withdrawal { + message, + signers, + block_height: _, + } => MinterTransaction::Withdrawal { + message, + signers, + block_height: new_block_height, + }, + }; assert!( !self.succeeded_transactions.contains(new_signature), "Attempted to resubmit with signature {new_signature:?} that already succeeded" @@ -614,10 +623,6 @@ impl State { !self.failed_transactions.contains_key(new_signature), "Attempted to resubmit with signature {new_signature:?} that already failed" ); - let new_transaction = SolanaTransaction { - block_height: new_block_height, - ..old_transaction - }; assert_eq!( self.submitted_transactions .insert(*new_signature, new_transaction), @@ -642,9 +647,9 @@ impl State { .unwrap_or_else(|| { panic!("Attempted to mark unknown transaction {signature:?} as succeeded") }); - match &transaction.purpose { - TransactionPurpose::WithdrawSol { .. } => {} - TransactionPurpose::SweepDeposits { .. } => self.deposits.finalize_swept(signature), + match transaction { + MinterTransaction::SweepDeposit { .. } => self.deposits.finalize_swept(signature), + MinterTransaction::Withdrawal { .. } => {} } assert!( !self.transactions_to_resubmit.contains_key(signature), @@ -676,8 +681,9 @@ impl State { !self.transactions_to_resubmit.contains_key(signature), "BUG: transaction {signature} is queued for resubmission but is being marked as failed" ); - if let TransactionPurpose::SweepDeposits { .. } = &transaction.purpose { - self.deposits.drop_swept(signature); + match transaction { + MinterTransaction::SweepDeposit { .. } => self.deposits.drop_swept(signature), + MinterTransaction::Withdrawal { .. } => {} } assert_eq!( self.failed_transactions.insert(*signature, transaction), @@ -861,12 +867,40 @@ pub enum TaskType { } #[derive(Clone, Debug, PartialEq, Eq)] -pub struct SolanaTransaction { - pub message: VersionedMessage, - pub signers: Vec, - /// The block height of the block whose blockhash the transaction uses. - pub block_height: BlockHeight, - pub purpose: TransactionPurpose, - /// Total transfer amount in lamports (excluding fees). - pub amount: Lamport, +pub enum MinterTransaction { + SweepDeposit { + message: VersionedMessage, + signers: Vec, + /// The block height of the block whose blockhash the transaction uses. + block_height: BlockHeight, + }, + Withdrawal { + message: VersionedMessage, + signers: Vec, + /// The block height of the block whose blockhash the transaction uses. + block_height: BlockHeight, + }, +} + +impl MinterTransaction { + pub fn message(&self) -> &VersionedMessage { + match self { + MinterTransaction::SweepDeposit { message, .. } + | MinterTransaction::Withdrawal { message, .. } => message, + } + } + + pub fn signers(&self) -> &[Signer] { + match self { + MinterTransaction::SweepDeposit { signers, .. } + | MinterTransaction::Withdrawal { signers, .. } => signers, + } + } + + pub fn block_height(&self) -> BlockHeight { + match self { + MinterTransaction::SweepDeposit { block_height, .. } + | MinterTransaction::Withdrawal { block_height, .. } => *block_height, + } + } } diff --git a/minter/src/state/tests.rs b/minter/src/state/tests.rs index 6a502c88..494e233d 100644 --- a/minter/src/state/tests.rs +++ b/minter/src/state/tests.rs @@ -167,22 +167,16 @@ mod swept_deposits { Some(&planned_sweep([(0, first), (2, third)])) ); assert_eq!(s.deposits().queued().keys().collect::>(), vec![&1]); - let transaction = s.submitted_transactions().get(&sweep_signature).unwrap(); - assert_eq!( - transaction.amount, - first.sweepable_amount() + third.sweepable_amount() - 2 * FEE_PER_SIGNATURE - ); - assert_eq!( - transaction.signers, - vec![ - Signer::Account(third.account), - Signer::Account(first.account) - ] - ); - assert_eq!( - transaction.purpose, - TransactionPurpose::SweepDeposits { - deposit_ids: vec![2, 0], + assert_matches!( + s.submitted_transactions().get(&sweep_signature).unwrap(), + MinterTransaction::SweepDeposit { signers, .. } => { + assert_eq!( + *signers, + vec![ + Signer::Account(third.account), + Signer::Account(first.account) + ] + ); } ); assert_eq!(s.balance(), 0); diff --git a/minter/src/test_fixtures/runtime.rs b/minter/src/test_fixtures/runtime.rs index 8987418b..931ccc0c 100644 --- a/minter/src/test_fixtures/runtime.rs +++ b/minter/src/test_fixtures/runtime.rs @@ -35,7 +35,7 @@ pub struct TestCanisterRuntime { msg_cycles_accepted: Arc>>, msg_cycles_available: Stubs, msg_cycles_refunded: Stubs, - set_timer_call_count: Arc>, + set_timer_delays: Arc>>, schnorr_public_key_results: Stubs>, schnorr_public_key_call_count: Arc>, } @@ -145,7 +145,11 @@ impl TestCanisterRuntime { } pub(crate) fn set_timer_call_count(&self) -> usize { - *self.set_timer_call_count.lock().unwrap() + self.set_timer_delays().len() + } + + pub(crate) fn set_timer_delays(&self) -> Vec { + self.set_timer_delays.lock().unwrap().clone() } pub(crate) fn schnorr_public_key_call_count(&self) -> usize { @@ -194,13 +198,13 @@ impl CanisterRuntime for TestCanisterRuntime { self.msg_cycles_refunded.next() } - fn set_timer(&self, _delay: Duration, _f: F) -> ic_cdk_timers::TimerId + fn set_timer(&self, delay: Duration, _f: F) -> ic_cdk_timers::TimerId where Self: Sized, F: FnOnce(Self) -> Fut + 'static, Fut: Future + 'static, { - *self.set_timer_call_count.lock().unwrap() += 1; + self.set_timer_delays.lock().unwrap().push(delay); Default::default() } diff --git a/minter/src/withdraw/tests.rs b/minter/src/withdraw/tests.rs index af2d4362..5c5f3f99 100644 --- a/minter/src/withdraw/tests.rs +++ b/minter/src/withdraw/tests.rs @@ -4,7 +4,7 @@ use crate::{ guard::{TimerGuard, withdrawal_guard}, rpc::BlockHeight, sol_transfer::MAX_WITHDRAWALS_PER_TX, - state::{TaskType, event::TransactionPurpose, read_state}, + state::{MinterTransaction, TaskType, read_state}, test_fixtures::{ EventsAssert, MINIMUM_WITHDRAWAL_AMOUNT, MINTER_ACCOUNT, MINTER_ADDRESS, NONCE_ACCOUNT, WITHDRAWAL_FEE, account, confirmed_block_at_height, events, init_balance, init_balance_to, @@ -486,12 +486,13 @@ mod process_pending_withdrawals_tests { assert_matches!(withdrawal_status(1), WithdrawalStatus::TxSent { .. }); read_state(|s| { let submitted = s.submitted_transactions().get(&tx_signature).unwrap(); - assert_eq!(submitted.block_height, block_height); + assert_eq!(submitted.block_height(), block_height); + assert_matches!(submitted, MinterTransaction::Withdrawal { .. }); assert_eq!( - submitted.purpose, - TransactionPurpose::WithdrawSol { - burn_indices: vec![1_u64.into()] - } + s.sent_withdrawal_requests() + .get(&1_u64.into()) + .map(|sent| sent.signature), + Some(tx_signature) ); }); }