From 3d5b769511ca625b70cb45ef180f840c3b83efc1 Mon Sep 17 00:00:00 2001 From: wiesche Date: Fri, 4 Sep 2026 20:47:07 +0200 Subject: [PATCH] derive contract signing state --- controller/tests/contract_srs.rs | 37 ++++++++++++++ libwallet/src/contract/actions/README.md | 8 +-- libwallet/src/contract/actions/new.rs | 11 +---- libwallet/src/contract/actions/setup.rs | 12 ++--- libwallet/src/contract/actions/sign.rs | 4 +- libwallet/src/contract/slate.rs | 7 +-- libwallet/src/contract/utils.rs | 63 +++++++++++++++--------- libwallet/src/slate.rs | 2 +- 8 files changed, 90 insertions(+), 54 deletions(-) diff --git a/controller/tests/contract_srs.rs b/controller/tests/contract_srs.rs index 374cd27..c1584aa 100644 --- a/controller/tests/contract_srs.rs +++ b/controller/tests/contract_srs.rs @@ -20,6 +20,7 @@ extern crate log; use grin_wallet_libwallet as libwallet; use grin_core::core::transaction::{CommitWrapper, Input, Inputs, OutputFeatures, Transaction}; +use grin_util::secp::Signature; use impls::test_framework::{self}; use libwallet::contract::my_fee_contribution; use libwallet::contract::types::{ContractNewArgsAPI, ContractSetupArgsAPI, OwnCommitmentStatus}; @@ -94,6 +95,30 @@ fn contract_srs_tx_impl(test_dir: &'static str) -> Result<(), libwallet::Error> signatures: 0, }, ); + wallet::controller::owner_single_use( + send_wallet.clone(), + send_mask, + PathBuf::from(test_dir), + |api, m| { + let progress = common::wallet_progress(api, m)?; + let mut invalid = slate.clone(); + invalid.participant_data[0].part_sig = + Some(Signature::from_raw_data(&[0; 64]).unwrap()); + let setup_args = ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), + net_change: Some(-5_000_000_000), + ..Default::default() + }; + let err = { + let mut owner = api.wallet_inst.lock(); + let wallet = owner.lc_provider()?.wallet_inst()?; + libwallet::contract::setup(wallet, m, &invalid, &setup_args).unwrap_err() + }; + assert!(matches!(err, libwallet::Error::LibTX(_))); + assert_eq!(common::wallet_progress(api, m)?, progress); + Ok(()) + }, + )?; wallet::controller::owner_single_use( recv_wallet.clone(), @@ -192,6 +217,18 @@ fn contract_srs_tx_impl(test_dir: &'static str) -> Result<(), libwallet::Error> if msg == &format!("Slate with id:{} has already been signed.", incoming.id) )); assert_eq!(common::wallet_progress(api, m)?, progress); + + let err = { + let mut owner = api.wallet_inst.lock(); + let wallet = owner.lc_provider()?.wallet_inst()?; + libwallet::contract::setup(wallet, m, &slate, &args).unwrap_err() + }; + assert!(matches!( + err, + libwallet::Error::GenericError(ref msg) + if msg == &format!("Slate with id:{} has already been signed.", slate.id) + )); + assert_eq!(common::wallet_progress(api, m)?, progress); Ok(()) }, )?; diff --git a/libwallet/src/contract/actions/README.md b/libwallet/src/contract/actions/README.md index cfa6f59..69be66a 100644 --- a/libwallet/src/contract/actions/README.md +++ b/libwallet/src/contract/actions/README.md @@ -65,16 +65,12 @@ Separating side effects until the 'save_step' part would make these functions mu - Decide whether `contract_accounts_switch.rs` is a legitimate scenario, or whether signing under a different account than setup should error. -#### save_step - - // TODO: - // - is_signed should be derived from the slate - // - Consider taking ownership of Context here. It should not be used after this is called. - ### Side-effects #### Setup + // Persisting setup::compute() requires verify_not_signed first // Side-effects: + // - contract_utils::verify_not_signed -> tx_log_iter // - height = w.w2n_client().get_chain_tip()?.0; // - maybe_context = w.get_private_context(keychain_mask, sl.id.as_bytes()) // - create_contract_ctx -> updater::refresh_outputs(wallet, keychain_mask, parent_key_id, false)?; diff --git a/libwallet/src/contract/actions/new.rs b/libwallet/src/contract/actions/new.rs index 897cf25..25001fd 100644 --- a/libwallet/src/contract/actions/new.rs +++ b/libwallet/src/contract/actions/new.rs @@ -39,17 +39,10 @@ where K: Keychain, { // Compute state for 'new' - let (slate, mut context) = compute(w, keychain_mask, setup_args, slate_id)?; + let (slate, context) = compute(w, keychain_mask, setup_args, slate_id)?; // Atomically commit state - contract::utils::save_step( - w, - keychain_mask, - &slate, - &mut context, - setup_args.add_outputs, - false, - )?; + contract::utils::save_step(w, keychain_mask, &slate, context, setup_args.add_outputs)?; Ok(slate) } diff --git a/libwallet/src/contract/actions/setup.rs b/libwallet/src/contract/actions/setup.rs index ddb8213..452cedf 100644 --- a/libwallet/src/contract/actions/setup.rs +++ b/libwallet/src/contract/actions/setup.rs @@ -35,18 +35,12 @@ where C: NodeClient, K: Keychain, { + contract::utils::verify_not_signed(w, slate.id)?; // Compute state for 'setup' - let (slate, mut context) = compute(w, keychain_mask, slate, setup_args)?; + let (slate, context) = compute(w, keychain_mask, slate, setup_args)?; // Atomically commit state - contract::utils::save_step( - w, - keychain_mask, - &slate, - &mut context, - setup_args.add_outputs, - false, - )?; + contract::utils::save_step(w, keychain_mask, &slate, context, setup_args.add_outputs)?; Ok(slate) } diff --git a/libwallet/src/contract/actions/sign.rs b/libwallet/src/contract/actions/sign.rs index 9e6cb8b..fcbfbde 100644 --- a/libwallet/src/contract/actions/sign.rs +++ b/libwallet/src/contract/actions/sign.rs @@ -70,7 +70,7 @@ where .map(|context| !context.has_inputs_or_outputs()) .unwrap_or(true); // Compute state for 'sign' - let (sl, mut context) = compute( + let (sl, context) = compute( w, keychain_mask, slate, @@ -79,7 +79,7 @@ where )?; // Atomically commit state - contract::utils::save_step(w, keychain_mask, &sl, &mut context, will_add_outputs, true)?; + contract::utils::save_step(w, keychain_mask, &sl, context, will_add_outputs)?; Ok(sl) } diff --git a/libwallet/src/contract/slate.rs b/libwallet/src/contract/slate.rs index e7b3216..dd306c2 100644 --- a/libwallet/src/contract/slate.rs +++ b/libwallet/src/contract/slate.rs @@ -465,9 +465,8 @@ pub(super) fn transition_state(slate: &mut Slate) -> Result<(), Error> { /// Add partial signature to the slate. // Nonce reuse is prevented by forgetting the signing context after a signed step: -// save_step deletes the private context (which holds sec_key/sec_nonce) once is_signed -// (except the deliberately-retained step2 context used for safe cancel). The context is -// keyed by slate.id, so a given nonce is only ever used for one message. +// save_step deletes sec_key/sec_nonce after signing, except for step2 contract view +// Contexts are keyed by slate.id so a nonce is only used for one message pub fn sign( w: &mut WalletBackend, keychain_mask: Option<&SecretKey>, @@ -490,6 +489,8 @@ where ))); } let keychain = w.keychain(keychain_mask)?; + // Do not let fill_round_2 skip a missing participant + slate.find_index_matching_context(&keychain, context)?; slate.fill_round_2(&keychain, &context.sec_key, &context.sec_nonce)?; debug!( "contract::sign => signed for slate fees: {}", diff --git a/libwallet/src/contract/utils.rs b/libwallet/src/contract/utils.rs index 83c85f5..962191c 100644 --- a/libwallet/src/contract/utils.rs +++ b/libwallet/src/contract/utils.rs @@ -60,12 +60,14 @@ pub fn create_tx_log_entry( Ok(t) } -/// Updates TxLogEntry for a contract with information available in the 'sign' step +/// Update TxLogEntry with data from the sign step +/// `participant_index` is our entry in `slate.participant_data` pub fn update_tx_log_entry( wallet: &mut WalletBackend, - keychain_mask: Option<&SecretKey>, + keychain: &K, slate: &Slate, context: &Context, + participant_index: usize, tx_log_entry: &mut TxLogEntry, ) -> Result<(), Error> where @@ -73,7 +75,6 @@ where K: Keychain, { // This is expected to be called when we are signing the contract and have already contributed inputs & outputs - let keychain = wallet.keychain(keychain_mask)?; let parent_key_id = context.parent_key_id.clone(); let current_height = wallet.w2n_client().get_chain_tip()?.0; // We have already contributed inputs and outputs so we know how much of each we contribute @@ -93,7 +94,7 @@ where // note we only use a single path for now let sender_address_path = 0u32; let sender_key = address::address_from_derivation_path( - &keychain, + keychain, &parent_key_id, sender_address_path, )?; @@ -102,14 +103,20 @@ where // We're looking for the OTHER party here, the recipient. The 'xor 1' pairing // only holds for a two-party slate, so check that before indexing: a slate // with a shorter participant list would otherwise panic here. - let sender_index = slate.find_index_matching_context(&keychain, context)?; if slate.participant_data.len() != 2 { return Err(Error::GenericError(format!( "Expected 2 participants for a payment proof, found {}", slate.participant_data.len() ))); } - let recipient_index = sender_index ^ 1; + let sender = slate + .participant_data + .get(participant_index) + .ok_or(Error::ContextToIndex)?; + let recipient = slate + .participant_data + .get(participant_index ^ 1) + .ok_or(Error::ContextToIndex)?; tx_log_entry.payment_proof = Some(StoredProofInfo { receiver_address: p.receiver_address, @@ -120,14 +127,12 @@ where // Filled as separate steps for now; could be merged into a general case // once we know which nonces here belong to the recipient. proof_type: Some(p.proof_type.as_u8()), - receiver_public_nonce: Some(slate.participant_data[recipient_index].public_nonce), - receiver_public_excess: Some( - slate.participant_data[recipient_index].public_blind_excess, - ), + receiver_public_nonce: Some(recipient.public_nonce), + receiver_public_excess: Some(recipient.public_blind_excess), timestamp: Some(timestamp), memo: p.memo.clone(), promise_signature: p.promise_signature, - sender_part_sig: slate.participant_data[sender_index].part_sig, + sender_part_sig: sender.part_sig, }); } } @@ -178,15 +183,15 @@ pub fn get_net_change( Ok(expected_net_change.unwrap()) } -/// Atomically locks the inputs and saves the changes of Context, TxLogEntry and OutputData. -/// Additionally, the transaction is saved in a file in case we signed it. +/// Lock inputs and store the Context, TxLogEntry and OutputData atomically +/// Consumes the context and derives the signed state from our participant data +/// Signed transactions are written outside the database batch pub fn save_step( w: &mut WalletBackend, keychain_mask: Option<&SecretKey>, slate: &Slate, - context: &mut Context, + mut context: Context, step_added_outputs: bool, - is_signed: bool, ) -> Result<(), Error> where C: NodeClient, @@ -198,9 +203,16 @@ where ); // Phase 1 - precompute the data needed for atomic update let parent_key_id = &context.parent_key_id; + let keychain = w.keychain(keychain_mask)?; + let participant_index = slate.find_index_matching_context(&keychain, &context)?; + let is_signed = slate.participant_data[participant_index].is_complete(); + if is_signed { + // Verify part_sig before using it as signed state + slate.verify_part_sigs(keychain.secp())?; + } let current_height = w.w2n_client().get_chain_tip()?.0; // We are at step2 if we don't have context.log_id and we have signed the slate - let is_step2 = !context.log_id.is_some() && is_signed; + let is_step2 = context.log_id.is_none() && is_signed; let mut tx_log_entry = { if context.log_id.is_none() { @@ -215,7 +227,14 @@ where // Update TxLogEntry if we have signed the contract (we have data about the kernel) if is_signed { - update_tx_log_entry(w, keychain_mask, &slate, &context, &mut tx_log_entry)?; + update_tx_log_entry( + w, + &keychain, + slate, + &context, + participant_index, + &mut tx_log_entry, + )?; // Record where the transaction is stored, as internal::selection does for a // standard send. store_tx below writes it under this name, and 'txs' reads the // field to report whether the transaction data is held. @@ -252,7 +271,7 @@ where let mut batch = w.batch(keychain_mask)?; // Update TxLogEntry - if !context.log_id.is_some() { + if context.log_id.is_none() { // If we just created the TxLogEntry, we have to assign it an id let log_id = batch.next_tx_log_id(&parent_key_id)?; tx_log_entry.id = log_id; @@ -279,8 +298,7 @@ where if is_signed && !is_step2 { // NOTE: We MUST forget the context when we sign. Ideally, these two would be atomic or perhaps // when we call slate::sigadd_partial_signaturen we could swap the secret key with a temporary one just to be safe. - // The reason we don't delete if we are at step2 is because in case we want to do safe cancel, - // we need to know which inputs are in the context to know which input we have to double-spend. + // Keep the step2 context for contract view batch.delete_private_context(slate.id.as_bytes())?; } else { batch.save_private_context(slate.id.as_bytes(), &context)?; @@ -288,10 +306,7 @@ where batch.commit()?; - // Defense in depth: once we have signed (and are past the step2 context that is kept - // deliberately for safe cancel), the signing context holding sec_key/sec_nonce must be - // gone so the nonce can never be reused. It is deleted in the is_signed && !is_step2 - // branch above; verify the deletion actually took effect. + // Confirm sec_key/sec_nonce are gone after signing, except for step2 if is_signed && !is_step2 { match w.get_private_context(keychain_mask, slate.id.as_bytes()) { Err(Error::NotFoundErr(_)) => {} diff --git a/libwallet/src/slate.rs b/libwallet/src/slate.rs index b3799da..0b8c658 100644 --- a/libwallet/src/slate.rs +++ b/libwallet/src/slate.rs @@ -704,7 +704,7 @@ impl Slate { } /// Verifies all of the partial signatures in the Slate are valid - fn verify_part_sigs(&self, secp: &secp::Secp256k1) -> Result<(), Error> { + pub(crate) fn verify_part_sigs(&self, secp: &secp::Secp256k1) -> Result<(), Error> { // collect public nonces for p in self.participant_data.iter() { if p.is_complete() {