From 4366de831a8374c4222ca86af09efd9bd4d0e318 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Thu, 8 Aug 2024 13:47:38 +0530 Subject: [PATCH 1/6] GH-711: introduce resolve_current_iteration_result_2 --- node/src/accountant/payment_adjuster/mod.rs | 40 ++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/node/src/accountant/payment_adjuster/mod.rs b/node/src/accountant/payment_adjuster/mod.rs index 878bd9721..6bdb8f89c 100644 --- a/node/src/accountant/payment_adjuster/mod.rs +++ b/node/src/accountant/payment_adjuster/mod.rs @@ -34,7 +34,7 @@ use crate::accountant::payment_adjuster::logging_and_diagnostics::log_functions: use crate::accountant::payment_adjuster::miscellaneous::data_structures::DecidedAccounts::{ LowGainingAccountEliminated, SomeAccountsProcessed, }; -use crate::accountant::payment_adjuster::miscellaneous::data_structures::{AdjustedAccountBeforeFinalization, AdjustmentIterationResult, RecursionResults, WeightedPayable}; +use crate::accountant::payment_adjuster::miscellaneous::data_structures::{AdjustedAccountBeforeFinalization, AdjustmentIterationResult, DecidedAccounts, RecursionResults, WeightedPayable}; use crate::accountant::payment_adjuster::miscellaneous::helper_functions::{ dump_unaffordable_accounts_by_transaction_fee, exhaust_cw_balance_entirely, find_largest_exceeding_balance, @@ -302,6 +302,44 @@ impl PaymentAdjusterReal { merged } + fn resolve_current_iteration_result_2( + &mut self, + adjustment_iteration_result: AdjustmentIterationResult, + ) -> RecursionResults { + let remaining_undecided_accounts = adjustment_iteration_result.remaining_undecided_accounts; + let decided_accounts = adjustment_iteration_result.decided_accounts; + if remaining_undecided_accounts.is_empty() { + return match decided_accounts { + LowGainingAccountEliminated => RecursionResults::new(vec![], vec![]), + SomeAccountsProcessed(accounts) => RecursionResults::new(accounts, vec![]), + }; + } + let here_decided_accounts = match decided_accounts { + LowGainingAccountEliminated => { + vec![] + } + SomeAccountsProcessed(accounts) => { + self.adjust_remaining_unallocated_cw_balance_down(&accounts); + accounts + } + }; + + let check_sum: u128 = sum_as(&remaining_undecided_accounts, |weighted_account| { + weighted_account.balance_minor() + }); + + let unallocated_cw_balance = self.inner.unallocated_cw_service_fee_balance_minor(); + + let down_stream_decided_accounts = if check_sum <= unallocated_cw_balance { + // Fast return after a direct conversion into the expected type + convert_collection(remaining_undecided_accounts) + } else { + self.propose_possible_adjustment_recursively(remaining_undecided_accounts) + }; + + RecursionResults::new(here_decided_accounts, down_stream_decided_accounts) + } + fn resolve_current_iteration_result( &mut self, adjustment_iteration_result: AdjustmentIterationResult, From 6f0b5c64e5079cb9d30d615fc8854d7bbe80cf1b Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Wed, 14 Aug 2024 13:52:38 +0530 Subject: [PATCH 2/6] GH-711: bring the recursive call --- .../miscellaneous/data_structures.rs | 9 +++ node/src/accountant/payment_adjuster/mod.rs | 70 ++++++++++--------- 2 files changed, 47 insertions(+), 32 deletions(-) diff --git a/node/src/accountant/payment_adjuster/miscellaneous/data_structures.rs b/node/src/accountant/payment_adjuster/miscellaneous/data_structures.rs index 0f3fd705f..143d20783 100644 --- a/node/src/accountant/payment_adjuster/miscellaneous/data_structures.rs +++ b/node/src/accountant/payment_adjuster/miscellaneous/data_structures.rs @@ -64,6 +64,15 @@ pub enum DecidedAccounts { SomeAccountsProcessed(Vec), } +impl From for Vec { + fn from(decided_accounts: DecidedAccounts) -> Self { + match decided_accounts { + DecidedAccounts::LowGainingAccountEliminated => vec![], + DecidedAccounts::SomeAccountsProcessed(accounts) => accounts, + } + } +} + #[derive(Debug, PartialEq, Eq, Clone)] pub struct AdjustedAccountBeforeFinalization { pub original_account: PayableAccount, diff --git a/node/src/accountant/payment_adjuster/mod.rs b/node/src/accountant/payment_adjuster/mod.rs index 6bdb8f89c..0f94f3c8b 100644 --- a/node/src/accountant/payment_adjuster/mod.rs +++ b/node/src/accountant/payment_adjuster/mod.rs @@ -54,6 +54,7 @@ use itertools::Either; use masq_lib::logger::Logger; use std::collections::HashMap; use std::fmt::{Display, Formatter}; +use std::ops::Add; use std::time::SystemTime; use thousands::Separable; use variant_count::VariantCount; @@ -290,9 +291,29 @@ impl PaymentAdjusterReal { logger, ); - let recursion_results = self.resolve_current_iteration_result(current_iteration_result); + let remaining_undecided_accounts = current_iteration_result.remaining_undecided_accounts; + let decided_accounts: Vec = + current_iteration_result.decided_accounts.into(); + if remaining_undecided_accounts.is_empty() { + return decided_accounts; + } + + if !decided_accounts.is_empty() { + self.adjust_remaining_unallocated_cw_balance_down(&decided_accounts) + } - let merged = recursion_results.merge_results_from_recursion(); + let merged = if self.is_cw_balance_under_limit(&remaining_undecided_accounts) { + // Fast return after a direct conversion into the expected type + Self::add_decided_accounts( + convert_collection(remaining_undecided_accounts), + decided_accounts, + ) + } else { + Self::add_decided_accounts( + self.propose_possible_adjustment_recursively(remaining_undecided_accounts), + decided_accounts, + ) + }; diagnostics!( "\nFINAL SET OF ADJUSTED ACCOUNTS IN CURRENT ITERATION:", @@ -302,42 +323,27 @@ impl PaymentAdjusterReal { merged } - fn resolve_current_iteration_result_2( - &mut self, - adjustment_iteration_result: AdjustmentIterationResult, - ) -> RecursionResults { - let remaining_undecided_accounts = adjustment_iteration_result.remaining_undecided_accounts; - let decided_accounts = adjustment_iteration_result.decided_accounts; - if remaining_undecided_accounts.is_empty() { - return match decided_accounts { - LowGainingAccountEliminated => RecursionResults::new(vec![], vec![]), - SomeAccountsProcessed(accounts) => RecursionResults::new(accounts, vec![]), - }; - } - let here_decided_accounts = match decided_accounts { - LowGainingAccountEliminated => { - vec![] - } - SomeAccountsProcessed(accounts) => { - self.adjust_remaining_unallocated_cw_balance_down(&accounts); - accounts - } - }; - - let check_sum: u128 = sum_as(&remaining_undecided_accounts, |weighted_account| { + fn is_cw_balance_under_limit( + &self, + remaining_undecided_accounts: &Vec, + ) -> bool { + let check_sum: u128 = sum_as(remaining_undecided_accounts, |weighted_account| { weighted_account.balance_minor() }); let unallocated_cw_balance = self.inner.unallocated_cw_service_fee_balance_minor(); - let down_stream_decided_accounts = if check_sum <= unallocated_cw_balance { - // Fast return after a direct conversion into the expected type - convert_collection(remaining_undecided_accounts) - } else { - self.propose_possible_adjustment_recursively(remaining_undecided_accounts) - }; + check_sum <= unallocated_cw_balance + } - RecursionResults::new(here_decided_accounts, down_stream_decided_accounts) + fn add_decided_accounts( + accounts: Vec, + decided_accounts: Vec, + ) -> Vec { + decided_accounts + .into_iter() + .chain(accounts.into_iter()) + .collect() } fn resolve_current_iteration_result( From d4af0fe2e3c4d3d2e445656f5201ed44a72fb4bf Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Wed, 14 Aug 2024 14:07:32 +0530 Subject: [PATCH 3/6] GH-711: more refactoring of propose_possible_adjustment_recursively --- node/src/accountant/payment_adjuster/mod.rs | 29 +++++++-------------- 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/node/src/accountant/payment_adjuster/mod.rs b/node/src/accountant/payment_adjuster/mod.rs index 0f94f3c8b..58c416e35 100644 --- a/node/src/accountant/payment_adjuster/mod.rs +++ b/node/src/accountant/payment_adjuster/mod.rs @@ -279,22 +279,17 @@ impl PaymentAdjusterReal { &mut self, weighed_accounts: Vec, ) -> Vec { - let disqualification_arbiter = &self.disqualification_arbiter; - let unallocated_cw_service_fee_balance = - self.inner.unallocated_cw_service_fee_balance_minor(); - let logger = &self.logger; - let current_iteration_result = self.service_fee_adjuster.perform_adjustment_by_service_fee( weighed_accounts, - disqualification_arbiter, - unallocated_cw_service_fee_balance, - logger, + &self.disqualification_arbiter, + self.inner.unallocated_cw_service_fee_balance_minor(), + &self.logger, ); - let remaining_undecided_accounts = current_iteration_result.remaining_undecided_accounts; + let undecided_accounts = current_iteration_result.remaining_undecided_accounts; let decided_accounts: Vec = current_iteration_result.decided_accounts.into(); - if remaining_undecided_accounts.is_empty() { + if undecided_accounts.is_empty() { return decided_accounts; } @@ -302,19 +297,15 @@ impl PaymentAdjusterReal { self.adjust_remaining_unallocated_cw_balance_down(&decided_accounts) } - let merged = if self.is_cw_balance_under_limit(&remaining_undecided_accounts) { + let remaining_accounts = if self.is_cw_balance_under_limit(&undecided_accounts) { // Fast return after a direct conversion into the expected type - Self::add_decided_accounts( - convert_collection(remaining_undecided_accounts), - decided_accounts, - ) + convert_collection(undecided_accounts) } else { - Self::add_decided_accounts( - self.propose_possible_adjustment_recursively(remaining_undecided_accounts), - decided_accounts, - ) + self.propose_possible_adjustment_recursively(undecided_accounts) }; + let merged = Self::add_decided_accounts(remaining_accounts, decided_accounts); + diagnostics!( "\nFINAL SET OF ADJUSTED ACCOUNTS IN CURRENT ITERATION:", &merged From e64d7190ce989986adc7131c0e9d15163bc50254 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Wed, 14 Aug 2024 14:19:32 +0530 Subject: [PATCH 4/6] GH-711: remove code; tests passing --- .../miscellaneous/data_structures.rs | 10 ---- node/src/accountant/payment_adjuster/mod.rs | 57 ++++--------------- 2 files changed, 11 insertions(+), 56 deletions(-) diff --git a/node/src/accountant/payment_adjuster/miscellaneous/data_structures.rs b/node/src/accountant/payment_adjuster/miscellaneous/data_structures.rs index 143d20783..7794651d3 100644 --- a/node/src/accountant/payment_adjuster/miscellaneous/data_structures.rs +++ b/node/src/accountant/payment_adjuster/miscellaneous/data_structures.rs @@ -40,16 +40,6 @@ pub struct RecursionResults { } impl RecursionResults { - pub fn new( - here_decided_accounts: Vec, - downstream_decided_accounts: Vec, - ) -> Self { - Self { - here_decided_accounts, - downstream_decided_accounts, - } - } - pub fn merge_results_from_recursion(self) -> Vec { self.here_decided_accounts .into_iter() diff --git a/node/src/accountant/payment_adjuster/mod.rs b/node/src/accountant/payment_adjuster/mod.rs index 58c416e35..35fe012de 100644 --- a/node/src/accountant/payment_adjuster/mod.rs +++ b/node/src/accountant/payment_adjuster/mod.rs @@ -16,7 +16,7 @@ mod test_utils; use crate::accountant::db_access_objects::payable_dao::PayableAccount; use crate::accountant::payment_adjuster::adjustment_runners::{ - AdjustmentRunner, ServiceFeeOnlyAdjustmentRunner, TransactionAndServiceFeeAdjustmentRunner, + AdjustmentRunner, TransactionAndServiceFeeAdjustmentRunner, }; use crate::accountant::payment_adjuster::criterion_calculators::balance_calculator::BalanceCriterionCalculator; use crate::accountant::payment_adjuster::criterion_calculators::CriterionCalculator; @@ -31,10 +31,7 @@ use crate::accountant::payment_adjuster::inner::{ use crate::accountant::payment_adjuster::logging_and_diagnostics::log_functions::{ accounts_before_and_after_debug, }; -use crate::accountant::payment_adjuster::miscellaneous::data_structures::DecidedAccounts::{ - LowGainingAccountEliminated, SomeAccountsProcessed, -}; -use crate::accountant::payment_adjuster::miscellaneous::data_structures::{AdjustedAccountBeforeFinalization, AdjustmentIterationResult, DecidedAccounts, RecursionResults, WeightedPayable}; +use crate::accountant::payment_adjuster::miscellaneous::data_structures::{AdjustedAccountBeforeFinalization, WeightedPayable}; use crate::accountant::payment_adjuster::miscellaneous::helper_functions::{ dump_unaffordable_accounts_by_transaction_fee, exhaust_cw_balance_entirely, find_largest_exceeding_balance, @@ -54,7 +51,6 @@ use itertools::Either; use masq_lib::logger::Logger; use std::collections::HashMap; use std::fmt::{Display, Formatter}; -use std::ops::Add; use std::time::SystemTime; use thousands::Separable; use variant_count::VariantCount; @@ -285,26 +281,22 @@ impl PaymentAdjusterReal { self.inner.unallocated_cw_service_fee_balance_minor(), &self.logger, ); - let undecided_accounts = current_iteration_result.remaining_undecided_accounts; let decided_accounts: Vec = current_iteration_result.decided_accounts.into(); + if undecided_accounts.is_empty() { return decided_accounts; } - if !decided_accounts.is_empty() { - self.adjust_remaining_unallocated_cw_balance_down(&decided_accounts) - } - + self.deduct_unallocated_cw_balance(&decided_accounts); let remaining_accounts = if self.is_cw_balance_under_limit(&undecided_accounts) { // Fast return after a direct conversion into the expected type convert_collection(undecided_accounts) } else { self.propose_possible_adjustment_recursively(undecided_accounts) }; - - let merged = Self::add_decided_accounts(remaining_accounts, decided_accounts); + let merged = Self::add_accounts(remaining_accounts, decided_accounts); diagnostics!( "\nFINAL SET OF ADJUSTED ACCOUNTS IN CURRENT ITERATION:", @@ -327,7 +319,7 @@ impl PaymentAdjusterReal { check_sum <= unallocated_cw_balance } - fn add_decided_accounts( + fn add_accounts( accounts: Vec, decided_accounts: Vec, ) -> Vec { @@ -337,37 +329,6 @@ impl PaymentAdjusterReal { .collect() } - fn resolve_current_iteration_result( - &mut self, - adjustment_iteration_result: AdjustmentIterationResult, - ) -> RecursionResults { - let remaining_undecided_accounts = adjustment_iteration_result.remaining_undecided_accounts; - let here_decided_accounts = match adjustment_iteration_result.decided_accounts { - LowGainingAccountEliminated => { - if remaining_undecided_accounts.is_empty() { - return RecursionResults::new(vec![], vec![]); - } - - vec![] - } - SomeAccountsProcessed(decided_accounts) => { - if remaining_undecided_accounts.is_empty() { - return RecursionResults::new(decided_accounts, vec![]); - } - - self.adjust_remaining_unallocated_cw_balance_down(&decided_accounts); - decided_accounts - } - }; - - let down_stream_decided_accounts = self.propose_adjustments_recursively( - remaining_undecided_accounts, - ServiceFeeOnlyAdjustmentRunner {}, - ); - - RecursionResults::new(here_decided_accounts, down_stream_decided_accounts) - } - fn calculate_weights(&self, accounts: Vec) -> Vec { self.apply_criteria(self.calculators.as_slice(), accounts) } @@ -404,10 +365,14 @@ impl PaymentAdjusterReal { .collect() } - fn adjust_remaining_unallocated_cw_balance_down( + fn deduct_unallocated_cw_balance( &mut self, processed_outweighed: &[AdjustedAccountBeforeFinalization], ) { + if processed_outweighed.is_empty() { + return; + } + let subtrahend_total: u128 = sum_as(processed_outweighed, |account| { account.proposed_adjusted_balance_minor }); From a877c6f992d88193cc8f15705f2da9d063c10b89 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Wed, 14 Aug 2024 14:24:17 +0530 Subject: [PATCH 5/6] GH-711: minor rename --- .../payment_adjuster/miscellaneous/data_structures.rs | 2 +- node/src/accountant/payment_adjuster/mod.rs | 4 ++-- .../accountant/payment_adjuster/service_fee_adjuster.rs | 7 +++---- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/node/src/accountant/payment_adjuster/miscellaneous/data_structures.rs b/node/src/accountant/payment_adjuster/miscellaneous/data_structures.rs index 7794651d3..753cc067b 100644 --- a/node/src/accountant/payment_adjuster/miscellaneous/data_structures.rs +++ b/node/src/accountant/payment_adjuster/miscellaneous/data_structures.rs @@ -31,7 +31,7 @@ impl WeightedPayable { #[derive(Debug, PartialEq, Eq)] pub struct AdjustmentIterationResult { pub decided_accounts: DecidedAccounts, - pub remaining_undecided_accounts: Vec, + pub undecided_accounts: Vec, } pub struct RecursionResults { diff --git a/node/src/accountant/payment_adjuster/mod.rs b/node/src/accountant/payment_adjuster/mod.rs index 35fe012de..aad100c04 100644 --- a/node/src/accountant/payment_adjuster/mod.rs +++ b/node/src/accountant/payment_adjuster/mod.rs @@ -281,7 +281,7 @@ impl PaymentAdjusterReal { self.inner.unallocated_cw_service_fee_balance_minor(), &self.logger, ); - let undecided_accounts = current_iteration_result.remaining_undecided_accounts; + let undecided_accounts = current_iteration_result.undecided_accounts; let decided_accounts: Vec = current_iteration_result.decided_accounts.into(); @@ -2211,7 +2211,7 @@ mod tests { // We care only for the params captured inside the container from above .perform_adjustment_by_service_fee_result(AdjustmentIterationResult { decided_accounts: SomeAccountsProcessed(vec![]), - remaining_undecided_accounts: vec![], + undecided_accounts: vec![], }); subject.service_fee_adjuster = Box::new(service_fee_adjuster_mock); diff --git a/node/src/accountant/payment_adjuster/service_fee_adjuster.rs b/node/src/accountant/payment_adjuster/service_fee_adjuster.rs index 5954a224d..08846b892 100644 --- a/node/src/accountant/payment_adjuster/service_fee_adjuster.rs +++ b/node/src/accountant/payment_adjuster/service_fee_adjuster.rs @@ -99,13 +99,12 @@ impl ServiceFeeAdjusterReal { if thriving_competitors.is_empty() { Either::Left(losing_competitors) } else { - let remaining_undecided_accounts: Vec = - convert_collection(losing_competitors); + let undecided_accounts: Vec = convert_collection(losing_competitors); let pre_processed_decided_accounts: Vec = convert_collection(thriving_competitors); Either::Right(AdjustmentIterationResult { decided_accounts: SomeAccountsProcessed(pre_processed_decided_accounts), - remaining_undecided_accounts, + undecided_accounts, }) } } @@ -127,7 +126,7 @@ impl ServiceFeeAdjusterReal { AdjustmentIterationResult { decided_accounts: LowGainingAccountEliminated, - remaining_undecided_accounts: remaining_reverted, + undecided_accounts: remaining_reverted, } } From accdef4343b49d8400c92522f37bdea1500f2391 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Wed, 14 Aug 2024 15:04:47 +0530 Subject: [PATCH 6/6] GH-711: final refactoring done --- .../payment_adjuster/adjustment_runners.rs | 1 + node/src/accountant/payment_adjuster/mod.rs | 21 +++++++++++++------ 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/node/src/accountant/payment_adjuster/adjustment_runners.rs b/node/src/accountant/payment_adjuster/adjustment_runners.rs index a02a0cee2..059b91274 100644 --- a/node/src/accountant/payment_adjuster/adjustment_runners.rs +++ b/node/src/accountant/payment_adjuster/adjustment_runners.rs @@ -46,6 +46,7 @@ impl AdjustmentRunner for TransactionAndServiceFeeAdjustmentRunner { } } +// TODO: GH-711: This could be deleted (the code is directly used) pub struct ServiceFeeOnlyAdjustmentRunner {} impl AdjustmentRunner for ServiceFeeOnlyAdjustmentRunner { diff --git a/node/src/accountant/payment_adjuster/mod.rs b/node/src/accountant/payment_adjuster/mod.rs index aad100c04..91dd4c749 100644 --- a/node/src/accountant/payment_adjuster/mod.rs +++ b/node/src/accountant/payment_adjuster/mod.rs @@ -192,10 +192,15 @@ impl PaymentAdjusterReal { analyzed_accounts: Vec, ) -> Result, PaymentAdjusterError> { let weighted_accounts = self.calculate_weights(analyzed_accounts); - let processed_accounts = self.propose_adjustments_recursively( - weighted_accounts, - TransactionAndServiceFeeAdjustmentRunner {}, - )?; + diagnostics!( + "\nUNRESOLVED QUALIFIED ACCOUNTS IN CURRENT ITERATION:", + &weighted_accounts + ); + let processed_accounts = if let Some(limit) = self.inner.transaction_fee_count_limit_opt() { + self.begin_with_adjustment_by_transaction_fee(weighted_accounts.clone(), limit)? + } else { + Either::Left(self.propose_possible_adjustment_recursively(weighted_accounts)) + }; if zero_affordable_accounts_found(&processed_accounts) { return Err(PaymentAdjusterError::AllAccountsEliminated); @@ -215,6 +220,7 @@ impl PaymentAdjusterReal { } } + // TODO: GH-711: This can be removed. The code for AdjustmentRunners is now used directly fn propose_adjustments_recursively( &mut self, unresolved_accounts: Vec, @@ -271,6 +277,9 @@ impl PaymentAdjusterReal { } } + // TODO: GH-711: This is the only recursive fn that is being called now. + // It's sibling function propose_adjustment_recursively() and it's + // subsequent adjustment runners can be safely removed fn propose_possible_adjustment_recursively( &mut self, weighed_accounts: Vec, @@ -320,12 +329,12 @@ impl PaymentAdjusterReal { } fn add_accounts( - accounts: Vec, + remaining_accounts: Vec, decided_accounts: Vec, ) -> Vec { decided_accounts .into_iter() - .chain(accounts.into_iter()) + .chain(remaining_accounts.into_iter()) .collect() }