diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 25c07ea4..dd7be67e 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -2,7 +2,7 @@ fail_fast: true repos: - repo: https://github.com/crate-ci/typos - rev: v1.22.9 + rev: v1.24.1 hooks: - id: typos - repo: local diff --git a/src/signing/collector/signatures_collector.rs b/src/signing/collector/signatures_collector.rs index 7992be38..cedee799 100644 --- a/src/signing/collector/signatures_collector.rs +++ b/src/signing/collector/signatures_collector.rs @@ -144,7 +144,38 @@ impl SignaturesCollector { Continue } - async fn sign_with_factors_of_kind(&self, factor_sources_of_kind: FactorSourcesOfKind) { + fn should_neglect_factors_due_to_irrelevant( + &self, + factor_sources_of_kind: &FactorSourcesOfKind, + ) -> bool { + let state = self.state.borrow(); + let petitions = state.petitions.borrow(); + petitions.should_neglect_factors_due_to_irrelevant(factor_sources_of_kind) + } + + fn neglected_factors_due_to_irrelevant( + &self, + factor_sources_of_kind: &FactorSourcesOfKind, + ) -> bool { + if self.should_neglect_factors_due_to_irrelevant(factor_sources_of_kind) { + info!( + "Neglecting all factors of kind: {} since they are all irrelevant (all TX referencing those factors have already failed)", + factor_sources_of_kind.kind + ); + self.process_batch_response(SignWithFactorsOutcome::irrelevant(factor_sources_of_kind)); + true + } else { + false + } + } + + async fn sign_with_factors_of_kind(&self, factor_sources_of_kind: &FactorSourcesOfKind) { + info!( + "Use(?) #{:?} factors of kind: {:?}", + &factor_sources_of_kind.factor_sources().len(), + &factor_sources_of_kind.kind + ); + let interactor = self .dependencies .interactors @@ -155,12 +186,7 @@ impl SignaturesCollector { SigningInteractor::Parallel(interactor) => { // Prepare the request for the interactor debug!("Creating parallel request for interactor"); - let request = self.request_for_parallel_interactor( - factor_sources - .into_iter() - .map(|f| f.factor_source_id()) - .collect(), - ); + let request = self.request_for_parallel_interactor(factor_sources_of_kind); if !request.invalid_transactions_if_neglected.is_empty() { info!( "If factors {:?} are neglected, invalid TXs: {:?}", @@ -212,15 +238,13 @@ impl SignaturesCollector { /// In decreasing "friction order" async fn sign_with_factors(&self) -> Result<()> { let factors_of_kind = self.dependencies.factors_of_kind.clone(); - for factor_sources_of_kind in factors_of_kind.into_iter() { + for factor_sources_of_kind in factors_of_kind.iter() { if self.continuation() == FinishEarly { break; } - info!( - "Use(?) #{:?} factors of kind: {:?}", - &factor_sources_of_kind.factor_sources().len(), - &factor_sources_of_kind.kind - ); + if self.neglected_factors_due_to_irrelevant(factor_sources_of_kind) { + continue; + } self.sign_with_factors_of_kind(factor_sources_of_kind).await; } info!("FINISHED WITH ALL FACTORS"); @@ -250,14 +274,19 @@ impl SignaturesCollector { *factor_source_id, ])) .into_iter() - .collect_vec(), + .collect::>(), ) } fn request_for_parallel_interactor( &self, - factor_source_ids: IndexSet, + factor_sources_of_kind: &FactorSourcesOfKind, ) -> ParallelBatchSigningRequest { + let factor_source_ids = factor_sources_of_kind + .factor_sources() + .iter() + .map(|f| f.factor_source_id()) + .collect::>(); let per_factor_source = factor_source_ids .clone() .iter() @@ -267,13 +296,12 @@ impl SignaturesCollector { let invalid_transactions_if_neglected = self.invalid_transactions_if_neglected_factor_sources(factor_source_ids); - info!( - "Invalid if neglected: {:?}", - invalid_transactions_if_neglected - ); - // Prepare the request for the interactor - ParallelBatchSigningRequest::new(per_factor_source, invalid_transactions_if_neglected) + ParallelBatchSigningRequest::new( + factor_sources_of_kind.kind, + per_factor_source, + invalid_transactions_if_neglected, + ) } fn invalid_transactions_if_neglected_factor_sources( @@ -441,6 +469,28 @@ mod tests { test(WhenAllTransactionsAreValid(Continue), 2).await; } + #[test] + fn factor_source_kinds_order() { + let kinds = HDFactorSource::all() + .into_iter() + .map(|f| f.factor_source_kind()) + .collect::>(); + let mut kinds = kinds.into_iter().collect_vec(); + kinds.sort(); + let kinds = kinds.into_iter().collect::>(); + assert_eq!( + kinds, + IndexSet::::from_iter([ + FactorSourceKind::Ledger, + FactorSourceKind::Arculus, + FactorSourceKind::Yubikey, + FactorSourceKind::SecurityQuestions, + FactorSourceKind::OffDeviceMnemonic, + FactorSourceKind::Device, + ]) + ) + } + #[test] fn test_profile() { let factor_sources = &HDFactorSource::all(); diff --git a/src/signing/collector/signing_finish_early_strategy.rs b/src/signing/collector/signing_finish_early_strategy.rs index afd87f35..2525ae37 100644 --- a/src/signing/collector/signing_finish_early_strategy.rs +++ b/src/signing/collector/signing_finish_early_strategy.rs @@ -14,7 +14,7 @@ pub struct WhenSomeTransactionIsInvalid(pub SignaturesCollectingContinuation); impl Default for WhenSomeTransactionIsInvalid { fn default() -> Self { - Self(SignaturesCollectingContinuation::FinishEarly) + Self(SignaturesCollectingContinuation::Continue) } } @@ -60,4 +60,17 @@ mod tests { SignaturesCollectingContinuation::Continue ); } + + #[test] + fn test_default_is_finish_when_valid_continue_if_invalid() { + let sut = Sut::default(); + assert_eq!( + sut.when_all_transactions_are_valid.0, + SignaturesCollectingContinuation::FinishEarly + ); + assert_eq!( + sut.when_some_transaction_is_invalid.0, + SignaturesCollectingContinuation::Continue + ); + } } diff --git a/src/signing/interactors/batch_tx_batch_key_signing_request.rs b/src/signing/interactors/batch_tx_batch_key_signing_request.rs index 86fe6269..ca965bef 100644 --- a/src/signing/interactors/batch_tx_batch_key_signing_request.rs +++ b/src/signing/interactors/batch_tx_batch_key_signing_request.rs @@ -17,28 +17,94 @@ pub struct BatchKeySigningRequest { } impl BatchKeySigningRequest { - pub fn signature_inputs(&self) -> IndexSet { - self.owned_factor_instances - .clone() - .into_iter() - .map(|fi| HDSignatureInput::new(self.intent_hash.clone(), fi)) - .collect() - } - + /// # Panics + /// Panics if any of the owned factor instances does not match the `factor_source_id`. + /// + /// Panics if `owned_factor_instances` is empty. pub fn new( intent_hash: IntentHash, factor_source_id: FactorSourceIDFromHash, owned_factor_instances: IndexSet, ) -> Self { + assert!( + !owned_factor_instances.is_empty(), + "Invalid input, `owned_factor_instances` must not be empty." + ); assert!(owned_factor_instances .iter() - .all(|f| f.by_factor_source(factor_source_id))); + .all(|f| f.by_factor_source(factor_source_id)), "Discrepancy! Mismatch between FactorSourceID of owned factor instances and specified FactorSourceID, this is a programmer error."); Self { intent_hash, factor_source_id, owned_factor_instances: owned_factor_instances.into_iter().collect_vec(), } } + + pub fn signature_inputs(&self) -> IndexSet { + self.owned_factor_instances + .clone() + .into_iter() + .map(|fi| HDSignatureInput::new(self.intent_hash.clone(), fi)) + .collect() + } +} + +impl HasSampleValues for BatchKeySigningRequest { + fn sample() -> Self { + Self::new( + IntentHash::sample(), + FactorSourceIDFromHash::sample(), + IndexSet::from_iter([OwnedFactorInstance::sample()]), + ) + } + + fn sample_other() -> Self { + Self::new( + IntentHash::sample_other(), + FactorSourceIDFromHash::sample_other(), + IndexSet::from_iter([OwnedFactorInstance::sample_other()]), + ) + } +} + +#[cfg(test)] +mod tests_batch_req { + use super::*; + + type Sut = BatchKeySigningRequest; + + #[test] + fn equality() { + assert_eq!(Sut::sample(), Sut::sample()); + assert_eq!(Sut::sample_other(), Sut::sample_other()); + } + + #[test] + fn inequality() { + assert_ne!(Sut::sample(), Sut::sample_other()); + } + + #[test] + #[should_panic(expected = "Invalid input, `owned_factor_instances` must not be empty.")] + fn panics_if_owned_factors_is_empty() { + Sut::new( + IntentHash::sample(), + FactorSourceIDFromHash::sample(), + IndexSet::new(), + ); + } + + #[test] + #[should_panic( + expected = "Discrepancy! Mismatch between FactorSourceID of owned factor instances and specified FactorSourceID, this is a programmer error." + )] + fn panics_mismatch_factor_source_id() { + Sut::new( + IntentHash::sample(), + FactorSourceIDFromHash::sample(), + IndexSet::from_iter([OwnedFactorInstance::sample_other()]), + ); + } } /// A batch of transactions each batching over multiple keys (derivation paths) @@ -53,16 +119,81 @@ pub struct BatchTXBatchKeySigningRequest { } impl BatchTXBatchKeySigningRequest { + /// # Panics + /// Panics if `per_transaction` is empty + /// + /// Also panics if `per_transaction` if the factor source id + /// of each request does not match `factor_source_id`. pub fn new( factor_source_id: FactorSourceIDFromHash, per_transaction: IndexSet, ) -> Self { + assert!( + !per_transaction.is_empty(), + "Invalid input. No transaction to sign, this is a programmer error." + ); + assert!(per_transaction .iter() - .all(|f| f.factor_source_id == factor_source_id)); + .all(|f| f.factor_source_id == factor_source_id), "Discprepancy! Input for one of the transactions has a mismatching FactorSourceID, this is a programmer error."); + Self { factor_source_id, per_transaction: per_transaction.into_iter().collect(), } } + + pub fn factor_source_kind(&self) -> FactorSourceKind { + self.factor_source_id.kind + } +} + +impl HasSampleValues for BatchTXBatchKeySigningRequest { + fn sample() -> Self { + Self::new( + FactorSourceIDFromHash::sample(), + IndexSet::from_iter([BatchKeySigningRequest::sample()]), + ) + } + + fn sample_other() -> Self { + Self::new( + FactorSourceIDFromHash::sample_other(), + IndexSet::from_iter([BatchKeySigningRequest::sample_other()]), + ) + } +} + +#[cfg(test)] +mod tests { + use super::*; + type Sut = BatchTXBatchKeySigningRequest; + + #[test] + fn equality() { + assert_eq!(Sut::sample(), Sut::sample()); + assert_eq!(Sut::sample_other(), Sut::sample_other()); + } + + #[test] + fn inequality() { + assert_ne!(Sut::sample(), Sut::sample_other()); + } + + #[test] + #[should_panic(expected = "Invalid input. No transaction to sign, this is a programmer error.")] + fn panics_if_per_transaction_is_empty() { + Sut::new(FactorSourceIDFromHash::sample(), IndexSet::new()); + } + + #[test] + #[should_panic( + expected = "Discprepancy! Input for one of the transactions has a mismatching FactorSourceID, this is a programmer error." + )] + fn panics_if_factor_source_mismatch() { + Sut::new( + FactorSourceIDFromHash::sample(), + IndexSet::from_iter([BatchKeySigningRequest::sample_other()]), + ); + } } diff --git a/src/signing/interactors/parallel_batch_signing_request.rs b/src/signing/interactors/parallel_batch_signing_request.rs index 2834352c..852f0c86 100644 --- a/src/signing/interactors/parallel_batch_signing_request.rs +++ b/src/signing/interactors/parallel_batch_signing_request.rs @@ -2,9 +2,11 @@ use crate::prelude::*; /// A collection of factor sources to use to sign, transactions with multiple keys /// (derivations paths). -#[derive(derive_more::Debug)] +#[derive(derive_more::Debug, Clone)] #[debug("per_factor_source: {:#?}", per_factor_source)] pub struct ParallelBatchSigningRequest { + factor_source_kind: FactorSourceKind, + /// Per factor source, a set of transactions to sign, with /// multiple derivations paths. pub per_factor_source: IndexMap, @@ -15,13 +17,63 @@ pub struct ParallelBatchSigningRequest { } impl ParallelBatchSigningRequest { + /// # Panics + /// Panics if `per_factor_source` is empty + /// + /// Panics if not all factor sources are of the same kind pub fn new( + factor_source_kind: FactorSourceKind, per_factor_source: IndexMap, invalid_transactions_if_neglected: IndexSet, ) -> Self { + assert!( + !per_factor_source.is_empty(), + "Invalid input, per_factor_source must not be empty, this is a programmer error." + ); + assert!( + per_factor_source + .values() + .all(|f| f.factor_source_id.kind == factor_source_kind), + "Discrepancy! All factor sources must be of the same kind, this is a programmer error." + ); + Self { + factor_source_kind, per_factor_source, invalid_transactions_if_neglected, } } + + pub fn factor_source_kind(&self) -> FactorSourceKind { + self.factor_source_kind + } +} + +#[cfg(test)] +mod tests { + use super::*; + type Sut = ParallelBatchSigningRequest; + + #[test] + #[should_panic( + expected = "Invalid input, per_factor_source must not be empty, this is a programmer error." + )] + fn panics_if_per_factor_source_is_empty() { + Sut::new(FactorSourceKind::Device, IndexMap::new(), IndexSet::new()); + } + + #[test] + #[should_panic( + expected = "Discrepancy! All factor sources must be of the same kind, this is a programmer error." + )] + fn panics_if_wrong_factor_source_kind() { + Sut::new( + FactorSourceKind::Arculus, + IndexMap::from_iter([( + FactorSourceIDFromHash::sample(), + BatchTXBatchKeySigningRequest::sample(), + )]), + IndexSet::new(), + ); + } } diff --git a/src/signing/interactors/serial_batch_signing_request.rs b/src/signing/interactors/serial_batch_signing_request.rs index 671093b6..60968b86 100644 --- a/src/signing/interactors/serial_batch_signing_request.rs +++ b/src/signing/interactors/serial_batch_signing_request.rs @@ -4,23 +4,27 @@ use crate::prelude::*; /// a collection of transactions to sign with multiple keys (derivation paths), /// and a collection of transactions which would be invalid if the user skips /// signing with this factor source, or if we fail to sign. -#[derive(derive_more::Debug)] +#[derive(derive_more::Debug, Clone)] #[debug("input: {:#?}", input)] pub struct SerialBatchSigningRequest { pub input: BatchTXBatchKeySigningRequest, /// A collection of transactions which would be invalid if the user skips /// signing with this factor source, or if we fail to sign - pub invalid_transactions_if_neglected: Vec, + pub invalid_transactions_if_neglected: IndexSet, } impl SerialBatchSigningRequest { pub fn new( input: BatchTXBatchKeySigningRequest, - invalid_transactions_if_neglected: Vec, + invalid_transactions_if_neglected: IndexSet, ) -> Self { Self { input, invalid_transactions_if_neglected, } } + + pub fn factor_source_kind(&self) -> FactorSourceKind { + self.input.factor_source_kind() + } } diff --git a/src/signing/petition_types/petition_entity.rs b/src/signing/petition_types/petition_entity.rs index 8ebf4ad4..c872b793 100644 --- a/src/signing/petition_types/petition_entity.rs +++ b/src/signing/petition_types/petition_entity.rs @@ -65,12 +65,18 @@ impl PetitionEntity { ) } - /// Returns `true` signatures requirement has been fulfilled, either by + /// Returns `true` if signatures requirement has been fulfilled, either by /// override factors or by threshold factors pub fn has_signatures_requirement_been_fulfilled(&self) -> bool { self.status() == PetitionFactorsStatus::Finished(PetitionFactorsStatusFinished::Success) } + /// Returns `true` if the transaction of this petition already has failed due + /// to too many factors neglected + pub fn has_failed(&self) -> bool { + self.status() == PetitionFactorsStatus::Finished(PetitionFactorsStatusFinished::Fail) + } + fn union_of(&self, map: F) -> IndexSet where T: Eq + std::hash::Hash + Clone, @@ -143,10 +149,6 @@ impl PetitionEntity { self.both(r#do, |_, _| ()) } - pub fn neglected_factor_source_if_relevant(&self, neglected: NeglectedFactor) { - self.both_void(|l| l.neglect_if_references(neglected.clone(), true)); - } - /// # Panics /// Panics if this factor source has already been neglected or signed with. /// @@ -164,6 +166,18 @@ impl PetitionEntity { }) } + pub(crate) fn should_neglect_factors_due_to_irrelevant( + &self, + factor_source_ids: IndexSet, + ) -> bool { + assert!(self.references_any_factor_source(&factor_source_ids)); + match self.status() { + PetitionFactorsStatus::Finished(PetitionFactorsStatusFinished::Fail) => true, + PetitionFactorsStatus::Finished(PetitionFactorsStatusFinished::Success) => false, + PetitionFactorsStatus::InProgress => false, + } + } + pub fn invalid_transactions_if_neglected_factors( &self, factor_source_ids: IndexSet, @@ -190,20 +204,33 @@ impl PetitionEntity { let simulation = self.clone(); for factor_source_id in factor_source_ids.iter() { simulation - .neglect_if_relevant( - NeglectedFactor::new( - NeglectFactorReason::UserExplicitlySkipped, - *factor_source_id, - ), - true, - ) + .neglect_if_referenced(NeglectedFactor::new( + NeglectFactorReason::Simulation, + *factor_source_id, + )) .unwrap(); } simulation.status() } - pub fn neglect_if_relevant(&self, neglected: NeglectedFactor, simulated: bool) -> Result<()> { - self.both_void(|p| p.neglect_if_relevant(neglected.clone(), simulated)); + pub fn references_any_factor_source( + &self, + factor_source_ids: &IndexSet, + ) -> bool { + factor_source_ids + .iter() + .any(|f| self.references_factor_source_with_id(f)) + } + + pub fn references_factor_source_with_id(&self, id: &FactorSourceIDFromHash) -> bool { + self.both( + |p| p.references_factor_source_with_id(id), + |a, b| a.unwrap_or(false) || b.unwrap_or(false), + ) + } + + pub fn neglect_if_referenced(&self, neglected: NeglectedFactor) -> Result<()> { + self.both_void(|p| p.neglect_if_referenced(neglected.clone())); Ok(()) } diff --git a/src/signing/petition_types/petition_factors_types/neglected_factor_instance.rs b/src/signing/petition_types/petition_factors_types/neglected_factor_instance.rs index 991c8c4f..cd53592a 100644 --- a/src/signing/petition_types/petition_factors_types/neglected_factor_instance.rs +++ b/src/signing/petition_types/petition_factors_types/neglected_factor_instance.rs @@ -81,4 +81,17 @@ pub enum NeglectFactorReason { #[display("Failure")] #[debug("Failure")] Failure, + + /// A FactorSource got neglected implicitly since it is irrelevant, + /// all transactions which references the FactorSource have already + /// failed, thus pointless in using it. + #[display("Irrelevant")] + #[debug("Irrelevant")] + Irrelevant, + + /// We simulate neglect in order to see what the status of petitions + /// would be if a FactorSource would be neglected. + #[display("Simulation")] + #[debug("Simulation")] + Simulation, } diff --git a/src/signing/petition_types/petition_factors_types/petition_factors.rs b/src/signing/petition_types/petition_factors_types/petition_factors.rs index 7bd713f6..49ed9f2d 100644 --- a/src/signing/petition_types/petition_factors_types/petition_factors.rs +++ b/src/signing/petition_types/petition_factors_types/petition_factors.rs @@ -73,14 +73,14 @@ impl PetitionFactors { )) } - pub fn neglect_if_relevant(&self, neglected: NeglectedFactor, simulated: bool) { + pub fn neglect_if_referenced(&self, neglected: NeglectedFactor) { let factor_source_id = &neglected.factor_source_id(); if let Some(_x_) = self.reference_to_factor_source_with_id(factor_source_id) { debug!( - "PetitionFactors = kind {:?} neglect factor source with id: {}, reason: {}, simulated: {}", - self.factor_list_kind, factor_source_id, neglected.reason, simulated + "PetitionFactors = kind {:?} neglect factor source with id: {}, reason: {}", + self.factor_list_kind, factor_source_id, neglected.reason ); - self.neglect(neglected, simulated) + self.neglect(neglected) } else { debug!( "PetitionFactors = kind {:?} did not reference factor source with id: {}", @@ -89,13 +89,15 @@ impl PetitionFactors { } } - fn neglect(&self, neglected: NeglectedFactor, simulated: bool) { + fn neglect(&self, neglected: NeglectedFactor) { let factor_instance = self.expect_reference_to_factor_source_with_id(&neglected.factor_source_id()); - self.state.borrow_mut().neglect( - &NeglectedFactorInstance::new(neglected.reason, factor_instance.clone()), - simulated, - ); + self.state + .borrow_mut() + .neglect(&NeglectedFactorInstance::new( + neglected.reason, + factor_instance.clone(), + )); } pub fn has_owned_instance_with_id(&self, owned_factor_instance: &OwnedFactorInstance) -> bool { @@ -133,12 +135,6 @@ impl PetitionFactors { .is_some() } - pub fn neglect_if_references(&self, neglected: NeglectedFactor, simulated: bool) { - if self.references_factor_source_with_id(&neglected.factor_source_id()) { - self.neglect(neglected, simulated) - } - } - fn expect_reference_to_factor_source_with_id( &self, factor_source_id: &FactorSourceIDFromHash, diff --git a/src/signing/petition_types/petition_factors_types/petition_factors_state.rs b/src/signing/petition_types/petition_factors_types/petition_factors_state.rs index 8b549901..3dc27a3d 100644 --- a/src/signing/petition_types/petition_factors_types/petition_factors_state.rs +++ b/src/signing/petition_types/petition_factors_types/petition_factors_state.rs @@ -58,8 +58,8 @@ impl PetitionFactorsState { /// # Panics /// Panics if this factor source has already been neglected or signed and /// this is not a simulation. - pub(crate) fn neglect(&self, neglected: &NeglectedFactorInstance, simulated: bool) { - if !simulated { + pub(crate) fn neglect(&self, neglected: &NeglectedFactorInstance) { + if neglected.reason != NeglectFactorReason::Simulation { self.assert_not_referencing_factor_source(neglected.factor_source_id()); } self.neglected.borrow_mut().insert(neglected); @@ -93,14 +93,15 @@ mod tests { type Sut = PetitionFactorsState; impl PetitionFactorsState { - fn skip(&self, id: &HierarchicalDeterministicFactorInstance, simulated: bool) { - self.neglect( - &NeglectedFactorInstance::new( - NeglectFactorReason::UserExplicitlySkipped, - id.clone(), - ), - simulated, - ) + fn test_neglect(&self, id: &HierarchicalDeterministicFactorInstance, simulated: bool) { + self.neglect(&NeglectedFactorInstance::new( + if simulated { + NeglectFactorReason::Simulation + } else { + NeglectFactorReason::UserExplicitlySkipped + }, + id.clone(), + )) } } @@ -109,8 +110,8 @@ mod tests { fn skipping_twice_panics() { let sut = Sut::new(); let fi = HierarchicalDeterministicFactorInstance::sample(); - sut.skip(&fi, false); - sut.skip(&fi, false); + sut.test_neglect(&fi, false); + sut.test_neglect(&fi, false); } #[test] @@ -141,7 +142,7 @@ mod tests { sut.add_signature(&signature); - sut.skip(&factor_instance, false); + sut.test_neglect(&factor_instance, false); } #[test] @@ -155,7 +156,7 @@ mod tests { FactorSourceIDFromHash::fs0(), ); - sut.skip(&factor_instance, false); + sut.test_neglect(&factor_instance, false); let sign_input = HDSignatureInput::new( intent_hash, diff --git a/src/signing/petition_types/petition_of_transaction.rs b/src/signing/petition_types/petition_of_transaction.rs index 5a0b839a..4749f1db 100644 --- a/src/signing/petition_types/petition_of_transaction.rs +++ b/src/signing/petition_types/petition_of_transaction.rs @@ -71,13 +71,30 @@ impl PetitionTransaction { .collect() } - pub fn all_factor_instances_of_source( + pub fn has_tx_failed(&self) -> bool { + self.for_entities.borrow().values().any(|p| p.has_failed()) + } + + pub fn all_relevant_factor_instances_of_source( &self, factor_source_id: &FactorSourceIDFromHash, ) -> IndexSet { - self._all_factor_instances() - .into_iter() - .filter(|f| f.factor_instance().factor_source_id == *factor_source_id) + assert!(!self.has_tx_failed()); + self.for_entities + .borrow() + .values() + .filter(|&p| { + if p.has_failed() { + debug!("OMITTING petition since it HAS failed: {:?}", p); + false + } else { + debug!("INCLUDING petition since it has NOT failed: {:?}", p); + true + } + }) + .cloned() + .flat_map(|petition| petition.all_factor_instances()) + .filter(|f| f.factor_source_id() == *factor_source_id) .collect() } @@ -89,10 +106,10 @@ impl PetitionTransaction { for_entity.add_signature(signature.clone()); } - pub fn neglected_factor_source(&self, neglected: NeglectedFactor) { + pub fn neglect_factor_source(&self, neglected: NeglectedFactor) { let mut for_entities = self.for_entities.borrow_mut(); for petition in for_entities.values_mut() { - petition.neglected_factor_source_if_relevant(neglected.clone()) + petition.neglect_if_referenced(neglected.clone()).unwrap() } } @@ -100,10 +117,13 @@ impl PetitionTransaction { &self, factor_source_id: &FactorSourceIDFromHash, ) -> BatchKeySigningRequest { + assert!(!self + .should_neglect_factors_due_to_irrelevant(IndexSet::from_iter([*factor_source_id]))); + assert!(!self.has_tx_failed()); BatchKeySigningRequest::new( self.intent_hash.clone(), *factor_source_id, - self.all_factor_instances_of_source(factor_source_id), + self.all_relevant_factor_instances_of_source(factor_source_id), ) } @@ -111,6 +131,10 @@ impl PetitionTransaction { &self, factor_source_ids: IndexSet, ) -> IndexSet { + if self.has_tx_failed() { + // No need to display already failed tx. + return IndexSet::new(); + } self.for_entities .borrow() .iter() @@ -120,6 +144,20 @@ impl PetitionTransaction { .collect() } + pub(crate) fn should_neglect_factors_due_to_irrelevant( + &self, + factor_source_ids: IndexSet, + ) -> bool { + self.for_entities + .borrow() + .values() + .filter(|&p| p.references_any_factor_source(&factor_source_ids)) + .cloned() + .all(|petition| { + petition.should_neglect_factors_due_to_irrelevant(factor_source_ids.clone()) + }) + } + #[allow(unused)] fn debug_str(&self) -> String { let entities = self diff --git a/src/signing/petition_types/petitions.rs b/src/signing/petition_types/petitions.rs index c8196fb1..adb4dc62 100644 --- a/src/signing/petition_types/petitions.rs +++ b/src/signing/petition_types/petitions.rs @@ -135,6 +135,38 @@ impl Petitions { .collect::>() } + pub(crate) fn should_neglect_factors_due_to_irrelevant( + &self, + factor_sources_of_kind: &FactorSourcesOfKind, + ) -> bool { + let factor_source_ids = factor_sources_of_kind + .factor_sources() + .iter() + .map(|f| f.factor_source_id()) + .collect::>(); + factor_sources_of_kind + .factor_sources() + .iter() + .map(|f| f.factor_source_id()) + .all(|f| { + self.factor_source_to_intent_hash + .get(&f) + .unwrap() + .iter() + .all(|intent_hash| { + let binding = self.txid_to_petition.borrow(); + let value = binding.get(intent_hash).unwrap(); + value.should_neglect_factors_due_to_irrelevant(factor_source_ids.clone()) + }) + }) + } + + /// # Panics + /// Panics if no petition deem usage of `FactorSource` with id + /// `factor_source_id` relevant. We SHOULD have checked this already with + /// `should_neglect_factors_due_to_irrelevant` from SignatureCollector main + /// loop, i.e. we should not have called this method from SignaturesCollector + /// if `should_neglect_factors_due_to_irrelevant` returned true. pub(crate) fn input_for_interactor( &self, factor_source_id: &FactorSourceIDFromHash, @@ -143,12 +175,17 @@ impl Petitions { .factor_source_to_intent_hash .get(factor_source_id) .unwrap(); + let per_transaction = intent_hashes .into_iter() - .map(|intent_hash| { + .filter_map(|intent_hash| { let binding = self.txid_to_petition.borrow(); let petition = binding.get(intent_hash).unwrap(); - petition.input_for_interactor(factor_source_id) + if petition.has_tx_failed() { + None + } else { + Some(petition.input_for_interactor(factor_source_id)) + } }) .collect::>(); @@ -161,7 +198,7 @@ impl Petitions { petition.add_signature(signature.clone()) } - fn neglected_factor_source_with_id(&self, neglected: NeglectedFactor) { + fn neglect_factor_source_with_id(&self, neglected: NeglectedFactor) { let binding = self.txid_to_petition.borrow(); let intent_hashes = self .factor_source_to_intent_hash @@ -169,7 +206,7 @@ impl Petitions { .unwrap(); intent_hashes.into_iter().for_each(|intent_hash| { let petition = binding.get(intent_hash).unwrap(); - petition.neglected_factor_source(neglected.clone()) + petition.neglect_factor_source(neglected.clone()) }); } @@ -191,7 +228,7 @@ impl Petitions { let reason = neglected_factors.reason; for neglected_factor_source_id in neglected_factors.content.iter() { info!("Neglected {}", neglected_factor_source_id); - self.neglected_factor_source_with_id(NeglectedFactor::new( + self.neglect_factor_source_with_id(NeglectedFactor::new( reason, *neglected_factor_source_id, )) diff --git a/src/signing/signatures_outcome_types/sign_with_factors_outcome.rs b/src/signing/signatures_outcome_types/sign_with_factors_outcome.rs index 776f0443..94b3855d 100644 --- a/src/signing/signatures_outcome_types/sign_with_factors_outcome.rs +++ b/src/signing/signatures_outcome_types/sign_with_factors_outcome.rs @@ -10,7 +10,7 @@ pub enum SignWithFactorsOutcome { }, /// The factor source got neglected, either due to user explicitly skipping - /// or due to failire + /// or due to failure #[debug("Neglected")] Neglected(NeglectedFactors), } @@ -36,4 +36,15 @@ impl SignWithFactorsOutcome { pub fn user_skipped_factor(id: FactorSourceIDFromHash) -> Self { Self::user_skipped_factors(IndexSet::from_iter([id])) } + + pub fn irrelevant(factor_sources_of_kind: &FactorSourcesOfKind) -> Self { + Self::Neglected(NeglectedFactors::new( + NeglectFactorReason::Irrelevant, + factor_sources_of_kind + .factor_sources() + .into_iter() + .map(|f| f.factor_source_id()) + .collect(), + )) + } } diff --git a/src/signing/signatures_outcome_types/signatures_outcome.rs b/src/signing/signatures_outcome_types/signatures_outcome.rs index fa0acac7..550f7f9b 100644 --- a/src/signing/signatures_outcome_types/signatures_outcome.rs +++ b/src/signing/signatures_outcome_types/signatures_outcome.rs @@ -112,6 +112,12 @@ impl SignaturesOutcome { self.ids_of_neglected_factor_sources_filter(|nf| nf.reason == NeglectFactorReason::Failure) } + pub fn ids_of_neglected_factor_sources_irrelevant(&self) -> IndexSet { + self.ids_of_neglected_factor_sources_filter(|nf| { + nf.reason == NeglectFactorReason::Irrelevant + }) + } + pub fn signatures_of_failed_transactions(&self) -> IndexSet { self.failed_transactions.all_signatures() } diff --git a/src/testing/signing/simulated_user.rs b/src/testing/signing/simulated_user.rs index 4fca4617..59841a02 100644 --- a/src/testing/signing/simulated_user.rs +++ b/src/testing/signing/simulated_user.rs @@ -1,13 +1,4 @@ -use std::{ - borrow::BorrowMut, - cell::{Cell, RefCell}, - collections::HashMap, - sync::{Arc, Mutex, RwLock}, -}; - -use indexmap::IndexSet; - -use crate::FactorSourceIDFromHash; +use crate::prelude::*; #[derive(Clone, Debug, PartialEq, Eq)] pub enum SigningUserInput { @@ -15,20 +6,30 @@ pub enum SigningUserInput { Skip, } -#[derive(Debug, Clone)] +#[derive(Clone, derive_more::Debug)] +#[debug("SimulatedUser(mode: {mode:?}, failures: {failures:?})")] pub struct SimulatedUser { + spy_on_request: Arc)>, mode: SimulatedUserMode, /// `None` means never failures failures: Option, } impl SimulatedUser { - pub fn new(mode: SimulatedUserMode, failures: impl Into>) -> Self { + pub fn with_spy( + spy_on_request: impl Fn(FactorSourceKind, IndexSet) + 'static, + mode: SimulatedUserMode, + failures: impl Into>, + ) -> Self { Self { + spy_on_request: Arc::new(spy_on_request), mode, failures: failures.into(), } } + pub fn new(mode: SimulatedUserMode, failures: impl Into>) -> Self { + Self::with_spy(|_, _| {}, mode, failures) + } } #[derive(Debug, Clone, Default)] @@ -106,6 +107,7 @@ impl SimulatedUser { } unsafe impl Sync for SimulatedUser {} +unsafe impl Send for SimulatedUser {} /// A very lazy user that defers all boring work such as signing stuff for as long /// as possible. Ironically, this sometimes leads to user signing more than she @@ -120,9 +122,17 @@ pub enum Laziness { } impl SimulatedUser { + pub fn spy_on_request_before_handled( + &self, + factor_source_kind: FactorSourceKind, + invalid_tx_if_skipped: IndexSet, + ) { + (self.spy_on_request)(factor_source_kind, invalid_tx_if_skipped.clone()); + } + pub fn sign_or_skip( &self, - invalid_tx_if_skipped: impl IntoIterator, + invalid_tx_if_skipped: impl IntoIterator, ) -> SigningUserInput { let invalid_tx_if_skipped = invalid_tx_if_skipped .into_iter() diff --git a/src/testing/signing/test_data.rs b/src/testing/signing/test_data.rs index b4829c9f..e6682c7b 100644 --- a/src/testing/signing/test_data.rs +++ b/src/testing/signing/test_data.rs @@ -219,6 +219,14 @@ impl MatrixOfFactorInstances { 5, ) } + /// Securified { Threshold 1/1 and Override factors #1 } + pub fn m8(fi: F) -> Self + where + F: Fn(FactorSourceIDFromHash) -> HierarchicalDeterministicFactorInstance, + { + type F = FactorSourceIDFromHash; + Self::new([F::fs1()].map(&fi), 1, [F::fs8()].map(&fi)) + } } impl Account { @@ -296,6 +304,16 @@ impl Account { pub fn a8() -> Self { Self::unsecurified_mainnet(8, "Jenny", FactorSourceIDFromHash::fs10()) } + + /// Klara | 9 | Securified { Threshold 1/1 and Override factors #1 } + pub fn a9() -> Self { + Self::securified_mainnet(9, "Klara", |idx| { + MatrixOfFactorInstances::m8(HierarchicalDeterministicFactorInstance::f( + Self::entity_kind(), + idx, + )) + }) + } } impl Persona { diff --git a/src/testing/signing/test_interactors/test_parallel_interactor.rs b/src/testing/signing/test_interactors/test_parallel_interactor.rs index 2f551333..30654383 100644 --- a/src/testing/signing/test_interactors/test_parallel_interactor.rs +++ b/src/testing/signing/test_interactors/test_parallel_interactor.rs @@ -20,6 +20,10 @@ impl IsTestInteractor for TestSigningParallelInteractor { #[async_trait::async_trait] impl SignWithFactorParallelInteractor for TestSigningParallelInteractor { async fn sign(&self, request: ParallelBatchSigningRequest) -> SignWithFactorsOutcome { + self.simulated_user.spy_on_request_before_handled( + request.clone().factor_source_kind(), + request.clone().invalid_transactions_if_neglected, + ); let ids = request .per_factor_source .keys() diff --git a/src/testing/signing/test_interactors/test_serial_interactor.rs b/src/testing/signing/test_interactors/test_serial_interactor.rs index 5a74a7ac..ac2c7e85 100644 --- a/src/testing/signing/test_interactors/test_serial_interactor.rs +++ b/src/testing/signing/test_interactors/test_serial_interactor.rs @@ -20,11 +20,16 @@ impl IsTestInteractor for TestSigningSerialInteractor { #[async_trait::async_trait] impl SignWithFactorSerialInteractor for TestSigningSerialInteractor { async fn sign(&self, request: SerialBatchSigningRequest) -> SignWithFactorsOutcome { - let ids = IndexSet::from_iter([request.input.factor_source_id]); + self.simulated_user.spy_on_request_before_handled( + request.clone().factor_source_kind(), + request.clone().invalid_transactions_if_neglected, + ); + let ids = IndexSet::from_iter([request.clone().input.factor_source_id]); if self.should_simulate_failure(ids.clone()) { return SignWithFactorsOutcome::failure_with_factors(ids); } - let invalid_transactions_if_neglected = request.invalid_transactions_if_neglected; + let invalid_transactions_if_neglected = request.clone().invalid_transactions_if_neglected; + match self .simulated_user .sign_or_skip(invalid_transactions_if_neglected) diff --git a/src/types/invalid_transaction_if_neglected.rs b/src/types/invalid_transaction_if_neglected.rs index 89628a18..6881ca29 100644 --- a/src/types/invalid_transaction_if_neglected.rs +++ b/src/types/invalid_transaction_if_neglected.rs @@ -1,7 +1,7 @@ use crate::prelude::*; /// A list of entities which would fail in a transaction if we would -/// neglect certain factor source, either by user explictlty skipping +/// neglect certain factor source, either by user explicitly skipping /// it or if implicitly neglected due to failure. #[derive(Clone, Debug, PartialEq, Eq, std::hash::Hash)] pub struct InvalidTransactionIfNeglected { diff --git a/src/types/owned_types/owned_factor_instance.rs b/src/types/owned_types/owned_factor_instance.rs index 638f3341..7589e46a 100644 --- a/src/types/owned_types/owned_factor_instance.rs +++ b/src/types/owned_types/owned_factor_instance.rs @@ -19,11 +19,15 @@ impl OwnedFactorInstance { &self.value } + pub fn factor_source_id(&self) -> FactorSourceIDFromHash { + self.factor_instance().factor_source_id() + } + /// Checks if this `OwnedFactorInstance` was created from the factor source /// with id `factor_source_id`. pub fn by_factor_source(&self, factor_source_id: impl Borrow) -> bool { let factor_source_id = factor_source_id.borrow(); - self.factor_instance().factor_source_id == *factor_source_id + self.factor_source_id() == *factor_source_id } } diff --git a/tests/main.rs b/tests/main.rs index 39be90f1..261182c8 100644 --- a/tests/main.rs +++ b/tests/main.rs @@ -713,6 +713,8 @@ mod signing_tests { } mod with_failure { + use std::rc::Rc; + use super::*; #[actix_rt::test] @@ -737,8 +739,52 @@ mod signing_tests { } #[actix_rt::test] - async fn many_failing_tx() { + async fn failed_threshold_successful_override() { sensible_env_logger::safe_init!(); + let factor_sources = &HDFactorSource::all(); + let a9 = &Account::a9(); + let tx0 = TransactionIntent::address_of([a9], []); + + let all_transactions = IndexSet::from_iter([tx0.clone()]); + + let profile = Profile::new(factor_sources.clone(), [a9], []); + + let collector = SignaturesCollector::new( + SigningFinishEarlyStrategy::default(), + all_transactions, + Arc::new(TestSignatureCollectingInteractors::new( + SimulatedUser::prudent_with_failures( + SimulatedFailures::with_simulated_failures([ + FactorSourceIDFromHash::fs1(), + ]), + ), + )), + &profile, + ) + .unwrap(); + + let outcome = collector.collect_signatures().await; + assert!(outcome.successful()); + assert_eq!( + outcome + .successful_transactions() + .into_iter() + .map(|t| t.intent_hash.clone()) + .collect_vec(), + vec![tx0.clone().intent_hash] + ); + assert_eq!( + outcome + .all_signatures() + .into_iter() + .map(|s| s.factor_source_id()) + .collect_vec(), + vec![FactorSourceIDFromHash::fs8()] + ); + } + + #[actix_rt::test] + async fn many_failing_tx() { let factor_sources = &HDFactorSource::all(); let a0 = &Account::a0(); let p3 = &Persona::p3(); @@ -800,6 +846,122 @@ mod signing_tests { vec![tx.intent_hash] ) } + + #[actix_rt::test] + async fn same_tx_is_not_shown_to_user_in_case_of_already_failure() { + sensible_env_logger::safe_init!(); + let factor_sources = HDFactorSource::all(); + + let a7 = Account::a7(); + let a0 = Account::a0(); + + let tx0 = TransactionIntent::new([a7.entity_address(), a0.entity_address()], []); + let tx1 = TransactionIntent::new([a0.entity_address()], []); + + let profile = Profile::new(factor_sources.clone(), [&a7, &a0], []); + + type Tuple = (FactorSourceKind, IndexSet); + type Tuples = Vec; + let tuples = Rc::>::new(RefCell::new(Tuples::default())); + let tuples_clone = tuples.clone(); + let collector = SignaturesCollector::new( + SigningFinishEarlyStrategy::default(), + IndexSet::from_iter([tx0.clone(), tx1.clone()]), + Arc::new(TestSignatureCollectingInteractors::new( + SimulatedUser::with_spy( + move |kind, invalid| { + let tuple = (kind, invalid); + let mut x = RefCell::borrow_mut(&tuples_clone); + x.push(tuple) + }, + SimulatedUserMode::Prudent, + SimulatedFailures::with_simulated_failures([ + FactorSourceIDFromHash::fs2(), // will cause any TX with a7 to fail + ]), + ), + )), + &profile, + ) + .unwrap(); + + let outcome = collector.collect_signatures().await; + + let tuples = tuples.borrow().clone(); + assert_eq!( + tuples, + vec![ + ( + FactorSourceKind::Ledger, + IndexSet::from_iter([InvalidTransactionIfNeglected::new( + tx0.clone().intent_hash, + [a7.address()] + )]) + ), + // Important that we do NOT display any mentioning of `tx0` here again! + ( + FactorSourceKind::Device, + IndexSet::from_iter([InvalidTransactionIfNeglected::new( + tx1.clone().intent_hash, + [a0.address()] + )]) + ), + ] + ); + + assert!(!outcome.successful()); + assert_eq!( + outcome.ids_of_neglected_factor_sources_failed(), + IndexSet::::from_iter([FactorSourceIDFromHash::fs2()]) + ); + assert_eq!( + outcome.ids_of_neglected_factor_sources_irrelevant(), + IndexSet::::from_iter([ + FactorSourceIDFromHash::fs6(), + FactorSourceIDFromHash::fs7(), + FactorSourceIDFromHash::fs8(), + FactorSourceIDFromHash::fs9() + ]) + ); + assert_eq!( + outcome + .successful_transactions() + .into_iter() + .map(|t| t.intent_hash) + .collect_vec(), + vec![tx1.intent_hash.clone()] + ); + + assert_eq!( + outcome + .failed_transactions() + .into_iter() + .map(|t| t.intent_hash) + .collect_vec(), + vec![tx0.intent_hash.clone()] + ); + + assert_eq!(outcome.all_signatures().len(), 1); + + assert!(outcome + .all_signatures() + .into_iter() + .map(|s| s.intent_hash().clone()) + .all(|i| i == tx1.intent_hash)); + + assert_eq!( + outcome + .all_signatures() + .into_iter() + .map(|s| s.derivation_path()) + .collect_vec(), + vec![DerivationPath::new( + NetworkID::Mainnet, + CAP26EntityKind::Account, + CAP26KeyKind::T9n, + HDPathComponent::non_hardened(0) + )] + ) + } } mod no_fail {