Uh oh!
There was an error while loading. Please reload this page.
GH-598: Improvement: fixes driven from debugging - #739
Conversation
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on November 14
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| accounting_msg_stats: self.stats.drain().collect(), | ||
| log_window_in_pcs_of_msgs: log_window_size, | ||
| }) | ||
| } else { |
There was a problem hiding this comment.
Bug: Misleading Debug Logs for Message Types
The loggable_stats method in AccountingMsgStats hardcodes AccountingMsgType::RoutingServiceProvided when generating debug logs. This causes logs for ExitServiceProvided and ServicesConsumed messages to incorrectly display "RoutingServiceProvided", making the debug output misleading.
Uh oh!
There was an error while loading. Please reload this page.
utkarshg6
left a comment
There was a problem hiding this comment.
Continue at node/src/accountant/mod.rs:833
| MessageIdGenerator, MessageIdGeneratorReal, | ||
| }; | ||
| pub struct LoggingUtils { |
There was a problem hiding this comment.
Please consider the following renames:
pubstructAccountingLogger{pubdebug_stats:AccountingMessageTracker,pubid_generator:Box<dynMessageIdGenerator>,}pubstructAccountingMessageTracker{routing_stats:AccountingBuffer,exit_stats:AccountingBuffer,consumed_stats:AccountingBuffer,}structAccountingBuffer{address_totals:HashMap<Address,u128>,message_count:usize,}Uh oh!
There was an error while loading. Please reload this page.
| report_sent_payables_sub_opt: Option<Recipient<SentPayables>>, | ||
| ui_message_sub_opt: Option<Recipient<NodeToUiMessage>>, | ||
| message_id_generator: Box<dyn MessageIdGenerator>, | ||
| logging_utils: LoggingUtils, |
There was a problem hiding this comment.
In case you rename this structure, rename the field name too.
| payload_size: usize, | ||
| wallet: &Wallet, | ||
| ) { | ||
| ) -> u128 { |
There was a problem hiding this comment.
This function is violating one principle: "A fn that mutates shouldn't return anything." These two responsibilities should be done by two different functions.
| ); | ||
| }) | ||
| let new_postings = NewPostingsDebugContainer::new(&self.logger) |
There was a problem hiding this comment.
Instead of "posting", can you please use "charges"?
utkarshg6
left a comment
There was a problem hiding this comment.
Continue at node/src/accountant/scanners/pending_payable_scanner/finish_scan.rs
| let new_postings = self.handle_routing_services_consumed(msg, msg_id, new_postings); | ||
| self.logging_utils.accounting_msgs_stats.manage_debug_log( |
There was a problem hiding this comment.
I'd prefer if you directly log it here... that'll be clearer. From the outside, it only appears it's just a log, but after a deep dive, I see use of &mut, and I don't think it's ideal to do it this way, we should do mutations and logging separately, using different functions step by step. Give it a thought. I didn't give an absolute example, because there is nesting in the code and you would need to refactor on multiple levels to fix that. I'd appreciate if you do.
| msg.routing_payload_size | ||
| ); | ||
| let sum = self.record_service_consumed( |
There was a problem hiding this comment.
It should be done in two steps rather than one. One function should just calculate total charge and other should update and log.
In accountant.rs:
pubstructServicesConsumed{pubservice_rate:u64,pubbyte_rate:u64,pubpayload_size:usize,}implDisplayforServicesConsumed{fnfmt(&self,f:&mutFormatter<'_>) -> std::fmt::Result{write!(
f,"service rate {}, byte rate {}, payload size {}",self.service_rate,self.byte_rate,self.payload_size
)}}implServicesConsumed{pubfncalculate_total_charge(&self) -> u128{let byte_charge = self.byte_rateasu128*(self.payload_sizeasu128);self.service_rateasu128 + byte_charge
}}In accountant/mod.rs:
implAccountant{
....fnupdate_records_in_payable(&self,total_charge:u128,services_consumed:&ServicesConsumed,timestamp:SystemTime,wallet:Wallet,){matchself.payable_dao.as_ref().more_money_payable(timestamp, wallet, total_charge){Ok(_) => (),Err(PayableDaoError::SignConversion(_)) => error!(self.logger,"Overflow error recording consumed services from {}: total charge {}, {}. Skipping",
wallet,
total_charge,
services_consumed
),Err(e) => panic!("Recording services consumed from {} but has hit fatal database error: {:?}",
wallet, e
),};}fnrecord_service_consumed(&self,services_consumed:ServicesConsumed,timestamp:SystemTime,wallet:&Wallet,) -> u128{ifself.our_wallet(wallet){warning!(self.logger,"Declining to record a payable against our wallet {} for service we provided",
wallet
);return0;}let total_charge = services_consumed.calculate_total_charge();self.update_records_in_payable(total_charge,&services_consumed, timestamp, wallet);
total_charge
}}Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
utkarshg6
left a comment
There was a problem hiding this comment.
Continue at node/src/accountant/logging_utils/accounting_msgs_debug.rs
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| fn handle_new_postings(&mut self, new_postings: Vec<NewPosting>) { | ||
| for new_posting in &new_postings { |
There was a problem hiding this comment.
Instead of using the & here, you can call new_postings.iter()
Like this:
for new_posting in new_postings.iter(){*self.stats.entry(new_posting.address).or_default() += new_posting.amount_wei;}There was a problem hiding this comment.
Why should I? Isn't it indifferent? It doesn't help in anything.
Uh oh!
There was an error while loading. Please reload this page.
utkarshg6
left a comment
There was a problem hiding this comment.
Continue at node/src/accountant/logging_utils/accounting_msgs_debug.rs:198
| ); | ||
| } | ||
| fn test_manage_debug_log( |
There was a problem hiding this comment.
This function is difficult to understand. It would be helpful if you could add comments that can act as a line of demarcation for different phases. It would be great to create those too (only refactor out such functions if you can find general purpose use case, otherwise don't bother yourself).
| fn record_new_posting_feed_in( | ||
| first_expected_stats: Vec<(Address, u128)>, | ||
| second_new_posting: Vec<NewPosting>, | ||
| ) -> Vec<(Address, u128)> { |
There was a problem hiding this comment.
Please do minor renames here:
record_new_posting_feed_in -> merge_stats_with_new_postingsfirst_expected_stats -> existing_statssecond_new_posting -> new_postings
| second_expected_stats | ||
| } | ||
| fn generate_posting_feeds_representing_msgs(gap_size: u16) -> Vec<Vec<NewPosting>> { |
There was a problem hiding this comment.
Consider the following renames:
gap_size -> window_sizeouter_idx -> message_numberinner_idx -> posting_index
utkarshg6
left a comment
There was a problem hiding this comment.
Continue at node/src/accountant/logging_utils/accounting_msgs_debug.rs:358
| new_postings_feeds | ||
| } | ||
| fn compute_expected_stats_from_new_posting_feeds( |
There was a problem hiding this comment.
You can do it far better here:
fncalculate_expected_accumulated_stats(message_batches:&[Vec<NewPosting>],) -> Vec<(Address,u128)>{
message_batches
.iter().flatten().fold(HashMap::new(), |mut totals, posting| {*totals.entry(posting.address).or_default() += posting.amount_wei;
totals
}).into_iter().sorted().collect()}
Note
Adds accounting debug stats and message ID utilities, refactors pending-payables scanner and log wording, and updates address/test helpers.
logging_utilswithaccounting_msgs_debug(aggregate per-address debits over a window) andmsg_id_generator.Accountant(logging_utils,DEFAULT_ACCOUNTING_MSG_LOG_WINDOW, debug containers);msg_id()now uses new generator.record_service_provided/consumedreturnu128; debug-posting collection forReport*handlers.start_scan.rsandfinish_scan.rs.Option), and tidy emptiness checks.MessageIdGeneratorfromsub_lib; add tests for new generator.blockchain::test_utils::make_addresslayout; adjust expected addresses in tests accordingly.Written by Cursor Bugbot for commit e8dc244. This will update automatically on new commits. Configure here.