From a5bd22dd74aa697cd484cbbd5579907b7388e398 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9gory=20Demay?= Date: Tue, 6 Oct 2026 14:57:00 +0000 Subject: [PATCH 01/10] feat(minter): submit withdrawals as durable-nonce transactions Co-Authored-By: Claude Opus 5.5 --- Cargo.lock | 1 + integration_tests/Cargo.toml | 1 + integration_tests/src/fixtures.rs | 64 +++ integration_tests/src/validator.rs | 50 ++- .../tests/solana_test_validator.rs | 11 + integration_tests/tests/tests.rs | 79 ++-- libs/types-internal/src/event.rs | 10 +- minter/cksol_minter.did | 10 +- minter/src/canbench.rs | 2 +- minter/src/main.rs | 11 +- minter/src/monitor/tests.rs | 295 ++++--------- minter/src/rpc/mod.rs | 32 +- minter/src/sol_transfer/mod.rs | 53 +-- minter/src/sol_transfer/tests.rs | 299 +++++-------- minter/src/state/event.rs | 14 +- minter/src/state/mod.rs | 137 ++---- minter/src/state/nonce_pool/mod.rs | 59 ++- minter/src/state/nonce_pool/tests.rs | 65 +++ minter/src/state/tests.rs | 197 +++++---- minter/src/test_fixtures/mod.rs | 120 ++++-- minter/src/test_fixtures/signer.rs | 17 +- minter/src/withdraw/mod.rs | 276 +++++++++--- minter/src/withdraw/tests.rs | 402 ++++++++++++------ 23 files changed, 1222 insertions(+), 983 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 39f8bc51..fa1902d0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -909,6 +909,7 @@ dependencies = [ "solana-keypair", "solana-message", "solana-native-token", + "solana-nonce", "solana-signature", "solana-system-interface 3.1.0", "solana-system-transaction", diff --git a/integration_tests/Cargo.toml b/integration_tests/Cargo.toml index 779df212..ae01573a 100644 --- a/integration_tests/Cargo.toml +++ b/integration_tests/Cargo.toml @@ -37,6 +37,7 @@ solana-hash = { workspace = true } solana-keypair = { workspace = true } solana-message = { workspace = true } solana-native-token = { workspace = true } +solana-nonce = { workspace = true } solana-signature = { workspace = true } solana-system-interface = { workspace = true, features = ["bincode"] } solana-system-transaction = { workspace = true } diff --git a/integration_tests/src/fixtures.rs b/integration_tests/src/fixtures.rs index 09042f6c..1ee59e59 100644 --- a/integration_tests/src/fixtures.rs +++ b/integration_tests/src/fixtures.rs @@ -14,6 +14,11 @@ use pocket_ic::nonblocking::PocketIc; use serde_json::json; use sol_rpc_types::Lamport; use solana_address::{Address, address}; +use solana_hash::Hash; +use solana_nonce::{ + state::{Data, DurableNonce, State}, + versions::Versions, +}; use solana_transaction::Transaction; use std::sync::Arc; use tokio::sync::Mutex; @@ -150,6 +155,31 @@ impl MockBuilder { ) } + /// Mock for `getAccountInfo` returning an initialized durable nonce account + /// whose nonce authority is the minter's main address. + pub fn get_nonce_account(self) -> Self { + self.expect( + get_account_info_request(), + get_account_info_nonce_response(), + ) + } + + /// Mocks for the withdrawal timer submitting a durable-nonce transaction: + /// `getAccountInfo` reading the nonce account → `sendTransaction`. + pub fn submit_withdrawal_transaction(self) -> Self { + self.get_nonce_account().expect( + send_transaction_request(), + send_transaction_response(SUBMITTED_SIGNATURE), + ) + } + + /// Mocks for `finalize_transactions` finding only in-flight withdrawal + /// transactions, which carry a durable nonce and need no current block: + /// a single `getSignatureStatuses` reporting them as finalized. + pub fn finalize_withdrawal_transaction(self, signature: &Signature) -> Self { + self.check_signature_statuses(signature, get_signature_statuses_finalized_response()) + } + /// Mocks for `finalize_transactions` finding the pending transaction with the given /// signature expired at `block_height`: `getSlot` → `getBlock` → `getSignatureStatuses` /// reporting it as not found. @@ -249,6 +279,40 @@ fn sweep_transaction_response(sweep: &Transaction, sweepable_amount: Lamport) -> })) } +fn get_account_info_request() -> JsonRpcRequestMatcher { + JsonRpcRequestMatcher::with_method("getAccountInfo") +} + +fn get_account_info_nonce_response() -> JsonRpcResponse { + JsonRpcResponse::from(json!({ + "jsonrpc": "2.0", + "result": { + "context": { "apiVersion": "2.0.15", "slot": 341_197_053 }, + "value": { + "data": [nonce_account_data(), "base64"], + "executable": false, + "lamports": 1_447_680, + "owner": "11111111111111111111111111111111", + "rentEpoch": 18_446_744_073_709_551_615_u64, + "space": 80 + } + }, + "id": 1 + })) +} + +fn nonce_account_data() -> String { + let nonce_account = Versions::new(State::Initialized(Data::new( + MINTER_ADDRESS, + DurableNonce::from_blockhash(&Hash::from([0x4E; 32])), + FEE_PER_SIGNATURE, + ))); + STANDARD.encode( + bincode::serialize(&nonce_account) + .expect("BUG: serializing a nonce account should succeed"), + ) +} + fn get_balance_request() -> JsonRpcRequestMatcher { JsonRpcRequestMatcher::with_method("getBalance") } diff --git a/integration_tests/src/validator.rs b/integration_tests/src/validator.rs index 97dce7f4..8b6a6503 100644 --- a/integration_tests/src/validator.rs +++ b/integration_tests/src/validator.rs @@ -1,4 +1,7 @@ -use crate::{Setup, SetupBuilder, fixtures::RENT_EXEMPTION_THRESHOLD}; +use crate::{ + Setup, SetupBuilder, + fixtures::{MINTER_ADDRESS, RENT_EXEMPTION_THRESHOLD}, +}; use cksol_types::WithdrawalStatus; use icrc_ledger_types::icrc1::account::Account; use sol_rpc_types::{InstallArgs, Lamport, OverrideProvider, RegexSubstitution, RoundingError}; @@ -127,12 +130,27 @@ impl SolanaTestValidator { rpc.get_fee_for_message(&transfer.message).await.ok() } - /// Creates a test setup whose SOL RPC canister talks to this validator. + /// Creates a test setup whose SOL RPC canister talks to this validator, + /// with a real durable nonce account created on the validator for the + /// minter's deterministic main address before the minter is installed, so + /// that withdrawal transactions can be submitted against it. pub async fn setup(&self) -> Setup { - self.setup_builder().build().await + let nonce_accounts = self + .create_nonce_accounts(1, &MINTER_ADDRESS) + .await + .iter() + .map(Address::to_string) + .collect(); + self.setup_builder() + .with_nonce_accounts(nonce_accounts) + .build() + .await } - /// A [`SetupBuilder`] preconfigured so the SOL RPC canister talks to this validator. + /// A [`SetupBuilder`] preconfigured so the SOL RPC canister talks to this + /// validator. The default nonce account pool is a placeholder that does not + /// exist on the validator, so a test whose minter submits withdrawals must + /// pass accounts from [`Self::create_nonce_accounts`] or use [`Self::setup`]. pub fn setup_builder(&self) -> SetupBuilder { SetupBuilder::new() .with_proxy_canister() @@ -263,6 +281,30 @@ impl SolanaTestValidator { addresses } + /// The nonce value currently stored by the given durable nonce account, + /// read at `finalized` commitment. + /// + /// # Panics + /// + /// Panics if the account does not exist or is not an initialized nonce account. + pub async fn get_nonce_value(&self, address: &Address) -> solana_hash::Hash { + let account = self + .rpc_client() + .get_account_with_commitment(address, CommitmentConfig::finalized()) + .await + .expect("Failed to read the nonce account") + .value + .unwrap_or_else(|| panic!("Nonce account {address} does not exist")); + let versions: solana_nonce::versions::Versions = bincode::deserialize(&account.data) + .unwrap_or_else(|e| panic!("Account {address} is not a nonce account: {e}")); + match versions.state() { + solana_nonce::state::State::Initialized(data) => data.blockhash(), + solana_nonce::state::State::Uninitialized => { + panic!("Nonce account {address} is not initialized") + } + } + } + pub async fn airdrop_and_confirm(&self, address: Address, airdrop_amount: Lamport) { let rpc = self.rpc_client(); diff --git a/integration_tests/tests/solana_test_validator.rs b/integration_tests/tests/solana_test_validator.rs index 079d9dbf..28cf6c24 100644 --- a/integration_tests/tests/solana_test_validator.rs +++ b/integration_tests/tests/solana_test_validator.rs @@ -142,6 +142,11 @@ async fn should_deposit_and_withdraw() { )) .await; + let nonce_account: Address = setup.minter().get_minter_info().await.nonce_accounts[0] + .parse() + .expect("the minter reports well-formed nonce accounts"); + let nonce_value_before = validator.get_nonce_value(&nonce_account).await; + // Advance time to trigger withdrawal processing and monitor timers setup.advance_time(Duration::from_mins(10)).await; @@ -149,6 +154,12 @@ async fn should_deposit_and_withdraw() { wait_for_withdrawal_finalized(&setup, burn_index).await; } + // The landed withdrawal transaction advanced the durable nonce it carried. + assert_ne!( + validator.get_nonce_value(&nonce_account).await, + nonce_value_before + ); + // Verify all ICRC accounts are drained for account in &accounts { let balance = setup.ledger().balance_of(*account).await; diff --git a/integration_tests/tests/tests.rs b/integration_tests/tests/tests.rs index a6e0413c..fbb4d80e 100644 --- a/integration_tests/tests/tests.rs +++ b/integration_tests/tests/tests.rs @@ -25,7 +25,6 @@ use tokio::join; const WITHDRAWAL_PROCESSING_DELAY: Duration = Duration::from_mins(1); const FINALIZE_TRANSACTIONS_DELAY: Duration = Duration::from_mins(2); -const RESUBMIT_TRANSACTIONS_DELAY: Duration = Duration::from_mins(3); const SWEEP_DEPOSITS_DELAY: Duration = Duration::from_mins(1); /// Number of blocks a blockhash stays valid for, as the minter counts them. const MAX_BLOCKHASH_AGE_IN_BLOCKS: u64 = 150; @@ -735,89 +734,75 @@ mod withdrawal_tests { .await .expect("withdraw should succeed"); + // The withdrawal timer reads the durable nonce account and submits the + // transaction with the nonce value in place of a recent blockhash. setup.advance_time(WITHDRAWAL_PROCESSING_DELAY).await; setup .execute_http_mocks( MockBuilder::with_start_id(32) - .submit_transaction(SUBMISSION_BLOCK_HEIGHT) + .submit_withdrawal_transaction() .build(), ) .await; + let nonce_account = Setup::DEFAULT_NONCE_ACCOUNT.to_string(); setup.minter().assert_that_events().await.satisfy(|events| { + check!(events.iter().any(|e| matches!( + e, + EventType::CreatedWithdrawalTransaction { + burn_indices, + nonce_account: account, + .. + } if burn_indices == &[block_index] && account.to_string() == nonce_account + ))); check!(events.iter().any(|e| matches!( e, EventType::SubmittedTransaction { - purpose: TransactionPurpose::Withdrawal { burn_indices, block_height }, + purpose: TransactionPurpose::Withdrawal { burn_indices }, .. } if burn_indices == &[block_index] - && *block_height == SUBMISSION_BLOCK_HEIGHT ))); }); // Withdrawal status should be TxSent with some signature let status = setup.minter().withdrawal_status(block_index).await; - let original_transaction_id = match &status { + let transaction_id = match &status { WithdrawalStatus::TxSent { transaction_id } => transaction_id.clone(), other => panic!("Expected TxSent, got: {other:?}"), }; - // Advance time to trigger finalize_transactions, which fetches the current block, - // checks statuses (not found), and marks the expired transaction for resubmission. - setup.advance_time(FINALIZE_TRANSACTIONS_DELAY).await; - setup - .execute_http_mocks( - MockBuilder::with_start_id(44) - .mark_transaction_expired(&original_transaction_id, EXPIRY_BLOCK_HEIGHT) - .build(), - ) - .await; - - // Advance time to trigger resubmit_transactions. finalize_transactions also - // fires but has no pending transactions, so it makes no HTTP outcalls. - setup.advance_time(RESUBMIT_TRANSACTIONS_DELAY).await; + // An upgrade replays the created and submitted withdrawal events; the + // in-flight transaction must survive the replay unchanged. setup - .execute_http_mocks( - MockBuilder::with_start_id(56) - .resubmit_transaction(EXPIRY_BLOCK_HEIGHT) - .build(), - ) - .await; - - // Withdrawal status should now have a different signature - let status = setup.minter().withdrawal_status(block_index).await; - let resubmitted_transaction_id = match &status { - WithdrawalStatus::TxSent { transaction_id } => { - assert_ne!( - *transaction_id, original_transaction_id, - "Expected transaction ID to change after resubmission" - ); - transaction_id.clone() + .minter() + .upgrade(UpgradeArgs::default()) + .await + .expect("upgrade should succeed"); + assert_eq!( + setup.minter().withdrawal_status(block_index).await, + WithdrawalStatus::TxSent { + transaction_id: transaction_id.clone() } - other => panic!("Expected TxSent after resubmission, got: {other:?}"), - }; + ); - // Advance time to trigger finalize_transactions again. This time the - // transaction is reported as finalized. + // Only the in-flight withdrawal is monitored, so the finalization timer + // checks the signature statuses without fetching a current block. setup.advance_time(FINALIZE_TRANSACTIONS_DELAY).await; setup .execute_http_mocks( - MockBuilder::with_start_id(68) - .finalize_transaction(&resubmitted_transaction_id, EXPIRY_BLOCK_HEIGHT) + MockBuilder::with_start_id(40) + .finalize_withdrawal_transaction(&transaction_id) .build(), ) .await; - // Withdrawal status should now be TxFinalized with Success let status = setup.minter().withdrawal_status(block_index).await; match &status { WithdrawalStatus::TxFinalized(TxFinalizedStatus::Success { - transaction_id, .. + transaction_id: finalized_transaction_id, + .. }) => { - assert_eq!( - *transaction_id, resubmitted_transaction_id, - "Expected finalized transaction ID to match resubmitted transaction ID" - ); + assert_eq!(*finalized_transaction_id, transaction_id); } other => panic!("Expected TxFinalized(Success), got: {other:?}"), } diff --git a/libs/types-internal/src/event.rs b/libs/types-internal/src/event.rs index 31c589f5..132a6974 100644 --- a/libs/types-internal/src/event.rs +++ b/libs/types-internal/src/event.rs @@ -183,18 +183,10 @@ pub enum TransactionPurpose { /// 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, - }, /// 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 { + Withdrawal { /// The burn transaction indices on the ckSOL ledger. burn_indices: Vec, }, diff --git a/minter/cksol_minter.did b/minter/cksol_minter.did index e51e8c9b..44ddc7f1 100644 --- a/minter/cksol_minter.did +++ b/minter/cksol_minter.did @@ -358,18 +358,10 @@ type TransactionPurpose = variant { // 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 { + Withdrawal : record { // The ledger burn indices of the withdrawal requests included in this transaction. burn_indices: vec LedgerBurnIndex; }; diff --git a/minter/src/canbench.rs b/minter/src/canbench.rs index 65bc4527..0567e8ea 100644 --- a/minter/src/canbench.rs +++ b/minter/src/canbench.rs @@ -160,7 +160,7 @@ fn accept_and_submit_withdrawal(account_index: usize, burn_index: u64, sig: Sign AMOUNT_TO_TRANSFER, )), signers: vec![Signer::Minter], - purpose: TransactionPurpose::NonceWithdrawal { burn_indices }, + purpose: TransactionPurpose::Withdrawal { burn_indices }, }); } diff --git a/minter/src/main.rs b/minter/src/main.rs index 1b65c803..6551ec6f 100644 --- a/minter/src/main.rs +++ b/minter/src/main.rs @@ -141,15 +141,8 @@ fn get_events( 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 { + TransactionPurpose::Withdrawal { burn_indices } => { + event::TransactionPurpose::Withdrawal { burn_indices: burn_indices.iter().map(|idx| *idx.get()).collect(), } } diff --git a/minter/src/monitor/tests.rs b/minter/src/monitor/tests.rs index 7e95d6ff..bee557be 100644 --- a/minter/src/monitor/tests.rs +++ b/minter/src/monitor/tests.rs @@ -2,26 +2,24 @@ use super::{ MAX_BLOCKHASH_AGE_IN_BLOCKS, MAX_SIGNATURES_PER_STATUS_CHECK, finalize_transactions, resubmit_transactions, }; -use crate::test_fixtures::signer::sign_as_minter; use crate::{ constants::MAX_CONCURRENT_RPC_CALLS, rpc::BlockHeight, state::{TaskType, event::EventType, mutate_state, read_state, reset_state}, storage::reset_events, test_fixtures::{ - 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, + EventsAssert, GetTransactionResult, MINIMUM_WITHDRAWAL_AMOUNT, account, + confirmed_block_at_height, durable_nonce, events, init_balance, init_schnorr_master_key, + init_state, runtime::TestCanisterRuntime, signature, }, }; use sol_rpc_types::{ - ConfirmedBlock, MultiRpcResult, RpcError, Signature, Slot, TransactionConfirmationStatus, + ConfirmedBlock, MultiRpcResult, RpcError, Slot, TransactionConfirmationStatus, TransactionError, TransactionStatus, }; type SlotResult = MultiRpcResult; type BlockResult = MultiRpcResult; -type SendTransactionResult = MultiRpcResult; type SignatureStatusesResult = MultiRpcResult>>; const CURRENT_SLOT: Slot = 408_807_102; @@ -49,7 +47,7 @@ mod finalization { #[tokio::test] async fn should_return_early_if_task_already_active() { setup(); - submit_withdrawal_transaction(CURRENT_BLOCK_HEIGHT); + submit_sweep_transaction(CURRENT_BLOCK_HEIGHT); mutate_state(|s| { s.active_tasks_mut().insert(TaskType::FinalizeTransactions); @@ -66,8 +64,8 @@ mod finalization { #[tokio::test] async fn should_finalize_but_not_expire_transactions_if_fetching_current_block_fails() { setup(); - let finalized = submit_withdrawal_transaction_with_signature(1, EXPIRED_BLOCK_HEIGHT); - let not_found = submit_withdrawal_transaction_with_signature(2, EXPIRED_BLOCK_HEIGHT); + let finalized = submit_withdrawal_transaction_with_signature(1); + let not_found = submit_sweep_transaction_with_signature(2, EXPIRED_BLOCK_HEIGHT); let runtime = TestCanisterRuntime::new() .with_increasing_time() @@ -87,10 +85,7 @@ mod finalization { 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()); - }); + read_state(|s| assert!(s.submitted_transactions().contains_key(¬_found))); } #[tokio::test] @@ -99,14 +94,11 @@ mod finalization { let num = MAX_CONCURRENT_RPC_CALLS * MAX_SIGNATURES_PER_STATUS_CHECK + 1; for i in 0..num { - submit_withdrawal_transaction_with_signature(i, CURRENT_BLOCK_HEIGHT); + submit_withdrawal_transaction_with_signature(i); } // Round 1: finalizes MAX_CONCURRENT_RPC_CALLS batches, 1 transaction unchecked → reschedule - let mut runtime = TestCanisterRuntime::new() - .with_increasing_time() - .add_stub_response(SlotResult::Consistent(Ok(CURRENT_SLOT))) - .add_stub_response(BlockResult::Consistent(Ok(current_block()))); + let mut runtime = TestCanisterRuntime::new().with_increasing_time(); for _ in 0..MAX_CONCURRENT_RPC_CALLS { runtime = runtime.add_stub_response(SignatureStatusesResult::Consistent(Ok( vec![Some(finalized_status()); MAX_SIGNATURES_PER_STATUS_CHECK], @@ -121,8 +113,6 @@ mod finalization { // Round 2: finalizes the remaining 1 transaction → no reschedule 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![Some( finalized_status(), )]))); @@ -137,7 +127,7 @@ mod finalization { async fn should_finalize_transaction_with_finalized_status() { setup(); - let signature = submit_withdrawal_transaction(CURRENT_BLOCK_HEIGHT); + let signature = submit_sweep_transaction(CURRENT_BLOCK_HEIGHT); let runtime = TestCanisterRuntime::new() .with_increasing_time() @@ -145,7 +135,8 @@ mod finalization { .add_stub_response(BlockResult::Consistent(Ok(current_block()))) .add_stub_response(SignatureStatusesResult::Consistent(Ok(vec![Some( finalized_status(), - )]))); + )]))) + .add_stub_response(GetTransactionResult::Consistent(Ok(None))); finalize_transactions(runtime).await; @@ -173,7 +164,7 @@ mod finalization { reset_events(); setup(); - submit_withdrawal_transaction(block_height); + submit_sweep_transaction(block_height); let runtime = TestCanisterRuntime::new() .with_increasing_time() @@ -191,11 +182,59 @@ mod finalization { read_state(|s| assert_eq!(s.submitted_transactions().len(), 1)); } + #[tokio::test] + async fn should_finalize_an_in_flight_withdrawal_without_fetching_a_block() { + setup(); + let signature = submit_withdrawal_transaction(); + + 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 }); + read_state(|s| { + assert!(s.submitted_transactions().is_empty()); + assert!(s.succeeded_transactions().contains(&signature)); + }); + } + + #[tokio::test] + async fn should_keep_a_missing_withdrawal_in_flight() { + setup(); + let signature = submit_withdrawal_transaction(); + let events_before = EventsAssert::from_recorded(); + + let runtime = TestCanisterRuntime::new() + .with_increasing_time() + .add_stub_response(SignatureStatusesResult::Consistent(Ok(vec![None]))); + + finalize_transactions(runtime).await; + + assert_eq!(EventsAssert::from_recorded(), events_before); + read_state(|s| assert!(s.submitted_transactions().contains_key(&signature))); + } + + fn submit_withdrawal_transaction() -> solana_signature::Signature { + submit_withdrawal_transaction_with_signature(0x77) + } + + fn submit_withdrawal_transaction_with_signature(i: usize) -> solana_signature::Signature { + let signature = signature(i); + events::accept_withdrawal(account(i), i as u64, MINIMUM_WITHDRAWAL_AMOUNT); + events::submit_withdrawal(signature, vec![i as u64]); + signature + } + #[tokio::test] async fn should_record_failed_transaction_event_on_error() { setup(); - let signature = submit_withdrawal_transaction(CURRENT_BLOCK_HEIGHT); + let signature = submit_sweep_transaction(CURRENT_BLOCK_HEIGHT); let runtime = TestCanisterRuntime::new() .with_increasing_time() @@ -229,9 +268,9 @@ mod finalization { let sig_a = 0x01; let sig_b = 0x02; let sig_c = 0x03; - submit_withdrawal_transaction_with_signature(sig_a, CURRENT_BLOCK_HEIGHT); - submit_withdrawal_transaction_with_signature(sig_b, CURRENT_BLOCK_HEIGHT); - submit_withdrawal_transaction_with_signature(sig_c, CURRENT_BLOCK_HEIGHT); + submit_sweep_transaction_with_signature(sig_a, CURRENT_BLOCK_HEIGHT); + submit_sweep_transaction_with_signature(sig_b, CURRENT_BLOCK_HEIGHT); + submit_sweep_transaction_with_signature(sig_c, CURRENT_BLOCK_HEIGHT); let runtime = TestCanisterRuntime::new() .with_increasing_time() @@ -241,7 +280,9 @@ mod finalization { Some(finalized_status()), None, Some(finalized_status()), - ]))); + ]))) + .add_stub_response(GetTransactionResult::Consistent(Ok(None))) + .add_stub_response(GetTransactionResult::Consistent(Ok(None))); finalize_transactions(runtime).await; @@ -281,8 +322,7 @@ mod finalization { #[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 sweep = submit_sweep_transaction_with_signature(1, EXPIRED_BLOCK_HEIGHT); let nonce_withdrawal = submit_nonce_withdrawal_transaction(2); let runtime = TestCanisterRuntime::new() @@ -293,10 +333,8 @@ mod finalization { finalize_transactions(runtime).await; - let events = - EventsAssert::from_recorded().expect_contains_event_eq(EventType::ExpiredTransaction { - signature: blockhash_withdrawal, - }); + let events = EventsAssert::from_recorded() + .expect_contains_event_eq(EventType::ExpiredTransaction { signature: sweep }); assert!(!events.contains_event(&EventType::ExpiredTransaction { signature: nonce_withdrawal })); @@ -336,7 +374,7 @@ mod finalization { reset_state(); reset_events(); setup(); - let signature = submit_withdrawal_transaction(case.transaction_block_height); + let signature = submit_sweep_transaction(case.transaction_block_height); let runtime = TestCanisterRuntime::new() .with_increasing_time() .add_stub_response(SlotResult::Consistent(Ok(CURRENT_SLOT))) @@ -349,12 +387,7 @@ mod finalization { .contains_event(&EventType::ExpiredTransaction { signature }); assert_eq!(expired, case.should_expire, "{}", case.name); read_state(|s| { - assert_eq!( - s.transactions_to_resubmit().contains_key(&signature), - case.should_expire, - "{}", - case.name - ); + assert!(s.transactions_to_resubmit().is_empty(), "{}", case.name); assert_eq!( s.submitted_transactions().contains_key(&signature), !case.should_expire, @@ -409,7 +442,7 @@ mod resubmission { #[tokio::test] async fn should_return_early_if_task_already_active() { setup(); - let sig = submit_withdrawal_transaction(EXPIRED_BLOCK_HEIGHT); + let sig = submit_sweep_transaction(EXPIRED_BLOCK_HEIGHT); events::expire_transaction(sig); mutate_state(|s| { @@ -424,87 +457,11 @@ mod resubmission { assert_eq!(events_before, events_after); } - #[tokio::test] - async fn should_resubmit_expired_transaction_with_no_status() { - setup(); - - let old_signature = submit_withdrawal_transaction(EXPIRED_BLOCK_HEIGHT); - let new_signature = minter_signature(); - events::expire_transaction(old_signature); - - read_state(|s| { - assert!(s.transactions_to_resubmit().contains_key(&old_signature)); - }); - - let resubmit_runtime = TestCanisterRuntime::new() - .with_increasing_time() - .add_stub_response(SlotResult::Consistent(Ok(RESUBMISSION_SLOT))) - .add_stub_response(BlockResult::Consistent(Ok(confirmed_block_at_height( - RESUBMISSION_BLOCK_HEIGHT, - )))) - .add_stub_response(SendTransactionResult::Consistent(Ok(new_signature.into()))) - .add_signer(sign_as_minter()); - - resubmit_transactions(resubmit_runtime).await; - - EventsAssert::from_recorded() - .expect_contains_event_eq(EventType::ExpiredTransaction { - signature: old_signature, - }) - .expect_contains_event_eq(EventType::ResubmittedTransaction { - old_signature, - new_signature, - new_block_height: RESUBMISSION_BLOCK_HEIGHT, - }); - - read_state(|s| { - assert_eq!(s.submitted_transactions().len(), 1); - let resubmitted = s.submitted_transactions().get(&new_signature).unwrap(); - assert_eq!(resubmitted.block_height(), Some(RESUBMISSION_BLOCK_HEIGHT)); - }); - } - - #[tokio::test] - async fn should_resubmit_expired_withdrawal_signed_by_the_minter() { - setup(); - init_balance(); - - let old_signature = signature(1); - let burn_index = 1; - events::accept_withdrawal(account(1), burn_index, MINIMUM_WITHDRAWAL_AMOUNT); - events::submit_withdrawal_at_height(old_signature, EXPIRED_BLOCK_HEIGHT, vec![burn_index]); - events::expire_transaction(old_signature); - - let new_signature = minter_signature(); - - let resubmit_runtime = TestCanisterRuntime::new() - .with_increasing_time() - .add_stub_response(SlotResult::Consistent(Ok(RESUBMISSION_SLOT))) - .add_stub_response(BlockResult::Consistent(Ok(confirmed_block_at_height( - RESUBMISSION_BLOCK_HEIGHT, - )))) - .add_stub_response(SendTransactionResult::Consistent(Ok(new_signature.into()))) - .add_signer(sign_as_minter()); - - resubmit_transactions(resubmit_runtime).await; - - EventsAssert::from_recorded().expect_contains_event_eq(EventType::ResubmittedTransaction { - old_signature, - new_signature, - new_block_height: RESUBMISSION_BLOCK_HEIGHT, - }); - - read_state(|s| { - assert_eq!(s.submitted_transactions().len(), 1); - assert!(s.submitted_transactions().contains_key(&new_signature)); - }); - } - #[tokio::test] async fn should_not_resubmit_expired_transaction_if_status_check_fails() { setup(); - submit_withdrawal_transaction(EXPIRED_BLOCK_HEIGHT); + submit_sweep_transaction(EXPIRED_BLOCK_HEIGHT); let events_before = EventsAssert::from_recorded(); @@ -528,95 +485,6 @@ mod resubmission { assert!(s.transactions_to_resubmit().is_empty()); }); } - - #[tokio::test] - async fn should_record_resubmission_event_even_if_submission_fails() { - setup(); - - let old_signature = submit_withdrawal_transaction(EXPIRED_BLOCK_HEIGHT); - let new_signature = minter_signature(); - events::expire_transaction(old_signature); - - let resubmit_runtime = TestCanisterRuntime::new() - .with_increasing_time() - .add_stub_response(SlotResult::Consistent(Ok(RESUBMISSION_SLOT))) - .add_stub_response(BlockResult::Consistent(Ok(confirmed_block_at_height( - RESUBMISSION_BLOCK_HEIGHT, - )))) - .add_stub_response(SendTransactionResult::Inconsistent(vec![])) - .add_signer(sign_as_minter()); - - resubmit_transactions(resubmit_runtime).await; - - EventsAssert::from_recorded() - .expect_contains_event_eq(EventType::ExpiredTransaction { - signature: old_signature, - }) - .expect_contains_event_eq(EventType::ResubmittedTransaction { - old_signature, - new_signature, - new_block_height: RESUBMISSION_BLOCK_HEIGHT, - }); - } - - #[tokio::test] - async fn should_reschedule_until_all_transactions_resubmitted() { - setup(); - - let num_transactions = MAX_CONCURRENT_RPC_CALLS + 1; - for i in 0..num_transactions { - let sig = submit_withdrawal_transaction_with_signature(i, EXPIRED_BLOCK_HEIGHT); - events::expire_transaction(sig); - } - - // Round 1: resubmits MAX_CONCURRENT_RPC_CALLS transactions, 1 remain → reschedule - let mut runtime = TestCanisterRuntime::new() - .with_increasing_time() - .add_stub_response(SlotResult::Consistent(Ok(RESUBMISSION_SLOT))) - .add_stub_response(BlockResult::Consistent(Ok(confirmed_block_at_height( - RESUBMISSION_BLOCK_HEIGHT, - )))) - .add_signer(sign_as_minter().times(MAX_CONCURRENT_RPC_CALLS)); - for i in 0..MAX_CONCURRENT_RPC_CALLS { - runtime = runtime - .add_stub_response(SendTransactionResult::Consistent(Ok( - signature(0xA0 + i).into() - ))); - } - - resubmit_transactions(runtime.clone()).await; - - read_state(|s| { - assert_eq!(s.submitted_transactions().len(), MAX_CONCURRENT_RPC_CALLS); - assert_eq!( - s.transactions_to_resubmit().len(), - num_transactions - MAX_CONCURRENT_RPC_CALLS - ); - }); - assert_eq!(runtime.set_timer_call_count(), 1); - - // Round 2: resubmits remaining transaction → no reschedule - let mut runtime = TestCanisterRuntime::new() - .with_increasing_time() - .add_stub_response(SlotResult::Consistent(Ok(RESUBMISSION_SLOT))) - .add_stub_response(BlockResult::Consistent(Ok(confirmed_block_at_height( - RESUBMISSION_BLOCK_HEIGHT, - )))) - .add_signer( - sign_as_minter().expect([Ok(minter_signature_nth(MAX_CONCURRENT_RPC_CALLS))]), - ); - for i in 0..(num_transactions - MAX_CONCURRENT_RPC_CALLS) { - runtime = runtime - .add_stub_response(SendTransactionResult::Consistent(Ok( - signature(0xB0 + i).into() - ))); - } - - resubmit_transactions(runtime.clone()).await; - - assert!(read_state(|s| s.transactions_to_resubmit().is_empty())); - assert_eq!(runtime.set_timer_call_count(), 0); - } } fn setup() { @@ -629,17 +497,18 @@ fn current_block() -> ConfirmedBlock { confirmed_block_at_height(CURRENT_BLOCK_HEIGHT) } -fn submit_withdrawal_transaction(block_height: BlockHeight) -> solana_signature::Signature { - submit_withdrawal_transaction_with_signature(1, block_height) +fn submit_sweep_transaction(block_height: BlockHeight) -> solana_signature::Signature { + submit_sweep_transaction_with_signature(1, block_height) } -fn submit_withdrawal_transaction_with_signature( +fn submit_sweep_transaction_with_signature( i: usize, block_height: BlockHeight, ) -> solana_signature::Signature { let signature = signature(i); - events::accept_withdrawal(account(i), i as u64, MINIMUM_WITHDRAWAL_AMOUNT); - events::submit_withdrawal_at_height(signature, block_height, vec![i as u64]); + let deposit_id = crate::state::read_state(|state| state.deposits().next_id()); + events::queue_deposit(deposit_id, account(i), 1_000_000); + events::submit_sweep_at_height(signature, vec![deposit_id], block_height); signature } diff --git a/minter/src/rpc/mod.rs b/minter/src/rpc/mod.rs index b4349557..09eab56f 100644 --- a/minter/src/rpc/mod.rs +++ b/minter/src/rpc/mod.rs @@ -161,9 +161,39 @@ impl From for DepositSolError { pub async fn submit_transaction( runtime: &R, transaction: Transaction, +) -> Result { + send_transaction(runtime, transaction, Preflight::Simulate).await +} + +/// Submits a withdrawal transaction without the providers' preflight +/// simulation, so that a doomed transaction lands and fails on-chain, +/// advancing its nonce and freeing its nonce account, instead of staying +/// unbroadcast and occupying the account until an operator intervenes. +pub async fn submit_transaction_skipping_preflight( + runtime: &R, + transaction: Transaction, +) -> Result { + send_transaction(runtime, transaction, Preflight::Skip).await +} + +enum Preflight { + Simulate, + Skip, +} + +async fn send_transaction( + runtime: &R, + transaction: Transaction, + preflight: Preflight, ) -> Result { let client = read_state(|state| state.sol_rpc_client(runtime.inter_canister_call_runtime())); - match client.send_transaction(transaction).try_send().await { + let request = match preflight { + Preflight::Simulate => client.send_transaction(transaction), + Preflight::Skip => client + .send_transaction(transaction) + .with_skip_preflight(true), + }; + match request.try_send().await { Ok(MultiRpcResult::Consistent(Ok(signature))) => Ok(signature), Ok(MultiRpcResult::Consistent(Err(e))) => Err(SubmitTransactionError::RpcError(e)), Ok(MultiRpcResult::Inconsistent(_)) => Err(SubmitTransactionError::InconsistentRpcResults), diff --git a/minter/src/sol_transfer/mod.rs b/minter/src/sol_transfer/mod.rs index a00af4c2..e0efdf14 100644 --- a/minter/src/sol_transfer/mod.rs +++ b/minter/src/sol_transfer/mod.rs @@ -1,5 +1,5 @@ use crate::{ - address::{DerivationPath, MinterPublicKeyNotYetAvailable, minter_address, minter_public_key}, + address::{DerivationPath, MINTER_DERIVATION_PATH}, constants::FEE_PER_SIGNATURE, runtime::CanisterRuntime, signer::{SchnorrSigner, sign_bytes}, @@ -12,7 +12,7 @@ use sol_rpc_types::Lamport; use solana_address::Address; use solana_hash::Hash; use solana_system_interface::instruction; -use solana_transaction::{Instruction, Message, Transaction}; +use solana_transaction::{Message, Transaction}; use std::collections::BTreeMap; use thiserror::Error; @@ -23,9 +23,9 @@ pub const MAX_SIGNATURES: u64 = 10; pub const MAX_TX_SIZE: usize = 1_232; const BYTES_PER_SIGNATURE: usize = 64; -/// Upper bound on the number of withdrawal transfers that fit in a single -/// Solana transaction when the fee-payer is the only signer. -pub const MAX_WITHDRAWALS_PER_TX: usize = 20; +/// Maximum number of withdrawal transfers batched into a single +/// durable-nonce transaction. +pub const MAX_WITHDRAWALS_PER_NONCE_TX: usize = 10; /// Fee charged for a batch withdrawal transaction, which is signed by the fee payer only. pub const BATCH_WITHDRAWAL_TX_FEE: Lamport = FEE_PER_SIGNATURE; @@ -36,8 +36,6 @@ pub enum CreateTransferError { TransactionTooLarge { max: usize, got: usize }, #[error("signing failed: {0}")] SigningFailed(SignCallError), - #[error(transparent)] - MinterPublicKeyNotYetAvailable(MinterPublicKeyNotYetAvailable), } /// Signs the transaction of a planned sweep with the deposit addresses it transfers from. @@ -102,38 +100,19 @@ pub fn build_batch_withdrawal_message( Ok(message) } -/// Creates a signed Solana transaction that transfers lamports from a single -/// minter-controlled address (the fee payer) to multiple target addresses. -/// -/// Returns the signed transaction and its signers: -/// only [`Signer::Minter`], the fee payer. -/// -/// # Panics -/// -/// Panics if the IC returns a signature that is not exactly 64 bytes. -pub async fn create_signed_batch_withdrawal_transaction( +/// Signs the given withdrawal message with the minter's master key. +pub async fn sign_batch_withdrawal_message( runtime: &R, - targets: &[(Address, Lamport)], - recent_blockhash: Hash, -) -> Result<(Transaction, Vec), CreateTransferError> { - let master_public_key = minter_public_key()?; - let fee_payer_address = minter_address(&master_public_key); - - let instructions: Vec = targets - .iter() - .map(|(target, amount)| instruction::transfer(&fee_payer_address, target, *amount)) - .collect(); - - let message = - Message::new_with_blockhash(&instructions, Some(&fee_payer_address), &recent_blockhash); + message: Message, +) -> Result { let mut transaction = Transaction::new_unsigned(message); - - let signers = vec![Signer::Minter]; - let derivation_paths: Vec = - signers.iter().map(Signer::derivation_path).collect(); - sign_transaction(&mut transaction, derivation_paths, &runtime.signer()).await?; - - Ok((transaction, signers)) + sign_transaction( + &mut transaction, + [MINTER_DERIVATION_PATH], + &runtime.signer(), + ) + .await?; + Ok(transaction) } // Sign transaction, return error if it exceeds the maximum transaction size. diff --git a/minter/src/sol_transfer/tests.rs b/minter/src/sol_transfer/tests.rs index 3ea350d7..874ad3af 100644 --- a/minter/src/sol_transfer/tests.rs +++ b/minter/src/sol_transfer/tests.rs @@ -136,187 +136,28 @@ mod sweep_tests { mod batch_withdrawal_tests { use super::*; + use solana_message::{MessageHeader, compiled_instruction::CompiledInstruction}; - #[tokio::test] - async fn should_create_batch_withdrawal_with_single_target() { - setup(); - let target = Address::new_from_array([0xAA; 32]); - let amount: Lamport = 500_000_000; - let blockhash = Hash::new_from_array([0xBB; 32]); - - let (tx, signers) = create_signed_batch_withdrawal_transaction( - &minter_signing_once(), - &[(target, amount)], - blockhash, - ) - .await - .expect("transaction creation should succeed"); - - assert_eq!(signers, vec![Signer::Minter]); - assert_eq!(tx.signatures.len(), 1); - assert_eq!(tx.signatures[0], minter_signature()); - assert_eq!(tx.message.account_keys[0], MINTER_ADDRESS); - assert!(tx.message.account_keys.contains(&target)); - assert_eq!(tx.message.instructions.len(), 1); - assert_eq!(tx.message.recent_blockhash, blockhash); - } - - #[tokio::test] - async fn should_create_batch_withdrawal_with_multiple_targets() { - setup(); - let target_1 = Address::new_from_array([0xAA; 32]); - let target_2 = Address::new_from_array([0xBB; 32]); - let target_3 = Address::new_from_array([0xCC; 32]); - let blockhash = Hash::new_from_array([0xDD; 32]); - - let (tx, signers) = create_signed_batch_withdrawal_transaction( - &minter_signing_once(), - &[(target_1, 100), (target_2, 200), (target_3, 300)], - blockhash, - ) - .await - .expect("transaction creation should succeed"); - - // Only the minter signs - assert_eq!(signers, vec![Signer::Minter]); - assert_eq!(tx.signatures.len(), 1); - - // Fee payer is at position 0 - assert_eq!(tx.message.account_keys[0], MINTER_ADDRESS); - - // All targets are in account keys - assert!(tx.message.account_keys.contains(&target_1)); - assert!(tx.message.account_keys.contains(&target_2)); - assert!(tx.message.account_keys.contains(&target_3)); - - // One instruction per target - assert_eq!(tx.message.instructions.len(), 3); - } - - #[tokio::test] - async fn should_fail_when_signing_fails() { - setup(); - let target = Address::new_from_array([0xAA; 32]); - let blockhash = Hash::new_from_array([0xBB; 32]); - - let runtime = TestCanisterRuntime::new().add_signer(sign_as_minter().expect([Err( - SignCallError::CallFailed( - CallRejected::with_rejection(4, "signing service unavailable".to_string()).into(), - ), - )])); - - let result = - create_signed_batch_withdrawal_transaction(&runtime, &[(target, 100)], blockhash).await; - - assert!(result.is_err()); - } - - #[tokio::test] - async fn should_create_batch_withdrawal_at_max_capacity() { - setup(); - let blockhash = Hash::new_from_array([0xDD; 32]); - - let targets: Vec<(Address, Lamport)> = (0..MAX_WITHDRAWALS_PER_TX) - .map(|i| { - let mut addr = [0u8; 32]; - addr[0] = i as u8; - addr[1] = (i >> 8) as u8; - (Address::new_from_array(addr), 1_000_000) - }) - .collect(); - - let (tx, signers) = - create_signed_batch_withdrawal_transaction(&minter_signing_once(), &targets, blockhash) - .await - .expect("transaction creation should succeed at max capacity"); - - assert_eq!(signers, vec![Signer::Minter]); - assert_eq!(tx.signatures.len(), 1); - assert_eq!(tx.message.instructions.len(), MAX_WITHDRAWALS_PER_TX); - } - - #[tokio::test] - async fn should_charge_the_fee_reserved_per_batch() { - setup(); - let blockhash = Hash::new_from_array([0xDD; 32]); - let targets: Vec<(Address, Lamport)> = (0..MAX_WITHDRAWALS_PER_TX) - .map(|i| { - let mut addr = [0u8; 32]; - addr[0] = i as u8; - addr[1] = (i >> 8) as u8; - (Address::new_from_array(addr), 1_000_000) - }) - .collect(); - - let (tx, _signers) = - create_signed_batch_withdrawal_transaction(&minter_signing_once(), &targets, blockhash) - .await - .expect("transaction creation should succeed at max capacity"); - - assert_eq!( - VersionedMessage::Legacy(tx.message).transaction_fee(), - BATCH_WITHDRAWAL_TX_FEE - ); - } - - #[tokio::test] - async fn should_return_error_when_exceeding_tx_size_limit() { - setup(); - let blockhash = Hash::new_from_array([0xDD; 32]); - - // Each additional target adds ~49 bytes (32-byte key + 17-byte instruction). - // With a base of ~166 bytes and MAX_TX_SIZE = 1232, the limit is around 21-22. - // Use 25 targets to reliably exceed the limit. - const NUM_TARGETS: usize = 25; - let targets: Vec<(Address, Lamport)> = (0..NUM_TARGETS) - .map(|i| { - let mut addr = [0u8; 32]; - addr[0] = i as u8; - (Address::new_from_array(addr), 1_000_000) - }) - .collect(); - - let result = create_signed_batch_withdrawal_transaction( - &TestCanisterRuntime::new(), - &targets, - blockhash, - ) - .await; + const NONCE_ACCOUNT: Address = Address::new_from_array([0x11; 32]); + const TARGET_1: Address = Address::new_from_array([0xAA; 32]); + const TARGET_2: Address = Address::new_from_array([0xBB; 32]); - assert_matches!( - result, - Err(CreateTransferError::TransactionTooLarge { - max: MAX_TX_SIZE, - .. - }) - ); + fn nonce_value() -> Hash { + Hash::new_from_array([0xCC; 32]) } -} - -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; + /// Historical withdrawal events replay only if the builder always compiles to the same + /// message, so a failure here means a dependency bump broke replay — not that the + /// expected message needs updating. #[test] - fn should_build_the_message_bound_to_the_nonce() { - let nonce_value = Hash::new_from_array([0xAA; 32]); - + fn should_build_the_recorded_withdrawal_message() { let message = build_batch_withdrawal_message( &MINTER_ADDRESS, &NONCE_ACCOUNT, - nonce_value, - &[(SECOND_TARGET, 20_000_000), (FIRST_TARGET, 10_000_000)], + nonce_value(), + &[(TARGET_1, 100), (TARGET_2, 200)], ) - .expect("message should fit in a transaction"); + .expect("the message fits within the transaction size limit"); assert_eq!( message, @@ -328,42 +169,48 @@ mod batch_withdrawal_message_tests { }, account_keys: vec![ MINTER_ADDRESS, - FIRST_TARGET, - SECOND_TARGET, NONCE_ACCOUNT, - system_program::ID, - recent_blockhashes::ID, + TARGET_1, + TARGET_2, + solana_system_interface::program::ID, + solana_sdk_ids::sysvar::recent_blockhashes::ID, ], - recent_blockhash: nonce_value, + recent_blockhash: nonce_value(), instructions: vec![ advance_nonce_instruction(), - transfer_instruction(2, 20_000_000), - transfer_instruction(1, 10_000_000), + transfer_instruction(2, 100), + transfer_instruction(3, 200), ], } ); } #[test] - fn should_fit_max_withdrawals_with_distinct_targets() { + fn should_fit_a_full_batch_within_the_maximum_transaction_size() { let message = build_batch_withdrawal_message( &MINTER_ADDRESS, &NONCE_ACCOUNT, - Hash::new_from_array([0xAA; 32]), - &distinct_transfers(MAX_WITHDRAWALS_PER_TX), + nonce_value(), + &targets(MAX_WITHDRAWALS_PER_NONCE_TX), ) - .expect("message should fit in a transaction at max capacity"); + .expect("a full batch fits within the transaction size limit"); - assert_eq!(message.instructions.len(), MAX_WITHDRAWALS_PER_TX + 1); + let transaction_size = 1 + message.serialize().len() + BYTES_PER_SIGNATURE; + assert!( + transaction_size <= MAX_TX_SIZE, + "transaction size {transaction_size} exceeds {MAX_TX_SIZE}" + ); } #[test] - fn should_reject_more_than_max_withdrawals_with_distinct_targets() { + fn should_reject_a_message_exceeding_the_maximum_transaction_size() { + const NUM_TARGETS: usize = 25; + let result = build_batch_withdrawal_message( &MINTER_ADDRESS, &NONCE_ACCOUNT, - Hash::new_from_array([0xAA; 32]), - &distinct_transfers(MAX_WITHDRAWALS_PER_TX + 1), + nonce_value(), + &targets(NUM_TARGETS), ); assert_matches!( @@ -375,24 +222,86 @@ mod batch_withdrawal_message_tests { ); } - fn distinct_transfers(count: usize) -> Vec<(Address, Lamport)> { + #[test] + fn should_charge_the_fee_reserved_per_batch() { + let message = build_batch_withdrawal_message( + &MINTER_ADDRESS, + &NONCE_ACCOUNT, + nonce_value(), + &targets(MAX_WITHDRAWALS_PER_NONCE_TX), + ) + .expect("a full batch fits within the transaction size limit"); + + assert_eq!( + VersionedMessage::Legacy(message).transaction_fee(), + BATCH_WITHDRAWAL_TX_FEE + ); + } + + #[tokio::test] + async fn should_sign_the_message_verbatim_with_the_minter_key_only() { + setup(); + let message = build_batch_withdrawal_message( + &MINTER_ADDRESS, + &NONCE_ACCOUNT, + nonce_value(), + &[(TARGET_1, 100)], + ) + .expect("the message fits within the transaction size limit"); + + let transaction = sign_batch_withdrawal_message(&minter_signing_once(), message.clone()) + .await + .expect("signing should succeed"); + + assert_eq!(transaction.signatures, vec![minter_signature()]); + assert_eq!(transaction.message, message); + } + + #[tokio::test] + async fn should_fail_when_signing_fails() { + setup(); + let message = build_batch_withdrawal_message( + &MINTER_ADDRESS, + &NONCE_ACCOUNT, + nonce_value(), + &[(TARGET_1, 100)], + ) + .expect("the message fits within the transaction size limit"); + + let runtime = TestCanisterRuntime::new().add_signer(sign_as_minter().expect([Err( + SignCallError::CallFailed( + CallRejected::with_rejection(4, "signing service unavailable".to_string()).into(), + ), + )])); + + let result = sign_batch_withdrawal_message(&runtime, message).await; + + assert!(result.is_err()); + } + + fn targets(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) + let mut address = [0u8; 32]; + address[0] = i as u8; + address[1] = (i >> 8) as u8; + (Address::new_from_array(address), 1_000_000) }) .collect() } fn advance_nonce_instruction() -> CompiledInstruction { const ADVANCE_NONCE_ACCOUNT_DISCRIMINANT: u32 = 4; + const SYSTEM_PROGRAM_INDEX: u8 = 4; + const NONCE_ACCOUNT_INDEX: u8 = 1; + const RECENT_BLOCKHASHES_SYSVAR_INDEX: u8 = 5; + const MINTER_INDEX: u8 = 0; CompiledInstruction { program_id_index: SYSTEM_PROGRAM_INDEX, accounts: vec![ NONCE_ACCOUNT_INDEX, - RECENT_BLOCKHASHES_INDEX, - MINTER_ADDRESS_INDEX, + RECENT_BLOCKHASHES_SYSVAR_INDEX, + MINTER_INDEX, ], data: ADVANCE_NONCE_ACCOUNT_DISCRIMINANT.to_le_bytes().to_vec(), } @@ -400,11 +309,13 @@ mod batch_withdrawal_message_tests { fn transfer_instruction(to_index: u8, amount: Lamport) -> CompiledInstruction { const TRANSFER_DISCRIMINANT: u32 = 2; + const SYSTEM_PROGRAM_INDEX: u8 = 4; + const MINTER_INDEX: u8 = 0; 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], + accounts: vec![MINTER_INDEX, to_index], data, } } diff --git a/minter/src/state/event.rs b/minter/src/state/event.rs index 4e4e4da5..89408e05 100644 --- a/minter/src/state/event.rs +++ b/minter/src/state/event.rs @@ -310,23 +310,11 @@ pub enum TransactionPurpose { #[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, - }, /// 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 { + Withdrawal { /// 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/mod.rs b/minter/src/state/mod.rs index 942e5ab9..e4efc0da 100644 --- a/minter/src/state/mod.rs +++ b/minter/src/state/mod.rs @@ -5,7 +5,7 @@ use crate::{ numeric::{LedgerBurnIndex, LedgerMintIndex}, rpc::BlockHeight, sol_transfer::{ - BATCH_WITHDRAWAL_TX_FEE, MAX_SIGNATURES, MAX_WITHDRAWALS_PER_TX, + BATCH_WITHDRAWAL_TX_FEE, MAX_SIGNATURES, MAX_WITHDRAWALS_PER_NONCE_TX, build_batch_withdrawal_message, }, state::event::{ @@ -211,6 +211,19 @@ impl State { &self.created_withdrawal_txs } + /// Transiently reserves up to `max` free durable nonce accounts for + /// withdrawal batches being processed, so that concurrently processed + /// batches can never pick the same account. + pub fn reserve_nonce_accounts(&mut self, max: usize) -> Vec
{ + self.nonce_pool.reserve_accounts(max) + } + + /// Releases the transient reservation of a nonce account whose batch was + /// not submitted. + pub fn unreserve_nonce_account(&mut self, address: &Address) { + self.nonce_pool.unreserve(address); + } + pub fn transactions_to_resubmit(&self) -> &InsertionOrderedMap { &self.transactions_to_resubmit } @@ -232,13 +245,7 @@ impl State { }); 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" - ), - MinterTransaction::NonceWithdrawal { .. } => { + MinterTransaction::Withdrawal { .. } => { panic!("BUG: durable-nonce withdrawal transaction {signature} cannot expire") } } @@ -508,10 +515,14 @@ impl State { pub fn withdrawal_batches(&self) -> WithdrawalBatches<'_> { WithdrawalBatches { pending_requests: self.pending_withdrawal_requests.values().peekable(), - available_balance: self.balance, + available_balance: self.balance.saturating_sub(RENT_EXEMPTION_THRESHOLD), } } + pub fn can_create_withdrawal_transaction(&self) -> bool { + self.nonce_pool.num_free_accounts() > 0 && self.withdrawal_batches().next().is_some() + } + /// Returns the creation timestamp (in nanoseconds) of the oldest incomplete withdrawal request. /// An incomplete withdrawal is one that has not yet been finalized (succeeded or failed). pub fn oldest_incomplete_withdrawal_created_at(&self) -> Option { @@ -560,44 +571,7 @@ impl State { let message = transaction.clone(); let signers = signers.to_vec(); let submitted_transaction = match purpose { - TransactionPurpose::Withdrawal { - burn_indices, - block_height, - } => { - 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 send transaction for unknown withdrawal request: {burn_index:?}") - }); - total += pending.request.amount_to_transfer; - 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:?}" - ); - } - let tx_fee = transaction.transaction_fee(); - self.balance = self - .balance - .checked_sub(total + tx_fee) - .expect("BUG: insufficient minter balance for withdrawal"); - MinterTransaction::Withdrawal { - message, - signers, - block_height: *block_height, - } - } - TransactionPurpose::NonceWithdrawal { burn_indices } => { + TransactionPurpose::Withdrawal { burn_indices } => { self.send_nonce_withdrawal(signature, message, signers, burn_indices) } TransactionPurpose::SweepDeposit { @@ -722,7 +696,7 @@ impl State { "Attempted to send transaction for already sent withdrawal request: {burn_index:?}" ); } - MinterTransaction::NonceWithdrawal { + MinterTransaction::Withdrawal { message, signers, nonce_account, @@ -772,8 +746,8 @@ impl State { fn process_transaction_resubmitted( &mut self, old_signature: &Signature, - new_signature: &Signature, - new_block_height: BlockHeight, + _new_signature: &Signature, + _new_block_height: BlockHeight, ) { let old_transaction = self .transactions_to_resubmit @@ -781,41 +755,13 @@ impl State { .unwrap_or_else(|| { panic!("Attempted to resubmit unknown transaction with signature {old_signature:?}") }); - let new_transaction = match old_transaction { + match old_transaction { MinterTransaction::SweepDeposit { .. } => panic!( "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, - }, - MinterTransaction::NonceWithdrawal { .. } => panic!( + MinterTransaction::Withdrawal { .. } => panic!( "BUG: durable-nonce withdrawal transaction {old_signature} must never be resubmitted" ), - }; - assert!( - !self.succeeded_transactions.contains(new_signature), - "Attempted to resubmit with signature {new_signature:?} that already succeeded" - ); - assert!( - !self.failed_transactions.contains_key(new_signature), - "Attempted to resubmit with signature {new_signature:?} that already failed" - ); - assert_eq!( - self.submitted_transactions - .insert(*new_signature, new_transaction), - None, - "Attempted to resubmit transaction with signature {new_signature:?} that already exists" - ); - for sent in self.sent_withdrawal_requests.values_mut() { - if &sent.signature == old_signature { - sent.signature = *new_signature; - } } } @@ -832,8 +778,7 @@ impl State { }); match transaction { MinterTransaction::SweepDeposit { .. } => self.deposits.finalize_swept(signature), - MinterTransaction::Withdrawal { .. } => {} - MinterTransaction::NonceWithdrawal { nonce_account, .. } => { + MinterTransaction::Withdrawal { nonce_account, .. } => { self.nonce_pool.free(&nonce_account) } } @@ -865,8 +810,7 @@ impl State { }); match &transaction { MinterTransaction::SweepDeposit { .. } => self.deposits.drop_swept(signature), - MinterTransaction::Withdrawal { .. } => {} - MinterTransaction::NonceWithdrawal { nonce_account, .. } => { + MinterTransaction::Withdrawal { nonce_account, .. } => { self.nonce_pool.free(nonce_account) } } @@ -995,7 +939,9 @@ pub struct PendingWithdrawalRequest { } /// Groups pending withdrawal requests, oldest first, into batches that the -/// minter balance can pay for, including one transaction fee per batch. +/// minter balance can pay for, including one transaction fee per batch and +/// holding back the rent exemption threshold so a withdrawal transaction can +/// never leave the main address below it. /// /// Iteration stops at the first request the remaining balance cannot cover, /// so requests are never reordered or skipped. @@ -1009,7 +955,7 @@ impl Iterator for WithdrawalBatches<'_> { fn next(&mut self) -> Option { let mut batch = Vec::new(); - while batch.len() < MAX_WITHDRAWALS_PER_TX { + while batch.len() < MAX_WITHDRAWALS_PER_NONCE_TX { let reserved_fee = if batch.is_empty() { BATCH_WITHDRAWAL_TX_FEE } else { @@ -1076,15 +1022,9 @@ pub enum MinterTransaction { /// 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, - }, /// A durable-nonce withdrawal transaction, which never expires: it stays /// in flight until it is finalized. - NonceWithdrawal { + Withdrawal { message: VersionedMessage, signers: Vec, /// The durable nonce account whose nonce value the transaction uses. @@ -1098,16 +1038,14 @@ impl MinterTransaction { pub fn message(&self) -> &VersionedMessage { match self { MinterTransaction::SweepDeposit { message, .. } - | MinterTransaction::Withdrawal { message, .. } - | MinterTransaction::NonceWithdrawal { message, .. } => message, + | MinterTransaction::Withdrawal { message, .. } => message, } } pub fn signers(&self) -> &[Signer] { match self { MinterTransaction::SweepDeposit { signers, .. } - | MinterTransaction::Withdrawal { signers, .. } - | MinterTransaction::NonceWithdrawal { signers, .. } => signers, + | MinterTransaction::Withdrawal { signers, .. } => signers, } } @@ -1115,9 +1053,8 @@ impl MinterTransaction { /// 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, .. } => Some(*block_height), - MinterTransaction::NonceWithdrawal { .. } => None, + MinterTransaction::SweepDeposit { block_height, .. } => Some(*block_height), + MinterTransaction::Withdrawal { .. } => None, } } } diff --git a/minter/src/state/nonce_pool/mod.rs b/minter/src/state/nonce_pool/mod.rs index 8085436d..e16f3b88 100644 --- a/minter/src/state/nonce_pool/mod.rs +++ b/minter/src/state/nonce_pool/mod.rs @@ -41,6 +41,37 @@ impl DurableNoncePool { Ok(()) } + /// Reserves up to `max` free accounts for withdrawal batches being processed, + /// so that concurrently processed batches can never pick the same account. + /// + /// A reservation is transient: it is either released with [`Self::unreserve`] + /// in the same timer round or superseded by [`Self::bind`]. + pub(super) fn reserve_accounts(&mut self, max: usize) -> Vec
{ + self.accounts + .iter_mut() + .filter(|(_, account)| account.is_free()) + .take(max) + .map(|(address, account)| { + account.state = NonceAccountState::Reserved; + *address + }) + .collect() + } + + /// Releases the reservation of an account whose batch was not submitted. + /// + /// # Panics + /// Panics if the account is not reserved. + pub(super) fn unreserve(&mut self, address: &Address) { + let account = self.account_mut(address); + assert_eq!( + account.state, + NonceAccountState::Reserved, + "BUG: cannot unreserve nonce account {address} that is not reserved" + ); + account.state = NonceAccountState::Free; + } + /// Binds the account to an in-flight withdrawal transaction carrying /// `nonce_value`, recording the value as seen. /// @@ -50,7 +81,7 @@ impl DurableNoncePool { pub(super) fn bind(&mut self, address: &Address, nonce_value: Hash) { let account = self.account_mut(address); match account.state { - NonceAccountState::Free => {} + NonceAccountState::Free | NonceAccountState::Reserved => {} NonceAccountState::Bound => { panic!("BUG: nonce account {address} is already bound to an in-flight transaction") } @@ -70,12 +101,20 @@ impl DurableNoncePool { let account = self.account_mut(address); match account.state { NonceAccountState::Bound => account.state = NonceAccountState::Free, - NonceAccountState::Free => { + NonceAccountState::Free | NonceAccountState::Reserved => { panic!("BUG: cannot free nonce account {address} that is not bound") } } } + /// Whether `nonce_value` was already bound to a transaction of this account, + /// in which case a read returning it is stale. + pub fn has_seen(&self, address: &Address, nonce_value: &Hash) -> bool { + self.accounts + .get(address) + .is_some_and(|account| account.seen_nonce_values.contains(nonce_value)) + } + pub fn addresses(&self) -> impl Iterator { self.accounts.keys() } @@ -88,6 +127,13 @@ impl DurableNoncePool { self.accounts.is_empty() } + pub fn num_free_accounts(&self) -> usize { + self.accounts + .values() + .filter(|account| account.is_free()) + .count() + } + fn account_mut(&mut self, address: &Address) -> &mut NonceAccount { self.accounts .get_mut(address) @@ -105,12 +151,21 @@ struct NonceAccount { seen_nonce_values: BTreeSet, } +impl NonceAccount { + fn is_free(&self) -> bool { + self.state == NonceAccountState::Free + } +} + /// The lifecycle state of a durable nonce account in the pool. #[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 transiently picked for a withdrawal batch being processed. + /// Reservations never survive a replay of the event log. + Reserved, /// The account is bound to an in-flight withdrawal transaction. Bound, } diff --git a/minter/src/state/nonce_pool/tests.rs b/minter/src/state/nonce_pool/tests.rs index 16d1a2b3..48a83aec 100644 --- a/minter/src/state/nonce_pool/tests.rs +++ b/minter/src/state/nonce_pool/tests.rs @@ -32,6 +32,43 @@ fn should_leave_the_pool_unchanged_when_an_add_fails() { assert_eq!(pool, pool_of([address(1)])); } +#[test] +fn should_reserve_at_most_the_free_accounts() { + let mut pool = pool_of([address(1), address(2)]); + + assert_eq!(pool.reserve_accounts(3), vec![address(1), address(2)]); + assert_eq!(pool.reserve_accounts(1), vec![]); +} + +#[test] +fn should_not_reserve_a_bound_account() { + let mut pool = pool_of([address(1), address(2)]); + pool.bind(&address(1), durable_nonce(1)); + + assert_eq!(pool.reserve_accounts(2), vec![address(2)]); +} + +#[test] +fn should_reserve_an_unreserved_account_again() { + let mut pool = pool_of([address(1)]); + assert_eq!(pool.reserve_accounts(1), vec![address(1)]); + + pool.unreserve(&address(1)); + + assert_eq!(pool.reserve_accounts(1), vec![address(1)]); +} + +#[test] +fn should_free_a_bound_account_for_a_new_reservation() { + let mut pool = pool_of([address(1)]); + pool.bind(&address(1), durable_nonce(1)); + assert_eq!(pool.reserve_accounts(1), vec![]); + + pool.free(&address(1)); + + assert_eq!(pool.reserve_accounts(1), vec![address(1)]); +} + #[test] #[should_panic(expected = "already bound")] fn should_panic_when_binding_a_bound_account() { @@ -68,6 +105,34 @@ fn should_panic_when_freeing_an_unbound_account() { pool.free(&address(1)); } +#[test] +fn should_remember_the_nonce_values_of_past_bindings() { + 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)); + + assert!(pool.has_seen(&address(1), &durable_nonce(1))); + assert!(pool.has_seen(&address(1), &durable_nonce(2))); + assert!(!pool.has_seen(&address(1), &durable_nonce(3))); +} + +#[test] +fn should_count_no_free_accounts_in_an_empty_pool() { + assert_eq!(DurableNoncePool::default().num_free_accounts(), 0); +} + +#[test] +fn should_count_only_the_free_accounts() { + let mut pool = pool_of([address(1), address(2), address(3)]); + assert_eq!(pool.num_free_accounts(), 3); + + pool.bind(&address(1), durable_nonce(1)); + assert_eq!(pool.reserve_accounts(1), vec![address(2)]); + + assert_eq!(pool.num_free_accounts(), 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 0dc8a252..844b9b72 100644 --- a/minter/src/state/tests.rs +++ b/minter/src/state/tests.rs @@ -886,38 +886,6 @@ fn should_track_balance_through_deposits_withdrawals_and_failures() { const TRANSFER_1: u64 = WITHDRAWAL_1 - WITHDRAWAL_FEE; const TRANSFER_2: u64 = WITHDRAWAL_2 - WITHDRAWAL_FEE; - /// Creates a Solana message with the given number of required signatures. - fn message_with_signers(num_signers: u8) -> solana_message::Message { - solana_message::Message { - header: solana_message::MessageHeader { - num_required_signatures: num_signers, - num_readonly_signed_accounts: 0, - num_readonly_unsigned_accounts: 0, - }, - account_keys: vec![], - recent_blockhash: Default::default(), - instructions: vec![], - } - } - - fn submit_transaction(sig: Signature, num_signers: u8, purpose: TransactionPurpose) { - let signers: Vec<_> = (0..num_signers) - .map(|i| Signer::Account(account(100 + i as usize))) - .collect(); - mutate_state(|state| { - process_event( - state, - EventType::SubmittedTransaction { - signature: sig, - message: message_with_signers(num_signers).into(), - signers, - purpose, - }, - &TestCanisterRuntime::new().add_times([0, 0]), - ) - }); - } - init_state(); init_schnorr_master_key(); assert_eq!(read_state(|s| s.balance()), 0); @@ -942,15 +910,8 @@ fn should_track_balance_through_deposits_withdrawals_and_failures() { accept_withdrawal(account(4), 1, WITHDRAWAL_2); assert_eq!(read_state(|s| s.balance()), expected); - // Submitting a withdrawal (1 signer): balance -= total_transfers + tx_fee - submit_transaction( - signature(0xBB), - 1, - TransactionPurpose::Withdrawal { - burn_indices: vec![0.into(), 1.into()], - block_height: BlockHeight::new(0), - }, - ); + // Creating a withdrawal transaction (1 signature): balance -= total_transfers + tx_fee + submit_withdrawal(signature(0xBB), vec![0, 1]); let expected = expected - TRANSFER_1 - TRANSFER_2 - FEE_PER_SIGNATURE; assert_eq!(read_state(|s| s.balance()), expected); @@ -1068,50 +1029,22 @@ mod oldest_incomplete_withdrawal_created_at { None ); } - - #[test] - fn should_preserve_created_at_through_resubmission() { - init_state(); - init_balance(); - accept_withdrawal_at(account(1), 0, AMOUNT, 1_000_000_000); - accept_withdrawal_at(account(2), 1, AMOUNT, 2_000_000_000); - - submit_withdrawal(signature(0xAA), vec![0, 1]); - - assert_eq!( - read_state(|s| s.oldest_incomplete_withdrawal_created_at()), - Some(1_000_000_000) - ); - - // Expire then resubmit the transaction with a new signature - expire_transaction(signature(0xAA)); - resubmit_transaction(signature(0xAA), signature(0xBB)); - - // created_at timestamps should be unchanged - assert_eq!( - read_state(|s| s.oldest_incomplete_withdrawal_created_at()), - Some(1_000_000_000) - ); - - // Finalize the resubmitted transaction - succeed_transaction(signature(0xBB)); - - assert_eq!( - read_state(|s| s.oldest_incomplete_withdrawal_created_at()), - None - ); - } } mod withdrawal_batches { use super::*; - use crate::sol_transfer::{BATCH_WITHDRAWAL_TX_FEE, MAX_WITHDRAWALS_PER_TX}; + use crate::{ + sol_transfer::{BATCH_WITHDRAWAL_TX_FEE, MAX_WITHDRAWALS_PER_NONCE_TX}, + test_fixtures::durable_nonce, + }; - const MAX_AMOUNT_TO_TRANSFER: Lamport = u64::MAX - WITHDRAWAL_FEE - BATCH_WITHDRAWAL_TX_FEE; - const NUM_REQUESTS_FOR_TWO_BATCHES: usize = MAX_WITHDRAWALS_PER_TX + 1; + const MAX_AMOUNT_TO_TRANSFER: Lamport = + u64::MAX - WITHDRAWAL_FEE - BATCH_WITHDRAWAL_TX_FEE - RENT_EXEMPTION_THRESHOLD; + const NUM_REQUESTS_FOR_TWO_BATCHES: usize = MAX_WITHDRAWALS_PER_NONCE_TX + 1; const COST_OF_TWO_BATCHES: Lamport = NUM_REQUESTS_FOR_TWO_BATCHES as u64 * MINIMUM_WITHDRAWAL_AMOUNT - + 2 * BATCH_WITHDRAWAL_TX_FEE; + + 2 * BATCH_WITHDRAWAL_TX_FEE + + RENT_EXEMPTION_THRESHOLD; #[test] fn should_be_empty_when_no_pending_withdrawals() { @@ -1126,7 +1059,7 @@ mod withdrawal_batches { amount_to_transfer in MINIMUM_WITHDRAWAL_AMOUNT..=MAX_AMOUNT_TO_TRANSFER ) { let mut state = state(); - state.balance = amount_to_transfer + BATCH_WITHDRAWAL_TX_FEE; + state.balance = amount_to_transfer + BATCH_WITHDRAWAL_TX_FEE + RENT_EXEMPTION_THRESHOLD; let requests = [withdrawal_request(0, amount_to_transfer)]; accept_withdrawal_requests(&mut state, requests.clone()); @@ -1141,7 +1074,8 @@ mod withdrawal_batches { shortfall in 1..=MINIMUM_WITHDRAWAL_AMOUNT + BATCH_WITHDRAWAL_TX_FEE ) { let mut state = state(); - state.balance = amount_to_transfer + BATCH_WITHDRAWAL_TX_FEE - shortfall; + state.balance = amount_to_transfer + BATCH_WITHDRAWAL_TX_FEE + RENT_EXEMPTION_THRESHOLD + - shortfall; let requests = [withdrawal_request(0, amount_to_transfer)]; accept_withdrawal_requests(&mut state, requests); @@ -1162,8 +1096,8 @@ mod withdrawal_batches { prop_assert_eq!( batches, vec![ - requests[..MAX_WITHDRAWALS_PER_TX].to_vec(), - requests[MAX_WITHDRAWALS_PER_TX..].to_vec() + requests[..MAX_WITHDRAWALS_PER_NONCE_TX].to_vec(), + requests[MAX_WITHDRAWALS_PER_NONCE_TX..].to_vec() ] ); } @@ -1179,10 +1113,25 @@ mod withdrawal_batches { let batches: Vec<_> = state.withdrawal_batches().collect(); - prop_assert_eq!(batches, vec![requests[..MAX_WITHDRAWALS_PER_TX].to_vec()]); + prop_assert_eq!(batches, vec![requests[..MAX_WITHDRAWALS_PER_NONCE_TX].to_vec()]); } } + #[test] + fn should_hold_back_the_rent_exemption_threshold() { + let mut state = state(); + let requests = [withdrawal_request(0, MINIMUM_WITHDRAWAL_AMOUNT)]; + accept_withdrawal_requests(&mut state, requests.clone()); + state.balance = + MINIMUM_WITHDRAWAL_AMOUNT + BATCH_WITHDRAWAL_TX_FEE + RENT_EXEMPTION_THRESHOLD - 1; + + assert_eq!(state.withdrawal_batches().next(), None); + + state.balance += 1; + + assert_eq!(state.withdrawal_batches().next(), Some(requests.to_vec())); + } + #[test] fn should_be_empty_when_cost_overflows() { let mut state = state(); @@ -1200,7 +1149,8 @@ mod withdrawal_batches { #[test] fn should_stop_at_first_unaffordable_request_without_skipping_it() { let mut state = state(); - state.balance = 2 * MINIMUM_WITHDRAWAL_AMOUNT + BATCH_WITHDRAWAL_TX_FEE; + state.balance = + 2 * MINIMUM_WITHDRAWAL_AMOUNT + BATCH_WITHDRAWAL_TX_FEE + RENT_EXEMPTION_THRESHOLD; let requests = [ withdrawal_request(0, MINIMUM_WITHDRAWAL_AMOUNT), withdrawal_request(1, 2 * MINIMUM_WITHDRAWAL_AMOUNT), @@ -1213,6 +1163,57 @@ mod withdrawal_batches { assert_eq!(batches, vec![vec![requests[0].clone()]]); } + #[test] + fn should_not_create_a_transaction_without_pending_withdrawals() { + let mut state = state(); + state.balance = u64::MAX; + state.nonce_pool.add_accounts([address(1)]).unwrap(); + + assert!(!state.can_create_withdrawal_transaction()); + } + + #[test] + fn should_not_create_a_transaction_without_a_free_nonce_account() { + let mut state = state(); + state.balance = u64::MAX; + accept_withdrawal_requests( + &mut state, + [withdrawal_request(0, MINIMUM_WITHDRAWAL_AMOUNT)], + ); + state.nonce_pool.add_accounts([address(1)]).unwrap(); + state.nonce_pool.bind(&address(1), durable_nonce(1)); + + assert!(!state.can_create_withdrawal_transaction()); + } + + #[test] + fn should_not_create_a_transaction_without_an_affordable_batch() { + let mut state = state(); + state.balance = + MINIMUM_WITHDRAWAL_AMOUNT + BATCH_WITHDRAWAL_TX_FEE + RENT_EXEMPTION_THRESHOLD - 1; + accept_withdrawal_requests( + &mut state, + [withdrawal_request(0, MINIMUM_WITHDRAWAL_AMOUNT)], + ); + state.nonce_pool.add_accounts([address(1)]).unwrap(); + + assert!(!state.can_create_withdrawal_transaction()); + } + + #[test] + fn should_create_a_transaction_with_an_affordable_batch_and_a_free_nonce_account() { + let mut state = state(); + state.balance = + MINIMUM_WITHDRAWAL_AMOUNT + BATCH_WITHDRAWAL_TX_FEE + RENT_EXEMPTION_THRESHOLD; + accept_withdrawal_requests( + &mut state, + [withdrawal_request(0, MINIMUM_WITHDRAWAL_AMOUNT)], + ); + state.nonce_pool.add_accounts([address(1)]).unwrap(); + + assert!(state.can_create_withdrawal_transaction()); + } + fn state() -> State { State::try_from(valid_init_args()).unwrap() } @@ -1295,7 +1296,7 @@ mod withdrawal_transactions { 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 { + let MinterTransaction::Withdrawal { nonce_account, nonce_value, .. @@ -1351,7 +1352,7 @@ mod withdrawal_transactions { assert!(s.submitted_transactions().is_empty()); assert_matches!( s.failed_transactions().get(&signature(7)), - Some(MinterTransaction::NonceWithdrawal { .. }) + Some(MinterTransaction::Withdrawal { .. }) ); assert_eq!( s.withdrawal_status(0), @@ -1365,7 +1366,7 @@ mod withdrawal_transactions { #[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())); + let mut replayed = replay_events(log_of(funded_log_until_created_transaction())); assert_eq!( replayed.balance(), @@ -1384,7 +1385,7 @@ mod withdrawal_transactions { Some(durable_nonce(1)) ); assert_eq!(replayed.withdrawal_status(0), WithdrawalStatus::Pending); - assert_eq!(replayed.nonce_pool, pool_bound_to(durable_nonce(1))); + assert_eq!(replayed.reserve_nonce_accounts(1), vec![]); } #[test] @@ -1399,7 +1400,7 @@ mod withdrawal_transactions { ) .into(), signers: vec![Signer::Minter], - purpose: TransactionPurpose::NonceWithdrawal { + purpose: TransactionPurpose::Withdrawal { burn_indices: vec![0_u64.into()], }, }); @@ -1411,7 +1412,7 @@ mod withdrawal_transactions { .submitted_transactions() .get(&signature(7)) .unwrap(); - let MinterTransaction::NonceWithdrawal { + let MinterTransaction::Withdrawal { nonce_account, nonce_value, .. @@ -1451,7 +1452,7 @@ mod withdrawal_transactions { &signature(7), &message_without_nonce_advance.into(), &[Signer::Minter], - &TransactionPurpose::NonceWithdrawal { + &TransactionPurpose::Withdrawal { burn_indices: vec![0_u64.into()], }, ) @@ -1476,7 +1477,7 @@ mod withdrawal_transactions { &signature(7), &message_with_another_amount.into(), &[Signer::Minter], - &TransactionPurpose::NonceWithdrawal { + &TransactionPurpose::Withdrawal { burn_indices: vec![0_u64.into()], }, ) @@ -1561,15 +1562,7 @@ mod withdrawal_transactions { } 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 + mutate_state(|s| assert_eq!(s.reserve_nonce_accounts(1), vec![NONCE_ACCOUNT])); + mutate_state(|s| s.unreserve_nonce_account(&NONCE_ACCOUNT)); } } diff --git a/minter/src/test_fixtures/mod.rs b/minter/src/test_fixtures/mod.rs index 7315d0df..b001fa44 100644 --- a/minter/src/test_fixtures/mod.rs +++ b/minter/src/test_fixtures/mod.rs @@ -724,15 +724,6 @@ pub mod events { use solana_address::Address; use solana_signature::Signature; - 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(), - ) - } - /// The runtime is only used by [`process_event`] to supply timestamps /// for the state transition and for the event log entry. fn runtime() -> TestCanisterRuntime { @@ -778,10 +769,27 @@ pub mod events { submit_sweep_to(signature, deposit_ids, MINTER_ADDRESS) } + pub fn submit_sweep_at_height( + signature: Signature, + deposit_ids: Vec, + block_height: BlockHeight, + ) { + submit_sweep_to_at_height(signature, deposit_ids, MINTER_ADDRESS, block_height) + } + pub fn submit_sweep_to( signature: Signature, deposit_ids: Vec, minter_address: Address, + ) { + submit_sweep_to_at_height(signature, deposit_ids, minter_address, DEFAULT_BLOCK_HEIGHT) + } + + fn submit_sweep_to_at_height( + signature: Signature, + deposit_ids: Vec, + minter_address: Address, + block_height: BlockHeight, ) { let deposits: Vec<_> = read_state(|state| { deposit_ids @@ -807,7 +815,7 @@ pub mod events { signers, purpose: TransactionPurpose::SweepDeposit { deposit_ids, - block_height: DEFAULT_BLOCK_HEIGHT, + block_height, }, }, &runtime(), @@ -941,7 +949,48 @@ pub mod events { signature, message: message.into(), signers: vec![Signer::Minter], - purpose: TransactionPurpose::NonceWithdrawal { burn_indices }, + purpose: TransactionPurpose::Withdrawal { burn_indices }, + }, + &runtime(), + ) + }); + } + + /// Marks the given withdrawals as sent under `signature` by creating and + /// submitting a withdrawal transaction bound to a dedicated nonce account + /// derived from the signature, which is first added to the pool through an + /// upgrade event, so that repeated calls never contend for one account. + pub fn submit_withdrawal(signature: Signature, burn_indices: Vec) { + let nonce_account = add_nonce_account_of(&signature); + let nonce_value = nonce_value_of(&signature); + let burn_indices: Vec = burn_indices + .into_iter() + .map(LedgerBurnIndex::from) + .collect(); + mutate_state(|state| { + process_event( + state, + EventType::CreatedWithdrawalTransaction { + burn_indices: burn_indices.clone(), + nonce_account, + nonce_value, + }, + &runtime(), + ) + }); + 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::Withdrawal { burn_indices }, }, &runtime(), ) @@ -963,33 +1012,28 @@ pub mod events { }) } - pub fn submit_withdrawal(signature: Signature, burn_indices: Vec) { - submit_withdrawal_at_height(signature, DEFAULT_BLOCK_HEIGHT, burn_indices); - } - - pub fn submit_withdrawal_at_height( - signature: Signature, - block_height: BlockHeight, - burn_indices: Vec, - ) { + fn add_nonce_account_of(signature: &Signature) -> Address { + use sha2::Digest; + let digest = sha2::Sha256::digest(signature.as_ref()); + let nonce_account = Address::from(<[u8; 32]>::from(digest)); mutate_state(|state| { process_event( state, - EventType::SubmittedTransaction { - signature, - message: message().into(), - signers: vec![Signer::Minter], - purpose: TransactionPurpose::Withdrawal { - burn_indices: burn_indices - .into_iter() - .map(LedgerBurnIndex::from) - .collect(), - block_height, - }, - }, + EventType::Upgrade(cksol_types_internal::UpgradeArgs { + nonce_accounts_to_add: Some(vec![nonce_account.to_string()]), + ..cksol_types_internal::UpgradeArgs::default() + }), &runtime(), ) }); + nonce_account + } + + fn nonce_value_of(signature: &Signature) -> solana_hash::Hash { + let bytes: [u8; 32] = signature.as_ref()[32..] + .try_into() + .expect("BUG: a signature holds exactly 64 bytes"); + solana_hash::Hash::from(bytes) } pub fn succeed_transaction(signature: Signature) { @@ -1387,18 +1431,8 @@ pub mod arb { 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 }), + .prop_map(|burn_indices| TransactionPurpose::Withdrawal { burn_indices }), ] } diff --git a/minter/src/test_fixtures/signer.rs b/minter/src/test_fixtures/signer.rs index f56ddbd1..06470d27 100644 --- a/minter/src/test_fixtures/signer.rs +++ b/minter/src/test_fixtures/signer.rs @@ -43,6 +43,7 @@ pub(super) fn derivation_path_signature( pub fn sign_for(account: &Account) -> SignerExpectation { SignerExpectation { derivation_path: derivation_path(account), + message: None, answers: Answers::Derived(1), } } @@ -52,6 +53,7 @@ pub fn sign_for(account: &Account) -> SignerExpectation { pub fn sign_as_minter() -> SignerExpectation { SignerExpectation { derivation_path: MINTER_DERIVATION_PATH, + message: None, answers: Answers::Derived(1), } } @@ -60,6 +62,7 @@ pub fn sign_as_minter() -> SignerExpectation { #[derive(Clone)] pub struct SignerExpectation { derivation_path: DerivationPath, + message: Option>, answers: Answers, } @@ -71,6 +74,12 @@ impl SignerExpectation { self } + /// Expects every signing request to be for exactly `message`. + pub fn of_message(mut self, message: Vec) -> Self { + self.message = Some(message); + self + } + /// Expects one signing request per given answer, in order. pub fn expect( mut self, @@ -174,8 +183,14 @@ impl MockSchnorrSigner { for signature in signatures { let expected_path = expectation.derivation_path.clone(); + let expected_message = expectation.message.clone(); mock.expect_sign() - .withf(move |_message, path| path == &expected_path) + .withf(move |message, path| { + path == &expected_path + && expected_message + .as_ref() + .is_none_or(|expected| expected == message) + }) .times(1) .return_once(move |_message, _path| { signature.map(|signature| signature.as_ref().to_vec()) diff --git a/minter/src/withdraw/mod.rs b/minter/src/withdraw/mod.rs index 7d027651..882a51eb 100644 --- a/minter/src/withdraw/mod.rs +++ b/minter/src/withdraw/mod.rs @@ -3,7 +3,10 @@ use std::time::Duration; use cksol_types::{WithdrawalError, WithdrawalOk, WithdrawalStatus}; use icrc_ledger_types::icrc1::account::Account; +use sol_rpc_types::Lamport; use solana_address::Address; +use solana_message::Message; +use solana_transaction::Transaction; use canlog::log; use cksol_types_internal::log::Priority; @@ -13,18 +16,21 @@ use crate::{ constants::MAX_CONCURRENT_RPC_CALLS, guard::{TimerGuard, withdrawal_guard}, ledger::{BurnError, burn}, - rpc::{Block, get_recent_block, submit_transaction}, + numeric::LedgerBurnIndex, + rpc::submit_transaction_skipping_preflight, runtime::CanisterRuntime, - sol_transfer::create_signed_batch_withdrawal_transaction, + sol_transfer::{build_batch_withdrawal_message, sign_batch_withdrawal_message}, state::{ - TaskType, + State, TaskType, audit::process_event, - event::{EventType, TransactionPurpose, VersionedMessage, WithdrawalRequest}, + event::{EventType, Signer, TransactionPurpose, WithdrawalRequest}, mutate_state, read_state, }, + withdraw::nonce::read_verified_nonce, }; pub const WITHDRAWAL_PROCESSING_DELAY: Duration = Duration::from_mins(1); +pub const WITHDRAWAL_PROCESSING_RETRY_DELAY: Duration = Duration::from_secs(10); pub mod nonce; mod reserved_account_keys; @@ -136,114 +142,248 @@ pub async fn process_pending_withdrawals(runtime: R) { } }; - let (batches, more_to_process) = read_state(|state| { - let mut affordable_batches = state.withdrawal_batches().peekable(); - let batches: Vec> = affordable_batches - .by_ref() - .take(MAX_CONCURRENT_RPC_CALLS) - .collect(); - (batches, affordable_batches.peek().is_some()) + let Some(minter_address) = read_state(|s| s.minter_public_key().map(minter_address)) else { + log!( + Priority::Info, + "Minter public key is not yet available, skipping withdrawal processing" + ); + return; + }; + + create_transactions_batch(&runtime, minter_address).await; + let signed_transactions = sign_transactions_batch(&runtime, minter_address).await; + send_transactions_batch(&runtime, signed_transactions).await; + + if read_state(|s| s.can_create_withdrawal_transaction()) { + runtime.set_timer( + WITHDRAWAL_PROCESSING_RETRY_DELAY, + process_pending_withdrawals, + ); + } +} + +struct ReservedBatch { + nonce_account: Address, + requests: Vec, +} + +struct BoundWithdrawal { + nonce_account: Address, + burn_indices: Vec, + message: Message, +} + +async fn create_transactions_batch(runtime: &R, minter_address: Address) { + let reserved_batches: Vec = mutate_state(|state| { + let max_batches = state + .nonce_pool() + .num_free_accounts() + .min(MAX_CONCURRENT_RPC_CALLS); + let batches: Vec<_> = state.withdrawal_batches().take(max_batches).collect(); + state + .reserve_nonce_accounts(batches.len()) + .into_iter() + .zip(batches) + .map(|(nonce_account, requests)| ReservedBatch { + nonce_account, + requests, + }) + .collect() }); - let reschedule = scopeguard::guard(runtime.clone(), |runtime| { - runtime.set_timer(Duration::ZERO, process_pending_withdrawals); + futures::future::join_all( + reserved_batches + .into_iter() + .map(async |batch| create_transaction(runtime, minter_address, batch).await), + ) + .await; +} + +async fn create_transaction( + runtime: &R, + minter_address: Address, + batch: ReservedBatch, +) { + let ReservedBatch { + nonce_account, + requests, + } = batch; + let unreserve = scopeguard::guard((), |()| { + mutate_state(|state| state.unreserve_nonce_account(&nonce_account)); }); - if batches.is_empty() { - // Nothing to process - scopeguard::ScopeGuard::into_inner(reschedule); + let nonce_value = match read_verified_nonce(runtime, nonce_account, minter_address).await { + Ok(nonce_value) => nonce_value, + Err(e) => { + log!( + Priority::Info, + "Failed to read nonce account {nonce_account}, skipping withdrawal batch this round: {e}" + ); + return; + } + }; + if read_state(|state| state.nonce_pool().has_seen(&nonce_account, &nonce_value)) { + log!( + Priority::Info, + "Read a stale nonce value for account {nonce_account}, skipping withdrawal batch this round" + ); return; } - if let Err(e) = minter_public_key() { - log!(Priority::Info, "Skipping withdrawal processing: {e}"); - scopeguard::ScopeGuard::into_inner(reschedule); + let burn_indices: Vec<_> = requests.iter().map(|r| r.burn_block_index).collect(); + if let Err(e) = build_batch_withdrawal_message( + &minter_address, + &nonce_account, + nonce_value, + &withdrawal_transfers(&requests), + ) { + log!( + Priority::Error, + "Failed to build batch withdrawal transaction for burn indices {burn_indices:?}: {e}" + ); return; } - let block = match get_recent_block(&runtime).await { - Ok(block) => block, - Err(e) => { - log!(Priority::Info, "Failed to fetch recent blockhash: {e}"); - return; - } - }; + scopeguard::ScopeGuard::into_inner(unreserve); + mutate_state(|state| { + process_event( + state, + EventType::CreatedWithdrawalTransaction { + burn_indices, + nonce_account, + nonce_value, + }, + runtime, + ) + }); +} +async fn sign_transactions_batch( + runtime: &R, + minter_address: Address, +) -> Vec { + let bound_withdrawals = read_state(|state| bound_withdrawals(state, &minter_address)); futures::future::join_all( - batches + bound_withdrawals .into_iter() - .map(async |batch| submit_withdrawal_transaction(&runtime, batch, block).await), + .map(async |withdrawal| sign_transaction(runtime, withdrawal).await), ) - .await; + .await + .into_iter() + .flatten() + .collect() +} - if !more_to_process { - // All work fits in this round - scopeguard::ScopeGuard::into_inner(reschedule); - } +fn bound_withdrawals(state: &State, minter_address: &Address) -> Vec { + state + .created_withdrawal_txs() + .iter() + .filter_map(|(nonce_account, created)| { + let requests: Vec = created + .burn_indices + .iter() + .map(|burn_index| { + state + .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 + .clone() + }) + .collect(); + match build_batch_withdrawal_message( + minter_address, + nonce_account, + created.nonce_value, + &withdrawal_transfers(&requests), + ) { + Ok(message) => Some(BoundWithdrawal { + nonce_account: *nonce_account, + burn_indices: created.burn_indices.clone(), + message, + }), + Err(e) => { + log!( + Priority::Error, + "Failed to rebuild withdrawal transaction bound to nonce account {nonce_account}: {e}" + ); + None + } + } + }) + .collect() } -async fn submit_withdrawal_transaction( - runtime: &R, - requests: Vec, - block: Block, -) { - let targets: Vec<_> = requests +fn withdrawal_transfers(requests: &[WithdrawalRequest]) -> Vec<(Address, Lamport)> { + requests .iter() .map(|request| { - let destination = Address::from(request.solana_address); - (destination, request.amount_to_transfer) + ( + Address::from(request.solana_address), + request.amount_to_transfer, + ) }) - .collect(); + .collect() +} - let (signed_tx, signers) = match create_signed_batch_withdrawal_transaction( - runtime, - &targets, - block.blockhash, - ) - .await - { - Ok(tx) => tx, +async fn sign_transaction( + runtime: &R, + withdrawal: BoundWithdrawal, +) -> Option { + let BoundWithdrawal { + nonce_account, + burn_indices, + message, + } = withdrawal; + let transaction = match sign_batch_withdrawal_message(runtime, message).await { + Ok(transaction) => transaction, Err(e) => { - let burn_indices: Vec<_> = requests.iter().map(|r| r.burn_block_index).collect(); log!( Priority::Error, - "Failed to create batch withdrawal transaction for burn indices {burn_indices:?}: {e}" + "Failed to sign withdrawal transaction bound to nonce account {nonce_account} (will be re-signed next round): {e}" ); - return; + return None; } }; - - let signature = signed_tx.signatures[0]; - let message = VersionedMessage::Legacy(signed_tx.message.clone()); - let burn_indices: Vec<_> = requests.iter().map(|r| r.burn_block_index).collect(); - mutate_state(|state| { process_event( state, EventType::SubmittedTransaction { - signature, - message, - signers, - purpose: TransactionPurpose::Withdrawal { - burn_indices: burn_indices.clone(), - block_height: block.block_height, - }, + signature: transaction.signatures[0], + message: transaction.message.clone().into(), + signers: vec![Signer::Minter], + purpose: TransactionPurpose::Withdrawal { burn_indices }, }, runtime, ) }); + Some(transaction) +} + +async fn send_transactions_batch(runtime: &R, transactions: Vec) { + futures::future::join_all( + transactions + .into_iter() + .map(async |transaction| send_transaction(runtime, transaction).await), + ) + .await; +} - match submit_transaction(runtime, signed_tx).await { +async fn send_transaction(runtime: &R, transaction: Transaction) { + let signature = transaction.signatures[0]; + match submit_transaction_skipping_preflight(runtime, transaction).await { Ok(_) => { log!( Priority::Info, - "Submitted withdrawal transaction {signature} for burn indices {burn_indices:?}" + "Submitted withdrawal transaction {signature}" ); } Err(e) => { log!( Priority::Info, - "Failed to send withdrawal transaction {signature} (will be resubmitted): {e}" + "Failed to send withdrawal transaction {signature}: {e}" ); } } diff --git a/minter/src/withdraw/tests.rs b/minter/src/withdraw/tests.rs index b668dd34..ee4cb291 100644 --- a/minter/src/withdraw/tests.rs +++ b/minter/src/withdraw/tests.rs @@ -1,31 +1,36 @@ use crate::test_fixtures::signer::sign_as_minter; use crate::{ - constants::{FEE_PER_SIGNATURE, MAX_CONCURRENT_RPC_CALLS}, + constants::{FEE_PER_SIGNATURE, MAX_CONCURRENT_RPC_CALLS, RENT_EXEMPTION_THRESHOLD}, guard::{TimerGuard, withdrawal_guard}, - rpc::BlockHeight, - sol_transfer::MAX_WITHDRAWALS_PER_TX, - state::{MinterTransaction, TaskType, read_state}, + sol_transfer::MAX_WITHDRAWALS_PER_NONCE_TX, + state::{ + MinterTransaction, TaskType, + event::{Signer, TransactionPurpose}, + 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, - init_schnorr_master_key, init_state, init_state_with_args, minter_signature, - minter_signature_nth, runtime::TestCanisterRuntime, signature, valid_init_args, + WITHDRAWAL_FEE, account, events, init_balance, init_balance_to, init_schnorr_master_key, + init_state, init_state_with_args, minter_signature, runtime::TestCanisterRuntime, + signature, valid_init_args, + }, + withdraw::{ + WITHDRAWAL_PROCESSING_RETRY_DELAY, process_pending_withdrawals, withdraw, withdrawal_status, }, - withdraw::{process_pending_withdrawals, withdraw, withdrawal_status}, }; use assert_matches::assert_matches; use candid::{Nat, Principal}; -use canlog::Log; use cksol_types::TxFinalizedStatus; use cksol_types::WithdrawalStatus; use cksol_types::{WithdrawalError, WithdrawalOk}; -use cksol_types_internal::{InitArgs, log::Priority}; +use cksol_types_internal::InitArgs; use ic_canister_runtime::IcError; use ic_cdk::call::CallRejected; use ic_cdk_management_canister::SignCallError; use icrc_ledger_types::{icrc1::account::Account, icrc2::transfer_from::TransferFromError}; -use sol_rpc_types::{MultiRpcResult, RpcError, Slot}; +use sol_rpc_types::{MultiRpcResult, RpcError}; use solana_signature::Signature; +use std::time::Duration; const VALID_ADDRESS: &str = "E4MpwNnMWs2XtW5gVrxZvyS7fMq31QD5HvbxmwP45Tz3"; @@ -321,14 +326,23 @@ async fn should_return_error_if_already_processing() { mod process_pending_withdrawals_tests { use super::*; + use crate::{ + sol_transfer::build_batch_withdrawal_message, + state::event::EventType, + test_fixtures::{ + address, durable_nonce, + events::{create_withdrawal_batch_transaction, submit_withdrawal_batch_transaction}, + nonce_account_info, + }, + }; - type GetSlotResult = MultiRpcResult; - type GetBlockResult = MultiRpcResult; + type GetAccountInfoResult = MultiRpcResult>; type SendTransactionResult = MultiRpcResult; #[tokio::test] async fn should_do_nothing_if_no_pending_withdrawals() { init_state(); + init_schnorr_master_key(); // We return early, therefore no RPC calls are made let runtime = TestCanisterRuntime::new(); @@ -353,6 +367,7 @@ mod process_pending_withdrawals_tests { #[tokio::test] async fn should_acquire_and_release_guard() { init_state(); + init_schnorr_master_key(); let runtime = TestCanisterRuntime::new(); process_pending_withdrawals(runtime).await; @@ -383,7 +398,7 @@ mod process_pending_withdrawals_tests { } #[tokio::test] - async fn should_not_panic_when_withdrawing_exactly_the_minter_balance() { + async fn should_hold_back_rent_when_withdrawing_exactly_the_minter_balance() { init_state(); init_schnorr_master_key(); @@ -412,9 +427,7 @@ mod process_pending_withdrawals_tests { let events_before = EventsAssert::from_recorded(); - let runtime = TestCanisterRuntime::new() - .add_recent_block(Ok(1)) - .with_increasing_time(); + let runtime = TestCanisterRuntime::new().with_increasing_time(); process_pending_withdrawals(runtime).await; @@ -430,12 +443,9 @@ mod process_pending_withdrawals_tests { #[tokio::test] async fn should_process_only_affordable_withdrawals() { init_state(); - init_balance_to(12_500_000); + init_balance_to(12_500_000 + RENT_EXEMPTION_THRESHOLD); init_schnorr_master_key(); - let tx_signature = minter_signature(); - let slot = 1; - // The minter balance is sufficient for the first two withdrawals events::accept_withdrawal(account(1), 0, 5_000_000 + WITHDRAWAL_FEE); events::accept_withdrawal(account(2), 1, 5_000_000 + WITHDRAWAL_FEE); @@ -444,8 +454,12 @@ mod process_pending_withdrawals_tests { let events_before = EventsAssert::from_recorded(); let runtime = TestCanisterRuntime::new() - .add_recent_block(Ok(slot)) - .add_stub_response(SendTransactionResult::Consistent(Ok(tx_signature.into()))) + .add_stub_response(GetAccountInfoResult::Consistent(Ok(Some( + nonce_account_info(MINTER_ADDRESS, 1), + )))) + .add_stub_response(SendTransactionResult::Consistent(Ok( + minter_signature().into() + ))) .add_signer(sign_as_minter()) .with_increasing_time(); @@ -456,208 +470,336 @@ mod process_pending_withdrawals_tests { assert_matches!(withdrawal_status(1), WithdrawalStatus::TxSent { .. }); assert_eq!(withdrawal_status(2), WithdrawalStatus::Pending); - // One new event (the submitted transaction batching both withdrawals) + // Two new events: the created and the submitted transaction batching both withdrawals let events_after = EventsAssert::from_recorded(); - assert_eq!(events_after.len(), events_before.len() + 1); + assert_eq!(events_after.len(), events_before.len() + 2); } #[tokio::test] - async fn should_process_when_pending_withdrawals_exist() { + async fn should_record_the_transaction_before_signing_and_submit_it() { init_state(); init_balance(); init_schnorr_master_key(); - let tx_signature = minter_signature(); - let slot = 100; - let block_height = BlockHeight::new(90); events::accept_withdrawal(account(1), 1, MINIMUM_WITHDRAWAL_AMOUNT); + let events_before = EventsAssert::from_recorded(); let runtime = TestCanisterRuntime::new() .with_increasing_time() - .add_stub_response(GetSlotResult::Consistent(Ok(slot))) - .add_stub_response(GetBlockResult::Consistent(Ok(confirmed_block_at_height( - block_height, + .add_stub_response(GetAccountInfoResult::Consistent(Ok(Some( + nonce_account_info(MINTER_ADDRESS, 1), )))) - .add_stub_response(SendTransactionResult::Consistent(Ok(tx_signature.into()))) + .add_stub_response(SendTransactionResult::Consistent(Ok( + minter_signature().into() + ))) .add_signer(sign_as_minter()); process_pending_withdrawals(runtime).await; - assert_matches!(withdrawal_status(1), WithdrawalStatus::TxSent { .. }); + let expected_message = build_batch_withdrawal_message( + &MINTER_ADDRESS, + &NONCE_ACCOUNT, + durable_nonce(1), + &[( + solana_address::Address::from([0u8; 32]), + MINIMUM_WITHDRAWAL_AMOUNT - WITHDRAWAL_FEE, + )], + ) + .unwrap(); + let events_after = EventsAssert::from_recorded(); + assert_eq!(events_after.len(), events_before.len() + 2); + events_after + .expect_contains_event_eq(EventType::CreatedWithdrawalTransaction { + burn_indices: vec![1_u64.into()], + nonce_account: NONCE_ACCOUNT, + nonce_value: durable_nonce(1), + }) + .expect_contains_event_eq(EventType::SubmittedTransaction { + signature: minter_signature(), + message: expected_message.clone().into(), + signers: vec![Signer::Minter], + purpose: TransactionPurpose::Withdrawal { + burn_indices: vec![1_u64.into()], + }, + }); + read_state(|s| { - let submitted = s.submitted_transactions().get(&tx_signature).unwrap(); - assert_eq!(submitted.block_height(), Some(block_height)); - assert_matches!(submitted, MinterTransaction::Withdrawal { .. }); - assert_eq!( - s.sent_withdrawal_requests() - .get(&1_u64.into()) - .map(|sent| sent.signature), - Some(tx_signature) - ); + let submitted = s.submitted_transactions().get(&minter_signature()).unwrap(); + let MinterTransaction::Withdrawal { + message, + nonce_account, + nonce_value, + .. + } = submitted + else { + panic!("expected a withdrawal transaction, got {submitted:?}"); + }; + assert_eq!(*message, expected_message.into()); + assert_eq!(*nonce_account, NONCE_ACCOUNT); + assert_eq!(*nonce_value, durable_nonce(1)); }); + assert_matches!(withdrawal_status(1), WithdrawalStatus::TxSent { .. }); } #[tokio::test] - async fn should_log_error_when_blockhash_fetch_fails() { + async fn should_skip_the_batch_when_the_nonce_read_fails() { init_state(); init_schnorr_master_key(); init_balance(); + init_schnorr_master_key(); events::accept_withdrawal(account(1), 1, MINIMUM_WITHDRAWAL_AMOUNT); - let events_before = EventsAssert::from_recorded(); let runtime = TestCanisterRuntime::new() .with_increasing_time() - .add_recent_block(Err(RpcError::ValidationError( - "slot unavailable".to_string(), + .add_stub_response(GetAccountInfoResult::Consistent(Err( + RpcError::ValidationError("account unavailable".to_string()), ))); process_pending_withdrawals(runtime).await; - // No withdrawal transaction event should be recorded - let events_after = EventsAssert::from_recorded(); - assert_eq!(events_before, events_after); - - let mut log: Log = Log::default(); - log.push_logs(Priority::Info); - assert!( - log.entries - .iter() - .any(|e| e.message.contains("Failed to fetch recent blockhash")), - "Expected info log about blockhash failure, got: {:?}", - log.entries - ); - + assert_eq!(EventsAssert::from_recorded(), events_before); assert_eq!(withdrawal_status(1), WithdrawalStatus::Pending); + assert_nonce_account_unreserved(); } #[tokio::test] - async fn should_not_process_batch_on_sig_error() { + async fn should_skip_the_batch_when_the_nonce_read_is_stale() { init_state(); init_balance(); init_schnorr_master_key(); - let slot = 1; events::accept_withdrawal(account(1), 1, MINIMUM_WITHDRAWAL_AMOUNT); - events::accept_withdrawal(account(2), 2, MINIMUM_WITHDRAWAL_AMOUNT); + create_withdrawal_batch_transaction(durable_nonce(1), vec![1]); + submit_withdrawal_batch_transaction(signature(0x50), durable_nonce(1), vec![1]); + events::succeed_transaction(signature(0x50)); + events::accept_withdrawal(account(2), 2, MINIMUM_WITHDRAWAL_AMOUNT); let events_before = EventsAssert::from_recorded(); let runtime = TestCanisterRuntime::new() .with_increasing_time() - .add_recent_block(Ok(slot)) - .add_signer(sign_as_minter().expect([Err(SignCallError::CallFailed( - CallRejected::with_rejection(4, "signing service unavailable".to_string()).into(), - ))])); + .add_stub_response(GetAccountInfoResult::Consistent(Ok(Some( + nonce_account_info(MINTER_ADDRESS, 1), + )))); process_pending_withdrawals(runtime).await; - // No transaction event should be recorded (signing failed) - let events_after = EventsAssert::from_recorded(); - assert_eq!(events_before, events_after); - - // An error should be logged for the whole batch - let mut log: Log = Log::default(); - log.push_logs(Priority::Error); - assert!( - log.entries.iter().any(|e| e - .message - .contains("Failed to create batch withdrawal transaction for burn indices")), - "Expected error log about batch sig failure, got: {:?}", - log.entries - ); - - // Both withdrawals remain pending since they were in the same batch - assert_matches!(withdrawal_status(1), WithdrawalStatus::Pending); - assert_matches!(withdrawal_status(2), WithdrawalStatus::Pending); + assert_eq!(EventsAssert::from_recorded(), events_before); + assert_eq!(withdrawal_status(2), WithdrawalStatus::Pending); + assert_nonce_account_unreserved(); } #[tokio::test] - async fn should_batch_withdrawals_into_transactions() { + async fn should_re_sign_the_identical_message_after_a_signing_failure() { init_state(); init_balance(); init_schnorr_master_key(); - let request_count = MAX_WITHDRAWALS_PER_TX as u64 + 1; - let slot = 1; + events::accept_withdrawal(account(1), 1, MINIMUM_WITHDRAWAL_AMOUNT); + let expected_message = build_batch_withdrawal_message( + &MINTER_ADDRESS, + &NONCE_ACCOUNT, + durable_nonce(1), + &[( + solana_address::Address::from([0u8; 32]), + MINIMUM_WITHDRAWAL_AMOUNT - WITHDRAWAL_FEE, + )], + ) + .unwrap(); + + let failing_runtime = TestCanisterRuntime::new() + .with_increasing_time() + .add_stub_response(GetAccountInfoResult::Consistent(Ok(Some( + nonce_account_info(MINTER_ADDRESS, 1), + )))) + .add_signer( + sign_as_minter() + .of_message(expected_message.serialize()) + .expect([Err(SignCallError::CallFailed( + CallRejected::with_rejection(4, "signing service unavailable".to_string()) + .into(), + ))]), + ); + + process_pending_withdrawals(failing_runtime).await; + + let num_events_after_failure = EventsAssert::from_recorded().len(); + EventsAssert::from_recorded().expect_contains_event_eq(EventType::CreatedWithdrawalTransaction { + burn_indices: vec![1_u64.into()], + nonce_account: NONCE_ACCOUNT, + nonce_value: durable_nonce(1), + }); + assert_eq!(withdrawal_status(1), WithdrawalStatus::Pending); + + let recovering_runtime = TestCanisterRuntime::new() + .with_increasing_time() + .add_signer(sign_as_minter().of_message(expected_message.serialize())) + .add_stub_response(SendTransactionResult::Consistent(Ok( + minter_signature().into() + ))); - for i in 0..request_count { - events::accept_withdrawal(account(i as usize), i, MINIMUM_WITHDRAWAL_AMOUNT); + process_pending_withdrawals(recovering_runtime).await; + + let events_after_recovery = EventsAssert::from_recorded(); + assert_eq!(events_after_recovery.len(), num_events_after_failure + 1); + events_after_recovery.expect_contains_event_eq(EventType::SubmittedTransaction { + signature: minter_signature(), + message: expected_message.into(), + signers: vec![Signer::Minter], + purpose: TransactionPurpose::Withdrawal { + burn_indices: vec![1_u64.into()], + }, + }); + assert_matches!(withdrawal_status(1), WithdrawalStatus::TxSent { .. }); + } + + #[tokio::test] + async fn should_never_share_a_nonce_account_between_concurrent_batches() { + let second_nonce_account = address(2); + init_state_with_args(InitArgs { + nonce_accounts: vec![NONCE_ACCOUNT.to_string(), second_nonce_account.to_string()], + ..valid_init_args() + }); + init_balance(); + init_schnorr_master_key(); + + let num_requests = MAX_WITHDRAWALS_PER_NONCE_TX + 1; + for i in 0..num_requests { + events::accept_withdrawal(account(i), i as u64, MINIMUM_WITHDRAWAL_AMOUNT); } let runtime = TestCanisterRuntime::new() .with_increasing_time() - .add_recent_block(Ok(slot)) + .add_stub_response(GetAccountInfoResult::Consistent(Ok(Some( + nonce_account_info(MINTER_ADDRESS, 1), + )))) + .add_stub_response(GetAccountInfoResult::Consistent(Ok(Some( + nonce_account_info(MINTER_ADDRESS, 2), + )))) .add_stub_response(SendTransactionResult::Consistent(Ok(signature(1).into()))) .add_stub_response(SendTransactionResult::Consistent(Ok(signature(2).into()))) .add_signer(sign_as_minter().times(2)); process_pending_withdrawals(runtime).await; - // All withdrawals should be processed in a single invocation - // (2 batches in 1 round, both within MAX_CONCURRENT_RPC_CALLS) - for i in 0..request_count { - assert_matches!(withdrawal_status(i), WithdrawalStatus::TxSent { .. }); + for i in 0..num_requests { + assert_matches!(withdrawal_status(i as u64), WithdrawalStatus::TxSent { .. }); } - - // Verify that withdrawals were split into 2 batches - read_state(|s| assert_eq!(s.submitted_transactions().len(), 2)); + read_state(|s| { + let nonce_accounts: std::collections::BTreeSet<_> = s + .submitted_transactions() + .iter() + .map(|(_, tx)| match tx { + MinterTransaction::Withdrawal { nonce_account, .. } => *nonce_account, + MinterTransaction::SweepDeposit { .. } => { + panic!("expected a withdrawal transaction, got {tx:?}") + } + }) + .collect(); + assert_eq!( + nonce_accounts, + [NONCE_ACCOUNT, second_nonce_account].into_iter().collect() + ); + }); } #[tokio::test] - async fn should_reschedule_until_all_withdrawals_processed() { + async fn should_cap_the_batches_at_the_free_nonce_accounts() { init_state(); init_balance(); init_schnorr_master_key(); - let num_requests = MAX_WITHDRAWALS_PER_TX * MAX_CONCURRENT_RPC_CALLS + 1; + let num_requests = MAX_WITHDRAWALS_PER_NONCE_TX + 1; for i in 0..num_requests { events::accept_withdrawal(account(i), i as u64, MINIMUM_WITHDRAWAL_AMOUNT); } - let slot = 1; - - // Round 1: processes MAX_CONCURRENT_RPC_CALLS batches, 1 request remains → reschedule - let mut runtime = TestCanisterRuntime::new() + let runtime = TestCanisterRuntime::new() .with_increasing_time() - .add_recent_block(Ok(slot)); - for i in 0..MAX_CONCURRENT_RPC_CALLS { - runtime = runtime - .add_stub_response(SendTransactionResult::Consistent(Ok( - signature(i + 1).into() - ))); + .add_stub_response(GetAccountInfoResult::Consistent(Ok(Some( + nonce_account_info(MINTER_ADDRESS, 1), + )))) + .add_stub_response(SendTransactionResult::Consistent(Ok( + minter_signature().into() + ))) + .add_signer(sign_as_minter()); + + process_pending_withdrawals(runtime.clone()).await; + + for i in 0..MAX_WITHDRAWALS_PER_NONCE_TX { + assert_matches!(withdrawal_status(i as u64), WithdrawalStatus::TxSent { .. }); } - let runtime = runtime.add_signer(sign_as_minter().times(MAX_CONCURRENT_RPC_CALLS)); + assert_eq!( + withdrawal_status(MAX_WITHDRAWALS_PER_NONCE_TX as u64), + WithdrawalStatus::Pending + ); + read_state(|s| assert_eq!(s.submitted_transactions().len(), 1)); + assert_eq!(runtime.set_timer_call_count(), 0); + } + + #[tokio::test] + async fn should_retry_later_when_an_affordable_batch_is_left_behind() { + init_state(); + init_balance(); + init_schnorr_master_key(); + + events::accept_withdrawal(account(1), 1, MINIMUM_WITHDRAWAL_AMOUNT); + + let runtime = TestCanisterRuntime::new() + .with_increasing_time() + .add_stub_response(GetAccountInfoResult::Consistent(Err( + RpcError::ValidationError("account unavailable".to_string()), + ))); process_pending_withdrawals(runtime.clone()).await; - read_state(|s| { - assert_eq!(s.submitted_transactions().len(), MAX_CONCURRENT_RPC_CALLS); - assert_eq!(s.pending_withdrawal_requests().len(), 1); - }); - assert_eq!(runtime.set_timer_call_count(), 1); + assert_eq!( + runtime.set_timer_delays(), + vec![WITHDRAWAL_PROCESSING_RETRY_DELAY] + ); + } + + #[tokio::test] + async fn should_not_reschedule_without_a_free_nonce_account_even_with_affordable_batches_left() + { + init_state(); + let num_affordable_batches = MAX_CONCURRENT_RPC_CALLS + 1; + let num_affordable_requests = MAX_CONCURRENT_RPC_CALLS * MAX_WITHDRAWALS_PER_NONCE_TX + 1; + init_balance_to( + FEE_PER_SIGNATURE + + num_affordable_requests as u64 * (MINIMUM_WITHDRAWAL_AMOUNT - WITHDRAWAL_FEE) + + num_affordable_batches as u64 * FEE_PER_SIGNATURE + + RENT_EXEMPTION_THRESHOLD, + ); + init_schnorr_master_key(); + + for i in 0..=num_affordable_requests { + events::accept_withdrawal(account(i), i as u64, MINIMUM_WITHDRAWAL_AMOUNT); + } - // Round 2: processes the remaining 1 request → no reschedule - let signature_continuing_round_1 = minter_signature_nth(MAX_CONCURRENT_RPC_CALLS); let runtime = TestCanisterRuntime::new() .with_increasing_time() - .add_recent_block(Ok(slot)) - .add_signer(sign_as_minter().expect([Ok(signature_continuing_round_1)])) + .add_stub_response(GetAccountInfoResult::Consistent(Ok(Some( + nonce_account_info(MINTER_ADDRESS, 1), + )))) .add_stub_response(SendTransactionResult::Consistent(Ok( - signature_continuing_round_1.into(), - ))); + minter_signature().into() + ))) + .add_signer(sign_as_minter()); process_pending_withdrawals(runtime.clone()).await; - read_state(|s| { - assert!(s.pending_withdrawal_requests().is_empty()); - assert_eq!( - s.submitted_transactions().len(), - MAX_CONCURRENT_RPC_CALLS + 1 - ); + read_state(|s| assert_eq!(s.submitted_transactions().len(), 1)); + assert_eq!(runtime.set_timer_delays(), Vec::::new()); + } + + fn assert_nonce_account_unreserved() { + crate::state::mutate_state(|s| { + assert_eq!(s.reserve_nonce_accounts(1), vec![NONCE_ACCOUNT]); + s.unreserve_nonce_account(&NONCE_ACCOUNT); }); - assert_eq!(runtime.set_timer_call_count(), 0); } } From 22b5a858b05893d47803d74aa318d5e5aba39d0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9gory=20Demay?= Date: Tue, 6 Oct 2026 15:59:44 +0000 Subject: [PATCH 02/10] refactor(minter): rebuild each bound withdrawal message in its own function Co-Authored-By: Claude Opus 5.5 --- minter/src/withdraw/mod.rs | 76 ++++++++++++++++++++++---------------- 1 file changed, 44 insertions(+), 32 deletions(-) diff --git a/minter/src/withdraw/mod.rs b/minter/src/withdraw/mod.rs index 882a51eb..44d4e8ad 100644 --- a/minter/src/withdraw/mod.rs +++ b/minter/src/withdraw/mod.rs @@ -19,9 +19,11 @@ use crate::{ numeric::LedgerBurnIndex, rpc::submit_transaction_skipping_preflight, runtime::CanisterRuntime, - sol_transfer::{build_batch_withdrawal_message, sign_batch_withdrawal_message}, + sol_transfer::{ + CreateTransferError, build_batch_withdrawal_message, sign_batch_withdrawal_message, + }, state::{ - State, TaskType, + CreatedWithdrawalTransaction, State, TaskType, audit::process_event, event::{EventType, Signer, TransactionPurpose, WithdrawalRequest}, mutate_state, read_state, @@ -279,43 +281,53 @@ fn bound_withdrawals(state: &State, minter_address: &Address) -> Vec = created - .burn_indices - .iter() - .map(|burn_index| { - state - .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 - .clone() - }) - .collect(); - match build_batch_withdrawal_message( - minter_address, - nonce_account, - created.nonce_value, - &withdrawal_transfers(&requests), - ) { - Ok(message) => Some(BoundWithdrawal { - nonce_account: *nonce_account, - burn_indices: created.burn_indices.clone(), - message, - }), - Err(e) => { + bound_withdrawal(state, minter_address, nonce_account, created) + .inspect_err(|e| { log!( Priority::Error, "Failed to rebuild withdrawal transaction bound to nonce account {nonce_account}: {e}" - ); - None - } - } + ) + }) + .ok() }) .collect() } +fn bound_withdrawal( + state: &State, + minter_address: &Address, + nonce_account: &Address, + created: &CreatedWithdrawalTransaction, +) -> Result { + let requests: Vec = created + .burn_indices + .iter() + .map(|burn_index| created_withdrawal_request(state, burn_index)) + .collect(); + let message = build_batch_withdrawal_message( + minter_address, + nonce_account, + created.nonce_value, + &withdrawal_transfers(&requests), + )?; + Ok(BoundWithdrawal { + nonce_account: *nonce_account, + burn_indices: created.burn_indices.clone(), + message, + }) +} + +fn created_withdrawal_request(state: &State, burn_index: &LedgerBurnIndex) -> WithdrawalRequest { + state + .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 + .clone() +} + fn withdrawal_transfers(requests: &[WithdrawalRequest]) -> Vec<(Address, Lamport)> { requests .iter() From e17caa1fc7dd4ccf352fcc6c1ba0431e839f9582 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9gory=20Demay?= Date: Wed, 7 Oct 2026 08:24:28 +0000 Subject: [PATCH 03/10] fix(minter): bound the number of withdrawal transactions signed concurrently Co-Authored-By: Claude Opus 5.5 --- minter/src/constants.rs | 4 ++ minter/src/test_fixtures/mod.rs | 12 ++++- minter/src/withdraw/mod.rs | 15 ++++--- minter/src/withdraw/tests.rs | 80 ++++++++++++++++++++++++++++++++- 4 files changed, 103 insertions(+), 8 deletions(-) diff --git a/minter/src/constants.rs b/minter/src/constants.rs index 2781519b..2f7d69f7 100644 --- a/minter/src/constants.rs +++ b/minter/src/constants.rs @@ -3,6 +3,10 @@ use std::{num::NonZeroUsize, time::Duration}; /// Maximum number of concurrent calls to the SOL RPC canister. pub const MAX_CONCURRENT_RPC_CALLS: usize = 10; +/// Maximum number of concurrent threshold signing requests to the management canister +/// in a single round of a timer. +pub const MAX_CONCURRENT_SIGNATURES: usize = 10; + /// Maximum number of attempts to fetch a recent block, each attempt consisting of /// at most one `getSlot` and one `getBlock` call. pub const GET_RECENT_BLOCK_MAX_TRIES: NonZeroUsize = diff --git a/minter/src/test_fixtures/mod.rs b/minter/src/test_fixtures/mod.rs index b001fa44..9395cfe3 100644 --- a/minter/src/test_fixtures/mod.rs +++ b/minter/src/test_fixtures/mod.rs @@ -908,6 +908,16 @@ pub mod events { pub fn create_withdrawal_batch_transaction( nonce_value: solana_hash::Hash, burn_indices: Vec, + ) { + create_withdrawal_batch_transaction_on(NONCE_ACCOUNT, nonce_value, burn_indices); + } + + /// Records a `CreatedWithdrawalTransaction` for the given withdrawals, binding + /// `nonce_account` to `nonce_value`. + pub fn create_withdrawal_batch_transaction_on( + nonce_account: Address, + nonce_value: solana_hash::Hash, + burn_indices: Vec, ) { mutate_state(|state| { process_event( @@ -917,7 +927,7 @@ pub mod events { .into_iter() .map(LedgerBurnIndex::from) .collect(), - nonce_account: NONCE_ACCOUNT, + nonce_account, nonce_value, }, &runtime(), diff --git a/minter/src/withdraw/mod.rs b/minter/src/withdraw/mod.rs index 44d4e8ad..405c78bc 100644 --- a/minter/src/withdraw/mod.rs +++ b/minter/src/withdraw/mod.rs @@ -13,7 +13,7 @@ use cksol_types_internal::log::Priority; use crate::{ address::{minter_address, minter_public_key}, - constants::MAX_CONCURRENT_RPC_CALLS, + constants::{MAX_CONCURRENT_RPC_CALLS, MAX_CONCURRENT_SIGNATURES}, guard::{TimerGuard, withdrawal_guard}, ledger::{BurnError, burn}, numeric::LedgerBurnIndex, @@ -34,6 +34,8 @@ use crate::{ pub const WITHDRAWAL_PROCESSING_DELAY: Duration = Duration::from_mins(1); pub const WITHDRAWAL_PROCESSING_RETRY_DELAY: Duration = Duration::from_secs(10); +const _: () = assert!(MAX_CONCURRENT_SIGNATURES <= MAX_CONCURRENT_RPC_CALLS); + pub mod nonce; mod reserved_account_keys; #[cfg(test)] @@ -180,7 +182,8 @@ async fn create_transactions_batch(runtime: &R, minter_addre let max_batches = state .nonce_pool() .num_free_accounts() - .min(MAX_CONCURRENT_RPC_CALLS); + .min(MAX_CONCURRENT_RPC_CALLS) + .min(MAX_CONCURRENT_SIGNATURES.saturating_sub(state.created_withdrawal_txs().len())); let batches: Vec<_> = state.withdrawal_batches().take(max_batches).collect(); state .reserve_nonce_accounts(batches.len()) @@ -277,9 +280,11 @@ async fn sign_transactions_batch( } fn bound_withdrawals(state: &State, minter_address: &Address) -> Vec { - state - .created_withdrawal_txs() - .iter() + let mut created_withdrawal_txs: Vec<_> = state.created_withdrawal_txs().iter().collect(); + created_withdrawal_txs.sort_by_key(|(_, created)| created.burn_indices.iter().min().copied()); + created_withdrawal_txs + .into_iter() + .take(MAX_CONCURRENT_SIGNATURES) .filter_map(|(nonce_account, created)| { bound_withdrawal(state, minter_address, nonce_account, created) .inspect_err(|e| { diff --git a/minter/src/withdraw/tests.rs b/minter/src/withdraw/tests.rs index ee4cb291..e6ced775 100644 --- a/minter/src/withdraw/tests.rs +++ b/minter/src/withdraw/tests.rs @@ -1,6 +1,9 @@ use crate::test_fixtures::signer::sign_as_minter; use crate::{ - constants::{FEE_PER_SIGNATURE, MAX_CONCURRENT_RPC_CALLS, RENT_EXEMPTION_THRESHOLD}, + constants::{ + FEE_PER_SIGNATURE, MAX_CONCURRENT_RPC_CALLS, MAX_CONCURRENT_SIGNATURES, + RENT_EXEMPTION_THRESHOLD, + }, guard::{TimerGuard, withdrawal_guard}, sol_transfer::MAX_WITHDRAWALS_PER_NONCE_TX, state::{ @@ -331,7 +334,10 @@ mod process_pending_withdrawals_tests { state::event::EventType, test_fixtures::{ address, durable_nonce, - events::{create_withdrawal_batch_transaction, submit_withdrawal_batch_transaction}, + events::{ + create_withdrawal_batch_transaction, create_withdrawal_batch_transaction_on, + submit_withdrawal_batch_transaction, + }, nonce_account_info, }, }; @@ -705,6 +711,76 @@ mod process_pending_withdrawals_tests { }); } + #[tokio::test] + async fn should_sign_at_most_the_concurrent_signatures_oldest_first_without_creating_more() { + let num_bound = MAX_CONCURRENT_SIGNATURES + 1; + let free_nonce_account = address(num_bound + 1); + let bound_nonce_account = |burn_index: usize| address(num_bound - burn_index); + init_state_with_args(InitArgs { + nonce_accounts: (0..num_bound) + .map(bound_nonce_account) + .chain([free_nonce_account]) + .map(|nonce_account| nonce_account.to_string()) + .collect(), + ..valid_init_args() + }); + init_balance(); + init_schnorr_master_key(); + + for burn_index in 0..num_bound { + events::accept_withdrawal( + account(burn_index), + burn_index as u64, + MINIMUM_WITHDRAWAL_AMOUNT, + ); + create_withdrawal_batch_transaction_on( + bound_nonce_account(burn_index), + durable_nonce(burn_index), + vec![burn_index as u64], + ); + } + let pending_burn_index = num_bound as u64; + events::accept_withdrawal( + account(num_bound), + pending_burn_index, + MINIMUM_WITHDRAWAL_AMOUNT, + ); + + let runtime = (0..MAX_CONCURRENT_SIGNATURES) + .fold(TestCanisterRuntime::new(), |runtime, i| { + runtime + .add_stub_response(SendTransactionResult::Consistent(Ok(signature(i).into()))) + }) + .add_stub_response(GetAccountInfoResult::Consistent(Ok(Some( + nonce_account_info(MINTER_ADDRESS, num_bound), + )))) + .with_increasing_time() + .add_signer(sign_as_minter().times(MAX_CONCURRENT_SIGNATURES)); + + process_pending_withdrawals(runtime).await; + + for burn_index in 0..MAX_CONCURRENT_SIGNATURES { + assert_matches!( + withdrawal_status(burn_index as u64), + WithdrawalStatus::TxSent { .. } + ); + } + assert_eq!( + withdrawal_status(MAX_CONCURRENT_SIGNATURES as u64), + WithdrawalStatus::Pending + ); + read_state(|s| { + assert_eq!( + s.created_withdrawal_txs().keys().collect::>(), + vec![&bound_nonce_account(MAX_CONCURRENT_SIGNATURES)] + ); + assert!( + s.pending_withdrawal_requests() + .contains_key(&pending_burn_index.into()) + ); + }); + } + #[tokio::test] async fn should_cap_the_batches_at_the_free_nonce_accounts() { init_state(); From a641d66d5ce5dbcb8b18052c0190f215d2214936 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9gory=20Demay?= Date: Wed, 7 Oct 2026 08:25:16 +0000 Subject: [PATCH 04/10] fix(minter): retry the withdrawal round while bound withdrawals are unsigned Co-Authored-By: Claude Opus 5.5 --- docs/design.md | 2 +- minter/src/state/mod.rs | 4 ++++ minter/src/withdraw/mod.rs | 4 +++- minter/src/withdraw/tests.rs | 26 ++++++++++++++++++++++++++ 4 files changed, 34 insertions(+), 2 deletions(-) diff --git a/docs/design.md b/docs/design.md index e0d9d465..72749429 100644 --- a/docs/design.md +++ b/docs/design.md @@ -470,7 +470,7 @@ The ckSOL minter's main address is the nonce authority of every account in the p For the oracle to be sound, a stale read must never be mistaken for an advance: nonce values are opaque hashes, so a value differing from the in-flight transaction's nonce could by itself be the account's past as well as its future, and a provider lagging behind an already observed state serves exactly such a past value. Since only the ckSOL minter can advance the nonce, the account's complete value history is the set of nonce values the ckSOL minter has bound to transactions, which the event log already records. Every read is therefore classified against that set: the bound value means the transaction has not landed; any other previously seen value is a stale response and yields no decision; a never-seen value can only be the account's new frontier, which proves the advance. -The pool size bounds the withdrawal throughput, since every withdrawal transaction occupies one nonce account while it is in flight. When no free nonce account is available, the affected withdrawal batches simply remain queued until a nonce account frees up; the processing timer retries after a short delay only while both a free nonce account and an affordable batch remain, and otherwise waits for its regular interval, so an exhausted pool stops retrying entirely and a stale nonce read, whose reservation is released, is retried at the delayed cadence rather than in a zero-delay loop. As a nonce account costs nothing beyond its rent exemption minimum, the pool can be sized generously; **5 accounts** are proposed initially, allowing 50 concurrent in-flight withdrawals at 10 transfers per transaction. +The pool size bounds the withdrawal throughput, since every withdrawal transaction occupies one nonce account while it is in flight. When no free nonce account is available, the affected withdrawal batches simply remain queued until a nonce account frees up; the processing timer retries after a short delay only while both a free nonce account and an affordable batch remain, or while a bound withdrawal transaction is still unsigned (after a signing failure, or because a round signs at most 10 transactions), and otherwise waits for its regular interval, so an exhausted pool whose bound transactions are all signed stops retrying entirely and a stale nonce read, whose reservation is released, is retried at the delayed cadence rather than in a zero-delay loop. As a nonce account costs nothing beyond its rent exemption minimum, the pool can be sized generously; **5 accounts** are proposed initially, allowing 50 concurrent in-flight withdrawals at 10 transfers per transaction. Deposit sweeps continue to use recent block hashes. The double-pay hazard is specific to withdrawals: a sweep only moves funds between addresses controlled by the ckSOL minter, nothing is credited before the finalized transaction has been positively observed, and an expired sweep is dropped rather than resubmitted, as described in [Section 3.1.3](#313-manual-flow). diff --git a/minter/src/state/mod.rs b/minter/src/state/mod.rs index e4efc0da..5da4458e 100644 --- a/minter/src/state/mod.rs +++ b/minter/src/state/mod.rs @@ -523,6 +523,10 @@ impl State { self.nonce_pool.num_free_accounts() > 0 && self.withdrawal_batches().next().is_some() } + pub fn has_unsigned_withdrawal_transaction(&self) -> bool { + !self.created_withdrawal_txs.is_empty() + } + /// Returns the creation timestamp (in nanoseconds) of the oldest incomplete withdrawal request. /// An incomplete withdrawal is one that has not yet been finalized (succeeded or failed). pub fn oldest_incomplete_withdrawal_created_at(&self) -> Option { diff --git a/minter/src/withdraw/mod.rs b/minter/src/withdraw/mod.rs index 405c78bc..55d5553f 100644 --- a/minter/src/withdraw/mod.rs +++ b/minter/src/withdraw/mod.rs @@ -158,7 +158,9 @@ pub async fn process_pending_withdrawals(runtime: R) { let signed_transactions = sign_transactions_batch(&runtime, minter_address).await; send_transactions_batch(&runtime, signed_transactions).await; - if read_state(|s| s.can_create_withdrawal_transaction()) { + if read_state(|s| { + s.can_create_withdrawal_transaction() || s.has_unsigned_withdrawal_transaction() + }) { runtime.set_timer( WITHDRAWAL_PROCESSING_RETRY_DELAY, process_pending_withdrawals, diff --git a/minter/src/withdraw/tests.rs b/minter/src/withdraw/tests.rs index e6ced775..5d978258 100644 --- a/minter/src/withdraw/tests.rs +++ b/minter/src/withdraw/tests.rs @@ -837,6 +837,32 @@ mod process_pending_withdrawals_tests { ); } + #[tokio::test] + async fn should_retry_later_when_a_bound_withdrawal_is_left_unsigned() { + init_state(); + init_balance(); + init_schnorr_master_key(); + + events::accept_withdrawal(account(1), 1, MINIMUM_WITHDRAWAL_AMOUNT); + + let runtime = TestCanisterRuntime::new() + .with_increasing_time() + .add_stub_response(GetAccountInfoResult::Consistent(Ok(Some( + nonce_account_info(MINTER_ADDRESS, 1), + )))) + .add_signer(sign_as_minter().expect([Err(SignCallError::CallFailed( + CallRejected::with_rejection(4, "signing service unavailable".to_string()).into(), + ))])); + + process_pending_withdrawals(runtime.clone()).await; + + read_state(|s| assert_eq!(s.created_withdrawal_txs().len(), 1)); + assert_eq!( + runtime.set_timer_delays(), + vec![WITHDRAWAL_PROCESSING_RETRY_DELAY] + ); + } + #[tokio::test] async fn should_not_reschedule_without_a_free_nonce_account_even_with_affordable_batches_left() { From a78aefc68896e1300b42fc3e089ae03bf8d18137 Mon Sep 17 00:00:00 2001 From: gregorydemay Date: Wed, 7 Oct 2026 10:58:25 +0200 Subject: [PATCH 05/10] refactor(minter): drop the transient Reserved state of nonce accounts Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/design.md | 6 ++-- minter/src/state/mod.rs | 13 -------- minter/src/state/nonce_pool/mod.rs | 50 ++++++---------------------- minter/src/state/nonce_pool/tests.rs | 34 ++++++++----------- minter/src/state/tests.rs | 12 ++++--- minter/src/withdraw/mod.rs | 21 ++++-------- minter/src/withdraw/tests.rs | 14 ++++---- 7 files changed, 50 insertions(+), 100 deletions(-) diff --git a/docs/design.md b/docs/design.md index 72749429..ff28c326 100644 --- a/docs/design.md +++ b/docs/design.md @@ -470,7 +470,7 @@ The ckSOL minter's main address is the nonce authority of every account in the p For the oracle to be sound, a stale read must never be mistaken for an advance: nonce values are opaque hashes, so a value differing from the in-flight transaction's nonce could by itself be the account's past as well as its future, and a provider lagging behind an already observed state serves exactly such a past value. Since only the ckSOL minter can advance the nonce, the account's complete value history is the set of nonce values the ckSOL minter has bound to transactions, which the event log already records. Every read is therefore classified against that set: the bound value means the transaction has not landed; any other previously seen value is a stale response and yields no decision; a never-seen value can only be the account's new frontier, which proves the advance. -The pool size bounds the withdrawal throughput, since every withdrawal transaction occupies one nonce account while it is in flight. When no free nonce account is available, the affected withdrawal batches simply remain queued until a nonce account frees up; the processing timer retries after a short delay only while both a free nonce account and an affordable batch remain, or while a bound withdrawal transaction is still unsigned (after a signing failure, or because a round signs at most 10 transactions), and otherwise waits for its regular interval, so an exhausted pool whose bound transactions are all signed stops retrying entirely and a stale nonce read, whose reservation is released, is retried at the delayed cadence rather than in a zero-delay loop. As a nonce account costs nothing beyond its rent exemption minimum, the pool can be sized generously; **5 accounts** are proposed initially, allowing 50 concurrent in-flight withdrawals at 10 transfers per transaction. +The pool size bounds the withdrawal throughput, since every withdrawal transaction occupies one nonce account while it is in flight. When no free nonce account is available, the affected withdrawal batches simply remain queued until a nonce account frees up; the processing timer retries after a short delay only while both a free nonce account and an affordable batch remain, or while a bound withdrawal transaction is still unsigned (after a signing failure, or because a round signs at most 10 transactions), and otherwise waits for its regular interval, so an exhausted pool whose bound transactions are all signed stops retrying entirely and a stale nonce read is retried at the delayed cadence rather than in a zero-delay loop. As a nonce account costs nothing beyond its rent exemption minimum, the pool can be sized generously; **5 accounts** are proposed initially, allowing 50 concurrent in-flight withdrawals at 10 transfers per transaction. Deposit sweeps continue to use recent block hashes. The double-pay hazard is specific to withdrawals: a sweep only moves funds between addresses controlled by the ckSOL minter, nothing is credited before the finalized transaction has been positively observed, and an expired sweep is dropped rather than resubmitted, as described in [Section 3.1.3](#313-manual-flow). @@ -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. 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. +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 single timer round processes withdrawals at a time, and it assigns distinct free nonce accounts to its batches 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 @@ -542,7 +542,7 @@ sequenceDiagram Note over Minter: ⏱️ Timer fires activate Minter - Note over Minter: Reserve a free nonce account from the pool + Note over Minter: Pick a free nonce account from the pool Minter->>+RPC: getAccountInfo(nonce_account) RPC->>+Solana: getAccountInfo(nonce_account) Solana-->>-RPC: nonce account state diff --git a/minter/src/state/mod.rs b/minter/src/state/mod.rs index 5da4458e..02495e25 100644 --- a/minter/src/state/mod.rs +++ b/minter/src/state/mod.rs @@ -211,19 +211,6 @@ impl State { &self.created_withdrawal_txs } - /// Transiently reserves up to `max` free durable nonce accounts for - /// withdrawal batches being processed, so that concurrently processed - /// batches can never pick the same account. - pub fn reserve_nonce_accounts(&mut self, max: usize) -> Vec
{ - self.nonce_pool.reserve_accounts(max) - } - - /// Releases the transient reservation of a nonce account whose batch was - /// not submitted. - pub fn unreserve_nonce_account(&mut self, address: &Address) { - self.nonce_pool.unreserve(address); - } - pub fn transactions_to_resubmit(&self) -> &InsertionOrderedMap { &self.transactions_to_resubmit } diff --git a/minter/src/state/nonce_pool/mod.rs b/minter/src/state/nonce_pool/mod.rs index e16f3b88..0cd0c139 100644 --- a/minter/src/state/nonce_pool/mod.rs +++ b/minter/src/state/nonce_pool/mod.rs @@ -41,37 +41,6 @@ impl DurableNoncePool { Ok(()) } - /// Reserves up to `max` free accounts for withdrawal batches being processed, - /// so that concurrently processed batches can never pick the same account. - /// - /// A reservation is transient: it is either released with [`Self::unreserve`] - /// in the same timer round or superseded by [`Self::bind`]. - pub(super) fn reserve_accounts(&mut self, max: usize) -> Vec
{ - self.accounts - .iter_mut() - .filter(|(_, account)| account.is_free()) - .take(max) - .map(|(address, account)| { - account.state = NonceAccountState::Reserved; - *address - }) - .collect() - } - - /// Releases the reservation of an account whose batch was not submitted. - /// - /// # Panics - /// Panics if the account is not reserved. - pub(super) fn unreserve(&mut self, address: &Address) { - let account = self.account_mut(address); - assert_eq!( - account.state, - NonceAccountState::Reserved, - "BUG: cannot unreserve nonce account {address} that is not reserved" - ); - account.state = NonceAccountState::Free; - } - /// Binds the account to an in-flight withdrawal transaction carrying /// `nonce_value`, recording the value as seen. /// @@ -81,7 +50,7 @@ impl DurableNoncePool { pub(super) fn bind(&mut self, address: &Address, nonce_value: Hash) { let account = self.account_mut(address); match account.state { - NonceAccountState::Free | NonceAccountState::Reserved => {} + NonceAccountState::Free => {} NonceAccountState::Bound => { panic!("BUG: nonce account {address} is already bound to an in-flight transaction") } @@ -101,7 +70,7 @@ impl DurableNoncePool { let account = self.account_mut(address); match account.state { NonceAccountState::Bound => account.state = NonceAccountState::Free, - NonceAccountState::Free | NonceAccountState::Reserved => { + NonceAccountState::Free => { panic!("BUG: cannot free nonce account {address} that is not bound") } } @@ -127,11 +96,15 @@ impl DurableNoncePool { self.accounts.is_empty() } - pub fn num_free_accounts(&self) -> usize { + pub fn free_accounts(&self) -> impl Iterator { self.accounts - .values() - .filter(|account| account.is_free()) - .count() + .iter() + .filter(|(_, account)| account.is_free()) + .map(|(address, _)| address) + } + + pub fn num_free_accounts(&self) -> usize { + self.free_accounts().count() } fn account_mut(&mut self, address: &Address) -> &mut NonceAccount { @@ -163,9 +136,6 @@ enum NonceAccountState { /// The account is not bound to any in-flight withdrawal transaction. #[default] Free, - /// The account is transiently picked for a withdrawal batch being processed. - /// Reservations never survive a replay of the event log. - Reserved, /// The account is bound to an in-flight withdrawal transaction. Bound, } diff --git a/minter/src/state/nonce_pool/tests.rs b/minter/src/state/nonce_pool/tests.rs index 48a83aec..f484c041 100644 --- a/minter/src/state/nonce_pool/tests.rs +++ b/minter/src/state/nonce_pool/tests.rs @@ -33,40 +33,30 @@ fn should_leave_the_pool_unchanged_when_an_add_fails() { } #[test] -fn should_reserve_at_most_the_free_accounts() { - let mut pool = pool_of([address(1), address(2)]); +fn should_list_every_account_of_a_new_pool_as_free() { + let pool = pool_of([address(1), address(2)]); - assert_eq!(pool.reserve_accounts(3), vec![address(1), address(2)]); - assert_eq!(pool.reserve_accounts(1), vec![]); + assert_eq!(free_accounts(&pool), vec![address(1), address(2)]); } #[test] -fn should_not_reserve_a_bound_account() { +fn should_not_list_a_bound_account_as_free() { let mut pool = pool_of([address(1), address(2)]); - pool.bind(&address(1), durable_nonce(1)); - - assert_eq!(pool.reserve_accounts(2), vec![address(2)]); -} - -#[test] -fn should_reserve_an_unreserved_account_again() { - let mut pool = pool_of([address(1)]); - assert_eq!(pool.reserve_accounts(1), vec![address(1)]); - pool.unreserve(&address(1)); + pool.bind(&address(1), durable_nonce(1)); - assert_eq!(pool.reserve_accounts(1), vec![address(1)]); + assert_eq!(free_accounts(&pool), vec![address(2)]); } #[test] -fn should_free_a_bound_account_for_a_new_reservation() { +fn should_list_a_freed_account_as_free_again() { let mut pool = pool_of([address(1)]); pool.bind(&address(1), durable_nonce(1)); - assert_eq!(pool.reserve_accounts(1), vec![]); + assert_eq!(free_accounts(&pool), vec![]); pool.free(&address(1)); - assert_eq!(pool.reserve_accounts(1), vec![address(1)]); + assert_eq!(free_accounts(&pool), vec![address(1)]); } #[test] @@ -128,11 +118,15 @@ fn should_count_only_the_free_accounts() { assert_eq!(pool.num_free_accounts(), 3); pool.bind(&address(1), durable_nonce(1)); - assert_eq!(pool.reserve_accounts(1), vec![address(2)]); + pool.bind(&address(2), durable_nonce(2)); assert_eq!(pool.num_free_accounts(), 1); } +fn free_accounts(pool: &DurableNoncePool) -> Vec
{ + pool.free_accounts().copied().collect() +} + 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 844b9b72..6fc36311 100644 --- a/minter/src/state/tests.rs +++ b/minter/src/state/tests.rs @@ -1366,7 +1366,7 @@ mod withdrawal_transactions { #[test] fn should_rebuild_the_bound_pool_and_created_bucket_from_a_log_ending_after_creation() { - let mut replayed = replay_events(log_of(funded_log_until_created_transaction())); + let replayed = replay_events(log_of(funded_log_until_created_transaction())); assert_eq!( replayed.balance(), @@ -1385,7 +1385,7 @@ mod withdrawal_transactions { Some(durable_nonce(1)) ); assert_eq!(replayed.withdrawal_status(0), WithdrawalStatus::Pending); - assert_eq!(replayed.reserve_nonce_accounts(1), vec![]); + assert_eq!(replayed.nonce_pool().num_free_accounts(), 0); } #[test] @@ -1562,7 +1562,11 @@ mod withdrawal_transactions { } fn assert_nonce_account_free() { - mutate_state(|s| assert_eq!(s.reserve_nonce_accounts(1), vec![NONCE_ACCOUNT])); - mutate_state(|s| s.unreserve_nonce_account(&NONCE_ACCOUNT)); + read_state(|s| { + assert_eq!( + s.nonce_pool().free_accounts().collect::>(), + vec![&NONCE_ACCOUNT] + ) + }); } } diff --git a/minter/src/withdraw/mod.rs b/minter/src/withdraw/mod.rs index 55d5553f..17ddb14e 100644 --- a/minter/src/withdraw/mod.rs +++ b/minter/src/withdraw/mod.rs @@ -180,17 +180,15 @@ struct BoundWithdrawal { } async fn create_transactions_batch(runtime: &R, minter_address: Address) { - let reserved_batches: Vec = mutate_state(|state| { - let max_batches = state - .nonce_pool() - .num_free_accounts() - .min(MAX_CONCURRENT_RPC_CALLS) + let reserved_batches: Vec = read_state(|state| { + let max_batches = MAX_CONCURRENT_RPC_CALLS .min(MAX_CONCURRENT_SIGNATURES.saturating_sub(state.created_withdrawal_txs().len())); - let batches: Vec<_> = state.withdrawal_batches().take(max_batches).collect(); state - .reserve_nonce_accounts(batches.len()) - .into_iter() - .zip(batches) + .nonce_pool() + .free_accounts() + .copied() + .zip(state.withdrawal_batches()) + .take(max_batches) .map(|(nonce_account, requests)| ReservedBatch { nonce_account, requests, @@ -215,10 +213,6 @@ async fn create_transaction( nonce_account, requests, } = batch; - let unreserve = scopeguard::guard((), |()| { - mutate_state(|state| state.unreserve_nonce_account(&nonce_account)); - }); - let nonce_value = match read_verified_nonce(runtime, nonce_account, minter_address).await { Ok(nonce_value) => nonce_value, Err(e) => { @@ -251,7 +245,6 @@ async fn create_transaction( return; } - scopeguard::ScopeGuard::into_inner(unreserve); mutate_state(|state| { process_event( state, diff --git a/minter/src/withdraw/tests.rs b/minter/src/withdraw/tests.rs index 5d978258..1d386c75 100644 --- a/minter/src/withdraw/tests.rs +++ b/minter/src/withdraw/tests.rs @@ -567,7 +567,7 @@ mod process_pending_withdrawals_tests { assert_eq!(EventsAssert::from_recorded(), events_before); assert_eq!(withdrawal_status(1), WithdrawalStatus::Pending); - assert_nonce_account_unreserved(); + assert_nonce_account_free(); } #[tokio::test] @@ -594,7 +594,7 @@ mod process_pending_withdrawals_tests { assert_eq!(EventsAssert::from_recorded(), events_before); assert_eq!(withdrawal_status(2), WithdrawalStatus::Pending); - assert_nonce_account_unreserved(); + assert_nonce_account_free(); } #[tokio::test] @@ -897,10 +897,12 @@ mod process_pending_withdrawals_tests { assert_eq!(runtime.set_timer_delays(), Vec::::new()); } - fn assert_nonce_account_unreserved() { - crate::state::mutate_state(|s| { - assert_eq!(s.reserve_nonce_accounts(1), vec![NONCE_ACCOUNT]); - s.unreserve_nonce_account(&NONCE_ACCOUNT); + fn assert_nonce_account_free() { + read_state(|s| { + assert_eq!( + s.nonce_pool().free_accounts().collect::>(), + vec![&NONCE_ACCOUNT] + ) }); } } From afce15805ee133cde56ff6f839c0da76a39193d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9gory=20Demay?= Date: Wed, 7 Oct 2026 09:16:16 +0000 Subject: [PATCH 06/10] refactor(minter): read the expiry block height of sweeps in the monitor Co-Authored-By: Claude Opus 5.5 --- minter/src/monitor/mod.rs | 9 +++++++-- minter/src/state/mod.rs | 9 --------- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/minter/src/monitor/mod.rs b/minter/src/monitor/mod.rs index d6f8c549..e05c0a18 100644 --- a/minter/src/monitor/mod.rs +++ b/minter/src/monitor/mod.rs @@ -10,7 +10,7 @@ use crate::{ runtime::CanisterRuntime, signer::sign_bytes, state::{ - TaskType, + MinterTransaction, TaskType, audit::process_event, event::{EventType, Signer, VersionedMessage}, mutate_state, read_state, @@ -76,7 +76,12 @@ async fn check_submitted_transactions(runtime: &R) -> bool { submitted.iter().map(|(sig, _)| *sig).collect(), submitted .iter() - .filter_map(|(sig, tx)| tx.block_height().map(|height| (*sig, height))) + .filter_map(|(sig, tx)| match tx { + MinterTransaction::SweepDeposit { block_height, .. } => { + Some((*sig, *block_height)) + } + MinterTransaction::Withdrawal { .. } => None, + }) .collect(), ) }); diff --git a/minter/src/state/mod.rs b/minter/src/state/mod.rs index 02495e25..7a817f49 100644 --- a/minter/src/state/mod.rs +++ b/minter/src/state/mod.rs @@ -1039,13 +1039,4 @@ impl MinterTransaction { | MinterTransaction::Withdrawal { signers, .. } => signers, } } - - /// 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, .. } => Some(*block_height), - MinterTransaction::Withdrawal { .. } => None, - } - } } From eb43d1ec5bbb5f1fb5b2ca42cabaf3caf56f6c32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9gory=20Demay?= Date: Wed, 7 Oct 2026 11:53:29 +0000 Subject: [PATCH 07/10] test(minter): assert which transaction submissions skip the preflight simulation Co-Authored-By: Claude Opus 5.5 --- minter/src/rpc/tests.rs | 38 ++++++++++++++++++++++++++++- minter/src/test_fixtures/runtime.rs | 7 +++++- 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/minter/src/rpc/tests.rs b/minter/src/rpc/tests.rs index b71d377f..8640eee8 100644 --- a/minter/src/rpc/tests.rs +++ b/minter/src/rpc/tests.rs @@ -4,6 +4,7 @@ use crate::{ Block, BlockHeight, GetBalanceError, GetNonceAccountError, GetRecentBlockError, GetTransactionError, NonceAccount, SubmitTransactionError, get_balance, get_nonce_account, get_recent_block, get_transaction, submit_transaction, + submit_transaction_skipping_preflight, }, test_fixtures::{ MINTER_ADDRESS, confirmed_block, confirmed_block_at_height, @@ -18,7 +19,10 @@ use crate::{ }; use assert_matches::assert_matches; use ic_canister_runtime::IcError; -use sol_rpc_types::{HttpOutcallError, RpcError, RpcSource, SupportedRpcProviderId}; +use sol_rpc_types::{ + HttpOutcallError, RpcConfig, RpcError, RpcSource, RpcSources, SendTransactionParams, + SupportedRpcProviderId, +}; use solana_transaction::{Message, Transaction}; use solana_transaction_status_client_types::{EncodedTransaction, TransactionBinaryEncoding}; @@ -315,6 +319,38 @@ mod submit_transaction_tests { assert_eq!(result, Err(SubmitTransactionError::InconsistentRpcResults)); } + #[tokio::test] + async fn should_keep_the_preflight_simulation() { + init_state(); + let runtime = TestCanisterRuntime::new() + .add_stub_response(SendTransactionResult::Consistent(Ok(signature()))); + + let result = submit_transaction(&runtime, transaction()).await; + + assert_eq!(result, Ok(signature().into())); + assert_eq!(sent_params(&runtime).skip_preflight, None); + } + + #[tokio::test] + async fn should_skip_the_preflight_simulation_when_requested() { + init_state(); + let runtime = TestCanisterRuntime::new() + .add_stub_response(SendTransactionResult::Consistent(Ok(signature()))); + + let result = submit_transaction_skipping_preflight(&runtime, transaction()).await; + + assert_eq!(result, Ok(signature().into())); + assert_eq!(sent_params(&runtime).skip_preflight, Some(true)); + } + + fn sent_params(runtime: &TestCanisterRuntime) -> SendTransactionParams { + let [call] = runtime.sent_update_calls().try_into().unwrap(); + assert_eq!(call.method, "sendTransaction"); + let (_sources, _config, params): (RpcSources, Option, SendTransactionParams) = + call.args(); + params + } + fn transaction() -> Transaction { let message = Message::new(&[], None); Transaction { diff --git a/minter/src/test_fixtures/runtime.rs b/minter/src/test_fixtures/runtime.rs index 931ccc0c..25147144 100644 --- a/minter/src/test_fixtures/runtime.rs +++ b/minter/src/test_fixtures/runtime.rs @@ -9,7 +9,7 @@ use crate::{ use async_trait::async_trait; use candid::{ CandidType, Principal, - utils::{ArgumentEncoder, decode_args, encode_args}, + utils::{ArgumentDecoder, ArgumentEncoder, decode_args, encode_args}, }; use ic_canister_runtime::{IcError, Runtime, StubRuntime}; use ic_cdk::call::{CallPerformFailed, Error as CallError}; @@ -239,6 +239,11 @@ impl SentUpdateCall { let (arg,) = decode_args(&self.args).expect("Failed to decode the call argument"); arg } + + /// Decodes all the Candid arguments of the recorded call. + pub fn args ArgumentDecoder<'a>>(&self) -> Args { + decode_args(&self.args).expect("Failed to decode the call arguments") + } } #[async_trait] From 0fce2b1c4f21a06c08a44cd6105a325a50fedad7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9gory=20Demay?= Date: Wed, 7 Oct 2026 11:57:16 +0000 Subject: [PATCH 08/10] test(minter): reuse the in-flight withdrawal fixtures in the nonce expiry test Co-Authored-By: Claude Opus 5.5 --- minter/src/monitor/tests.rs | 32 +++----------------------------- 1 file changed, 3 insertions(+), 29 deletions(-) diff --git a/minter/src/monitor/tests.rs b/minter/src/monitor/tests.rs index bee557be..9d410232 100644 --- a/minter/src/monitor/tests.rs +++ b/minter/src/monitor/tests.rs @@ -9,8 +9,8 @@ use crate::{ storage::reset_events, test_fixtures::{ EventsAssert, GetTransactionResult, MINIMUM_WITHDRAWAL_AMOUNT, account, - confirmed_block_at_height, durable_nonce, events, init_balance, init_schnorr_master_key, - init_state, runtime::TestCanisterRuntime, signature, + confirmed_block_at_height, events, init_balance, init_schnorr_master_key, init_state, + runtime::TestCanisterRuntime, signature, }, }; use sol_rpc_types::{ @@ -301,29 +301,11 @@ 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 sweep = submit_sweep_transaction_with_signature(1, EXPIRED_BLOCK_HEIGHT); - let nonce_withdrawal = submit_nonce_withdrawal_transaction(2); + let nonce_withdrawal = submit_withdrawal_transaction_with_signature(2); let runtime = TestCanisterRuntime::new() .with_increasing_time() @@ -511,11 +493,3 @@ fn submit_sweep_transaction_with_signature( events::submit_sweep_at_height(signature, vec![deposit_id], block_height); 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 -} From 565c2b1ed2d161509b5c4490bc37d86822d51b37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9gory=20Demay?= Date: Wed, 7 Oct 2026 14:06:55 +0000 Subject: [PATCH 09/10] fix(minter): log nonce account read failures as errors Co-Authored-By: Claude Opus 5.5 --- minter/src/withdraw/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/minter/src/withdraw/mod.rs b/minter/src/withdraw/mod.rs index 17ddb14e..6813f609 100644 --- a/minter/src/withdraw/mod.rs +++ b/minter/src/withdraw/mod.rs @@ -217,7 +217,7 @@ async fn create_transaction( Ok(nonce_value) => nonce_value, Err(e) => { log!( - Priority::Info, + Priority::Error, "Failed to read nonce account {nonce_account}, skipping withdrawal batch this round: {e}" ); return; From 519d2f9290f03f606f7db6035501f913d102662e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9gory=20Demay?= Date: Wed, 7 Oct 2026 14:07:49 +0000 Subject: [PATCH 10/10] fix(minter): retry the withdrawal round early only after progress Co-Authored-By: Claude Opus 5.5 --- docs/design.md | 2 +- minter/src/withdraw/mod.rs | 30 ++++++++++++------- minter/src/withdraw/tests.rs | 57 +++++++++++++++++++++++++++++++----- 3 files changed, 70 insertions(+), 19 deletions(-) diff --git a/docs/design.md b/docs/design.md index ff28c326..f35ab56a 100644 --- a/docs/design.md +++ b/docs/design.md @@ -470,7 +470,7 @@ The ckSOL minter's main address is the nonce authority of every account in the p For the oracle to be sound, a stale read must never be mistaken for an advance: nonce values are opaque hashes, so a value differing from the in-flight transaction's nonce could by itself be the account's past as well as its future, and a provider lagging behind an already observed state serves exactly such a past value. Since only the ckSOL minter can advance the nonce, the account's complete value history is the set of nonce values the ckSOL minter has bound to transactions, which the event log already records. Every read is therefore classified against that set: the bound value means the transaction has not landed; any other previously seen value is a stale response and yields no decision; a never-seen value can only be the account's new frontier, which proves the advance. -The pool size bounds the withdrawal throughput, since every withdrawal transaction occupies one nonce account while it is in flight. When no free nonce account is available, the affected withdrawal batches simply remain queued until a nonce account frees up; the processing timer retries after a short delay only while both a free nonce account and an affordable batch remain, or while a bound withdrawal transaction is still unsigned (after a signing failure, or because a round signs at most 10 transactions), and otherwise waits for its regular interval, so an exhausted pool whose bound transactions are all signed stops retrying entirely and a stale nonce read is retried at the delayed cadence rather than in a zero-delay loop. As a nonce account costs nothing beyond its rent exemption minimum, the pool can be sized generously; **5 accounts** are proposed initially, allowing 50 concurrent in-flight withdrawals at 10 transfers per transaction. +The pool size bounds the withdrawal throughput, since every withdrawal transaction occupies one nonce account while it is in flight. When no free nonce account is available, the affected withdrawal batches simply remain queued until a nonce account frees up; the processing timer retries after a short delay only if the round made progress, i.e., it created or signed at least one withdrawal transaction, and either both a free nonce account and an affordable batch remain, or a bound withdrawal transaction is still unsigned (after a signing failure, or because a round signs at most 10 transactions). Otherwise it waits for its regular interval, so an exhausted pool whose bound transactions are all signed stops retrying entirely, and a round in which every nonce read or every signature failed, e.g., because of a misconfigured nonce account or an unavailable signing service, is retried at the regular cadence rather than every few seconds. As a nonce account costs nothing beyond its rent exemption minimum, the pool can be sized generously; **5 accounts** are proposed initially, allowing 50 concurrent in-flight withdrawals at 10 transfers per transaction. Deposit sweeps continue to use recent block hashes. The double-pay hazard is specific to withdrawals: a sweep only moves funds between addresses controlled by the ckSOL minter, nothing is credited before the finalized transaction has been positively observed, and an expired sweep is dropped rather than resubmitted, as described in [Section 3.1.3](#313-manual-flow). diff --git a/minter/src/withdraw/mod.rs b/minter/src/withdraw/mod.rs index 6813f609..3579847d 100644 --- a/minter/src/withdraw/mod.rs +++ b/minter/src/withdraw/mod.rs @@ -154,13 +154,16 @@ pub async fn process_pending_withdrawals(runtime: R) { return; }; - create_transactions_batch(&runtime, minter_address).await; + let num_created = create_transactions_batch(&runtime, minter_address).await; let signed_transactions = sign_transactions_batch(&runtime, minter_address).await; + let made_progress = num_created > 0 || !signed_transactions.is_empty(); send_transactions_batch(&runtime, signed_transactions).await; - if read_state(|s| { - s.can_create_withdrawal_transaction() || s.has_unsigned_withdrawal_transaction() - }) { + if made_progress + && read_state(|s| { + s.can_create_withdrawal_transaction() || s.has_unsigned_withdrawal_transaction() + }) + { runtime.set_timer( WITHDRAWAL_PROCESSING_RETRY_DELAY, process_pending_withdrawals, @@ -179,7 +182,10 @@ struct BoundWithdrawal { message: Message, } -async fn create_transactions_batch(runtime: &R, minter_address: Address) { +async fn create_transactions_batch( + runtime: &R, + minter_address: Address, +) -> usize { let reserved_batches: Vec = read_state(|state| { let max_batches = MAX_CONCURRENT_RPC_CALLS .min(MAX_CONCURRENT_SIGNATURES.saturating_sub(state.created_withdrawal_txs().len())); @@ -201,14 +207,17 @@ async fn create_transactions_batch(runtime: &R, minter_addre .into_iter() .map(async |batch| create_transaction(runtime, minter_address, batch).await), ) - .await; + .await + .into_iter() + .filter(|created| *created) + .count() } async fn create_transaction( runtime: &R, minter_address: Address, batch: ReservedBatch, -) { +) -> bool { let ReservedBatch { nonce_account, requests, @@ -220,7 +229,7 @@ async fn create_transaction( Priority::Error, "Failed to read nonce account {nonce_account}, skipping withdrawal batch this round: {e}" ); - return; + return false; } }; if read_state(|state| state.nonce_pool().has_seen(&nonce_account, &nonce_value)) { @@ -228,7 +237,7 @@ async fn create_transaction( Priority::Info, "Read a stale nonce value for account {nonce_account}, skipping withdrawal batch this round" ); - return; + return false; } let burn_indices: Vec<_> = requests.iter().map(|r| r.burn_block_index).collect(); @@ -242,7 +251,7 @@ async fn create_transaction( Priority::Error, "Failed to build batch withdrawal transaction for burn indices {burn_indices:?}: {e}" ); - return; + return false; } mutate_state(|state| { @@ -256,6 +265,7 @@ async fn create_transaction( runtime, ) }); + true } async fn sign_transactions_batch( diff --git a/minter/src/withdraw/tests.rs b/minter/src/withdraw/tests.rs index 1d386c75..001a5d08 100644 --- a/minter/src/withdraw/tests.rs +++ b/minter/src/withdraw/tests.rs @@ -632,11 +632,13 @@ mod process_pending_withdrawals_tests { process_pending_withdrawals(failing_runtime).await; let num_events_after_failure = EventsAssert::from_recorded().len(); - EventsAssert::from_recorded().expect_contains_event_eq(EventType::CreatedWithdrawalTransaction { - burn_indices: vec![1_u64.into()], - nonce_account: NONCE_ACCOUNT, - nonce_value: durable_nonce(1), - }); + EventsAssert::from_recorded().expect_contains_event_eq( + EventType::CreatedWithdrawalTransaction { + burn_indices: vec![1_u64.into()], + nonce_account: NONCE_ACCOUNT, + nonce_value: durable_nonce(1), + }, + ); assert_eq!(withdrawal_status(1), WithdrawalStatus::Pending); let recovering_runtime = TestCanisterRuntime::new() @@ -817,26 +819,65 @@ mod process_pending_withdrawals_tests { #[tokio::test] async fn should_retry_later_when_an_affordable_batch_is_left_behind() { - init_state(); + let second_nonce_account = address(2); + init_state_with_args(InitArgs { + nonce_accounts: vec![NONCE_ACCOUNT.to_string(), second_nonce_account.to_string()], + ..valid_init_args() + }); init_balance(); init_schnorr_master_key(); - events::accept_withdrawal(account(1), 1, MINIMUM_WITHDRAWAL_AMOUNT); + let num_requests = MAX_WITHDRAWALS_PER_NONCE_TX + 1; + for i in 0..num_requests { + events::accept_withdrawal(account(i), i as u64, MINIMUM_WITHDRAWAL_AMOUNT); + } let runtime = TestCanisterRuntime::new() .with_increasing_time() + .add_stub_response(GetAccountInfoResult::Consistent(Ok(Some( + nonce_account_info(MINTER_ADDRESS, 1), + )))) .add_stub_response(GetAccountInfoResult::Consistent(Err( RpcError::ValidationError("account unavailable".to_string()), - ))); + ))) + .add_stub_response(SendTransactionResult::Consistent(Ok( + minter_signature().into() + ))) + .add_signer(sign_as_minter()); process_pending_withdrawals(runtime.clone()).await; + read_state(|s| assert_eq!(s.submitted_transactions().len(), 1)); + assert_eq!( + withdrawal_status(MAX_WITHDRAWALS_PER_NONCE_TX as u64), + WithdrawalStatus::Pending + ); assert_eq!( runtime.set_timer_delays(), vec![WITHDRAWAL_PROCESSING_RETRY_DELAY] ); } + #[tokio::test] + async fn should_not_retry_early_when_the_round_made_no_progress() { + init_state(); + init_balance(); + init_schnorr_master_key(); + + events::accept_withdrawal(account(1), 1, MINIMUM_WITHDRAWAL_AMOUNT); + + let runtime = TestCanisterRuntime::new() + .with_increasing_time() + .add_stub_response(GetAccountInfoResult::Consistent(Err( + RpcError::ValidationError("account unavailable".to_string()), + ))); + + process_pending_withdrawals(runtime.clone()).await; + + assert_eq!(withdrawal_status(1), WithdrawalStatus::Pending); + assert_eq!(runtime.set_timer_delays(), Vec::::new()); + } + #[tokio::test] async fn should_retry_later_when_a_bound_withdrawal_is_left_unsigned() { init_state();