Fix risk adjusted performance penalty

[ward-taoshi]

Taoshi Pull Request

Description

[Provide a brief description of the changes introduced by this pull request.]

Related Issues (JIRA)

[Reference any related issues or tasks that this pull request addresses or closes.]

Checklist

• I have tested my changes on testnet.
• I have updated any necessary documentation.
• I have added unit tests for my changes (if applicable).
• If there are breaking changes for validators, I have (or will) notify the community in Discord of the release.

Reviewer Instructions

[Provide any specific instructions or areas you would like the reviewer to focus on.]

Definition of Done

• Code has been reviewed.
• All checks and tests pass.
• Documentation is up to date.
• Approved by at least one reviewer.

Checklist (for the reviewer)

• Code follows project conventions.
• Code is well-documented.
• Changes are necessary and align with the project's goals.
• No breaking changes introduced.

Optional: Deploy Notes

[Any instructions or notes related to deployment, if applicable.]

/cc @mention_reviewer

[github-actions]

🤖 Claude AI Code Review

Last reviewed on: 14:23:47

---

Summary

This PR makes three main changes: (1) adds temporary handling for Tiingo API rate limiting (429 responses), (2) modifies risk-adjusted performance penalty calculation by re-enabling weighting, and (3) simplifies asset ledger penalty calculation by removing subcategory iteration logic in favor of direct asset class evaluation.

---

✅ Strengths

• The simplification in penalty_ledger.py (lines 901-908) removes unnecessary complexity and makes the code more maintainable
• Version bump is included, indicating proper release management
• The change to direct asset class lookup (get_asset_selection() instead of get_asset_selections().get()) is cleaner

---

⚠️ Concerns

Critical Issues

1. Silent Failure on Rate Limiting (tiingo_data_service.py lines 481-482, 587-588)

   • Using continue on 429 responses silently skips data without logging or retry logic
   • This could lead to incomplete data sets and difficult-to-diagnose issues
   • No backoff/retry mechanism means the service will hammer the API repeatedly
   • Impact: Data integrity issues, potential for complete data loss during rate limit periods

2. Breaking Logic Change Without Justification (penalty_ledger.py lines 898-908)

   • The removal of subcategory penalty aggregation is a significant algorithmic change
   • Old logic: averaged penalties across all subcategories within an asset class
   • New logic: only evaluates the asset class directly
   • No explanation in PR description or comments about why this change is necessary
   • Could dramatically affect scoring/penalty calculations
   • Impact: This could be a breaking change for validators

3. Uncommented Parameter Change (position_penalties.py line 220)

   • Re-enabling weighting=True without explanation
   • Comment says "discuss scoring weighting" but appears undiscussed
   • Could significantly alter performance calculations
   • Impact: Changes miner scoring behavior

Major Issues

4. Typo in TODO Comment (tiingo_data_service.py)

   • "badwidth" should be "bandwidth"
   • Suggests TODO is temporary but no ticket reference or timeline

5. Missing Error Context

   • The 429 handling provides no visibility into how often rate limiting occurs
   • No metrics, no warnings, no way to monitor the impact

---

💡 Suggestions

For Tiingo Rate Limiting (Priority: High)

# Replace silent continue with proper handling
if requestResponse.status_code == 429:
    bt.logging.warning(f"Tiingo API rate limit hit for batch. Retrying after delay...")
    time.sleep(1)  # Or exponential backoff
    # Consider: add to retry queue instead of continue
    continue

Or better yet, implement proper retry logic:

from requests.adapters import HTTPAdapter
from requests.packages.urllib3.util.retry import Retry

# Add retry strategy with backoff
retry_strategy = Retry(
    total=3,
    status_forcelist=[429, 500, 502, 503, 504],
    backoff_factor=1
)

For Penalty Calculation Change (Priority: Critical)

• Add detailed comment explaining why subcategory logic was removed
• Include unit tests comparing old vs new behavior
• Document the expected impact on penalty values
• Consider feature flag to gradually roll out this change

For Weighting Parameter (Priority: High)

• Document why weighting is being re-enabled
• Add comment referencing discussion or ticket
• Include test cases that verify expected behavior with weighting enabled

General Improvements

# Line 901: Add null safety
asset_class = self._asset_selection_client.get_asset_selection(miner_hotkey)
if not asset_class:
    bt.logging.debug(f"No asset class found for miner {miner_hotkey}")
    penalty_value = 0

---

🔒 Security Notes

1. No Direct Security Vulnerabilities - No SQL injection, XSS, or authentication issues
2. Rate Limiting Bypass Concern - The silent continue on 429 could be seen as attempting to bypass rate limits, though unintentional. Proper backoff is more respectful to the API provider
3. Data Integrity Risk - Silent failures could lead to incomplete datasets used in financial calculations

---

📋 Additional Comments

1. Testing Requirements (from PR checklist - unchecked)

   • ❌ No unit tests mentioned for the penalty calculation change
   • ❌ No testnet testing confirmation
   • This is concerning given the algorithmic changes

2. Documentation Requirements (from PR checklist - unchecked)

   • ❌ No documentation updates
   • The penalty calculation change should be documented

3. Breaking Changes Notification (from PR checklist - unchecked)

   • The penalty calculation change could be breaking for validators
   • Discord notification should be considered

4. PR Description Issues

   • Template placeholders not filled in
   • No description of actual changes
   • No related JIRA issues
   • No reviewer instructions

---

🎯 Recommendation

DO NOT MERGE without addressing:

1. Add proper logging/retry for 429 responses
2. Provide justification and documentation for the penalty calculation algorithm change
3. Add unit tests demonstrating the penalty calculation behaves as expected
4. Fill out PR description with actual context
5. Confirm whether this is a breaking change requiring community notification
6. Add comment explaining why weighting=True was re-enabled

The penalty calculation change appears to be the actual "fix" referenced in the PR title, but without context on what was broken and why this fixes it, it's impossible to properly review.