GH-598: changes driven from discoveries the debugging gave us #739

bertllll · 2025-11-04T03:41:31Z

Note

Adds accounting debug stats and message ID utilities, refactors pending-payables scanner and log wording, and updates address/test helpers.

Logging/Debugging:
- Introduce logging_utils with accounting_msgs_debug (aggregate per-address debits over a window) and msg_id_generator.
- Wire into Accountant (logging_utils, DEFAULT_ACCOUNTING_MSG_LOG_WINDOW, debug containers); msg_id() now uses new generator.
Accountant:
- record_service_provided/consumed return u128; debug-posting collection for Report* handlers.
- Improved trace/info wording for pending-payables reruns.
PendingPayableScanner:
- Split into start_scan.rs and finish_scan.rs.
- Rename caches/wording from "suspected" to "supposed"; log "Collected … supposed failures".
- Simplify harvesting/merging APIs (use vectors, no Option), and tidy emptiness checks.
Tests/Strings:
- Update expectations across tests and multinode integration to new log messages and behaviors.
- Remove old MessageIdGenerator from sub_lib; add tests for new generator.
Utils:
- Change blockchain::test_utils::make_address layout; adjust expected addresses in tests accordingly.

^{Written by Cursor Bugbot for commit e8dc244. This will update automatically on new commits. Configure here.}

cursor

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.

cursor · 2025-11-04T03:43:11Z

node/src/accountant/logging_utils/accounting_msgs_debug.rs

+                accounting_msg_stats: self.stats.drain().collect(),
+                log_window_in_pcs_of_msgs: log_window_size,
+            })
+        } else {


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.

multinode_integration_tests/tests/verify_bill_payment.rs

utkarshg6

Continue at node/src/accountant/mod.rs:833

utkarshg6 · 2025-11-04T08:37:06Z

node/src/accountant/logging_utils/mod.rs

+    MessageIdGenerator, MessageIdGeneratorReal,
+};
+
+pub struct LoggingUtils {


Please consider the following renames:

pub struct AccountingLogger { pub debug_stats: AccountingMessageTracker, pub id_generator: Box<dyn MessageIdGenerator>, } pub struct AccountingMessageTracker { routing_stats: AccountingBuffer, exit_stats: AccountingBuffer, consumed_stats: AccountingBuffer, } struct AccountingBuffer { address_totals: HashMap<Address, u128>, message_count: usize, }

node/src/accountant/logging_utils/msg_id_generator.rs

utkarshg6 · 2025-11-04T08:55:43Z

node/src/accountant/mod.rs

    report_sent_payables_sub_opt: Option<Recipient<SentPayables>>,
    ui_message_sub_opt: Option<Recipient<NodeToUiMessage>>,
-    message_id_generator: Box<dyn MessageIdGenerator>,
+    logging_utils: LoggingUtils,


In case you rename this structure, rename the field name too.

utkarshg6 · 2025-11-04T09:33:18Z

node/src/accountant/mod.rs

        payload_size: usize,
        wallet: &Wallet,
-    ) {
+    ) -> u128 {


This function is violating one principle: "A fn that mutates shouldn't return anything." These two responsibilities should be done by two different functions.

utkarshg6 · 2025-11-04T09:50:02Z

node/src/accountant/mod.rs

-            );
-        })
+
+        let new_postings = NewPostingsDebugContainer::new(&self.logger)


Instead of "posting", can you please use "charges"?

utkarshg6

Continue at node/src/accountant/scanners/pending_payable_scanner/finish_scan.rs

utkarshg6 · 2025-11-04T13:09:07Z

node/src/accountant/mod.rs

+
+        let new_postings = self.handle_routing_services_consumed(msg, msg_id, new_postings);
+
+        self.logging_utils.accounting_msgs_stats.manage_debug_log(


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.

utkarshg6 · 2025-11-04T14:02:43Z

node/src/accountant/mod.rs

+                    msg.routing_payload_size
+                );
+
+                let sum = self.record_service_consumed(


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:

pub struct ServicesConsumed { pub service_rate: u64, pub byte_rate: u64, pub payload_size: usize, } impl Display for ServicesConsumed { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!( f, "service rate {}, byte rate {}, payload size {}", self.service_rate, self.byte_rate, self.payload_size ) } } impl ServicesConsumed { pub fn calculate_total_charge(&self) -> u128 { let byte_charge = self.byte_rate as u128 * (self.payload_size as u128); self.service_rate as u128 + byte_charge } }

In accountant/mod.rs:

impl Accountant { .... fn update_records_in_payable( &self, total_charge: u128, services_consumed: &ServicesConsumed, timestamp: SystemTime, wallet: Wallet, ) { match self .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 ), }; } fn record_service_consumed( &self, services_consumed: ServicesConsumed, timestamp: SystemTime, wallet: &Wallet, ) -> u128 { if self.our_wallet(wallet) { warning!( self.logger, "Declining to record a payable against our wallet {} for service we provided", wallet ); return 0; } let total_charge = services_consumed.calculate_total_charge(); self.update_records_in_payable(total_charge, &services_consumed, timestamp, wallet); total_charge } }

node/src/accountant/scanners/mod.rs

node/src/accountant/scanners/pending_payable_scanner/mod.rs

node/src/accountant/scanners/pending_payable_scanner/tx_receipt_interpreter.rs

node/src/accountant/test_utils.rs

utkarshg6

Continue at node/src/accountant/logging_utils/accounting_msgs_debug.rs

node/src/accountant/logging_utils/accounting_msgs_debug.rs

utkarshg6 · 2025-11-05T08:26:47Z

node/src/accountant/logging_utils/accounting_msgs_debug.rs

+    }
+
+    fn handle_new_postings(&mut self, new_postings: Vec<NewPosting>) {
+        for new_posting in &new_postings {


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; }

Why should I? Isn't it indifferent? It doesn't help in anything.

node/src/accountant/logging_utils/accounting_msgs_debug.rs

utkarshg6

Continue at node/src/accountant/logging_utils/accounting_msgs_debug.rs:198

utkarshg6 · 2025-11-05T09:01:04Z

node/src/accountant/logging_utils/accounting_msgs_debug.rs

+        );
+    }
+
+    fn test_manage_debug_log(


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).

utkarshg6 · 2025-11-05T09:15:53Z

node/src/accountant/logging_utils/accounting_msgs_debug.rs

+    fn record_new_posting_feed_in(
+        first_expected_stats: Vec<(Address, u128)>,
+        second_new_posting: Vec<NewPosting>,
+    ) -> Vec<(Address, u128)> {


Please do minor renames here:

record_new_posting_feed_in -> merge_stats_with_new_postings
first_expected_stats -> existing_stats
second_new_posting -> new_postings

utkarshg6 · 2025-11-05T09:23:58Z

node/src/accountant/logging_utils/accounting_msgs_debug.rs

+        second_expected_stats
+    }
+
+    fn generate_posting_feeds_representing_msgs(gap_size: u16) -> Vec<Vec<NewPosting>> {


Consider the following renames:

gap_size -> window_size
outer_idx -> message_number
inner_idx -> posting_index

utkarshg6

Continue at node/src/accountant/logging_utils/accounting_msgs_debug.rs:358

utkarshg6 · 2025-11-05T09:42:07Z

node/src/accountant/logging_utils/accounting_msgs_debug.rs

+        new_postings_feeds
+    }
+
+    fn compute_expected_stats_from_new_posting_feeds(


You can do it far better here:

fn calculate_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() }

utkarshg6

Done!

Bert added 3 commits October 31, 2025 20:09

GH-598: Done with the little changes for now

69357b9

GH-598: before trying to harden the bulky test

64b00d8

GH-598: finished

e8dc244

cursor bot reviewed Nov 4, 2025

View reviewed changes

utkarshg6 reviewed Nov 4, 2025

View reviewed changes

multinode_integration_tests/tests/verify_bill_payment.rs Show resolved Hide resolved

utkarshg6 reviewed Nov 4, 2025

View reviewed changes

utkarshg6 reviewed Nov 5, 2025

View reviewed changes

node/src/accountant/scanners/pending_payable_scanner/mod.rs Show resolved Hide resolved

utkarshg6 reviewed Nov 5, 2025

View reviewed changes

node/src/accountant/scanners/pending_payable_scanner/tx_receipt_interpreter.rs Show resolved Hide resolved

utkarshg6 reviewed Nov 5, 2025

View reviewed changes

node/src/accountant/test_utils.rs Show resolved Hide resolved

utkarshg6 reviewed Nov 5, 2025

View reviewed changes

node/src/accountant/logging_utils/accounting_msgs_debug.rs Show resolved Hide resolved

utkarshg6 reviewed Nov 5, 2025

View reviewed changes

node/src/accountant/logging_utils/accounting_msgs_debug.rs Show resolved Hide resolved

utkarshg6 reviewed Nov 5, 2025

View reviewed changes

node/src/accountant/logging_utils/accounting_msgs_debug.rs Show resolved Hide resolved

utkarshg6 reviewed Nov 5, 2025

View reviewed changes


		let new_postings = self.handle_routing_services_consumed(msg, msg_id, new_postings);

		self.logging_utils.accounting_msgs_stats.manage_debug_log(

GH-598: changes driven from discoveries the debugging gave us #739

Are you sure you want to change the base?

GH-598: changes driven from discoveries the debugging gave us #739

Uh oh!

Conversation

bertllll commented Nov 4, 2025 • edited by cursor bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cursor bot left a comment

Choose a reason for hiding this comment

This is the final PR Bugbot will review for you during this billing cycle

Uh oh!

cursor bot Nov 4, 2025

Choose a reason for hiding this comment

Bug: Misleading Debug Logs for Message Types

Uh oh!

Uh oh!

utkarshg6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

utkarshg6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

utkarshg6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

utkarshg6 left a comment

Choose a reason for hiding this comment

Uh oh!

utkarshg6 Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

utkarshg6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

utkarshg6 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bertllll commented Nov 4, 2025 •

edited by cursor bot

Loading

utkarshg6 Nov 5, 2025 •

edited

Loading