Remove per trade pair perf ledgers

[sli-tao]

Taoshi Pull Request

Description

• Remove per trade pair perf ledgers
• Remove asset segmentation

Now that miners select an asset class, we may simply compute perf ledgers for their overall portfolio. The need to segment asset classes is deprecated.

• perf_ledger.py — removed TP_ID_PORTFOLIO, tp_id field, last_known_prices tp_id validation
• perf_ledger_manager.py — collapsed Dict[str, Dict[str, PerfLedger]] → Dict[str, PerfLedger]; removed build_portfolio_ledgers_only; inline disk migration for old V2 bundle format; removed all [TP_ID_PORTFOLIO] accesses
• perf_ledger_server.py / perf_ledger_client.py — removed portfolio_only param from all RPC methods
• scoring.py — score_miners() now works on flat dict[str, PerfLedger]; AssetSegmentation removed
• penalty_ledger.py — ASSET_LEDGER enum removed; risk_adjusted_performance changed to PenaltyInputType.LEDGER (uses portfolio ledger directly)
• position_penalties.py — risk_adjusted_performance_penalty() kept
• challengeperiod_manager.py, debt_ledger_manager.py, emissions_ledger.py, elimination_manager.py, miner_statistics_manager.py, metrics.py, ledger_utils.py, core_outputs_manager.py, backtest_manager.py — all updated
• asset_segmentation.py — deleted
• test_asset_segmentation.py — deleted
• All test files updated to use flat PerfLedger instead of bundle dicts

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 removes per-trade-pair performance ledgers in favor of portfolio-level tracking only. The refactoring collapses a nested dictionary structure (Dict[str, Dict[str, PerfLedger]]) to a flat structure (Dict[str, PerfLedger]), removes asset segmentation logic, and updates all dependent code. This is a significant architectural change that simplifies the codebase by eliminating the distinction between portfolio and individual trade-pair tracking.

---

✅ Strengths

• Clear intent: The refactoring has a well-defined goal of simplifying the ledger structure now that miners select asset classes
• Comprehensive coverage: The changes propagate through the entire codebase systematically
• Test updates: Test files have been updated to match the new structure
• Removes complexity: Eliminates the AssetSegmentation class and associated complexity
• Consistent patterns: The flat dictionary structure is applied uniformly across all modules

---

⚠️ Concerns

CRITICAL: Data Migration Strategy

# runnable/local_debt_ledger.py:366
# No build_portfolio_ledgers_only parameter removed

• Issue: The PR mentions "inline disk migration for old V2 bundle format" in perf_ledger_manager.py, but the diff doesn't show this migration logic
• Impact: Existing validators with persisted performance ledgers in the old format may fail to load data
• Recommendation: Ensure migration logic exists to handle the transition from Dict[str, Dict[str, PerfLedger]] to Dict[str, PerfLedger] on disk

CRITICAL: Breaking Changes Not Fully Documented

# Multiple files show parameter removals without deprecation warnings
def initialize_components(hotkeys, parallel_mode, running_unit_tests=False, skip_port_kill=False):

• Issue: The build_portfolio_ledgers_only parameter is removed without deprecation path
• Impact: Any external code or scripts using this parameter will break immediately
• Recommendation: Consider a deprecation warning or document breaking changes clearly in release notes

HIGH: Loss of Granular Trade-Pair Metrics

# tests/vali_tests/test_dynamic_minimum_days_robust.py
# Portfolio-level tracking means all trade pairs count equally

• Issue: The removal of per-trade-pair ledgers means you can no longer track performance metrics at the individual trade pair level
• Impact:
  • Cannot identify which specific pairs a miner is strong/weak in
  • Cannot apply different minimum day requirements per asset class accurately
  • Loss of diagnostic information for debugging miner behavior
• Question: Is this intentional? The description says "miners select an asset class" but doesn't explain why portfolio-level is sufficient

MEDIUM: Test Coverage Gaps

# tests/vali_tests/test_asset_segmentation.py - deleted

• Issue: An entire test file with 483 lines is deleted without replacement tests for the new portfolio-level logic
• Impact: Loss of test coverage for edge cases that may still be relevant
• Recommendation: Review deleted tests and ensure critical scenarios are still covered elsewhere

MEDIUM: Inconsistent Error Handling

# tests/vali_tests/test_ledger_utils.py:668-675
def test_is_valid_trading_day_portfolio_always_valid(self):
    # Default (non-portfolio) also returns True
    self.assertTrue(LedgerUtils.is_valid_trading_day(ledger, saturday_date))

• Issue: Tests suggest portfolio ledgers "always return True" but error handling tests show None ledger returns False
• Impact: Unclear contract for the function - when does it return False?
• Recommendation: Document the function contract more clearly

---

💡 Suggestions

1. Add Data Validation at Boundaries

# Suggested addition to perf_ledger_manager.py or similar
def validate_ledger_structure(ledgers: Dict[str, Any]) -> bool:
    """Validate that ledgers are in the new flat format, not nested."""
    for hotkey, ledger in ledgers.items():
        if isinstance(ledger, dict):
            raise ValueError(
                f"Ledger for {hotkey} is in old nested format. "
                "Please run migration script."
            )
        if not isinstance(ledger, PerfLedger):
            raise TypeError(f"Expected PerfLedger, got {type(ledger)}")
    return True

2. Consider Preserving Trade-Pair Metrics Separately

If diagnostic capabilities are important, consider:

# Optional: Store aggregated metrics per trade pair separately
class MinerMetrics:
    portfolio_ledger: PerfLedger
    trade_pair_stats: Dict[str, TradePairStats]  # Lightweight stats, not full ledgers

3. Add Migration Helper Function

def migrate_v2_to_v3_ledgers(old_ledgers: Dict[str, Dict[str, PerfLedger]]) -> Dict[str, PerfLedger]:
    """Migrate from nested (v2) to flat (v3) ledger structure."""
    new_ledgers = {}
    for hotkey, bundles in old_ledgers.items():
        # Use portfolio ledger if exists, otherwise aggregate
        new_ledgers[hotkey] = bundles.get(TP_ID_PORTFOLIO) or aggregate_ledgers(bundles)
    return new_ledgers

4. Improve Documentation

# tests/vali_tests/test_dynamic_minimum_days_robust.py:89
def create_portfolio_ledger_dict(
    self,
    miner_participation: Dict[str, Dict[str, int]]
) -> Dict[str, PerfLedger]:
    """
    Create a portfolio-level ledger dictionary where each miner has a single PerfLedger.
    The portfolio ledger uses the max participation days across all trade pairs.
    
    Note: With portfolio-level tracking (post-refactor), individual trade pair
    participation is no longer tracked separately. This affects minimum day
    calculations and scoring granularity.
    """

5. Performance Consideration

# backtest_manager.py:314
def validate_last_update_ms(self, prev_end_time_ms):
    perf_ledger_bundles = self.perf_ledger_client.get_perf_ledgers()
    for hk, ledger in perf_ledger_bundles.items():  # O(n) iteration

• The flat structure is more efficient than nested loops
• Good change: Reduces time complexity from O(n*m) to O(n)

---

🔒 Security Notes

LOW: No Major Security Issues Identified

• The changes are primarily structural/architectural
• No new external inputs or API endpoints introduced
• Data validation should be added as noted above

MEDIUM: Data Integrity During Migration

• Risk: If migration fails partially, validators could have inconsistent state
• Mitigation: Add atomic migration with rollback capability

def safe_migrate_ledgers(ledger_path: str):
    """Atomically migrate ledgers with backup."""
    backup_path = f"{ledger_path}.backup"
    try:
        shutil.copy(ledger_path, backup_path)
        # Perform migration
        # Validate new format
    except Exception as e:
        shutil.copy(backup_path, ledger_path)  # Rollback
        raise

---

🎯 Action Items

Before Merge:

1. ✅ CRITICAL: Verify disk migration logic exists and is tested for the V2→V3 transition
2. ✅ CRITICAL: Update release notes/Discord announcement about breaking changes
3. ⚠️ HIGH: Document the rationale for removing per-trade-pair tracking (why is portfolio-level sufficient?)
4. ⚠️ MEDIUM: Add migration helper script/function that validators can run
5. ⚠️ MEDIUM: Review deleted test cases from test_asset_segmentation.py and ensure critical scenarios are covered

Nice to Have:
6. 💡 Add data validation at module boundaries
7. 💡 Improve inline documentation explaining the new architecture
8. 💡 Consider adding lightweight trade-pair statistics if diagnostic info is valuable

---

📋 Checklist Review

The PR checklist items are mostly unchecked. Before merging:

• Test on testnet - Critical for this scale of change
• Update documentation - docs/validator_rest_server.md has only a typo fix, needs architecture docs
• Notify community in Discord - Breaking changes mentioned in description
• Migration guide - Not mentioned but needed

---

Final Verdict

This is a well-executed refactoring that simplifies the codebase, but it requires careful attention to migration and documentation before deployment. The removal of per-trade-pair granularity is a significant architectural decision that should be validated against product requirements.

Recommendation: Request clarification on migration strategy and trade-pair tracking rationale before approval. Once those are addressed, this is a solid improvement to code maintainability.