Subaccount challenge promotion

[sli-tao]

Taoshi Pull Request

Description

• improve logging
• use min(balance, equity) for promotion threshold

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 several changes to the challenge period promotion logic:

1. Uses min(balance, equity) for promotion threshold calculations
2. Improves logging by adding drawdown stats to log messages
3. Adds idle miner elimination functionality
4. Fixes bugs in unfilled orders handling and daily returns processing
5. Refactors _compute_portfolio_return to return both equity and balance returns

---

✅ Strengths

• Good defensive programming: Adding default drawdown stats cache entries when ledger data is unavailable prevents potential KeyErrors
• Practical bug fixes: The unfilled orders fix (line 504) and daily returns parsing fix (lines 1117-1119) address real issues
• Enhanced observability: Including drawdown stats in logs makes debugging easier
• Conservative promotion logic: Using min(balance, equity) prevents promoting miners with unrealized gains

---

⚠️ Concerns

CRITICAL: Hardcoded timestamp

Lines: 773 in elimination_manager.py

IDLE_ELIMINATION_ACTIVATION_MS = 1774310400000 + ValiConfig.IDLE_MINER_MAXIMUM_MS

• Magic number 1774310400000 (allegedly 2026-03-20) is hardcoded
• No validation that this timestamp is correct
• Should use a named constant with clear documentation
• Consider using datetime for readability: datetime(2026, 3, 20).timestamp() * 1000

CRITICAL: Return type annotation inconsistency

Line 433:

def _compute_portfolio_return(self, hotkey: str, account: Optional[dict] = None) -> tuple[float, float] | None:

• Using modern union syntax tuple[float, float] | None but mixing with Optional[dict]
• Should be consistent: either use Optional[tuple[float, float]] or dict | None for both
• Affects Python 3.9 compatibility if not using from __future__ import annotations

HIGH: Incomplete error handling

Lines 440-445, 522:

if current_return is None:
    continue

• When _compute_portfolio_return returns None, it silently continues without logging
• Could hide issues with account data or position client failures
• Should log at least a debug message explaining why the miner was skipped

HIGH: Tuple unpacking with ignored value

Line 671:

current_return, _ = self._compute_portfolio_return(hotkey, accounts.get(hotkey))

• In funded evaluation, balance_return is computed but not used
• This is intentional (promotion check doesn't apply to funded), but could cause confusion
• Add a comment explaining why balance_return is unused here

MEDIUM: Hardcoded test data left in production

Lines 986-988 in position_manager.py:

miners_to_wipe = []
position_uuids_to_delete = []
position_uuids_to_archive = []
miners_to_promote = []

• Previous values were commented out/removed, but the pattern suggests this was for testing
• These variables are only initialized but never populated in the shown code
• Verify this isn't dead code that should be removed

MEDIUM: Inefficient nested loop

Lines 795-800 in elimination_manager.py:

for position in positions:
    for order in position.orders:
        if last_order_ms is None or order.processed_ms > last_order_ms:
            last_order_ms = order.processed_ms

• For miners with many positions/orders, this could be slow
• Consider using max() with a generator expression:

  all_order_times = (order.processed_ms for position in positions for order in position.orders)
  last_order_ms = max(all_order_times, default=None)

LOW: Missing format string separator

Lines 581-583:

f"returns {returns_percentage:.2f}% >= {returns_threshold}%"
f"balance {accounts.get(hotkey).get('balance')}, unrealized_pnl {self._position_client.get_unrealized_pnl(hotkey)}"
f"drawdown stats: {self._drawdown_stats_cache[hotkey]}"

• Missing spaces/newlines between concatenated f-strings
• Will produce unreadable log output
• Should add ", " or " | " separators

---

💡 Suggestions

Code Quality

1. Extract magic numbers to constants:

   NEAR_ELIMINATION_THRESHOLD_FACTOR = 0.75
   NEAR_PROMOTION_THRESHOLD_FACTOR = 0.75

   Used on lines 597, 723

2. Add docstring update for _compute_portfolio_return:

   """Compute current portfolio return metrics.
   
   Returns:
       tuple[float, float] | None: (equity_return, balance_return) where
           equity_return = (balance + unrealized_pnl) / account_size
           balance_return = balance / account_size
       Returns None if account data is unavailable.
   """

3. Simplify eod_hwm calculation (line 471):

   eod_hwm = max(1.0, *(cp.equity_ret for cp in midnight_cps))

   Current implementation calls max() twice unnecessarily.

4. Add type hints for cache structure:

   DrawdownStats = TypedDict('DrawdownStats', {
       'current_equity': float,
       'daily_open_equity': float,
       # ... other fields
   })

   This would make the cache structure self-documenting.

5. Extract repeated drawdown stats dict to a helper method:

   def _create_default_drawdown_stats(self, intraday_threshold: float, eod_threshold: float) -> dict:
       return {
           'current_equity': 1.0,
           'daily_open_equity': 1.0,
           # ... rest of fields
       }

   Used in multiple places (lines 525-537, 652-666, etc.)

Testing Considerations

1. Add unit tests for:

   • min(balance, equity) logic with various scenarios (positive/negative unrealized PnL)
   • Idle miner detection edge cases (no positions, no orders, exactly at threshold)
   • Promotion logic when equity > balance vs balance > equity

2. Integration test scenarios:

   • Miner with high unrealized profit shouldn't be promoted if balance is low
   • Miner at exactly 60 days idle should/shouldn't be eliminated
   • Proper handling when position client returns None/empty data

Performance

1. Batch position queries: The idle miner check calls get_positions_for_hotkeys with all candidates at once (good!), but verify this doesn't cause memory issues with large datasets.

2. Cache drawdown stats: Already implemented, but ensure cache invalidation is correct when account state changes.

---

🔒 Security Notes

MEDIUM: Potential for promoting miners with manipulated unrealized PnL

• The change to use min(balance, equity) is good security practice
• However, if unrealized PnL can be manipulated (e.g., by placing orders in illiquid markets), a miner could still game the system by withdrawing while showing high balance
• Consider additional validation: check that balance hasn't decreased significantly right before promotion

LOW: Account data access without validation

Line 582:

f"balance {accounts.get(hotkey).get('balance')}, unrealized_pnl {self._position_client.get_unrealized_pnl(hotkey)}"

• accounts.get(hotkey) could return None, causing AttributeError
• Should use accounts.get(hotkey, {}).get('balance', 0)

LOW: Division by zero potential

Line 448:

balance_ret = balance / account_size

• If account_size is 0, this will raise ZeroDivisionError
• The check on line 441 validates account_size, but it checks account.get('account_size') while this uses the extracted account_size variable
• Should add explicit zero check: if account_size <= 0: return None

---

📋 Action Items

Must Fix Before Merge:

1. ✅ Fix log message formatting (missing separators) on lines 581-583
2. ✅ Replace hardcoded timestamp with named constant/datetime
3. ✅ Fix potential None.get() error on line 582
4. ✅ Add consistent type hints (PEP 604 vs Optional)

Should Fix:
5. ⚠️ Add logging when current_return is None (helps debugging)
6. ⚠️ Extract repeated drawdown stats dict to helper method (DRY principle)
7. ⚠️ Optimize idle miner order time calculation

Nice to Have:
8. 💭 Add comprehensive unit tests for new promotion logic
9. 💭 Document the rationale for using min(balance, equity) in code comments
10. 💭 Consider adding metrics/alerts when miners are near promotion/elimination thresholds

---

Final Recommendation

🟡 CONDITIONAL APPROVAL - The core logic changes are sound and address real issues, but there are several bugs that must be fixed before merging. The min(balance, equity) change is a good security improvement. Fix the critical items above, and this will be good to merge.