Add Weight Submission Divergence Validation #16
Conversation
📝 WalkthroughWalkthroughThe PR introduces divergence validation during weight submission finalization in the commit-reveal module. A new helper function detects inconsistencies in hotkeys and weights across submissions with a 0.1% tolerance threshold. When divergence is detected, an error is logged and the first submission is used for aggregation instead of proceeding without validation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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: 2
🤖 Fix all issues with AI agents
In @crates/epoch/src/commit_reveal.rs:
- Around line 144-157: The current code picks the baseline submission from
submissions collected via self.reveals.values(), which is non-deterministic; to
fix, build submissions deterministically (e.g., collect (validator_hotkey,
submission) pairs from self.reveals.iter(), sort by validator_hotkey) and then
use the first element of that sorted list as the baseline before calling
check_submission_divergence and weights::normalize_weights; update the logic
around submissions, divergence detection (check_submission_divergence) and the
aggregated = weights::normalize_weights(...) call to use the deterministically
chosen submission.
- Around line 202-262: The function check_submission_divergence assumes hotkeys
are unique within each Vec<WeightAssignment> but doesn't validate that; add a
defensive duplicate-hotkey check at the start of the function (before comparing
to the first submission) that iterates each submission Vec<WeightAssignment>,
builds a HashSet of hotkey strings, and if a duplicate is encountered logs a
warn (include self.epoch and a truncated hotkey similar to existing messages)
and returns true to signal divergence; keep the rest of
check_submission_divergence logic unchanged.
🧹 Nitpick comments (2)
crates/epoch/src/commit_reveal.rs (2)
9-9: Remove unused HashSet import.The
HashSettype is imported but never used in this file. The divergence check uses onlyHashMap.♻️ Proposed fix
-use std::collections::{HashMap, HashSet}; +use std::collections::HashMap;
217-218: Document the tolerance rationale.The 0.1% tolerance for weight differences is reasonable, but consider documenting why this specific threshold was chosen to help future maintainers understand the tradeoff between strictness and floating-point precision.
📝 Suggested documentation
-// Tolerance for floating-point comparison (0.1% difference allowed) +// Tolerance for floating-point comparison (0.1% difference allowed) +// This threshold accounts for minor floating-point arithmetic variations +// while still detecting meaningful weight divergence between validators const WEIGHT_TOLERANCE: f64 = 0.001;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/epoch/src/commit_reveal.rs
🧰 Additional context used
🧬 Code graph analysis (1)
crates/epoch/src/commit_reveal.rs (1)
crates/challenge-sdk/src/weights.rs (1)
normalize_weights(36-46)
⏰ 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: Test
- GitHub Check: Clippy
🔇 Additional comments (1)
crates/epoch/src/commit_reveal.rs (1)
239-239: LGTM: Safe string truncation for logging.The hotkey truncation using
.min(weight.hotkey.len())safely prevents panics while keeping logs readable.Also applies to: 249-249
| // Validate that all submissions are consistent | ||
| // All validators read from shared chain DB, so submissions should be identical | ||
| // Just take the first one and normalize | ||
| let aggregated = submissions | ||
| .into_iter() | ||
| .next() | ||
| .map(|w| weights::normalize_weights(w)) | ||
| .unwrap_or_default(); | ||
| let first = &submissions[0]; | ||
| let divergence_detected = self.check_submission_divergence(&submissions); | ||
|
|
||
| if divergence_detected { | ||
| error!( | ||
| "Epoch {}: Weight submissions diverged across {} validators! Using first submission.", | ||
| self.epoch, | ||
| submissions.len() | ||
| ); | ||
| } | ||
|
|
||
| let aggregated = weights::normalize_weights(first.clone()); |
There was a problem hiding this comment.
Critical: Non-deterministic "first submission" selection undermines validation.
The PR objectives state that non-deterministic HashMap iteration order is a problem being addressed, but lines 134-135 collect submissions from self.reveals.values(), which iterates a HashMap in non-deterministic order. This means:
- Which submission is considered "first" (the baseline) varies between runs
- Different validators' submissions may be used as the baseline in different executions
- If validator A and validator B submit different weights, sometimes A's weights will be used (when A is "first"), and sometimes B's will be used
This makes the divergence detection inconsistent and doesn't solve the stated problem of non-deterministic behavior.
🔧 Recommended fix
Use a deterministic ordering for submissions, such as sorting by validator hotkey:
// Collect valid submissions
-let submissions: Vec<Vec<WeightAssignment>> =
- self.reveals.values().map(|r| r.weights.clone()).collect();
+let mut reveals_sorted: Vec<_> = self.reveals.iter().collect();
+reveals_sorted.sort_by(|a, b| a.0.cmp(b.0)); // Sort by hotkey
+let submissions: Vec<Vec<WeightAssignment>> =
+ reveals_sorted.iter().map(|(_, r)| r.weights.clone()).collect();This ensures the same validator's submission is always used as the baseline when present.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @crates/epoch/src/commit_reveal.rs around lines 144 - 157, The current code
picks the baseline submission from submissions collected via
self.reveals.values(), which is non-deterministic; to fix, build submissions
deterministically (e.g., collect (validator_hotkey, submission) pairs from
self.reveals.iter(), sort by validator_hotkey) and then use the first element of
that sorted list as the baseline before calling check_submission_divergence and
weights::normalize_weights; update the logic around submissions, divergence
detection (check_submission_divergence) and the aggregated =
weights::normalize_weights(...) call to use the deterministically chosen
submission.
| /// Check if submissions from different validators have diverged. | ||
| /// Returns true if divergence is detected. | ||
| fn check_submission_divergence(&self, submissions: &[Vec<WeightAssignment>]) -> bool { | ||
| if submissions.len() <= 1 { | ||
| return false; | ||
| } | ||
|
|
||
| let first = &submissions[0]; | ||
|
|
||
| // Build a map of hotkey -> weight for the first submission | ||
| let first_weights: HashMap<&str, f64> = first | ||
| .iter() | ||
| .map(|w| (w.hotkey.as_str(), w.weight)) | ||
| .collect(); | ||
|
|
||
| // Tolerance for floating-point comparison (0.1% difference allowed) | ||
| const WEIGHT_TOLERANCE: f64 = 0.001; | ||
|
|
||
| for (idx, submission) in submissions.iter().enumerate().skip(1) { | ||
| // Check if same number of weight assignments | ||
| if submission.len() != first.len() { | ||
| warn!( | ||
| "Epoch {}: Submission {} has {} weights, first has {}", | ||
| self.epoch, | ||
| idx, | ||
| submission.len(), | ||
| first.len() | ||
| ); | ||
| return true; | ||
| } | ||
|
|
||
| // Check if same hotkeys with similar weights | ||
| for weight in submission { | ||
| match first_weights.get(weight.hotkey.as_str()) { | ||
| None => { | ||
| warn!( | ||
| "Epoch {}: Submission {} has hotkey {} not in first submission", | ||
| self.epoch, idx, &weight.hotkey[..16.min(weight.hotkey.len())] | ||
| ); | ||
| return true; | ||
| } | ||
| Some(&first_weight) => { | ||
| let diff = (weight.weight - first_weight).abs(); | ||
| if diff > WEIGHT_TOLERANCE { | ||
| warn!( | ||
| "Epoch {}: Weight divergence for hotkey {}: {} vs {} (diff: {:.4})", | ||
| self.epoch, | ||
| &weight.hotkey[..16.min(weight.hotkey.len())], | ||
| first_weight, | ||
| weight.weight, | ||
| diff | ||
| ); | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| false | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for uniqueness validation on WeightAssignment collections
echo "=== Searching for uniqueness/duplicate checks ==="
rg -n -C3 'unique|duplicat|distinct' --type=rust crates/epoch/src/
echo -e "\n=== Checking WeightAssignment struct definition ==="
rg -n -C5 'struct WeightAssignment' --type=rust
echo -e "\n=== Looking for deduplication logic ==="
rg -n '\.dedup\(\)' --type=rust crates/epoch/src/
echo -e "\n=== Searching for weight validation or collection building ==="
rg -n -C3 'WeightAssignment|submissions' --type=rust crates/epoch/src/commit_reveal.rs | head -100Repository: PlatformNetwork/platform
Length of output: 4985
Document the uniqueness assumption for hotkeys within submissions or add validation.
The divergence detection logic assumes hotkeys are unique within each submission. While no explicit validation exists in the codebase, this assumption is reasonable given that submissions all derive from the shared chain DB (as noted in the code comment). However, the check would fail to detect certain divergences if duplicate hotkeys ever occurred (e.g., first has [A, A, B] and another has [A, B, B]—both pass the length and existence checks).
Consider either:
- Adding a comment explicitly documenting that hotkeys must be unique per submission, or
- Adding a defensive check to validate this assumption during submission processing.
🤖 Prompt for AI Agents
In @crates/epoch/src/commit_reveal.rs around lines 202 - 262, The function
check_submission_divergence assumes hotkeys are unique within each
Vec<WeightAssignment> but doesn't validate that; add a defensive
duplicate-hotkey check at the start of the function (before comparing to the
first submission) that iterates each submission Vec<WeightAssignment>, builds a
HashSet of hotkey strings, and if a duplicate is encountered logs a warn
(include self.epoch and a truncated hotkey similar to existing messages) and
returns true to signal divergence; keep the rest of check_submission_divergence
logic unchanged.
|
@echobt I'd love to get your review and approval |
2 participants
Summary
Adds validation to detect when validators submit different weights during the commit-reveal finalization phase. Previously, the system silently used only the first submission, potentially masking consensus issues.
Problem
File:
crates/epoch/src/commit_reveal.rsThe
finalize()function assumed all validator submissions were identical:Issues:
Solution
Added
check_submission_divergence()method that compares all submissions against the first:Validation checks:
Logging:
warn!for each specific divergence with detailserror!summary when finalizing with divergenceBehavior: Still uses first submission to avoid breaking production. Divergence indicates a bug or network issue requiring investigation.
Changes
check_submission_divergence()helper methodfinalize()to call validation before aggregationHashSetimport anderrorlog levelSummary by CodeRabbit
✏️ 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