GH-598: fixes driven from debugging - review 2 by bertllll · Pull Request #743 · MASQ-Project/Node

bertllll · 2025-11-11T12:59:56Z

No description provided.

…t - what should it take?

cursor · 2025-11-11T13:00:00Z

You have run out of free Bugbot PR reviews for this billing cycle. This will reset on November 14.

To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

utkarshg6 · 2025-11-12T12:07:35Z

node/src/accountant/logging_utils/accounting_msgs_debug.rs

 }

-impl NewPostingsDebugContainer {
+impl NewChargessDebugContainer {


We want single 's' in the Charges.

utkarshg6 · 2025-11-17T06:38:05Z

node/src/accountant/logging_utils/msg_id_generator.rs

        MSG_ID_INCREMENTER.fetch_add(1, Ordering::Relaxed)
    }
+    fn last_used_id(&self) -> u32 {
+        MSG_ID_INCREMENTER.load(Ordering::Relaxed) - 1


Why is there -1 at the end? Correct me if I'm wrong but the last I'd should be the current value, not current value -1.

utkarshg6 · 2025-11-17T06:42:17Z

node/src/accountant/logging_utils/msg_id_generator.rs

+        let same_id_2 = subject.last_used_id();
+        let new_id_2 = subject.new_id();
+
+        assert_eq!(new_id, same_id_1);


I would suggest if you increment the value first and then return it in the new_id method. That way you won't need to use -1 in the load_id method.

utkarshg6

Continue at node/src/sub_lib/accountant.rs:136

utkarshg6 · 2025-11-17T06:56:12Z

node/src/accountant/test_utils.rs

    }
+
+    fn last_used_id(&self) -> u32 {
+        todo!()


You can write something here, I don't like it empty.

utkarshg6 · 2025-11-17T07:02:24Z

node/src/hopper/routing_service.rs

-                        payload_size,
-                        service_rate: self.per_routing_service,
-                        byte_rate: self.per_routing_byte,
+                        service: ServiceProvided {


I dunno why you nested it like that, looks unnecessary, unless there's a sibling field.

utkarshg6 · 2025-11-17T07:14:29Z

node/src/sub_lib/accountant.rs

-    pub payload_size: usize,
-    pub service_rate: u64,
-    pub byte_rate: u64,
+pub trait AccountableServiceWithTraceLog {


This would be additional complexity while searching for logs in the production code. I personally prefer a direct print of log using macros. Introducing a layer of abstraction here apparently can make it harder to search for logs.

utkarshg6 · 2025-11-17T10:04:49Z

node/src/sub_lib/accountant.rs

+    fn trace_log_msg(&self, msg_id: u32) -> String;
+}
+
+pub trait MessageWithServicesProvided {


Prefer enum based approach here

#[derive(Clone, PartialEq, Eq, Debug, Message)] pub struct ReportServiceProvidedMessage { pub service: ServiceProvided, pub service_type: ServiceType, } #[derive(Clone, PartialEq, Eq, Debug)] pub enum ServiceType { Exit, Routing, } impl ReportServiceProvidedMessage { pub fn new_exit_service(service: ServiceProvided) -> Self { Self { service, service_type: ServiceType::Exit, } } pub fn new_routing_service(service: ServiceProvided) -> Self { Self { service, service_type: ServiceType::Routing, } } } impl ReportServiceProvidedMessage { fn services_provided(&self) -> &ServiceProvided { &self.service } fn service_type(&self) -> ServiceType { self.service_type } }

utkarshg6 · 2025-11-17T10:12:48Z

node/src/sub_lib/accountant.rs

+    pub routing: RoutingServicesConsumed,
+}
+
+impl AccountableServiceWithTraceLog for ExitServiceConsumed {


Lines 219...234 could be replaced by a single log message which just prints the service type similar to other values in the trace log message.

utkarshg6 · 2025-11-17T10:30:46Z

node/src/sub_lib/accountant.rs

    pub byte_rate: u64,
 }

+pub enum ServiceProvidedTraceLogWrapper<'a> {


Again it should be done other way around. You should use the enum ServiceType here. Refer to this comment - https://github.com/MASQ-Project/Node/pull/743/files#r2533460766

utkarshg6 · 2025-11-17T10:33:04Z

node/src/sub_lib/accountant.rs

+        let exit_service_msg = ReportExitServiceProvidedMessage {
+            service: service.clone(),
+        };
+        let routing_service_msg = ReportRoutingServiceProvidedMessage { service };


This test will change slightly. You should consider a test with 2 cases along with a helper function.

utkarshg6

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

utkarshg6 · 2025-11-17T12:56:22Z

node/src/accountant/logging_utils/accounting_msgs_debug.rs

+    ) -> Option<LoggableStats> {
+        self.record_new_charges_by_msg_type(new_charges, msg_type);
+
+        self.maybe_dump_stats_by_msg_type(log_window_size, msg_type)


I'd safely remove the suffix _by_msg_type from the fn names.

utkarshg6 · 2025-11-17T13:05:31Z

node/src/accountant/logging_utils/accounting_msgs_debug.rs

+        let mut new_charge_feeds_per_msg =
+            generate_new_charge_feeds_representing_msgs(log_window_size);
+        let expected_sorted_stats =
+            construct_expected_sorted_stats_from_generated_new_charges(&new_charge_feeds_per_msg);


The variable names are less clear and verbose. Kindly change it to something familiar like:

new_charge_feeds_per_msg -> test_msgs
expected_sorted_stats -> expected_stats

utkarshg6 · 2025-11-17T13:07:40Z

node/src/accountant/logging_utils/accounting_msgs_debug.rs

+        let mut subject = AccountingMessageTracker::default();
+        assert_empty_stats(&subject);
+        let charge_msg_matching_the_window_size =
+            new_charge_feeds_per_msg.remove(log_window_size as usize - 1 - 1);


These two -1 are less clear. It's hard to decipher what individual subtractions are trying to achieve. I'd suggest to break this into two steps and use different variable names. It will shed some light on why these subtractions happened.

utkarshg6 · 2025-11-17T13:12:09Z

node/src/accountant/logging_utils/accounting_msgs_debug.rs

+        test_msgs_of_count_window_size_minus_one(
+            &mut subject,
+            msg_type,
+            log_window_size,


Also, the use of window size is unclear in this testing fn as whole. I'd like if we can make it clearer.

utkarshg6 · 2025-11-17T13:13:44Z

node/src/accountant/logging_utils/accounting_msgs_debug.rs

+        assert_provided_loggable_stats(result, msg_type, log_window_size, expected_sorted_stats);
+        assert_empty_stats(&subject);
+
+        retest_after_emptied(&mut subject, msg_type);


Optional: There are so many fns being used, I'd converted it into a builder pattern and would implement these fns on the structure. If you're ready for more adventure then only embark on it, otherwise it's totally fine to leave it this way.

utkarshg6

Continue at node/src/accountant/mod.rs

utkarshg6 · 2025-11-17T14:28:12Z

node/src/accountant/mod.rs

+    fn record_service_provided(&self, service: &ServiceProvided) -> Option<NewCharge> {
+        let byte_charge = service.byte_rate as u128 * (service.payload_size as u128);
+        let total_charge = service.service_rate as u128 + byte_charge;
+        let wallet = &service.paying_wallet;


Nicely Done!

utkarshg6 · 2025-11-17T15:33:48Z

node/src/accountant/mod.rs

-    }
+    fn handle_report_service_provided_message<AccountingMessage>(&mut self, msg: AccountingMessage)
+    where
+        AccountingMessage: MessageWithServicesProvided,


This should go away once you decide to use the structure instead of the trait implementation. The enum based approach will make things easier for you here. You can thank me later.

utkarshg6 · 2025-11-17T15:37:39Z

node/src/accountant/mod.rs

-            msg.payload_size,
-            &msg.paying_wallet,
-        );
+        trace_log_wrapper.log_trace(self.logging_kit(MsgIdRequested::New));


This code is worrisome. I think if we had to find the log once we have the log file with us, then at that time, it'll be difficult for us to find this.

This layer of abstraction is the issue, if it wasn't for a use case like logging, I'd appreciate it and here, I'm concerned.

utkarshg6

Done!

Bert added 3 commits November 9, 2025 22:19

GH-598-some-cosmetics-2: mostly answered the review; now just big tes…

cb812be

…t - what should it take?

GH-598-some-cosmetics-2: interim commit

1c306a1

GH-598-some-cosmetics-2: finalization

ea75968

utkarshg6 reviewed Nov 12, 2025

View reviewed changes

utkarshg6 reviewed Nov 17, 2025

View reviewed changes

Conversation

bertllll commented Nov 11, 2025

Uh oh!

cursor bot commented Nov 11, 2025

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!

Choose a reason for hiding this comment

Uh oh!

utkarshg6 Nov 17, 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!

utkarshg6 Nov 17, 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!

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!

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 Nov 17, 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!

utkarshg6 Nov 17, 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!

utkarshg6 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

utkarshg6 Nov 17, 2025 •

edited

Loading

utkarshg6 Nov 17, 2025 •

edited

Loading

utkarshg6 Nov 17, 2025 •

edited

Loading

utkarshg6 Nov 17, 2025 •

edited

Loading