diff --git a/docs/design.md b/docs/design.md index cb11ee3d..e0d9d465 100644 --- a/docs/design.md +++ b/docs/design.md @@ -531,7 +531,7 @@ There is a **minimum withdrawal amount**, which is defined in [Section 3.3.3](#3 The funds for each withdrawal are taken from the main account. Since ckSOL is only minted once the corresponding SOL has reached the main account (see [Section 3.1.3](#313-manual-flow)), the main account always covers the minted supply, and a withdrawal never waits for or triggers a consolidation. The only delay a user can experience is between a `deposit_sol` call and the mint of their own deposit. -Contrary to sweeps, a withdrawal transaction does not follow the transaction submission flow of [Section 3.1.4](#314-consolidation), since it must not reference a recent block hash. Instead, the ckSOL minter picks a free durable nonce account from the pool of [Section 3.2.1](#321-durable-nonce-accounts) and reads its current nonce value with `getAccountInfo` at the `finalized` commitment level; a response showing a nonce value already bound to an earlier transaction of that account is stale, and the batch waits for the next round. The transaction consists of an `AdvanceNonceAccount` instruction first, followed by one transfer per withdrawal request, and carries the nonce value in place of the recent block hash. The main address is the fee payer, the source of all transfers, and the nonce authority, so the transaction has a single signature. A nonce account is reserved for a batch synchronously, before the first await point, so that concurrently processed batches can never pick the same account. Once the message is built, a `CreatedTransaction` event records the unsigned transaction together with the burn indices of the withdrawals it serves and the nonce account and nonce value it uses, *before* the threshold signature is requested: should the signing fail or be interrupted, the reservation survives, and the ckSOL minter signs the recorded message again instead of building a new one, so that no two different messages are ever signed for the same nonce value. The signed transaction is likewise persisted before it is sent, so that the identical transaction can later be re-broadcast. +Contrary to sweeps, a withdrawal transaction does not follow the transaction submission flow of [Section 3.1.4](#314-consolidation), since it must not reference a recent block hash. Instead, the ckSOL minter picks a free durable nonce account from the pool of [Section 3.2.1](#321-durable-nonce-accounts) and reads its current nonce value with `getAccountInfo` at the `finalized` commitment level; a response showing a nonce value already bound to an earlier transaction of that account is stale, and the batch waits for the next round. The transaction consists of an `AdvanceNonceAccount` instruction first, followed by one transfer per withdrawal request, and carries the nonce value in place of the recent block hash. The main address is the fee payer, the source of all transfers, and the nonce authority, so the transaction has a single signature. A nonce account is reserved for a batch synchronously, before the first await point, so that concurrently processed batches can never pick the same account. Before the threshold signature is requested, a `CreatedWithdrawalTransaction` event binds the nonce account and its nonce value to the burn indices of the withdrawals the transaction serves. Since each burn index identifies a withdrawal request, i.e., a destination and an amount, the binding fully determines the message to be signed. Threshold signing is treated as fallible: should it fail or be interrupted, the binding survives, and the ckSOL minter rebuilds the identical message from the binding, without reading the nonce account again, and signs it again, so that no two different messages are ever signed for the same nonce value. The signed transaction is recorded with a `SubmittedTransaction` event *before* it is sent, so that the identical transaction can later be re-broadcast. When processing this event, the ckSOL minter checks that the transaction advances a bound nonce account, serves the bound withdrawal requests, and carries exactly the message it rebuilds from the binding, signed by the main address only. ```mermaid sequenceDiagram @@ -547,11 +547,11 @@ sequenceDiagram RPC->>+Solana: getAccountInfo(nonce_account) Solana-->>-RPC: nonce account state RPC-->>-Minter: nonce account state - Note over Minter: Build transaction: AdvanceNonceAccount first,
then one transfer per withdrawal,
nonce value in place of the recent block hash - Note over Minter: Record CreatedTransaction (unsigned transaction,
nonce account, nonce value) + Note over Minter: Record CreatedWithdrawalTransaction (burn indices,
nonce account, nonce value) + Note over Minter: Build transaction from the binding: AdvanceNonceAccount first,
then one transfer per withdrawal,
nonce value in place of the recent block hash Minter->>+Signer: sign_with_schnorr(Ed25519, main derivation path, message) Signer-->>-Minter: signature - Note over Minter: Serialize and persist signed transaction + Note over Minter: Record SubmittedTransaction (signed transaction) Minter->>+RPC: sendTransaction(transaction) RPC->>+Solana: sendTransaction(transaction) Solana-->>-RPC: signature diff --git a/integration_tests/tests/tests.rs b/integration_tests/tests/tests.rs index 3706c6b1..a6e0413c 100644 --- a/integration_tests/tests/tests.rs +++ b/integration_tests/tests/tests.rs @@ -748,11 +748,10 @@ mod withdrawal_tests { check!(events.iter().any(|e| matches!( e, EventType::SubmittedTransaction { - purpose: TransactionPurpose::WithdrawSol { burn_indices }, - block_height, + purpose: TransactionPurpose::Withdrawal { burn_indices, block_height }, .. } if burn_indices == &[block_index] - && block_height == &SUBMISSION_BLOCK_HEIGHT + && *block_height == SUBMISSION_BLOCK_HEIGHT ))); }); @@ -1000,7 +999,7 @@ mod deposit_sol_tests { e, EventType::SubmittedTransaction { signature, - purpose: TransactionPurpose::SweepDeposits { deposit_ids }, + purpose: TransactionPurpose::SweepDeposit { deposit_ids, .. }, .. } if *signature == sweep_signature && deposit_ids == &[deposit_id] ))); diff --git a/libs/types-internal/src/event.rs b/libs/types-internal/src/event.rs index db011d2c..31c589f5 100644 --- a/libs/types-internal/src/event.rs +++ b/libs/types-internal/src/event.rs @@ -4,7 +4,7 @@ use crate::{InitArgs, UpgradeArgs}; use candid::CandidType; use icrc_ledger_types::icrc1::account::Account; use serde::Deserialize; -use sol_rpc_types::{Lamport, Pubkey as Address, Signature}; +use sol_rpc_types::{Hash, Lamport, Pubkey as Address, Signature}; /// A minter event that can be serialized to Candid. #[derive(Clone, Debug, PartialEq, CandidType, Deserialize)] @@ -44,10 +44,9 @@ pub enum EventType { transaction: VersionedTransactionMessage, /// The signers in signature order (fee payer first). signers: Vec, - /// The purpose of this transaction. + /// The purpose of this transaction, with what the minter needs to + /// track it until it is finalized. purpose: TransactionPurpose, - /// The block height of the block whose blockhash the transaction uses. - block_height: u64, }, /// A previously submitted transaction was resubmitted with a new signature. ResubmittedTransaction { @@ -139,6 +138,17 @@ pub enum EventType { /// The identifier of the deposit whose pending mint was quarantined. deposit_id: u64, }, + /// The minter bound a durable nonce account and its nonce value to the + /// withdrawal requests of the given burn indices, before requesting the + /// threshold signature. The binding determines the transaction message. + CreatedWithdrawalTransaction { + /// The ledger burn indices of the withdrawal requests served by this transaction. + burn_indices: Vec, + /// The durable nonce account bound to this transaction. + nonce_account: Address, + /// The nonce value the transaction carries in place of a recent blockhash. + nonce_value: Hash, + }, } /// The mint enqueued for one deposit of a `CreditedSweep` event. @@ -164,15 +174,29 @@ pub enum Signer { /// The purpose of a submitted Solana transaction. #[derive(Clone, Debug, PartialEq, CandidType, Deserialize)] pub enum TransactionPurpose { - /// Send withdrawals to users' Solana addresses. - WithdrawSol { + /// Sweep the deposit addresses of deposits queued by `deposit_sol` into + /// the minter's main account. The transaction uses a recent blockhash and + /// is dropped once the blockhash expires. + SweepDeposit { + /// The ids of the swept deposits. + deposit_ids: Vec, + /// The block height of the block whose blockhash the transaction uses. + block_height: u64, + }, + /// Send withdrawals to users' Solana addresses. The transaction uses a + /// recent blockhash and is resubmitted once the blockhash expires. + Withdrawal { /// The burn transaction indices on the ckSOL ledger. burn_indices: Vec, + /// The block height of the block whose blockhash the transaction uses. + block_height: u64, }, - /// Sweep the deposit addresses of deposits queued by `deposit_sol` into the minter's main account. - SweepDeposits { - /// The ids of the swept deposits. - deposit_ids: Vec, + /// Send withdrawals to users' Solana addresses. The transaction carries + /// the nonce value of a durable nonce account instead of a recent + /// blockhash, so it never expires. + NonceWithdrawal { + /// The burn transaction indices on the ckSOL ledger. + burn_indices: Vec, }, } diff --git a/minter/canbench_results.yml b/minter/canbench_results.yml index 4273b574..5dffca25 100644 --- a/minter/canbench_results.yml +++ b/minter/canbench_results.yml @@ -2,7 +2,7 @@ benches: post_upgrade_10k_events: total: calls: 1 - instructions: 194836563 + instructions: 244554653 heap_increase: 0 stable_memory_increase: 0 scopes: {} diff --git a/minter/cksol_minter.did b/minter/cksol_minter.did index 01c04b83..e51e8c9b 100644 --- a/minter/cksol_minter.did +++ b/minter/cksol_minter.did @@ -35,6 +35,10 @@ type GetDepositAddressArgs = record { // Example: "5LrcE2f6uvydKRquEJ8xp19heGxSvqsVbcqUeFoiWbXe8JNip7ftPQNTAVPyTK7ijVdpkzmKKaAQR7MWMmujAhXD" type Signature = text; +// A 32-byte Solana hash as a base-58 encoded string, +// such as the nonce value stored by a durable nonce account. +type Hash = text; + // Smallest denomination of SOL, the native token on Solana, // i.e. 1_000_000_000 Lamports is 1 SOL type Lamport = nat64; @@ -345,16 +349,29 @@ type Signer = variant { // *WARNING*: This type is used exclusively by the debug `get_events` endpoint. // Backwards-compatibility is not guaranteed. type TransactionPurpose = variant { - // Withdraw SOL to users' Solana addresses. - WithdrawSol : record { - // The ledger burn indices of the withdrawal requests included in this transaction. - burn_indices: vec LedgerBurnIndex; - }; // Sweep the deposit addresses of deposits queued by `deposit_sol` into the - // minter's main account. - SweepDeposits : record { + // minter's main account. The transaction uses a recent blockhash and is + // dropped once the blockhash expires. + SweepDeposit : record { // The ids of the swept deposits. deposit_ids: vec DepositSolId; + // The block height of the block whose blockhash the transaction uses. + block_height: nat64; + }; + // Withdraw SOL to users' Solana addresses. The transaction uses a recent + // blockhash and is resubmitted once the blockhash expires. + Withdrawal : record { + // The ledger burn indices of the withdrawal requests included in this transaction. + burn_indices: vec LedgerBurnIndex; + // The block height of the block whose blockhash the transaction uses. + block_height: nat64; + }; + // Withdraw SOL to users' Solana addresses. The transaction carries the + // nonce value of a durable nonce account instead of a recent blockhash, + // so it never expires. + NonceWithdrawal : record { + // The ledger burn indices of the withdrawal requests included in this transaction. + burn_indices: vec LedgerBurnIndex; }; }; @@ -380,10 +397,9 @@ type EventType = variant { transaction: VersionedTransactionMessage; // The signers in signature order (fee payer first). signers: vec Signer; - // The purpose of this transaction. + // The purpose of this transaction, with what the minter needs to + // track it until it is finalized. purpose: TransactionPurpose; - // The block height of the block whose blockhash the transaction uses. - block_height: nat64; }; // A previously submitted transaction was resubmitted with a new signature. ResubmittedTransaction : record { @@ -476,6 +492,17 @@ type EventType = variant { // The identifier of the deposit whose pending mint was quarantined. deposit_id: DepositSolId; }; + // The minter bound a durable nonce account and its nonce value to the + // withdrawal requests of the given burn indices, before requesting the + // threshold signature. The binding determines the transaction message. + CreatedWithdrawalTransaction : record { + // The ledger burn indices of the withdrawal requests served by this transaction. + burn_indices: vec LedgerBurnIndex; + // The durable nonce account bound to this transaction. + nonce_account: Address; + // The nonce value the transaction carries in place of a recent blockhash. + nonce_value: Hash; + }; }; // A minter event. diff --git a/minter/src/canbench.rs b/minter/src/canbench.rs index 00dcb512..65bc4527 100644 --- a/minter/src/canbench.rs +++ b/minter/src/canbench.rs @@ -5,6 +5,7 @@ use crate::{ numeric::{LedgerBurnIndex, LedgerMintIndex}, rpc::BlockHeight, runtime::IcCanisterRuntime, + sol_transfer::build_batch_withdrawal_message, state::{ DepositBalance, QueuedDeposit, SchnorrPublicKey, Sweep, audit::{process_event, replay_events}, @@ -27,7 +28,6 @@ const INDEX_OFFSET_QUARANTINE: usize = 10_000; const INDEX_OFFSET_WITHDRAWAL: usize = 20_000; const INDEX_OFFSET_DROPPED: usize = 30_000; const INDEX_OFFSET_EXPIRED: usize = 40_000; -const INDEX_OFFSET_RESUBMIT: usize = 50_000; fn init_args() -> InitArgs { InitArgs { @@ -40,10 +40,20 @@ fn init_args() -> InitArgs { deposit_sol_required_cycles: 1_000_000_000_000, solana_network: SolanaNetwork::Mainnet, deposit_sol_fee: 10_000_000_000, - nonce_accounts: vec![], + nonce_accounts: vec![nonce_account().to_string()], } } +fn nonce_account() -> solana_address::Address { + solana_address::Address::from([0x4E; 32]) +} + +fn nonce_value(i: usize) -> solana_hash::Hash { + let mut bytes = [0u8; 32]; + bytes[..8].copy_from_slice(&(i as u64).to_le_bytes()); + solana_hash::Hash::from(bytes) +} + fn signature(i: usize) -> Signature { let mut bytes = [0u8; 64]; bytes[..8].copy_from_slice(&(i as u64).to_le_bytes()); @@ -72,9 +82,18 @@ fn master_key() -> SchnorrPublicKey { } } -fn message() -> solana_message::Message { - let payer = solana_address::Address::from([0x42; 32]); - solana_message::Message::new_with_blockhash(&[], Some(&payer), &solana_message::Hash::default()) +fn nonce_withdrawal_message( + nonce_value: solana_hash::Hash, + destination: solana_address::Address, + amount: u64, +) -> solana_message::Message { + build_batch_withdrawal_message( + &minter_address(&master_key()), + &nonce_account(), + nonce_value, + &[(destination, amount)], + ) + .expect("BUG: a single-transfer withdrawal message fits in a transaction") } fn record(event: EventType) { @@ -101,10 +120,10 @@ fn queue_and_sweep(deposit_id: u64, account_index: usize, amount: u64, sig: Sign signature: sig, message: VersionedMessage::Legacy(sweep.sweep_message(solana_message::Hash::default())), signers: vec![Signer::Account(account)], - purpose: TransactionPurpose::SweepDeposits { + purpose: TransactionPurpose::SweepDeposit { deposit_ids: vec![deposit_id], + block_height: BlockHeight::new(0), }, - block_height: BlockHeight::new(0), }); } @@ -117,22 +136,31 @@ fn deposit_address(account_index: usize) -> solana_address::Address { fn accept_and_submit_withdrawal(account_index: usize, burn_index: u64, sig: Signature) { const WITHDRAWAL_FEE: u64 = 5_000_000; const WITHDRAWAL_AMOUNT: u64 = 10_000_000; + const AMOUNT_TO_TRANSFER: u64 = WITHDRAWAL_AMOUNT - WITHDRAWAL_FEE; + let destination = [0u8; 32]; + let burn_indices = vec![LedgerBurnIndex::from(burn_index)]; record(EventType::AcceptedWithdrawalRequest(WithdrawalRequest { account: account(account_index), - solana_address: [0u8; 32], + solana_address: destination, burn_block_index: LedgerBurnIndex::from(burn_index), burned_amount: WITHDRAWAL_AMOUNT, - amount_to_transfer: WITHDRAWAL_AMOUNT - WITHDRAWAL_FEE, + amount_to_transfer: AMOUNT_TO_TRANSFER, })); + record(EventType::CreatedWithdrawalTransaction { + burn_indices: burn_indices.clone(), + nonce_account: nonce_account(), + nonce_value: nonce_value(account_index), + }); record(EventType::SubmittedTransaction { signature: sig, - message: VersionedMessage::Legacy(message()), + message: VersionedMessage::Legacy(nonce_withdrawal_message( + nonce_value(account_index), + solana_address::Address::from(destination), + AMOUNT_TO_TRANSFER, + )), signers: vec![Signer::Minter], - purpose: TransactionPurpose::WithdrawSol { - burn_indices: vec![LedgerBurnIndex::from(burn_index)], - }, - block_height: BlockHeight::new(0), + purpose: TransactionPurpose::NonceWithdrawal { burn_indices }, }); } @@ -193,8 +221,8 @@ fn setup_10k_events() { record(EventType::QuarantinedSweep { signature: sig }); } - // Withdrawal cycles: accept withdrawal → submit withdrawal → succeed - // 500 × 3 = 1500 events + // Withdrawal cycles: accept withdrawal → create transaction → submit → succeed + // 500 × 4 = 2000 events for i in 0..500 { let sig = signature(INDEX_OFFSET_WITHDRAWAL + i); @@ -213,28 +241,19 @@ fn setup_10k_events() { record(EventType::FailedTransaction { signature: sig }); } - // Expired + resubmitted withdrawal cycles: accept → submit → expire → resubmit → succeed - // 300 × 5 = 1500 events + // Expired sweeps: queue → sweep → expire + // 300 × 3 = 900 events for i in 0..300 { - let old_sig = signature(INDEX_OFFSET_EXPIRED + i); - let new_sig = signature(INDEX_OFFSET_RESUBMIT + i); - - accept_and_submit_withdrawal( - INDEX_OFFSET_EXPIRED + i, - (INDEX_OFFSET_EXPIRED + i) as u64, - old_sig, - ); - record(EventType::ExpiredTransaction { signature: old_sig }); - record(EventType::ResubmittedTransaction { - old_signature: old_sig, - new_signature: new_sig, - new_block_height: BlockHeight::new(1), - }); - record(EventType::SucceededTransaction { signature: new_sig }); + let deposit_id = next_deposit_id; + next_deposit_id += 1; + let sig = signature(INDEX_OFFSET_EXPIRED + i); + + queue_and_sweep(deposit_id, INDEX_OFFSET_EXPIRED + i, amount, sig); + record(EventType::ExpiredTransaction { signature: sig }); } - // Total: 1 (init) + 1 (minter public key) + 5000 + 800 + 1500 + 1500 + 1500 = 10302 events - assert_eq!(total_event_count(), 10302); + // Total: 1 (init) + 1 (minter public key) + 5000 + 800 + 2000 + 1500 + 900 = 10202 events + assert_eq!(total_event_count(), 10202); reset_state(); } diff --git a/minter/src/dashboard/mod.rs b/minter/src/dashboard/mod.rs index 96353481..499dc748 100644 --- a/minter/src/dashboard/mod.rs +++ b/minter/src/dashboard/mod.rs @@ -6,7 +6,7 @@ use askama::Template; use candid::Principal; use cksol_types_internal::SolanaNetwork; use ic_http_types::HttpRequest; -use std::str::FromStr; +use std::{collections::BTreeMap, str::FromStr}; const LAMPORTS_PER_SOL: u64 = 1_000_000_000; @@ -331,7 +331,12 @@ impl DashboardTemplate { } // Pending and sent (active) newest-first, then finalized (succeeded/failed) newest-first. - for (burn_index, pending) in state.pending_withdrawal_requests().iter().rev() { + let pending_and_created_requests: BTreeMap<_, _> = state + .pending_withdrawal_requests() + .iter() + .chain(state.created_withdrawal_requests()) + .collect(); + for (burn_index, pending) in pending_and_created_requests.into_iter().rev() { push_withdrawal( &mut withdrawals, burn_index, diff --git a/minter/src/deposit/sweep/timer.rs b/minter/src/deposit/sweep/timer.rs index 8ac6c040..103ef025 100644 --- a/minter/src/deposit/sweep/timer.rs +++ b/minter/src/deposit/sweep/timer.rs @@ -134,10 +134,10 @@ async fn submit_sweep_transaction( signature, message: transaction.message.clone().into(), signers, - purpose: TransactionPurpose::SweepDeposits { + purpose: TransactionPurpose::SweepDeposit { deposit_ids: sweep.deposits().keys().copied().collect(), + block_height: block.block_height, }, - block_height: block.block_height, }, runtime, ) diff --git a/minter/src/deposit/sweep/timer/tests.rs b/minter/src/deposit/sweep/timer/tests.rs index c5d95302..ba178a03 100644 --- a/minter/src/deposit/sweep/timer/tests.rs +++ b/minter/src/deposit/sweep/timer/tests.rs @@ -120,8 +120,7 @@ async fn should_sweep_batch_with_largest_deposit_as_fee_payer() { signature, message, signers, - purpose: TransactionPurpose::SweepDeposits { deposit_ids }, - block_height, + purpose: TransactionPurpose::SweepDeposit { deposit_ids, block_height }, } => { assert_eq!(signature, fee_payer_signature); assert_eq!(signers[0], Signer::Account(account(2))); @@ -172,7 +171,7 @@ async fn should_record_event_even_if_transaction_submission_fails() { .expect_event(|e| { assert_matches!(e, EventType::SubmittedTransaction { signature, - purpose: TransactionPurpose::SweepDeposits { deposit_ids }, + purpose: TransactionPurpose::SweepDeposit { deposit_ids, .. }, .. } if signature == fee_payer_signature && deposit_ids == vec![0]) }) @@ -218,14 +217,14 @@ async fn should_split_deposits_into_batches_of_max_size() { .expect_event(|e| { assert_matches!(e, EventType::SubmittedTransaction { signature, - purpose: TransactionPurpose::SweepDeposits { deposit_ids }, + purpose: TransactionPurpose::SweepDeposit { deposit_ids, .. }, .. } if signature == fee_payer_signature_1 && deposit_ids == batch_1_ids) }) .expect_event(|e| { assert_matches!(e, EventType::SubmittedTransaction { signature, - purpose: TransactionPurpose::SweepDeposits { deposit_ids }, + purpose: TransactionPurpose::SweepDeposit { deposit_ids, .. }, .. } if signature == fee_payer_signature_2 && deposit_ids == batch_2_ids) }) diff --git a/minter/src/main.rs b/minter/src/main.rs index 45891e00..1b65c803 100644 --- a/minter/src/main.rs +++ b/minter/src/main.rs @@ -132,17 +132,27 @@ fn get_events( message, signers, purpose, - block_height, } => { let purpose = match purpose { - TransactionPurpose::WithdrawSol { burn_indices } => { - event::TransactionPurpose::WithdrawSol { + TransactionPurpose::SweepDeposit { + deposit_ids, + block_height, + } => event::TransactionPurpose::SweepDeposit { + deposit_ids, + block_height: block_height.get(), + }, + TransactionPurpose::Withdrawal { + burn_indices, + block_height, + } => event::TransactionPurpose::Withdrawal { + burn_indices: burn_indices.iter().map(|idx| *idx.get()).collect(), + block_height: block_height.get(), + }, + TransactionPurpose::NonceWithdrawal { burn_indices } => { + event::TransactionPurpose::NonceWithdrawal { burn_indices: burn_indices.iter().map(|idx| *idx.get()).collect(), } } - TransactionPurpose::SweepDeposits { deposit_ids } => { - event::TransactionPurpose::SweepDeposits { deposit_ids } - } }; event::EventType::SubmittedTransaction { signature: signature.into(), @@ -162,7 +172,6 @@ fn get_events( }) .collect(), purpose, - block_height: block_height.get(), } } EventType::ResubmittedTransaction { @@ -231,6 +240,15 @@ fn get_events( EventType::QuarantinedPendingMint { deposit_id } => { event::EventType::QuarantinedPendingMint { deposit_id } } + EventType::CreatedWithdrawalTransaction { + burn_indices, + nonce_account, + nonce_value, + } => event::EventType::CreatedWithdrawalTransaction { + burn_indices: burn_indices.iter().map(|idx| *idx.get()).collect(), + nonce_account: nonce_account.into(), + nonce_value: nonce_value.into(), + }, } } diff --git a/minter/src/metrics.rs b/minter/src/metrics.rs index 9e32fcd7..dafc9316 100644 --- a/minter/src/metrics.rs +++ b/minter/src/metrics.rs @@ -72,7 +72,8 @@ pub fn encode_metrics(w: &mut MetricsEncoder>, s: &State) -> std::io::Re )?; w.encode_gauge( "pending_withdrawal_requests", - s.pending_withdrawal_requests().len().metric_value(), + (s.pending_withdrawal_requests().len() + s.created_withdrawal_requests().len()) + .metric_value(), "Number of pending withdrawal requests.", )?; w.encode_gauge( diff --git a/minter/src/monitor/mod.rs b/minter/src/monitor/mod.rs index a054cc03..d6f8c549 100644 --- a/minter/src/monitor/mod.rs +++ b/minter/src/monitor/mod.rs @@ -67,30 +67,34 @@ pub async fn finalize_transactions(runtime: R) { /// Returns whether the finalization timer must run again immediately. async fn check_submitted_transactions(runtime: &R) -> bool { - let all_transactions: BTreeMap = read_state(|state| { - state - .submitted_transactions() - .iter() - .map(|(sig, tx)| (*sig, tx.block_height())) - .collect() + let (signatures, blockhash_transactions): ( + BTreeSet, + BTreeMap, + ) = read_state(|state| { + let submitted = state.submitted_transactions(); + ( + submitted.iter().map(|(sig, _)| *sig).collect(), + submitted + .iter() + .filter_map(|(sig, tx)| tx.block_height().map(|height| (*sig, height))) + .collect(), + ) }); - if all_transactions.is_empty() { + if signatures.is_empty() { return false; } // Fetch the current block before checking statuses: if a transaction finalizes // after we snapshot the block, the status check will see it as finalized rather // than missing, so it will never be incorrectly marked as expired. - let current_block = match get_recent_block(runtime).await { - Ok(block) => block, - Err(e) => { - log!(Priority::Info, "Failed to get current block: {e}"); - return true; - } + let current_block_height = if blockhash_transactions.is_empty() { + None + } else { + fetch_current_block_height(runtime).await }; - let signatures: Vec = all_transactions.keys().copied().collect(); - let statuses = check_transaction_statuses(runtime, signatures).await; + let num_transactions = signatures.len(); + let statuses = check_transaction_statuses(runtime, signatures.into_iter().collect()).await; for (signature, error) in &statuses.errored { log!( @@ -121,8 +125,42 @@ async fn check_submitted_transactions(runtime: &R) -> bool { }); } - for signature in &statuses.not_found { - if !is_blockhash_expired(all_transactions[signature], current_block.block_height) { + if let Some(current_block_height) = current_block_height { + expire_transactions( + runtime, + &statuses.not_found, + &blockhash_transactions, + current_block_height, + ); + } + + num_transactions > MAX_CONCURRENT_RPC_CALLS * MAX_SIGNATURES_PER_STATUS_CHECK +} + +async fn fetch_current_block_height(runtime: &R) -> Option { + match get_recent_block(runtime).await { + Ok(block) => Some(block.block_height), + Err(e) => { + log!( + Priority::Info, + "Failed to get current block, skipping the expiry check this round: {e}" + ); + None + } + } +} + +fn expire_transactions( + runtime: &R, + not_found: &BTreeSet, + blockhash_transactions: &BTreeMap, + current_block_height: BlockHeight, +) { + for signature in not_found { + let Some(transaction_block_height) = blockhash_transactions.get(signature) else { + continue; + }; + if !is_blockhash_expired(*transaction_block_height, current_block_height) { continue; } log!(Priority::Info, "Transaction {signature} expired"); @@ -136,8 +174,6 @@ async fn check_submitted_transactions(runtime: &R) -> bool { ) }); } - - all_transactions.len() > MAX_CONCURRENT_RPC_CALLS * MAX_SIGNATURES_PER_STATUS_CHECK } fn is_blockhash_expired( diff --git a/minter/src/monitor/tests.rs b/minter/src/monitor/tests.rs index 60270107..7e95d6ff 100644 --- a/minter/src/monitor/tests.rs +++ b/minter/src/monitor/tests.rs @@ -9,9 +9,9 @@ use crate::{ state::{TaskType, event::EventType, mutate_state, read_state, reset_state}, storage::reset_events, test_fixtures::{ - EventsAssert, MINIMUM_WITHDRAWAL_AMOUNT, account, confirmed_block_at_height, events, - init_balance, init_schnorr_master_key, init_state, minter_signature, minter_signature_nth, - runtime::TestCanisterRuntime, signature, + EventsAssert, MINIMUM_WITHDRAWAL_AMOUNT, account, confirmed_block_at_height, durable_nonce, + events, init_balance, init_schnorr_master_key, init_state, minter_signature, + minter_signature_nth, runtime::TestCanisterRuntime, signature, }, }; use sol_rpc_types::{ @@ -64,19 +64,33 @@ mod finalization { } #[tokio::test] - async fn should_return_early_if_fetching_current_block_fails() { + async fn should_finalize_but_not_expire_transactions_if_fetching_current_block_fails() { setup(); - submit_withdrawal_transaction(EXPIRED_BLOCK_HEIGHT); - - let events_before = EventsAssert::from_recorded(); + let finalized = submit_withdrawal_transaction_with_signature(1, EXPIRED_BLOCK_HEIGHT); + let not_found = submit_withdrawal_transaction_with_signature(2, EXPIRED_BLOCK_HEIGHT); let runtime = TestCanisterRuntime::new() - .add_recent_block(Err(RpcError::ValidationError("Error".to_string()))); + .with_increasing_time() + .add_recent_block(Err(RpcError::ValidationError("Error".to_string()))) + .add_stub_response(SignatureStatusesResult::Consistent(Ok(vec![ + Some(finalized_status()), + None, + ]))); finalize_transactions(runtime).await; - let events_after = EventsAssert::from_recorded(); - assert_eq!(events_before, events_after); + let events = EventsAssert::from_recorded().expect_contains_event_eq( + EventType::SucceededTransaction { + signature: finalized, + }, + ); + assert!(!events.contains_event(&EventType::ExpiredTransaction { + signature: not_found + })); + read_state(|s| { + assert!(s.submitted_transactions().contains_key(¬_found)); + assert!(s.transactions_to_resubmit().is_empty()); + }); } #[tokio::test] @@ -246,6 +260,52 @@ mod finalization { }); } + #[tokio::test] + async fn should_finalize_nonce_withdrawal_without_fetching_current_block() { + setup(); + let signature = submit_nonce_withdrawal_transaction(1); + + let runtime = TestCanisterRuntime::new() + .with_increasing_time() + .add_stub_response(SignatureStatusesResult::Consistent(Ok(vec![Some( + finalized_status(), + )]))); + + finalize_transactions(runtime).await; + + EventsAssert::from_recorded() + .expect_contains_event_eq(EventType::SucceededTransaction { signature }); + assert!(read_state(|s| s.submitted_transactions().is_empty())); + } + + #[tokio::test] + async fn should_never_expire_nonce_withdrawal_with_missing_status() { + setup(); + let blockhash_withdrawal = + submit_withdrawal_transaction_with_signature(1, EXPIRED_BLOCK_HEIGHT); + let nonce_withdrawal = submit_nonce_withdrawal_transaction(2); + + let runtime = TestCanisterRuntime::new() + .with_increasing_time() + .add_stub_response(SlotResult::Consistent(Ok(CURRENT_SLOT))) + .add_stub_response(BlockResult::Consistent(Ok(current_block()))) + .add_stub_response(SignatureStatusesResult::Consistent(Ok(vec![None, None]))); + + finalize_transactions(runtime).await; + + let events = + EventsAssert::from_recorded().expect_contains_event_eq(EventType::ExpiredTransaction { + signature: blockhash_withdrawal, + }); + assert!(!events.contains_event(&EventType::ExpiredTransaction { + signature: nonce_withdrawal + })); + read_state(|s| { + assert!(s.submitted_transactions().contains_key(&nonce_withdrawal)); + assert!(!s.transactions_to_resubmit().contains_key(&nonce_withdrawal)); + }); + } + struct ExpiryCase { name: &'static str, transaction_block_height: BlockHeight, @@ -400,7 +460,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(), Some(RESUBMISSION_BLOCK_HEIGHT)); }); } @@ -582,3 +642,11 @@ fn submit_withdrawal_transaction_with_signature( events::submit_withdrawal_at_height(signature, block_height, vec![i as u64]); signature } + +fn submit_nonce_withdrawal_transaction(i: usize) -> solana_signature::Signature { + let signature = signature(i); + events::accept_withdrawal(account(i), i as u64, MINIMUM_WITHDRAWAL_AMOUNT); + events::create_withdrawal_batch_transaction(durable_nonce(i), vec![i as u64]); + events::submit_withdrawal_batch_transaction(signature, durable_nonce(i), vec![i as u64]); + signature +} diff --git a/minter/src/sol_transfer/mod.rs b/minter/src/sol_transfer/mod.rs index 4a8793c3..a00af4c2 100644 --- a/minter/src/sol_transfer/mod.rs +++ b/minter/src/sol_transfer/mod.rs @@ -76,6 +76,32 @@ pub async fn sign_sweep_transaction( Ok((transaction, signers)) } +/// Builds the unsigned message of a batch withdrawal transaction: an +/// `AdvanceNonceAccount` instruction first, followed by one transfer from the +/// minter's main address per withdrawal request, carrying the nonce value in +/// place of a recent blockhash. The main address is the fee payer, the source +/// of all transfers, and the nonce authority, so the transaction has a single +/// signature. +pub fn build_batch_withdrawal_message( + minter_address: &Address, + nonce_account: &Address, + nonce_value: Hash, + transfers: &[(Address, Lamport)], +) -> Result { + let mut instructions = vec![instruction::advance_nonce_account( + nonce_account, + minter_address, + )]; + instructions.extend( + transfers + .iter() + .map(|(target, amount)| instruction::transfer(minter_address, target, *amount)), + ); + let message = Message::new_with_blockhash(&instructions, Some(minter_address), &nonce_value); + ensure_within_transaction_size(&message)?; + Ok(message) +} + /// Creates a signed Solana transaction that transfers lamports from a single /// minter-controlled address (the fee payer) to multiple target addresses. /// diff --git a/minter/src/sol_transfer/tests.rs b/minter/src/sol_transfer/tests.rs index e9a6217c..3ea350d7 100644 --- a/minter/src/sol_transfer/tests.rs +++ b/minter/src/sol_transfer/tests.rs @@ -292,3 +292,120 @@ mod batch_withdrawal_tests { ); } } + +mod batch_withdrawal_message_tests { + use super::*; + use solana_message::{MessageHeader, compiled_instruction::CompiledInstruction}; + use solana_sdk_ids::{system_program, sysvar::recent_blockhashes}; + + const NONCE_ACCOUNT: Address = Address::new_from_array([0x4E; 32]); + const FIRST_TARGET: Address = Address::new_from_array([0x01; 32]); + const SECOND_TARGET: Address = Address::new_from_array([0x02; 32]); + const MINTER_ADDRESS_INDEX: u8 = 0; + const NONCE_ACCOUNT_INDEX: u8 = 3; + const SYSTEM_PROGRAM_INDEX: u8 = 4; + const RECENT_BLOCKHASHES_INDEX: u8 = 5; + + #[test] + fn should_build_the_message_bound_to_the_nonce() { + let nonce_value = Hash::new_from_array([0xAA; 32]); + + let message = build_batch_withdrawal_message( + &MINTER_ADDRESS, + &NONCE_ACCOUNT, + nonce_value, + &[(SECOND_TARGET, 20_000_000), (FIRST_TARGET, 10_000_000)], + ) + .expect("message should fit in a transaction"); + + assert_eq!( + message, + Message { + header: MessageHeader { + num_required_signatures: 1, + num_readonly_signed_accounts: 0, + num_readonly_unsigned_accounts: 2, + }, + account_keys: vec![ + MINTER_ADDRESS, + FIRST_TARGET, + SECOND_TARGET, + NONCE_ACCOUNT, + system_program::ID, + recent_blockhashes::ID, + ], + recent_blockhash: nonce_value, + instructions: vec![ + advance_nonce_instruction(), + transfer_instruction(2, 20_000_000), + transfer_instruction(1, 10_000_000), + ], + } + ); + } + + #[test] + fn should_fit_max_withdrawals_with_distinct_targets() { + let message = build_batch_withdrawal_message( + &MINTER_ADDRESS, + &NONCE_ACCOUNT, + Hash::new_from_array([0xAA; 32]), + &distinct_transfers(MAX_WITHDRAWALS_PER_TX), + ) + .expect("message should fit in a transaction at max capacity"); + + assert_eq!(message.instructions.len(), MAX_WITHDRAWALS_PER_TX + 1); + } + + #[test] + fn should_reject_more_than_max_withdrawals_with_distinct_targets() { + let result = build_batch_withdrawal_message( + &MINTER_ADDRESS, + &NONCE_ACCOUNT, + Hash::new_from_array([0xAA; 32]), + &distinct_transfers(MAX_WITHDRAWALS_PER_TX + 1), + ); + + assert_matches!( + result, + Err(CreateTransferError::TransactionTooLarge { + max: MAX_TX_SIZE, + .. + }) + ); + } + + fn distinct_transfers(count: usize) -> Vec<(Address, Lamport)> { + (0..count) + .map(|i| { + let mut target = [0xF0; 32]; + target[0] = i as u8; + (Address::new_from_array(target), 1_000_000) + }) + .collect() + } + + fn advance_nonce_instruction() -> CompiledInstruction { + const ADVANCE_NONCE_ACCOUNT_DISCRIMINANT: u32 = 4; + CompiledInstruction { + program_id_index: SYSTEM_PROGRAM_INDEX, + accounts: vec![ + NONCE_ACCOUNT_INDEX, + RECENT_BLOCKHASHES_INDEX, + MINTER_ADDRESS_INDEX, + ], + data: ADVANCE_NONCE_ACCOUNT_DISCRIMINANT.to_le_bytes().to_vec(), + } + } + + fn transfer_instruction(to_index: u8, amount: Lamport) -> CompiledInstruction { + const TRANSFER_DISCRIMINANT: u32 = 2; + let mut data = TRANSFER_DISCRIMINANT.to_le_bytes().to_vec(); + data.extend_from_slice(&amount.to_le_bytes()); + CompiledInstruction { + program_id_index: SYSTEM_PROGRAM_INDEX, + accounts: vec![MINTER_ADDRESS_INDEX, to_index], + data, + } + } +} diff --git a/minter/src/state/audit.rs b/minter/src/state/audit.rs index 546a0aec..ca31dc6e 100644 --- a/minter/src/state/audit.rs +++ b/minter/src/state/audit.rs @@ -32,15 +32,8 @@ fn apply_state_transition(state: &mut State, payload: &EventType, timestamp: u64 message, signers, purpose, - block_height, } => { - state.process_transaction_submitted( - signature, - message, - signers, - purpose, - *block_height, - ); + state.process_transaction_submitted(signature, message, signers, purpose); } EventType::ResubmittedTransaction { old_signature, @@ -94,6 +87,13 @@ fn apply_state_transition(state: &mut State, payload: &EventType, timestamp: u64 EventType::QuarantinedPendingMint { deposit_id } => { state.process_quarantined_pending_mint(*deposit_id); } + EventType::CreatedWithdrawalTransaction { + burn_indices, + nonce_account, + nonce_value, + } => { + state.process_transaction_created(burn_indices, nonce_account, *nonce_value); + } } } diff --git a/minter/src/state/event.rs b/minter/src/state/event.rs index 96ac04ea..4e4e4da5 100644 --- a/minter/src/state/event.rs +++ b/minter/src/state/event.rs @@ -14,8 +14,11 @@ use icrc_ledger_types::icrc1::account::Account; use minicbor::{Decode, Encode}; use sol_rpc_types::Lamport; use solana_address::Address; +use solana_hash::Hash; use solana_message::Message; +use solana_sdk_ids::system_program; use solana_signature::Signature; +use solana_system_interface::instruction::SystemInstruction; use std::borrow::Cow; /// A versioned Solana transaction message, allowing the minter to support @@ -35,6 +38,29 @@ impl VersionedMessage { let VersionedMessage::Legacy(message) = self; FEE_PER_SIGNATURE * message.header.num_required_signatures as u64 } + + /// The nonce account advanced by the first instruction of the message, + /// or `None` if the first instruction is not a system-program + /// `AdvanceNonceAccount` instruction. + pub fn advanced_nonce_account(&self) -> Option
{ + let VersionedMessage::Legacy(message) = self; + let instruction = message.instructions.first()?; + let program_id = message + .account_keys + .get(usize::from(instruction.program_id_index))?; + let is_advance_nonce_account = matches!( + bincode::deserialize(&instruction.data), + Ok(SystemInstruction::AdvanceNonceAccount) + ); + if *program_id != system_program::ID || !is_advance_nonce_account { + return None; + } + let nonce_account_index = instruction.accounts.first()?; + message + .account_keys + .get(usize::from(*nonce_account_index)) + .copied() + } } mod cbor; @@ -73,13 +99,10 @@ pub enum EventType { /// The signers in signature order (fee payer first). #[n(2)] signers: Vec, - /// The purpose of this transaction. + /// The purpose of this transaction, with what the minter needs to + /// track it until it is finalized. #[n(3)] purpose: TransactionPurpose, - /// The block height of the block whose blockhash the transaction uses. - /// The blockhash is valid for 150 blocks after that height. - #[n(4)] - block_height: BlockHeight, }, /// A previously submitted transaction was resubmitted with a new signature. /// The transaction message and signers remain the same. @@ -196,6 +219,24 @@ pub enum EventType { #[n(0)] deposit_id: DepositSolId, }, + /// The minter bound a durable nonce account and its nonce value to the + /// withdrawal requests of the given burn indices, before requesting the + /// threshold signature. Since each burn index identifies a withdrawal + /// request, the binding determines the transaction message, so that a + /// signing failure leads to re-signing the identical message and never to + /// a second message being signed for the same nonce value. + #[n(14)] + CreatedWithdrawalTransaction { + /// The ledger burn indices of the withdrawal requests served by this transaction. + #[cbor(n(0), with = "cbor::id_vec")] + burn_indices: Vec, + /// The durable nonce account bound to this transaction. + #[cbor(n(1), with = "cbor::address")] + nonce_account: Address, + /// The nonce value the transaction carries in place of a recent blockhash. + #[cbor(n(2), with = "cbor::hash")] + nonce_value: Hash, + }, } /// The mint enqueued for one deposit of a `CreditedSweep` event. @@ -250,21 +291,45 @@ impl Signer { } } +/// The purpose of a submitted transaction, mirroring [`MinterTransaction`]: +/// each variant carries what the minter needs to track the transaction. +/// +/// [`MinterTransaction`]: crate::state::MinterTransaction #[derive(Clone, Eq, PartialEq, Debug, Decode, Encode)] pub enum TransactionPurpose { - /// Withdraw SOL to users' Solana addresses. - #[n(0)] - WithdrawSol { + /// Sweep the deposit addresses of deposits queued by `deposit_sol` into + /// the minter's main account. The transaction uses a recent blockhash and + /// is dropped once the blockhash expires. + #[n(2)] + SweepDeposit { + /// The ids of the swept deposits. + #[n(0)] + deposit_ids: Vec, + /// The block height of the block whose blockhash the transaction uses. + /// The blockhash is valid for 150 blocks after that height. + #[n(1)] + block_height: BlockHeight, + }, + /// Withdraw SOL to users' Solana addresses. The transaction uses a recent + /// blockhash and is resubmitted once the blockhash expires. + #[n(3)] + Withdrawal { /// The ledger burn indices of the withdrawal requests included in this transaction. #[cbor(n(0), with = "cbor::id_vec")] burn_indices: Vec, + /// The block height of the block whose blockhash the transaction uses. + /// The blockhash is valid for 150 blocks after that height. + #[n(1)] + block_height: BlockHeight, }, - /// Sweep the deposit addresses of deposits queued by `deposit_sol` into the minter's main account. - #[n(1)] - SweepDeposits { - /// The ids of the swept deposits. - #[n(0)] - deposit_ids: Vec, + /// Withdraw SOL to users' Solana addresses. The transaction carries the + /// nonce value of a durable nonce account instead of a recent blockhash, + /// so it never expires. + #[n(4)] + NonceWithdrawal { + /// The ledger burn indices of the withdrawal requests included in this transaction. + #[cbor(n(0), with = "cbor::id_vec")] + burn_indices: Vec, }, } diff --git a/minter/src/state/event/cbor/mod.rs b/minter/src/state/event/cbor/mod.rs index 2e74bb50..4a310196 100644 --- a/minter/src/state/event/cbor/mod.rs +++ b/minter/src/state/event/cbor/mod.rs @@ -97,6 +97,31 @@ pub mod ed25519_public_key { } } +pub mod hash { + use minicbor::{ + decode::{Decoder, Error}, + encode::{Encoder, Write}, + }; + use solana_hash::{HASH_BYTES, Hash}; + + pub fn decode(d: &mut Decoder<'_>, _ctx: &mut Ctx) -> Result { + let bytes: [u8; HASH_BYTES] = d + .bytes()? + .try_into() + .map_err(|_| Error::message(format!("expected {HASH_BYTES} hash bytes")))?; + Ok(Hash::from(bytes)) + } + + pub fn encode( + v: &Hash, + e: &mut Encoder, + _ctx: &mut Ctx, + ) -> Result<(), minicbor::encode::Error> { + e.bytes(v.as_ref())?; + Ok(()) + } +} + pub mod deposit_balance { use crate::state::DepositBalance; use minicbor::{ diff --git a/minter/src/state/event/cbor/tests.rs b/minter/src/state/event/cbor/tests.rs index c8c2365b..21a7623f 100644 --- a/minter/src/state/event/cbor/tests.rs +++ b/minter/src/state/event/cbor/tests.rs @@ -141,6 +141,32 @@ mod ed25519_public_key_tests { } } +mod hash_tests { + use super::*; + use crate::test_fixtures::arb::arb_hash; + + proptest! { + #[test] + fn hash_minicbor_roundtrip(hash in arb_hash()) { + let encoded = encode_hash(&hash); + let decoded = decode_hash(&encoded); + prop_assert_eq!(hash, decoded); + } + } + + fn encode_hash(hash: &solana_hash::Hash) -> Vec { + let mut buf = Vec::new(); + let mut encoder = minicbor::Encoder::new(&mut buf); + cbor::hash::encode(hash, &mut encoder, &mut ()).unwrap(); + buf + } + + fn decode_hash(bytes: &[u8]) -> solana_hash::Hash { + let mut decoder = minicbor::Decoder::new(bytes); + cbor::hash::decode(&mut decoder, &mut ()).unwrap() + } +} + mod message_tests { use super::*; diff --git a/minter/src/state/mod.rs b/minter/src/state/mod.rs index 818c0b40..942e5ab9 100644 --- a/minter/src/state/mod.rs +++ b/minter/src/state/mod.rs @@ -4,7 +4,10 @@ use crate::{ ledger::client::LedgerClient, numeric::{LedgerBurnIndex, LedgerMintIndex}, rpc::BlockHeight, - sol_transfer::{BATCH_WITHDRAWAL_TX_FEE, MAX_SIGNATURES, MAX_WITHDRAWALS_PER_TX}, + sol_transfer::{ + BATCH_WITHDRAWAL_TX_FEE, MAX_SIGNATURES, MAX_WITHDRAWALS_PER_TX, + build_batch_withdrawal_message, + }, state::event::{ CreditedDeposit, Signer, TransactionPurpose, VersionedMessage, WithdrawalRequest, }, @@ -20,6 +23,8 @@ use icrc_ledger_types::icrc1::account::Account; use sol_rpc_client::SolRpcClient; use sol_rpc_types::{ConsensusStrategy, Lamport, RpcSources, SolanaCluster}; use solana_address::Address; +use solana_hash::Hash; +use solana_message::Message; use solana_signature::Signature; use std::{ cell::RefCell, @@ -108,10 +113,12 @@ pub struct State { pending_withdrawal_request_guards: BTreeSet, deposits: Deposits, pending_withdrawal_requests: BTreeMap, + created_withdrawal_requests: BTreeMap, sent_withdrawal_requests: BTreeMap, successful_withdrawal_requests: BTreeMap, failed_withdrawal_requests: BTreeMap, submitted_transactions: InsertionOrderedMap, + created_withdrawal_txs: BTreeMap, transactions_to_resubmit: InsertionOrderedMap, succeeded_transactions: BTreeSet, failed_transactions: InsertionOrderedMap, @@ -200,6 +207,10 @@ impl State { &self.submitted_transactions } + pub fn created_withdrawal_txs(&self) -> &BTreeMap { + &self.created_withdrawal_txs + } + pub fn transactions_to_resubmit(&self) -> &InsertionOrderedMap { &self.transactions_to_resubmit } @@ -227,6 +238,9 @@ impl State { .is_none(), "BUG: transaction {signature} is already queued for resubmission" ), + MinterTransaction::NonceWithdrawal { .. } => { + panic!("BUG: durable-nonce withdrawal transaction {signature} cannot expire") + } } } @@ -374,6 +388,7 @@ impl State { let incomplete_destinations: BTreeSet
= self .pending_withdrawal_requests .values() + .chain(self.created_withdrawal_requests.values()) .map(|pending| &pending.request) .chain( self.sent_withdrawal_requests @@ -454,7 +469,9 @@ impl State { pub fn withdrawal_status(&self, block_index: u64) -> WithdrawalStatus { let burn_index = LedgerBurnIndex::from(block_index); - if self.pending_withdrawal_requests.contains_key(&burn_index) { + if self.pending_withdrawal_requests.contains_key(&burn_index) + || self.created_withdrawal_requests.contains_key(&burn_index) + { return WithdrawalStatus::Pending; } if let Some(sent) = self.sent_withdrawal_requests.get(&burn_index) { @@ -482,6 +499,12 @@ impl State { &self.pending_withdrawal_requests } + pub fn created_withdrawal_requests( + &self, + ) -> &BTreeMap { + &self.created_withdrawal_requests + } + pub fn withdrawal_batches(&self) -> WithdrawalBatches<'_> { WithdrawalBatches { pending_requests: self.pending_withdrawal_requests.values().peekable(), @@ -496,8 +519,12 @@ impl State { .pending_withdrawal_requests .values() .map(|r| r.created_at); + let created = self + .created_withdrawal_requests + .values() + .map(|r| r.created_at); let sent = self.sent_withdrawal_requests.values().map(|r| r.created_at); - pending.chain(sent).min() + pending.chain(created).chain(sent).min() } fn process_accepted_withdrawal(&mut self, request: &WithdrawalRequest, created_at: u64) { @@ -521,7 +548,6 @@ impl State { transaction: &VersionedMessage, signers: &[Signer], purpose: &TransactionPurpose, - block_height: BlockHeight, ) { assert!( !self.succeeded_transactions.contains(signature), @@ -534,7 +560,10 @@ impl State { let message = transaction.clone(); let signers = signers.to_vec(); let submitted_transaction = match purpose { - TransactionPurpose::WithdrawSol { burn_indices } => { + TransactionPurpose::Withdrawal { + burn_indices, + block_height, + } => { let mut total: Lamport = 0; for burn_index in burn_indices { let pending = self @@ -565,10 +594,16 @@ impl State { MinterTransaction::Withdrawal { message, signers, - block_height, + block_height: *block_height, } } - TransactionPurpose::SweepDeposits { deposit_ids } => { + TransactionPurpose::NonceWithdrawal { burn_indices } => { + self.send_nonce_withdrawal(signature, message, signers, burn_indices) + } + TransactionPurpose::SweepDeposit { + deposit_ids, + block_height, + } => { let sweep_destination = minter_address(self.minter_public_key.as_ref().expect( "BUG: a sweep was submitted before the minter public key was recorded", )); @@ -577,7 +612,7 @@ impl State { MinterTransaction::SweepDeposit { message, signers, - block_height, + block_height: *block_height, } } }; @@ -589,6 +624,151 @@ impl State { ); } + fn process_transaction_created( + &mut self, + burn_indices: &[LedgerBurnIndex], + nonce_account: &Address, + nonce_value: Hash, + ) { + self.nonce_pool.bind(nonce_account, nonce_value); + let mut total: Lamport = 0; + for burn_index in burn_indices { + let pending = self + .pending_withdrawal_requests + .remove(burn_index) + .unwrap_or_else(|| { + panic!( + "Attempted to create transaction for unknown withdrawal request: {burn_index:?}" + ) + }); + total = total + .checked_add(pending.request.amount_to_transfer) + .expect("BUG: total amount of a withdrawal transaction overflows"); + assert_eq!( + self.created_withdrawal_requests + .insert(*burn_index, pending), + None, + "Attempted to create transaction for already created withdrawal request: {burn_index:?}" + ); + } + self.balance = self + .balance + .checked_sub(total + BATCH_WITHDRAWAL_TX_FEE) + .expect("BUG: insufficient minter balance for withdrawal"); + assert_eq!( + self.created_withdrawal_txs.insert( + *nonce_account, + CreatedWithdrawalTransaction { + nonce_value, + burn_indices: burn_indices.to_vec(), + } + ), + None, + "BUG: nonce account {nonce_account} already has a created transaction" + ); + } + + fn send_nonce_withdrawal( + &mut self, + signature: &Signature, + message: VersionedMessage, + signers: Vec, + burn_indices: &[LedgerBurnIndex], + ) -> MinterTransaction { + let nonce_account = message.advanced_nonce_account().unwrap_or_else(|| { + panic!("BUG: withdrawal transaction {signature} does not start with an AdvanceNonceAccount instruction") + }); + let created = self + .created_withdrawal_txs + .remove(&nonce_account) + .unwrap_or_else(|| { + panic!( + "BUG: no withdrawal transaction was created for nonce account {nonce_account}" + ) + }); + assert_eq!( + burn_indices, created.burn_indices, + "BUG: withdrawal transaction {signature} does not serve the withdrawal requests bound to nonce account {nonce_account}" + ); + assert_eq!( + message, + VersionedMessage::Legacy(self.bound_withdrawal_message(&nonce_account, &created)), + "BUG: withdrawal transaction {signature} does not carry the message bound to nonce account {nonce_account}" + ); + assert_eq!( + signers, + [Signer::Minter], + "BUG: withdrawal transaction {signature} must be signed by the minter only" + ); + for burn_index in &created.burn_indices { + let pending = self + .created_withdrawal_requests + .remove(burn_index) + .unwrap_or_else(|| { + panic!( + "BUG: withdrawal request {burn_index:?} of a created transaction is not in the created bucket" + ) + }); + assert_eq!( + self.sent_withdrawal_requests.insert( + *burn_index, + SentWithdrawalRequest { + request: pending.request, + signature: *signature, + created_at: pending.created_at, + }, + ), + None, + "Attempted to send transaction for already sent withdrawal request: {burn_index:?}" + ); + } + MinterTransaction::NonceWithdrawal { + message, + signers, + nonce_account, + nonce_value: created.nonce_value, + } + } + + fn bound_withdrawal_message( + &self, + nonce_account: &Address, + created: &CreatedWithdrawalTransaction, + ) -> Message { + let minter_address = + minter_address(self.minter_public_key.as_ref().expect( + "BUG: a withdrawal was submitted before the minter public key was recorded", + )); + let transfers: Vec<(Address, Lamport)> = created + .burn_indices + .iter() + .map(|burn_index| { + let request = &self + .created_withdrawal_requests + .get(burn_index) + .unwrap_or_else(|| { + panic!( + "BUG: withdrawal request {burn_index:?} of a created transaction is not in the created bucket" + ) + }) + .request; + ( + Address::from(request.solana_address), + request.amount_to_transfer, + ) + }) + .collect(); + build_batch_withdrawal_message( + &minter_address, + nonce_account, + created.nonce_value, + &transfers, + ) + .unwrap_or_else(|e| { + panic!("BUG: cannot rebuild the withdrawal message bound to nonce account {nonce_account}: {e}") + }) + } + fn process_transaction_resubmitted( &mut self, old_signature: &Signature, @@ -614,6 +794,9 @@ impl State { signers, block_height: new_block_height, }, + MinterTransaction::NonceWithdrawal { .. } => panic!( + "BUG: durable-nonce withdrawal transaction {old_signature} must never be resubmitted" + ), }; assert!( !self.succeeded_transactions.contains(new_signature), @@ -650,6 +833,9 @@ impl State { match transaction { MinterTransaction::SweepDeposit { .. } => self.deposits.finalize_swept(signature), MinterTransaction::Withdrawal { .. } => {} + MinterTransaction::NonceWithdrawal { nonce_account, .. } => { + self.nonce_pool.free(&nonce_account) + } } assert!( !self.transactions_to_resubmit.contains_key(signature), @@ -677,19 +863,22 @@ impl State { .unwrap_or_else(|| { panic!("Attempted to mark unknown transaction {signature:?} as failed") }); - assert!( - !self.transactions_to_resubmit.contains_key(signature), - "BUG: transaction {signature} is queued for resubmission but is being marked as failed" - ); - match transaction { + match &transaction { MinterTransaction::SweepDeposit { .. } => self.deposits.drop_swept(signature), MinterTransaction::Withdrawal { .. } => {} + MinterTransaction::NonceWithdrawal { nonce_account, .. } => { + self.nonce_pool.free(nonce_account) + } } assert_eq!( self.failed_transactions.insert(*signature, transaction), None, "Attempted to fail transaction {signature:?} twice" ); + assert!( + !self.transactions_to_resubmit.contains_key(signature), + "BUG: transaction {signature} is queued for resubmission but is being marked as failed" + ); self.sent_withdrawal_requests .extract_if(.., |_, sent| &sent.signature == signature) .for_each(|(burn_index, sent)| { @@ -780,10 +969,12 @@ impl TryFrom for State { pending_withdrawal_request_guards: BTreeSet::new(), deposits: Deposits::default(), pending_withdrawal_requests: BTreeMap::new(), + created_withdrawal_requests: BTreeMap::new(), sent_withdrawal_requests: BTreeMap::new(), successful_withdrawal_requests: BTreeMap::new(), failed_withdrawal_requests: BTreeMap::new(), submitted_transactions: InsertionOrderedMap::new(), + created_withdrawal_txs: BTreeMap::new(), transactions_to_resubmit: InsertionOrderedMap::new(), succeeded_transactions: BTreeSet::new(), failed_transactions: InsertionOrderedMap::new(), @@ -843,6 +1034,17 @@ impl Iterator for WithdrawalBatches<'_> { } } +/// The binding of a durable nonce account and its nonce value to the +/// withdrawal requests of a transaction whose threshold signature was not yet +/// recorded. The binding determines the transaction message, so that a signing +/// failure leads to re-signing the identical message instead of building a new +/// one for the same nonce value. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct CreatedWithdrawalTransaction { + pub nonce_value: Hash, + pub burn_indices: Vec, +} + /// A withdrawal request that has been submitted in a Solana transaction. #[derive(Clone, Debug, PartialEq, Eq)] pub struct SentWithdrawalRequest { @@ -880,27 +1082,42 @@ pub enum MinterTransaction { /// The block height of the block whose blockhash the transaction uses. block_height: BlockHeight, }, + /// A durable-nonce withdrawal transaction, which never expires: it stays + /// in flight until it is finalized. + NonceWithdrawal { + message: VersionedMessage, + signers: Vec, + /// The durable nonce account whose nonce value the transaction uses. + nonce_account: Address, + /// The durable nonce value the transaction uses instead of a recent blockhash. + nonce_value: Hash, + }, } impl MinterTransaction { pub fn message(&self) -> &VersionedMessage { match self { MinterTransaction::SweepDeposit { message, .. } - | MinterTransaction::Withdrawal { message, .. } => message, + | MinterTransaction::Withdrawal { message, .. } + | MinterTransaction::NonceWithdrawal { message, .. } => message, } } pub fn signers(&self) -> &[Signer] { match self { MinterTransaction::SweepDeposit { signers, .. } - | MinterTransaction::Withdrawal { signers, .. } => signers, + | MinterTransaction::Withdrawal { signers, .. } + | MinterTransaction::NonceWithdrawal { signers, .. } => signers, } } - pub fn block_height(&self) -> BlockHeight { + /// The block height of the block whose blockhash the transaction uses, + /// or `None` for a durable-nonce transaction, which never expires. + pub fn block_height(&self) -> Option { match self { MinterTransaction::SweepDeposit { block_height, .. } - | MinterTransaction::Withdrawal { block_height, .. } => *block_height, + | MinterTransaction::Withdrawal { block_height, .. } => Some(*block_height), + MinterTransaction::NonceWithdrawal { .. } => None, } } } diff --git a/minter/src/state/nonce_pool/mod.rs b/minter/src/state/nonce_pool/mod.rs index da1249e0..8085436d 100644 --- a/minter/src/state/nonce_pool/mod.rs +++ b/minter/src/state/nonce_pool/mod.rs @@ -1,5 +1,6 @@ use solana_address::Address; -use std::collections::BTreeMap; +use solana_hash::Hash; +use std::collections::{BTreeMap, BTreeSet}; #[cfg(test)] mod tests; @@ -12,7 +13,7 @@ mod tests; /// in which case no withdrawal transaction can be submitted. #[derive(Debug, Default, PartialEq, Eq)] pub struct DurableNoncePool { - accounts: BTreeMap, + accounts: BTreeMap, } impl DurableNoncePool { @@ -31,7 +32,7 @@ impl DurableNoncePool { let mut additions = BTreeMap::new(); for address in addresses { if self.accounts.contains_key(&address) - || additions.insert(address, NonceAccountState::Free).is_some() + || additions.insert(address, NonceAccount::default()).is_some() { return Err(NoncePoolError::DuplicateAccount(address)); } @@ -40,6 +41,41 @@ impl DurableNoncePool { Ok(()) } + /// Binds the account to an in-flight withdrawal transaction carrying + /// `nonce_value`, recording the value as seen. + /// + /// # Panics + /// Panics if the account is already bound or if the nonce value was + /// already bound to an earlier transaction of this account. + pub(super) fn bind(&mut self, address: &Address, nonce_value: Hash) { + let account = self.account_mut(address); + match account.state { + NonceAccountState::Free => {} + NonceAccountState::Bound => { + panic!("BUG: nonce account {address} is already bound to an in-flight transaction") + } + } + assert!( + account.seen_nonce_values.insert(nonce_value), + "BUG: nonce value {nonce_value} of account {address} was already seen" + ); + account.state = NonceAccountState::Bound; + } + + /// Releases the account once its in-flight transaction has landed. + /// + /// # Panics + /// Panics if the account is not bound. + pub(super) fn free(&mut self, address: &Address) { + let account = self.account_mut(address); + match account.state { + NonceAccountState::Bound => account.state = NonceAccountState::Free, + NonceAccountState::Free => { + panic!("BUG: cannot free nonce account {address} that is not bound") + } + } + } + pub fn addresses(&self) -> impl Iterator { self.accounts.keys() } @@ -51,13 +87,32 @@ impl DurableNoncePool { pub fn is_empty(&self) -> bool { self.accounts.is_empty() } + + fn account_mut(&mut self, address: &Address) -> &mut NonceAccount { + self.accounts + .get_mut(address) + .unwrap_or_else(|| panic!("BUG: nonce account {address} is not in the pool")) + } +} + +/// A durable nonce account of the pool with the nonce values the minter has +/// bound to transactions of this account, which classify every nonce read: +/// a never-seen value is the account's new frontier, while a seen value is +/// either the in-flight binding or a stale response. +#[derive(Debug, Default, PartialEq, Eq)] +struct NonceAccount { + state: NonceAccountState, + seen_nonce_values: BTreeSet, } /// The lifecycle state of a durable nonce account in the pool. -#[derive(Clone, Copy, Debug, PartialEq, Eq)] +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] enum NonceAccountState { /// The account is not bound to any in-flight withdrawal transaction. + #[default] Free, + /// The account is bound to an in-flight withdrawal transaction. + Bound, } #[derive(Clone, Debug, PartialEq, Eq)] diff --git a/minter/src/state/nonce_pool/tests.rs b/minter/src/state/nonce_pool/tests.rs index ed29eb86..16d1a2b3 100644 --- a/minter/src/state/nonce_pool/tests.rs +++ b/minter/src/state/nonce_pool/tests.rs @@ -1,6 +1,6 @@ use crate::{ state::nonce_pool::{DurableNoncePool, NoncePoolError}, - test_fixtures::address, + test_fixtures::{address, durable_nonce}, }; use solana_address::Address; @@ -32,6 +32,42 @@ fn should_leave_the_pool_unchanged_when_an_add_fails() { assert_eq!(pool, pool_of([address(1)])); } +#[test] +#[should_panic(expected = "already bound")] +fn should_panic_when_binding_a_bound_account() { + let mut pool = pool_of([address(1)]); + pool.bind(&address(1), durable_nonce(1)); + + pool.bind(&address(1), durable_nonce(2)); +} + +#[test] +#[should_panic(expected = "already seen")] +fn should_panic_when_binding_a_seen_nonce_value() { + let mut pool = pool_of([address(1)]); + pool.bind(&address(1), durable_nonce(1)); + pool.free(&address(1)); + + pool.bind(&address(1), durable_nonce(1)); +} + +#[test] +fn should_bind_a_freed_account_to_a_new_nonce_value() { + let mut pool = pool_of([address(1)]); + pool.bind(&address(1), durable_nonce(1)); + pool.free(&address(1)); + + pool.bind(&address(1), durable_nonce(2)); +} + +#[test] +#[should_panic(expected = "not bound")] +fn should_panic_when_freeing_an_unbound_account() { + let mut pool = pool_of([address(1)]); + + pool.free(&address(1)); +} + fn pool_of(addresses: impl IntoIterator) -> DurableNoncePool { DurableNoncePool::new(addresses).expect("the addresses are pairwise distinct") } diff --git a/minter/src/state/tests.rs b/minter/src/state/tests.rs index 494e233d..0dc8a252 100644 --- a/minter/src/state/tests.rs +++ b/minter/src/state/tests.rs @@ -10,9 +10,10 @@ use crate::{ arb::arb_event, deposit_id, events::{ - accept_withdrawal, accept_withdrawal_at, credit_sweep, expire_transaction, - fail_transaction, queue_deposit, resubmit_transaction, submit_sweep, submit_withdrawal, - succeed_transaction, + accept_withdrawal, accept_withdrawal_at, create_withdrawal_batch_transaction, + credit_sweep, expire_transaction, fail_transaction, queue_deposit, + resubmit_transaction, submit_sweep, submit_withdrawal, + submit_withdrawal_batch_transaction, succeed_transaction, }, init_balance, init_schnorr_master_key, init_state, ledger_canister_id, planned_sweep, queued_deposit, @@ -394,17 +395,17 @@ mod swept_deposits { &signature(SWEEP_SIGNATURE_INDEX), &sweep_message([(0, queued_deposit(0))]), &[Signer::Account(queued_deposit(0).account)], - &TransactionPurpose::SweepDeposits { + &TransactionPurpose::SweepDeposit { deposit_ids: vec![0], + block_height: DEFAULT_BLOCK_HEIGHT, }, - DEFAULT_BLOCK_HEIGHT, ); } } mod nonce_accounts { use super::*; - use crate::state::audit::replay_events; + use crate::{state::audit::replay_events, test_fixtures::durable_nonce}; #[test] fn should_only_add_a_nonce_account_once_no_incomplete_withdrawal_targets_it() { @@ -428,7 +429,15 @@ mod nonce_accounts { )) ); - submit_withdrawal(signature(1), vec![0]); + create_withdrawal_batch_transaction(durable_nonce(1), vec![0]); + assert_eq!( + add_destination(), + Err(InvalidStateError::NonceAccountIsWithdrawalDestination( + destination + )) + ); + + submit_withdrawal_batch_transaction(signature(1), durable_nonce(1), vec![0]); assert_eq!( add_destination(), Err(InvalidStateError::NonceAccountIsWithdrawalDestination( @@ -714,10 +723,12 @@ mod state_from_init_args { pending_withdrawal_request_guards: BTreeSet::new(), deposits: Deposits::default(), pending_withdrawal_requests: BTreeMap::new(), + created_withdrawal_requests: BTreeMap::new(), sent_withdrawal_requests: BTreeMap::new(), successful_withdrawal_requests: BTreeMap::new(), failed_withdrawal_requests: BTreeMap::new(), submitted_transactions: InsertionOrderedMap::new(), + created_withdrawal_txs: BTreeMap::new(), transactions_to_resubmit: InsertionOrderedMap::new(), succeeded_transactions: BTreeSet::new(), failed_transactions: InsertionOrderedMap::new(), @@ -901,7 +912,6 @@ fn should_track_balance_through_deposits_withdrawals_and_failures() { message: message_with_signers(num_signers).into(), signers, purpose, - block_height: BlockHeight::new(0), }, &TestCanisterRuntime::new().add_times([0, 0]), ) @@ -936,8 +946,9 @@ fn should_track_balance_through_deposits_withdrawals_and_failures() { submit_transaction( signature(0xBB), 1, - TransactionPurpose::WithdrawSol { + TransactionPurpose::Withdrawal { burn_indices: vec![0.into(), 1.into()], + block_height: BlockHeight::new(0), }, ); let expected = expected - TRANSFER_1 - TRANSFER_2 - FEE_PER_SIGNATURE; @@ -1231,3 +1242,334 @@ mod withdrawal_batches { } } } + +mod withdrawal_transactions { + use super::*; + use crate::{ + sol_transfer::BATCH_WITHDRAWAL_TX_FEE, + state::audit::replay_events, + test_fixtures::{ + MINTER_ADDRESS, NONCE_ACCOUNT, durable_nonce, + events::{create_withdrawal_batch_transaction, submit_withdrawal_batch_transaction}, + minter_public_key_fetched_event, queued_deposit_of, sweep_message, + withdrawal_batch_message, + }, + }; + use cksol_types::{TxFinalizedStatus, WithdrawalStatus}; + + const AMOUNT_TO_TRANSFER: u64 = MINIMUM_WITHDRAWAL_AMOUNT - WITHDRAWAL_FEE; + + #[test] + fn should_debit_the_balance_and_bucket_the_requests_at_creation() { + init_state(); + init_balance(); + let balance_before = read_state(|s| s.balance()); + accept_withdrawal(account(1), 0, MINIMUM_WITHDRAWAL_AMOUNT); + + create_withdrawal_batch_transaction(durable_nonce(1), vec![0]); + + read_state(|s| { + assert_eq!( + s.balance(), + balance_before - AMOUNT_TO_TRANSFER - BATCH_WITHDRAWAL_TX_FEE + ); + assert!(s.pending_withdrawal_requests().is_empty()); + assert!(s.created_withdrawal_requests().contains_key(&0_u64.into())); + assert!(s.created_withdrawal_txs().contains_key(&NONCE_ACCOUNT)); + assert_eq!(s.withdrawal_status(0), WithdrawalStatus::Pending); + }); + } + + #[test] + fn should_move_the_created_transaction_in_flight_once_submitted() { + init_state(); + init_balance(); + accept_withdrawal(account(1), 0, MINIMUM_WITHDRAWAL_AMOUNT); + create_withdrawal_batch_transaction(durable_nonce(1), vec![0]); + let balance_after_creation = read_state(|s| s.balance()); + + submit_withdrawal_batch_transaction(signature(7), durable_nonce(1), vec![0]); + + read_state(|s| { + assert_eq!(s.balance(), balance_after_creation); + assert!(s.created_withdrawal_txs().is_empty()); + assert!(s.created_withdrawal_requests().is_empty()); + let transaction = s.submitted_transactions().get(&signature(7)).unwrap(); + let MinterTransaction::NonceWithdrawal { + nonce_account, + nonce_value, + .. + } = transaction + else { + panic!("expected a withdrawal transaction, got {transaction:?}"); + }; + assert_eq!(*nonce_account, NONCE_ACCOUNT); + assert_eq!(*nonce_value, durable_nonce(1)); + assert_eq!( + s.withdrawal_status(0), + WithdrawalStatus::TxSent { + transaction_id: signature(7).into() + } + ); + }); + } + + #[test] + fn should_free_the_nonce_account_and_settle_the_request_on_success() { + init_state(); + init_balance(); + accept_withdrawal(account(1), 0, MINIMUM_WITHDRAWAL_AMOUNT); + create_withdrawal_batch_transaction(durable_nonce(1), vec![0]); + submit_withdrawal_batch_transaction(signature(7), durable_nonce(1), vec![0]); + + succeed_transaction(signature(7)); + + read_state(|s| { + assert!(s.submitted_transactions().is_empty()); + assert_eq!( + s.withdrawal_status(0), + WithdrawalStatus::TxFinalized(TxFinalizedStatus::Success { + transaction_id: signature(7).into(), + effective_transaction_fee: None, + }) + ); + }); + assert_nonce_account_free(); + } + + #[test] + fn should_free_the_nonce_account_and_settle_the_request_on_failure() { + init_state(); + init_balance(); + accept_withdrawal(account(1), 0, MINIMUM_WITHDRAWAL_AMOUNT); + create_withdrawal_batch_transaction(durable_nonce(1), vec![0]); + submit_withdrawal_batch_transaction(signature(7), durable_nonce(1), vec![0]); + + fail_transaction(signature(7)); + + read_state(|s| { + assert!(s.submitted_transactions().is_empty()); + assert_matches!( + s.failed_transactions().get(&signature(7)), + Some(MinterTransaction::NonceWithdrawal { .. }) + ); + assert_eq!( + s.withdrawal_status(0), + WithdrawalStatus::TxFinalized(TxFinalizedStatus::Failure { + transaction_id: signature(7).into(), + }) + ); + }); + assert_nonce_account_free(); + } + + #[test] + fn should_rebuild_the_bound_pool_and_created_bucket_from_a_log_ending_after_creation() { + let replayed = replay_events(log_of(funded_log_until_created_transaction())); + + assert_eq!( + replayed.balance(), + DEPOSIT_AMOUNT - FEE_PER_SIGNATURE - AMOUNT_TO_TRANSFER - BATCH_WITHDRAWAL_TX_FEE + ); + assert!( + replayed + .created_withdrawal_requests() + .contains_key(&0_u64.into()) + ); + assert_eq!( + replayed + .created_withdrawal_txs() + .get(&NONCE_ACCOUNT) + .map(|tx| tx.nonce_value), + Some(durable_nonce(1)) + ); + assert_eq!(replayed.withdrawal_status(0), WithdrawalStatus::Pending); + assert_eq!(replayed.nonce_pool, pool_bound_to(durable_nonce(1))); + } + + #[test] + fn should_rebuild_the_in_flight_map_from_a_log_ending_after_submission() { + let mut events = funded_log_until_created_transaction(); + events.push(EventType::SubmittedTransaction { + signature: signature(7), + message: withdrawal_batch_message( + NONCE_ACCOUNT, + durable_nonce(1), + &[(Address::from([0u8; 32]), AMOUNT_TO_TRANSFER)], + ) + .into(), + signers: vec![Signer::Minter], + purpose: TransactionPurpose::NonceWithdrawal { + burn_indices: vec![0_u64.into()], + }, + }); + + let replayed = replay_events(log_of(events)); + + assert!(replayed.created_withdrawal_txs().is_empty()); + let transaction = replayed + .submitted_transactions() + .get(&signature(7)) + .unwrap(); + let MinterTransaction::NonceWithdrawal { + nonce_account, + nonce_value, + .. + } = transaction + else { + panic!("expected a withdrawal transaction, got {transaction:?}"); + }; + assert_eq!(*nonce_account, NONCE_ACCOUNT); + assert_eq!(*nonce_value, durable_nonce(1)); + assert_eq!( + replayed.withdrawal_status(0), + WithdrawalStatus::TxSent { + transaction_id: signature(7).into() + } + ); + } + + #[test] + #[should_panic(expected = "does not start with an AdvanceNonceAccount instruction")] + fn should_panic_when_a_nonce_withdrawal_does_not_advance_a_nonce_account() { + init_state(); + init_balance(); + accept_withdrawal(account(1), 0, MINIMUM_WITHDRAWAL_AMOUNT); + create_withdrawal_batch_transaction(durable_nonce(1), vec![0]); + let message_without_nonce_advance = solana_message::Message::new_with_blockhash( + &[solana_system_interface::instruction::transfer( + &MINTER_ADDRESS, + &NONCE_ACCOUNT, + AMOUNT_TO_TRANSFER, + )], + Some(&MINTER_ADDRESS), + &durable_nonce(1), + ); + + mutate_state(|s| { + s.process_transaction_submitted( + &signature(7), + &message_without_nonce_advance.into(), + &[Signer::Minter], + &TransactionPurpose::NonceWithdrawal { + burn_indices: vec![0_u64.into()], + }, + ) + }); + } + + #[test] + #[should_panic(expected = "does not carry the message bound to nonce account")] + fn should_panic_when_a_nonce_withdrawal_differs_from_the_bound_message() { + init_state(); + init_balance(); + accept_withdrawal(account(1), 0, MINIMUM_WITHDRAWAL_AMOUNT); + create_withdrawal_batch_transaction(durable_nonce(1), vec![0]); + let message_with_another_amount = withdrawal_batch_message( + NONCE_ACCOUNT, + durable_nonce(1), + &[(Address::from([0u8; 32]), AMOUNT_TO_TRANSFER + 1)], + ); + + mutate_state(|s| { + s.process_transaction_submitted( + &signature(7), + &message_with_another_amount.into(), + &[Signer::Minter], + &TransactionPurpose::NonceWithdrawal { + burn_indices: vec![0_u64.into()], + }, + ) + }); + } + + #[test] + #[should_panic(expected = "does not serve the withdrawal requests bound to nonce account")] + fn should_panic_when_a_nonce_withdrawal_serves_other_withdrawal_requests() { + init_state(); + init_balance(); + accept_withdrawal(account(1), 0, MINIMUM_WITHDRAWAL_AMOUNT); + accept_withdrawal(account(2), 1, MINIMUM_WITHDRAWAL_AMOUNT); + create_withdrawal_batch_transaction(durable_nonce(1), vec![0, 1]); + + submit_withdrawal_batch_transaction(signature(7), durable_nonce(1), vec![0]); + } + + const DEPOSIT_AMOUNT: u64 = 500_000_000; + + fn funded_log_until_created_transaction() -> Vec { + let credited_amount = DEPOSIT_AMOUNT - FEE_PER_SIGNATURE; + let funding_deposit = queued_deposit_of(account(9), credited_amount); + vec![ + minter_public_key_fetched_event(), + EventType::QueuedDeposit { + deposit_id: deposit_id(0), + account: funding_deposit.account, + address: funding_deposit.address, + balance: funding_deposit.balance, + }, + EventType::SubmittedTransaction { + signature: signature(9), + message: sweep_message([(deposit_id(0), funding_deposit)]), + signers: vec![Signer::Account(funding_deposit.account)], + purpose: TransactionPurpose::SweepDeposit { + deposit_ids: vec![deposit_id(0)], + block_height: BlockHeight::new(0), + }, + }, + EventType::SucceededTransaction { + signature: signature(9), + }, + EventType::CreditedSweep { + signature: signature(9), + amount_received: credited_amount, + mints: vec![CreditedDeposit { + deposit_id: deposit_id(0), + amount_to_mint: credited_amount, + }], + }, + EventType::MintedSweptDeposit { + deposit_id: deposit_id(0), + mint_block_index: 9_u64.into(), + }, + EventType::AcceptedWithdrawalRequest(WithdrawalRequest { + account: account(1), + solana_address: [0u8; 32], + burn_block_index: 0_u64.into(), + burned_amount: MINIMUM_WITHDRAWAL_AMOUNT, + amount_to_transfer: AMOUNT_TO_TRANSFER, + }), + EventType::CreatedWithdrawalTransaction { + burn_indices: vec![0_u64.into()], + nonce_account: NONCE_ACCOUNT, + nonce_value: durable_nonce(1), + }, + ] + } + + fn log_of(events: Vec) -> Vec { + std::iter::once(EventType::Init(InitArgs { + nonce_accounts: vec![NONCE_ACCOUNT.to_string()], + ..valid_init_args() + })) + .chain(events) + .map(|payload| Event { + timestamp: 0, + payload, + }) + .collect() + } + + fn assert_nonce_account_free() { + let mut expected = pool_bound_to(durable_nonce(1)); + expected.free(&NONCE_ACCOUNT); + read_state(|s| assert_eq!(s.nonce_pool, expected)); + } + + fn pool_bound_to(nonce_value: solana_hash::Hash) -> DurableNoncePool { + let mut pool = DurableNoncePool::new([NONCE_ACCOUNT]) + .expect("a single nonce account is pairwise distinct"); + pool.bind(&NONCE_ACCOUNT, nonce_value); + pool + } +} diff --git a/minter/src/test_fixtures/mod.rs b/minter/src/test_fixtures/mod.rs index 60a8eb17..7315d0df 100644 --- a/minter/src/test_fixtures/mod.rs +++ b/minter/src/test_fixtures/mod.rs @@ -2,6 +2,7 @@ use crate::{ address::{MINTER_DERIVATION_PATH, account_address, derivation_path}, constants::{FEE_PER_SIGNATURE, RENT_EXEMPTION_THRESHOLD}, rpc::{BlockHeight, FetchedTransaction}, + sol_transfer::build_batch_withdrawal_message, state::{ DepositBalance, QueuedDeposit, SchnorrPublicKey, State, Sweep, event::{Event, EventType, VersionedMessage}, @@ -185,6 +186,18 @@ pub fn nonce_account_address() -> Address { Address::from([0x4E; 32]) } +/// Returns the batch withdrawal message of [`MINTER_ADDRESS`] that advances +/// `nonce_account`, carries `nonce_value` in place of a recent blockhash and +/// performs the given transfers. +pub fn withdrawal_batch_message( + nonce_account: Address, + nonce_value: solana_hash::Hash, + transfers: &[(Address, Lamport)], +) -> solana_message::Message { + build_batch_withdrawal_message(&MINTER_ADDRESS, &nonce_account, nonce_value, transfers) + .expect("BUG: the withdrawal batch message exceeds the transaction size") +} + /// Returns the nonce value stored by [`nonce_account_info`] for the same `nonce_seed`. pub fn durable_nonce(nonce_seed: usize) -> solana_hash::Hash { *DurableNonce::from_blockhash(&seed_hash(nonce_seed)).as_hash() @@ -691,8 +704,8 @@ pub mod devnet_sweep { /// All helpers operate on the global thread-local state via [`mutate_state`]. pub mod events { use super::{ - DEFAULT_BLOCK_HEIGHT, MINTER_ADDRESS, WITHDRAWAL_FEE, queued_deposit, queued_deposit_of, - runtime::TestCanisterRuntime, + DEFAULT_BLOCK_HEIGHT, MINTER_ADDRESS, NONCE_ACCOUNT, WITHDRAWAL_FEE, queued_deposit, + queued_deposit_of, runtime::TestCanisterRuntime, withdrawal_batch_message, }; use crate::deposit::sweep::deposit_status; use crate::{ @@ -792,8 +805,10 @@ pub mod events { .sweep_message(solana_hash::Hash::default()) .into(), signers, - purpose: TransactionPurpose::SweepDeposits { deposit_ids }, - block_height: DEFAULT_BLOCK_HEIGHT, + purpose: TransactionPurpose::SweepDeposit { + deposit_ids, + block_height: DEFAULT_BLOCK_HEIGHT, + }, }, &runtime(), ) @@ -880,6 +895,74 @@ pub mod events { }); } + /// Records a `CreatedWithdrawalTransaction` for the given withdrawals, binding + /// [`NONCE_ACCOUNT`] to `nonce_value`. + pub fn create_withdrawal_batch_transaction( + nonce_value: solana_hash::Hash, + burn_indices: Vec, + ) { + mutate_state(|state| { + process_event( + state, + EventType::CreatedWithdrawalTransaction { + burn_indices: burn_indices + .into_iter() + .map(LedgerBurnIndex::from) + .collect(), + nonce_account: NONCE_ACCOUNT, + nonce_value, + }, + &runtime(), + ) + }); + } + + /// Records a `SubmittedTransaction` for the durable-nonce withdrawal + /// transaction advancing [`NONCE_ACCOUNT`] with `nonce_value` and + /// transferring the created withdrawal requests of `burn_indices`. + pub fn submit_withdrawal_batch_transaction( + signature: Signature, + nonce_value: solana_hash::Hash, + burn_indices: Vec, + ) { + let burn_indices: Vec = burn_indices + .into_iter() + .map(LedgerBurnIndex::from) + .collect(); + let message = withdrawal_batch_message( + NONCE_ACCOUNT, + nonce_value, + &created_withdrawal_transfers(&burn_indices), + ); + mutate_state(|state| { + process_event( + state, + EventType::SubmittedTransaction { + signature, + message: message.into(), + signers: vec![Signer::Minter], + purpose: TransactionPurpose::NonceWithdrawal { burn_indices }, + }, + &runtime(), + ) + }); + } + + fn created_withdrawal_transfers(burn_indices: &[LedgerBurnIndex]) -> Vec<(Address, Lamport)> { + read_state(|state| { + burn_indices + .iter() + .map(|burn_index| { + let request = &state.created_withdrawal_requests()[burn_index].request; + ( + Address::from(request.solana_address), + request.amount_to_transfer, + ) + }) + .collect() + }) + } + pub fn submit_withdrawal(signature: Signature, burn_indices: Vec) { submit_withdrawal_at_height(signature, DEFAULT_BLOCK_HEIGHT, burn_indices); } @@ -896,13 +979,13 @@ pub mod events { signature, message: message().into(), signers: vec![Signer::Minter], - purpose: TransactionPurpose::WithdrawSol { + purpose: TransactionPurpose::Withdrawal { burn_indices: burn_indices .into_iter() .map(LedgerBurnIndex::from) .collect(), + block_height, }, - block_height, }, &runtime(), ) @@ -1208,21 +1291,14 @@ pub mod arb { arb_signature(), arb_message(), prop::collection::vec(arb_signer(), 1..10), - prop_oneof![ - prop::collection::vec(arb_ledger_burn_index(), 1..10) - .prop_map(|burn_indices| TransactionPurpose::WithdrawSol { burn_indices }), - prop::collection::vec(any::(), 1..10) - .prop_map(|deposit_ids| TransactionPurpose::SweepDeposits { deposit_ids }), - ], - arb_block_height(), + arb_transaction_purpose(), ) - .prop_map(|(signature, message, signers, purpose, block_height)| { + .prop_map(|(signature, message, signers, purpose)| { EventType::SubmittedTransaction { signature, message: message.into(), signers, purpose, - block_height, } }), (arb_signature(), arb_signature(), arb_block_height(),).prop_map( @@ -1277,6 +1353,18 @@ pub mod arb { } }), any::().prop_map(|deposit_id| EventType::QuarantinedPendingMint { deposit_id }), + ( + prop::collection::vec(arb_ledger_burn_index(), 1..10), + arb_address(), + arb_hash(), + ) + .prop_map(|(burn_indices, nonce_account, nonce_value)| { + EventType::CreatedWithdrawalTransaction { + burn_indices, + nonce_account, + nonce_value, + } + }), ] } @@ -1287,6 +1375,33 @@ pub mod arb { }) } + fn arb_transaction_purpose() -> impl Strategy { + prop_oneof![ + ( + prop::collection::vec(any::(), 1..10), + arb_block_height() + ) + .prop_map(|(deposit_ids, block_height)| { + TransactionPurpose::SweepDeposit { + deposit_ids, + block_height, + } + }), + ( + prop::collection::vec(arb_ledger_burn_index(), 1..10), + arb_block_height() + ) + .prop_map(|(burn_indices, block_height)| { + TransactionPurpose::Withdrawal { + burn_indices, + block_height, + } + }), + prop::collection::vec(arb_ledger_burn_index(), 1..10) + .prop_map(|burn_indices| TransactionPurpose::NonceWithdrawal { burn_indices }), + ] + } + pub fn arb_event() -> impl Strategy { (any::(), arb_event_type()) .prop_map(|(timestamp, payload)| Event { timestamp, payload }) diff --git a/minter/src/withdraw/mod.rs b/minter/src/withdraw/mod.rs index b71cdd05..7d027651 100644 --- a/minter/src/withdraw/mod.rs +++ b/minter/src/withdraw/mod.rs @@ -224,10 +224,10 @@ async fn submit_withdrawal_transaction( signature, message, signers, - purpose: TransactionPurpose::WithdrawSol { + purpose: TransactionPurpose::Withdrawal { burn_indices: burn_indices.clone(), + block_height: block.block_height, }, - block_height: block.block_height, }, runtime, ) diff --git a/minter/src/withdraw/tests.rs b/minter/src/withdraw/tests.rs index 5c5f3f99..b668dd34 100644 --- a/minter/src/withdraw/tests.rs +++ b/minter/src/withdraw/tests.rs @@ -486,7 +486,7 @@ 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(), Some(block_height)); assert_matches!(submitted, MinterTransaction::Withdrawal { .. }); assert_eq!( s.sent_withdrawal_requests()