diff --git a/README.md b/README.md index a5c4fef4..d921875e 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ > [!IMPORTANT] -> This TEMPRORARY repo is intended for **internal use only** and will later be merged into [Sargon]https://github.com/radixdlt/sargon. +> This TEMPORARY repo is intended for **internal use only** and will later be merged into [Sargon]https://github.com/radixdlt/sargon. > Interfaces will be changing regularly, and we do not recommend other developers integrate the library or align with these standards. MFA work. \ No newline at end of file diff --git a/src/lib.rs b/src/lib.rs index b0becfc2..d384f0f8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,6 +1,7 @@ #![allow(internal_features)] #![feature(core_intrinsics)] #![feature(iter_repeat_n)] +#![feature(async_closure)] mod derivation; mod signing; diff --git a/src/signing/collector/mod.rs b/src/signing/collector/mod.rs index 2a6424ac..a56fa356 100644 --- a/src/signing/collector/mod.rs +++ b/src/signing/collector/mod.rs @@ -1,7 +1,11 @@ +mod signatures_collecting_continuation; mod signatures_collector; mod signatures_collector_dependencies; mod signatures_collector_preprocessor; mod signatures_collector_state; +mod signing_finish_early_strategy; +pub use signatures_collecting_continuation::*; pub use signatures_collector::*; pub use signatures_collector_preprocessor::*; +pub use signing_finish_early_strategy::*; diff --git a/src/signing/collector/signatures_collecting_continuation.rs b/src/signing/collector/signatures_collecting_continuation.rs new file mode 100644 index 00000000..45d6351b --- /dev/null +++ b/src/signing/collector/signatures_collecting_continuation.rs @@ -0,0 +1,14 @@ +/// Whether to continue collecting signatures or finish early. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum SignaturesCollectingContinuation { + /// It is meaningless to continue collecting signatures, either since either + /// all transactions are valid, and the collector is configured to finish early + /// in that case, or some transaction is invalid and the collector is configured + /// finish early in that case. + FinishEarly, + + /// We should continue collecting signatures, either since the collector is + /// configured to not finish early, even though we can, or since we cannot + /// finish early since not enough factor sources have been signed with. + Continue, +} diff --git a/src/signing/collector/signatures_collector.rs b/src/signing/collector/signatures_collector.rs index 39d2220b..2181bbf4 100644 --- a/src/signing/collector/signatures_collector.rs +++ b/src/signing/collector/signatures_collector.rs @@ -27,7 +27,7 @@ impl SignaturesCollector { /// Used by our tests. But Sargon will typically wanna use `SignaturesCollector::new` and passing /// it a pub(crate) fn with( - finish_early_when_all_transactions_are_valid: bool, + finish_early_strategy: SigningFinishEarlyStrategy, all_factor_sources_in_profile: IndexSet, transactions: IndexSet, interactors: Arc, @@ -36,11 +36,8 @@ impl SignaturesCollector { let preprocessor = SignaturesCollectorPreprocessor::new(transactions); let (petitions, factors) = preprocessor.preprocess(all_factor_sources_in_profile); - let dependencies = SignaturesCollectorDependencies::new( - finish_early_when_all_transactions_are_valid, - interactors, - factors, - ); + let dependencies = + SignaturesCollectorDependencies::new(finish_early_strategy, interactors, factors); let state = SignaturesCollectorState::new(petitions); Self { @@ -50,7 +47,7 @@ impl SignaturesCollector { } pub fn with_signers_extraction( - finish_early_when_all_transactions_are_valid: bool, + finish_early_strategy: SigningFinishEarlyStrategy, all_factor_sources_in_profile: IndexSet, transactions: IndexSet, interactors: Arc, @@ -65,7 +62,7 @@ impl SignaturesCollector { .collect::>>()?; let collector = Self::with( - finish_early_when_all_transactions_are_valid, + finish_early_strategy, all_factor_sources_in_profile, transactions, interactors, @@ -75,13 +72,13 @@ impl SignaturesCollector { } pub fn new( - finish_early_when_all_transactions_are_valid: bool, + finish_early_strategy: SigningFinishEarlyStrategy, transactions: IndexSet, interactors: Arc, profile: &Profile, ) -> Result { Self::with_signers_extraction( - finish_early_when_all_transactions_are_valid, + finish_early_strategy, profile.factor_sources.clone(), transactions, interactors, @@ -102,18 +99,49 @@ impl SignaturesCollector { } } +use SignaturesCollectingContinuation::*; + // === PRIVATE === impl SignaturesCollector { - /// If all transactions already would fail, or if all transactions already are done, then - /// no point in continuing. + /// Returning `Continue` means that we should continue collecting signatures. /// - /// `Ok(true)` means "continue", `Ok(false)` means "stop, we are done". `Err(_)` means "stop, we have failed". - fn continue_if_necessary(&self) -> Result { - self.state - .borrow() - .petitions - .borrow() - .continue_if_necessary() + /// Returning `FinishEarly` if it is meaningless to continue collecting signatures, + /// either since all transactions are valid and this collector is configured + /// to finish early in that case, or if some transaction is invalid and this + /// collector is configured to finish early in that case. + /// + /// N.B. this method does not concern itself with how many or which + /// factor sources are left to sign with, that is handled by the main loop, + /// i.e. this might return `false` even though there is not factor sources + /// left to sign with. + fn continuation(&self) -> SignaturesCollectingContinuation { + let finish_early_strategy = self.dependencies.finish_early_strategy; + let when_all_transactions_are_valid = + finish_early_strategy.when_all_transactions_are_valid.0; + let when_some_transaction_is_invalid = + finish_early_strategy.when_some_transaction_is_invalid.0; + + let petitions_status = self.state.borrow().petitions.borrow().status(); + + if petitions_status.are_all_valid() { + if when_all_transactions_are_valid == FinishEarly { + info!("All valid && should finish early => finish early"); + return FinishEarly; + } else { + debug!( + "All valid, BUT the collector is configured to NOT finish early => Continue" + ); + } + } else if petitions_status.is_some_invalid() { + if when_some_transaction_is_invalid == FinishEarly { + info!("Some invalid && should finish early => finish early"); + return FinishEarly; + } else { + debug!("Some transactions invalid, BUT the collector is configured to NOT finish early in case of failures => Continue"); + } + } + + Continue } async fn use_factor_sources(&self, factor_sources_of_kind: &FactorSourcesOfKind) -> Result<()> { @@ -172,7 +200,7 @@ impl SignaturesCollector { // Report the results back to the collector self.process_batch_response(response); - if !self.continue_if_necessary()? { + if self.continuation() == FinishEarly { break; } } @@ -195,13 +223,8 @@ impl SignaturesCollector { 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() { - if self - .dependencies - .finish_early_when_all_transactions_are_valid - && !self.continue_if_necessary()? - { - info!("Finished early"); - break; // finished early, we have fulfilled signing requirements of all transactions + if self.continuation() == FinishEarly { + break; } info!( "Use(?) #{:?} factors of kind: {:?}", @@ -315,7 +338,7 @@ mod tests { #[test] fn invalid_profile_unknown_account() { let res = SignaturesCollector::new( - true, + SigningFinishEarlyStrategy::default(), IndexSet::from_iter([TransactionIntent::new([Account::a0().entity_address()], [])]), Arc::new(TestSignatureCollectingInteractors::new( SimulatedUser::prudent_no_fail(), @@ -328,7 +351,7 @@ mod tests { #[test] fn invalid_profile_unknown_persona() { let res = SignaturesCollector::new( - true, + SigningFinishEarlyStrategy::default(), IndexSet::from_iter([TransactionIntent::new([], [Persona::p0().entity_address()])]), Arc::new(TestSignatureCollectingInteractors::new( SimulatedUser::prudent_no_fail(), @@ -343,7 +366,7 @@ mod tests { let factors_sources = HDFactorSource::all(); let persona = Persona::p0(); let collector = SignaturesCollector::new( - true, + SigningFinishEarlyStrategy::default(), IndexSet::from_iter([TransactionIntent::new([], [persona.entity_address()])]), Arc::new(TestSignatureCollectingInteractors::new( SimulatedUser::prudent_no_fail(), @@ -355,6 +378,75 @@ mod tests { assert!(outcome.successful()) } + #[actix_rt::test] + async fn continues_even_with_failed_tx_when_configured_to() { + let factor_sources = &HDFactorSource::all(); + let a0 = &Account::a0(); + let a1 = &Account::a1(); + + let t0 = TransactionIntent::address_of([a1], []); + let t1 = TransactionIntent::address_of([a0], []); + + let profile = Profile::new(factor_sources.clone(), [a0, a1], []); + + let collector = SignaturesCollector::new( + SigningFinishEarlyStrategy::new( + WhenAllTransactionsAreValid(FinishEarly), + WhenSomeTransactionIsInvalid(Continue), + ), + IndexSet::::from_iter([t0.clone(), t1.clone()]), + 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.failed_transactions().len(), 1); + assert_eq!(outcome.successful_transactions().len(), 1); + } + + #[actix_rt::test] + async fn continues_even_when_all_valid_if_configured_to() { + sensible_env_logger::safe_init!(); + let test = async move |when_all_valid: WhenAllTransactionsAreValid, + expected_sig_count: usize| { + let factor_sources = &HDFactorSource::all(); + let a5 = &Account::a5(); + + let t0 = TransactionIntent::address_of([a5], []); + + let profile = Profile::new(factor_sources.clone(), [a5], []); + + let collector = SignaturesCollector::new( + SigningFinishEarlyStrategy::new( + when_all_valid, + WhenSomeTransactionIsInvalid::default(), + ), + IndexSet::::from_iter([t0.clone()]), + Arc::new(TestSignatureCollectingInteractors::new( + SimulatedUser::prudent_no_fail(), + )), + &profile, + ) + .unwrap(); + + let outcome = collector.collect_signatures().await; + assert!(outcome.successful()); + assert_eq!( + outcome.signatures_of_successful_transactions().len(), + expected_sig_count + ); + }; + + test(WhenAllTransactionsAreValid(FinishEarly), 1).await; + test(WhenAllTransactionsAreValid(Continue), 2).await; + } + #[test] fn test_profile() { let factor_sources = &HDFactorSource::all(); @@ -376,7 +468,7 @@ mod tests { let profile = Profile::new(factor_sources.clone(), [a0, a1, a2, a6], [p0, p1, p2, p6]); let collector = SignaturesCollector::new( - true, + SigningFinishEarlyStrategy::default(), IndexSet::::from_iter([ t0.clone(), t1.clone(), diff --git a/src/signing/collector/signatures_collector_dependencies.rs b/src/signing/collector/signatures_collector_dependencies.rs index 0ab0b692..c5a8c556 100644 --- a/src/signing/collector/signatures_collector_dependencies.rs +++ b/src/signing/collector/signatures_collector_dependencies.rs @@ -4,7 +4,7 @@ pub(super) struct SignaturesCollectorDependencies { /// If `true` we stop collecting signatures as soon as all transactions are /// valid. This is typically always set to `true`, but can be useful for /// tests to set to `false` to see how the system behaves. - pub(super) finish_early_when_all_transactions_are_valid: bool, + pub(super) finish_early_strategy: SigningFinishEarlyStrategy, /// A collection of "interactors" used to sign with factor sources. pub(super) interactors: Arc, @@ -23,12 +23,12 @@ pub(super) struct SignaturesCollectorDependencies { impl SignaturesCollectorDependencies { pub fn new( - finish_early_when_all_transactions_are_valid: bool, + finish_early_strategy: SigningFinishEarlyStrategy, interactors: Arc, factors_of_kind: IndexSet, ) -> Self { Self { - finish_early_when_all_transactions_are_valid, + finish_early_strategy, interactors, factors_of_kind, } diff --git a/src/signing/collector/signing_finish_early_strategy.rs b/src/signing/collector/signing_finish_early_strategy.rs new file mode 100644 index 00000000..da8be37e --- /dev/null +++ b/src/signing/collector/signing_finish_early_strategy.rs @@ -0,0 +1,37 @@ +use crate::prelude::*; + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct WhenAllTransactionsAreValid(pub SignaturesCollectingContinuation); + +impl Default for WhenAllTransactionsAreValid { + fn default() -> Self { + Self(SignaturesCollectingContinuation::FinishEarly) + } +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct WhenSomeTransactionIsInvalid(pub SignaturesCollectingContinuation); + +impl Default for WhenSomeTransactionIsInvalid { + fn default() -> Self { + Self(SignaturesCollectingContinuation::FinishEarly) + } +} + +/// Strategy to use for finishing early, i.e. stop collecting more signatures +#[derive(Clone, Default, Copy, Debug, PartialEq, Eq)] +pub struct SigningFinishEarlyStrategy { + pub when_all_transactions_are_valid: WhenAllTransactionsAreValid, + pub when_some_transaction_is_invalid: WhenSomeTransactionIsInvalid, +} +impl SigningFinishEarlyStrategy { + pub fn new( + when_all_transactions_are_valid: WhenAllTransactionsAreValid, + when_some_transaction_is_invalid: WhenSomeTransactionIsInvalid, + ) -> Self { + Self { + when_all_transactions_are_valid, + when_some_transaction_is_invalid, + } + } +} diff --git a/src/signing/petition_types/petition_entity.rs b/src/signing/petition_types/petition_entity.rs index 9dcd5ffe..e99d7ba2 100644 --- a/src/signing/petition_types/petition_entity.rs +++ b/src/signing/petition_types/petition_entity.rs @@ -183,17 +183,6 @@ impl PetitionEntity { } } - /// `Ok(true)` means "continue", `Ok(false)` means "stop, we are done". `Err(_)` means "stop, we have failed". - pub(super) fn continue_if_necessary(&self) -> Result { - match self.status() { - PetitionFactorsStatus::InProgress => Ok(true), - PetitionFactorsStatus::Finished(PetitionFactorsStatusFinished::Fail) => { - Err(CommonError::Failure) - } - PetitionFactorsStatus::Finished(PetitionFactorsStatusFinished::Success) => Ok(false), - } - } - pub fn status_if_skipped_factors( &self, factor_source_ids: IndexSet, diff --git a/src/signing/petition_types/petitions.rs b/src/signing/petition_types/petitions.rs index 59009aed..e1184f65 100644 --- a/src/signing/petition_types/petitions.rs +++ b/src/signing/petition_types/petitions.rs @@ -23,6 +23,25 @@ pub(crate) struct Petitions { pub txid_to_petition: RefCell>, } +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum PetitionsStatus { + InProgressNoneInvalid, + AllAreValid, + SomeIsInvalid, +} +impl PetitionsStatus { + // pub fn are_all_done(&self) -> bool { + // matches!(self, Self::Done { .. }) + // } + pub fn are_all_valid(&self) -> bool { + matches!(self, Self::AllAreValid) + } + + pub fn is_some_invalid(&self) -> bool { + matches!(self, Self::SomeIsInvalid) + } +} + impl Petitions { pub(crate) fn new( factor_to_txid: HashMap>, @@ -56,9 +75,8 @@ impl Petitions { ) } - /// `Ok(true)` means "continue", `Ok(false)` means "stop, we are done". `Err(_)` means "stop, we have failed". - pub fn continue_if_necessary(&self) -> Result { - let should_continue_signals = self + pub fn status(&self) -> PetitionsStatus { + let statuses = self .txid_to_petition .borrow() .iter() @@ -67,27 +85,31 @@ impl Petitions { .for_entities .borrow() .iter() - .map(|(_, petition)| { - let s = petition.status(); - if matches!( - s, - PetitionFactorsStatus::Finished(PetitionFactorsStatusFinished::Fail) - ) { - warn!( - "Fail, entity: {} in tx: {:?}", - petition.entity, petition.intent_hash - ) - }; - petition.continue_if_necessary() - }) + .map(|(_, petition)| petition.status()) .collect_vec() }) - .collect::>>()?; + .collect::>(); - // If **any** petition says we should continue, we must continue. - let should_continue_signal = should_continue_signals.into_iter().any(|b| b); + let are_all_valid = statuses.iter().all(|s| { + matches!( + s, + PetitionFactorsStatus::Finished(PetitionFactorsStatusFinished::Success) + ) + }); + if are_all_valid { + return PetitionsStatus::AllAreValid; + } - Ok(should_continue_signal) + let is_some_invalid = statuses.iter().any(|s| { + matches!( + s, + PetitionFactorsStatus::Finished(PetitionFactorsStatusFinished::Fail) + ) + }); + if is_some_invalid { + return PetitionsStatus::SomeIsInvalid; + } + PetitionsStatus::InProgressNoneInvalid } pub fn invalid_transactions_if_skipped_factors( diff --git a/src/testing/signing/test_signatures_collector.rs b/src/testing/signing/test_signatures_collector.rs index 6199444d..78116ab1 100644 --- a/src/testing/signing/test_signatures_collector.rs +++ b/src/testing/signing/test_signatures_collector.rs @@ -4,27 +4,27 @@ impl SignaturesCollector { /// Used by our tests. But Sargon will typically wanna use `SignaturesCollector::new` and passing /// it a pub fn new_test_with( - finish_early_when_all_transactions_are_valid: bool, + finish_early_strategy: SigningFinishEarlyStrategy, all_factor_sources_in_profile: IndexSet, transactions: IndexSet, interactors: Arc, ) -> Self { sensible_env_logger::safe_init!(); Self::with( - finish_early_when_all_transactions_are_valid, + finish_early_strategy, all_factor_sources_in_profile, transactions, interactors, ) } pub fn new_test( - finish_early_when_all_transactions_are_valid: bool, + finish_early_strategy: SigningFinishEarlyStrategy, all_factor_sources_in_profile: impl IntoIterator, transactions: impl IntoIterator, simulated_user: SimulatedUser, ) -> Self { Self::new_test_with( - finish_early_when_all_transactions_are_valid, + finish_early_strategy, all_factor_sources_in_profile.into_iter().collect(), transactions.into_iter().collect(), Arc::new(TestSignatureCollectingInteractors::new(simulated_user)), @@ -32,12 +32,12 @@ impl SignaturesCollector { } pub fn test_prudent_with_factors_and_finish_early( - finish_early_when_all_transactions_are_valid: bool, + finish_early_strategy: SigningFinishEarlyStrategy, all_factor_sources_in_profile: impl IntoIterator, transactions: impl IntoIterator, ) -> Self { Self::new_test( - finish_early_when_all_transactions_are_valid, + finish_early_strategy, all_factor_sources_in_profile, transactions, SimulatedUser::prudent_no_fail(), @@ -45,18 +45,18 @@ impl SignaturesCollector { } pub fn test_prudent_with_finish_early( - finish_early_when_all_transactions_are_valid: bool, + finish_early_strategy: SigningFinishEarlyStrategy, transactions: impl IntoIterator, ) -> Self { Self::test_prudent_with_factors_and_finish_early( - finish_early_when_all_transactions_are_valid, + finish_early_strategy, HDFactorSource::all(), transactions, ) } pub fn test_prudent(transactions: impl IntoIterator) -> Self { - Self::test_prudent_with_finish_early(true, transactions) + Self::test_prudent_with_finish_early(SigningFinishEarlyStrategy::default(), transactions) } pub fn test_prudent_with_failures( @@ -64,7 +64,7 @@ impl SignaturesCollector { simulated_failures: SimulatedFailures, ) -> Self { Self::new_test( - true, + SigningFinishEarlyStrategy::default(), HDFactorSource::all(), transactions, SimulatedUser::prudent_with_failures(simulated_failures), @@ -76,7 +76,7 @@ impl SignaturesCollector { transactions: impl IntoIterator, ) -> Self { Self::new_test( - true, + SigningFinishEarlyStrategy::default(), all_factor_sources_in_profile, transactions, SimulatedUser::lazy_sign_minimum([]), @@ -94,7 +94,7 @@ impl SignaturesCollector { transactions: impl IntoIterator, ) -> Self { Self::new_test( - true, + SigningFinishEarlyStrategy::default(), all_factor_sources_in_profile, transactions, SimulatedUser::lazy_always_skip_no_fail(), diff --git a/tests/main.rs b/tests/main.rs index 5b57061a..20556628 100644 --- a/tests/main.rs +++ b/tests/main.rs @@ -548,7 +548,7 @@ mod signing_tests { let profile = Profile::new(factor_sources.clone(), [a0, a1, a2], [p0, p1, p2]); let collector = SignaturesCollector::new( - true, + SigningFinishEarlyStrategy::default(), IndexSet::::from_iter([t0.clone(), t1.clone(), t2.clone()]), Arc::new(TestSignatureCollectingInteractors::new(sim)), &profile, @@ -660,7 +660,7 @@ mod signing_tests { let profile = Profile::new(factor_sources.clone(), [a4, a5, a6], [p4, p5, p6]); let collector = SignaturesCollector::new( - true, + SigningFinishEarlyStrategy::default(), IndexSet::::from_iter([ t0.clone(), t1.clone(), @@ -751,7 +751,7 @@ mod signing_tests { let profile = Profile::new(factor_sources.clone(), [a0], [p3]); let collector = SignaturesCollector::new( - true, + SigningFinishEarlyStrategy::default(), all_transactions, Arc::new(TestSignatureCollectingInteractors::new( SimulatedUser::prudent_with_failures(