Optimize Memory Usage in Commit-Reveal Validation #26
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/platform-server/src/models/mod.rs (1)
62-73: Excellent refactor—Display is more idiomatic and flexible.This change aligns with Rust API guidelines and provides automatic ToString implementation while enabling use with formatters. The implementation correctly covers all variants with matching string representations.
♻️ Optional micro-optimization
Since
sis already a&str, you can write it directly instead of using thewrite!macro:impl std::fmt::Display for SubmissionStatus { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let s = match self { SubmissionStatus::Pending => "pending", SubmissionStatus::Evaluating => "evaluating", SubmissionStatus::Completed => "completed", SubmissionStatus::Failed => "failed", SubmissionStatus::Rejected => "rejected", }; - write!(f, "{}", s) + f.write_str(s) } }This avoids the formatting machinery entirely for a minor performance improvement.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/platform-server/src/models/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Clippy
- GitHub Check: Test
eureka928
force-pushed
the
refactor/tostring
branch
from
e35d481 to
3e1fe50
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/platform-server/src/models/mod.rs (1)
62-72: UseFormatter::write_strfor a simpler/leanerDisplayimpl.Current code is correct and keeps the exact wire strings, but
f.write_str(s)is more direct thanwrite!(f, "{}", s).Proposed tweak
impl std::fmt::Display for SubmissionStatus { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let s = match self { SubmissionStatus::Pending => "pending", SubmissionStatus::Evaluating => "evaluating", SubmissionStatus::Completed => "completed", SubmissionStatus::Failed => "failed", SubmissionStatus::Rejected => "rejected", }; - write!(f, "{}", s) + f.write_str(s) } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/platform-server/src/models/mod.rs
🧰 Additional context used
🧬 Code graph analysis (1)
crates/platform-server/src/models/mod.rs (1)
crates/epoch/src/lib.rs (1)
fmt(69-76)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
eureka928
force-pushed
the
refactor/tostring
branch
from
3e1fe50 to
f3e84ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/platform-server/src/api/evaluations.rs (1)
39-61: Consider checking the validator's active status.The code fetches the validator but only checks if they exist. The
Validatorstruct includes anis_activefield (visible in theget_validatorsnippet), but it's not being checked.Currently, inactive validators can still submit evaluations. If this is intentional, consider adding a comment explaining why. Otherwise, add a check:
♻️ Proposed enhancement to check validator active status
- if validator.is_none() { + match validator { + None => { + warn!( + "Evaluation submission from unregistered validator: {}", + &req.validator_hotkey[..16.min(req.validator_hotkey.len())] + ); + return Err(( + StatusCode::UNAUTHORIZED, + Json(serde_json::json!({ + "success": false, + "error": "Not a registered validator" + })), + )); + } + Some(v) if !v.is_active => { + warn!( + "Evaluation submission from inactive validator: {}", + &req.validator_hotkey[..16.min(req.validator_hotkey.len())] + ); + return Err(( + StatusCode::UNAUTHORIZED, + Json(serde_json::json!({ + "success": false, + "error": "Validator is not active" + })), + )); + } + Some(_) => { + // Validator is registered and active, proceed + } + }Alternatively, if inactive validators should be allowed, add a comment explaining the rationale.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/platform-server/src/api/evaluations.rs
🧰 Additional context used
🧬 Code graph analysis (1)
crates/platform-server/src/api/evaluations.rs (2)
crates/platform-server/src/api/auth.rs (1)
verify_signature(96-139)crates/platform-server/src/db/queries.rs (1)
get_validator(57-76)
🔇 Additional comments (3)
crates/platform-server/src/api/evaluations.rs (3)
1-96: Major inconsistency: PR objectives don't match the actual code changes.The PR title and description indicate this is a refactor to replace
ToStringwithDisplayfor theSubmissionStatusenum incrates/platform-server/src/models/mod.rs. However, the actual changes in this file add signature verification and validator registration checks to the evaluation submission endpoint—an entirely different scope.This suggests either:
- Wrong files were included in this PR
- The PR description is incorrect
- There's a git branch or merge issue
Please verify that the correct changes are being reviewed and update either the PR description or the included files accordingly.
3-3: LGTM: Imports are appropriate.The added imports for
verify_signatureand logging macros are correctly used throughout the file and follow good separation of concerns.Also applies to: 13-13
19-70: LGTM: Well-structured security implementation.The security checks follow best practices:
- Signature verification (authentication) happens before validator lookup (authorization)
- Both checks complete before any state mutations (
create_evaluation)- Early returns on security failures prevent further processing
- Appropriate status codes (401 for authentication failures)
- Security events are logged with properly truncated sensitive data
- Clear error messages for debugging without exposing internals
eureka928
force-pushed
the
refactor/tostring
branch
from
f3e84ae to
24a9cba
Compare
eureka928
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/epoch/src/commit_reveal.rs (1)
114-114: Remove or implement the unusedsmoothingparameter.The
smoothingparameter is accepted byfinalize()at both the instance method (line 114) and manager method (line 359) levels but is never used in the implementation. Thesmoothing_appliedfield in the result is hardcoded to0.0regardless of the parameter value. Either implement the smoothing logic or remove the parameter to avoid dead code.
🧹 Nitpick comments (1)
crates/epoch/src/commit_reveal.rs (1)
219-243: Add validation to prevent duplicate hotkeys in weight vectors or document the uniqueness requirement.The
weights_matchfunction builds a HashMap from the first weight vector, which means duplicate hotkeys would silently overwrite earlier values (last wins). If both vectors contain identical duplicates in different orders, the comparison would incorrectly detect divergence. While weight vectors should logically have unique hotkeys by design, this is not enforced. Consider either:
- Adding a deduplication step or validation that rejects vectors with duplicate hotkeys, or
- Explicitly documenting that weight vectors must have unique hotkeys and this is validated at the challenge/validator layer
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/epoch/src/commit_reveal.rs
🔇 Additional comments (5)
crates/epoch/src/commit_reveal.rs (5)
133-140: LGTM! Efficient counting without cloning.Counting reveals up-front instead of cloning all submissions is a clean optimization that reduces memory pressure.
153-167: LGTM! Reference-based divergence check achieves memory optimization.The refactor successfully reduces memory allocation from O(N * M) to O(M) by checking divergence using references and cloning only the first submission for the final output. The use of
reveal_countin error messages is consistent.
214-215: LGTM! Clear tolerance constant for floating-point comparison.The 0.1% tolerance is appropriate for comparing floating-point weight values and is well-documented.
245-262: LGTM! Clean reference-based divergence detection.The iterator-based approach correctly compares all submissions against the first without cloning. The function signature and return semantics are clear.
142-151: No action needed — HashMap iteration order is consistent within this instance.The code correctly uses the first reveal from
self.reveals.values().next()for final output and iterates the same HashMap without modification on line 156. While Rust's HashMap uses randomized seeding to prevent HashDoS attacks (so iteration order varies between runs), the order remains stable across multiple iterations of the same HashMap instance when unchanged. The divergence check on line 156 will correctly compare against the same first submission used on line 142-151.
2 participants
Summary
Reduces memory allocation in the commit-reveal weight validation by using references instead of cloning all weight vectors. Previously, this operation was O(N * M) memory where N = validators and M = miners.
Changes
File:
crates/epoch/src/commit_reveal.rsweights_match()helper function for comparing two weight vectorscheck_submission_divergence()function that accepts an iterator of referencesfinalize()to use references instead of cloning all weight vectorsCode Changes
New helper functions:
Performance Impact
Breaking Changes
None. Internal implementation change only.
Test Plan
Summary by CodeRabbit
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.
Contribution by Gittensor, see my contribution statistics at https://gittensor.io/miners/details?githubId=42954461