Update payment tracker to not update any capacities if they are zero.#106
Update payment tracker to not update any capacities if they are zero.#106
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds validation to prevent updating the payment tracker with zero capacity values, which can occur during contract upgrades when storage slots change and default capacities aren't properly initialized. The change prevents the payment tracker from being configured with invalid zero values that would cause division-by-zero errors in capacity calculations.
Changes:
- Add validation in the usage processor to reject node capacity configs with any zero values
- Add Debug trait to NodeCapacityConfig to enable logging of rejected configurations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rust/lit-node/lit-node/src/tasks/payment.rs | Adds validation logic to check all capacity fields are non-zero before updating payment tracker, with warning logs for rejected configs |
| rust/lit-node/lit-node/src/payment/payment_tracker.rs | Adds Debug trait to NodeCapacityConfig struct to support debug logging |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if node_capacity_config.global_max_capacity > 0 | ||
| && node_capacity_config.pkp_sign_max_concurrency > 0 | ||
| && node_capacity_config.enc_sign_max_concurrency > 0 | ||
| && node_capacity_config.lit_action_max_concurrency > 0 | ||
| && node_capacity_config.sign_session_key_max_concurrency > 0 | ||
| { | ||
| payment_tracker_for_capacity | ||
| .update_node_capacity_config(node_capacity_config); | ||
| } else { | ||
| warn!( | ||
| "Invalid node capacity config ( will not update payment tracker ): {:?}", | ||
| node_capacity_config | ||
| ); |
There was a problem hiding this comment.
The validation prevents updating the payment tracker with zero values, which is good for preventing the immediate update. However, this creates a critical problem: if the payment tracker was initialized with default values (all zeros) and this validation prevents the first valid update from being applied, the payment tracker will remain in an invalid state with zeros. This will cause division by zero panics when:
get_op_capacity()dividesglobal_max_capacityby any of the concurrency values (lines 29-36 in payment_tracker.rs)get_usage_percentage()divides byglobal_max_capacity(line 74 in payment_tracker.rs)
These methods are called during normal operation when registering/deregistering usage. Consider either:
- Initializing PaymentTracker with valid non-zero default values using
NodeCapacityConfig::new()instead ofDefault::default(), or - Adding checks in the PaymentTracker methods to handle zero values gracefully, or
- Making the validation more sophisticated to allow updates if the current config is all zeros (first initialization)
| if node_capacity_config.global_max_capacity > 0 | |
| && node_capacity_config.pkp_sign_max_concurrency > 0 | |
| && node_capacity_config.enc_sign_max_concurrency > 0 | |
| && node_capacity_config.lit_action_max_concurrency > 0 | |
| && node_capacity_config.sign_session_key_max_concurrency > 0 | |
| { | |
| payment_tracker_for_capacity | |
| .update_node_capacity_config(node_capacity_config); | |
| } else { | |
| warn!( | |
| "Invalid node capacity config ( will not update payment tracker ): {:?}", | |
| node_capacity_config | |
| ); | |
| let all_zero = node_capacity_config.global_max_capacity == 0 | |
| && node_capacity_config.pkp_sign_max_concurrency == 0 | |
| && node_capacity_config.enc_sign_max_concurrency == 0 | |
| && node_capacity_config.lit_action_max_concurrency == 0 | |
| && node_capacity_config.sign_session_key_max_concurrency == 0; | |
| if all_zero { | |
| warn!( | |
| "Invalid node capacity config (all zeros; will not update payment tracker): {:?}", | |
| node_capacity_config | |
| ); | |
| } else { | |
| payment_tracker_for_capacity | |
| .update_node_capacity_config(node_capacity_config); |
| if node_capacity_config.global_max_capacity > 0 | ||
| && node_capacity_config.pkp_sign_max_concurrency > 0 | ||
| && node_capacity_config.enc_sign_max_concurrency > 0 | ||
| && node_capacity_config.lit_action_max_concurrency > 0 | ||
| && node_capacity_config.sign_session_key_max_concurrency > 0 | ||
| { | ||
| payment_tracker_for_capacity | ||
| .update_node_capacity_config(node_capacity_config); | ||
| } else { | ||
| warn!( | ||
| "Invalid node capacity config ( will not update payment tracker ): {:?}", | ||
| node_capacity_config | ||
| ); | ||
| } |
There was a problem hiding this comment.
This validation logic lacks test coverage. Given that this is a critical safety check to prevent invalid capacity configurations from being applied (which could cause division by zero panics), it would be valuable to add tests that verify:
- Valid configurations are accepted and applied
- Configurations with any zero values are rejected and logged
- The payment tracker state remains unchanged when invalid configurations are rejected
The codebase has comprehensive test coverage for other payment-related functionality (see rust/lit-node/lit-node/src/payment/batches.rs:139), so this new validation should follow the same pattern.
|
PASS [ 44.508s] (3/3) lit_node::test toxiproxy::perf_tests::load_with_no_latency |
WHAT
Update payment tracker to not update any capacities if they are zero.
This can happen while upgrading contracts if the storage slots change on the diamond facet in price feed and the new slots don't have the default capacities in place. ( Likely if the network is running, while upgrading! )
See slack here: https://litprotocol.slack.com/archives/C08PH6TN204/p1770375336618479