feat: query multiple beacon nodes in parallel

[petarjuki7]

Issue Addressed

WAD - Weighted Attestation Data

Proposed Changes

• Added parallel fetching from all configured beacon nodes instead of sequential fallback
• Implemented scoring algorithm based on checkpoint epochs + head slot proximity
• Added dual timeout strategy: soft timeout at 500ms (return if we have responses), hard timeout at 1s (return unconditionally)

The implementation uses FuturesUnordered to query all beacon nodes concurrently and selects the highest-scoring attestation data. Scoring works by summing source and target epochs (base score) plus a proximity bonus based on how close the head slot is to the attestation slot.

Additional Info

[claude-code-actions-sigp]

Claude finished @petarjuki7's task —— View job

---

Code Review Complete

I've completed a comprehensive review of the parallel beacon node querying implementation. While the approach is sound, there are several critical issues that need to be addressed before this can be merged.

Critical Issues

1. Compilation Error (Line 193): The futures mapping captures self incorrectly, which will cause compilation issues. The async blocks need proper handling of the self reference.

2. Unused Return Value (Line 177): The weighted_calculation function returns attestation data, but the caller discards it and uses the fallback path instead. This makes the entire weighted calculation pointless in its current form.

3. Feature Disabled (Line 91): The feature is hardcoded to false, making all this code unreachable. This should be a configuration parameter.

Design Concerns

4. Scoring Algorithm Issues (Line 376):

   • Base scores (sum of epochs ~400k) completely dwarf the proximity bonus (~1.0)
   • Multiple attestations with same epochs but different roots get identical scores
   • No validation of checkpoint consistency or majority agreement

5. Error Handling (Line 93): Weighted calculation errors are ignored and execution falls through to the old path, masking potential issues.

6. Performance: The 250ms block lookup timeout per client (line 426) can accumulate to significant delays when querying many nodes.

Minor Issues

7. Magic Numbers (Line 20): Timeout constants lack documentation explaining the rationale
8. Race Condition (Line 259): Minor inefficiency in response counting logic

Testing Gaps

• No tests for the new functionality
• No tests for timeout behavior (soft vs hard)
• No tests for scoring algorithm edge cases
• No tests for error conditions (all nodes failing, some nodes timing out, etc.)

Recommendations

Before merging:

1. Fix the compilation error - restructure the futures to properly handle self
2. Integrate weighted_calculation properly - either use its return value or change the flow
3. Make feature configurable - add proper configuration management
4. Revise scoring algorithm - normalize scores or weight the bonus more significantly
5. Add comprehensive tests - especially for the scoring logic and timeout behavior
6. Implement safety checks - add the majority agreement validation mentioned in your TODO
7. Add metrics - track which beacon node was selected (as mentioned in your TODO)

The parallel querying approach is a solid improvement over sequential fallback, but the implementation needs refinement before it's production-ready.

[claude-code-actions-sigp]

Magic Numbers: These timeout values should be documented or made configurable. Consider:

1. Why is soft timeout exactly 500ms?
2. How do these values relate to slot timing (typically 12s)?
3. What happens if beacon nodes are consistently slower than 500ms but faster than 1s?

Add documentation explaining the rationale, or make these configurable based on network conditions.

[claude-code-actions-sigp]

Scoring Algorithm Concerns:

1. Epoch Collision: Multiple attestation data with different roots but same source/target epochs get identical base scores. This could select suboptimal data.

2. Bonus Weighting: The proximity bonus (1.0 / (1 + distance)) is tiny compared to base score. For example:

   • Base score: ~400,000 (epoch 200,000)
   • Max bonus: 1.0

   The bonus is essentially negligible and won't affect selection.

3. Missing Safety Checks: No validation that source < target, or that checkpoints are reasonable.

Consider:

• Normalizing scores or making bonus more significant
• Adding validation of attestation data consistency
• Implementing majority agreement checking (as mentioned in TODO)

[claude-code-actions-sigp]

Unbounded Timeout: If the block lookup hangs or is very slow, this adds 250ms to every attestation fetch. Consider:

1. Using a shared timeout for all block lookups
2. Caching block slot lookups
3. Making block lookup optional with a fast-fail path

The 250ms timeout per client could accumulate to significant delays when querying many nodes.

[claude-code-actions-sigp]

Claude finished @petarjuki7's task in 4m 21s —— View job

---

Code Review Complete

I've completed a comprehensive review of the parallel beacon node querying implementation. The PR adds good functionality, but there are several critical issues that must be addressed before merging.

---

Critical Issues

1. Compilation Error - Async Closure Captures self Incorrectly (Line 193-199)

The futures mapping captures self incorrectly, which will cause compilation failures. The async blocks each try to capture &self, but they're moved into FuturesUnordered.

Problem:

let mut futures: FuturesUnordered<_> = clients
    .into_iter()
    .map(|(addr, client)| async move {
        let result = self.fetch_and_score(&client, slot).await;
        (addr, result)
    })
    .collect();

Why this fails: Each async block needs to capture self to call self.fetch_and_score(), but multiple async blocks can't all capture the same &self when moved into the collection.

Solution: Restructure to properly handle the self reference. Consider:

• Making the async blocks clone what they need from self
• Restructuring to avoid capturing self in the closures
• Using Arc or similar patterns if MetadataService can be cloned

2. Scoring Algorithm Ineffective (Line 376-406)

The scoring algorithm has a fundamental flaw that makes the proximity bonus meaningless:

• Base score: Sum of source + target epochs (~400,000 for epoch 200,000)
• Proximity bonus: 1.0 / (1 + distance) (maximum value: 1.0)

The bonus is 0.00025% of the base score, making it completely negligible. Multiple attestations with identical epochs but different roots will get identical scores, potentially selecting suboptimal data.

Recommendations:

• Normalize scores to make the bonus meaningful
• Weight the proximity bonus more significantly (e.g., multiply by 10,000)
• Add validation that attestations have consistent checkpoints
• Implement the majority agreement validation mentioned in the TODO

3. Missing Test Coverage (No tests found)

There are no tests for this critical new functionality. This is a serious gap for production code.

Required tests:

• Parallel query success/failure scenarios
• Scoring algorithm correctness and edge cases
• Timeout behavior (soft vs hard)
• Error handling (all nodes fail, partial failures)
• Score comparison logic
• Block lookup timeout handling

---

Design Concerns

4. Feature Requires Manual Enable Flag (Line 94)

The feature requires users to explicitly enable it via --with-weighted-attestation-data. This is good for gradual rollout, but:

• No documentation about when to use this flag
• No validation that multiple beacon nodes are configured
• No metrics to track which beacon node was selected
• No clear upgrade path or deprecation plan for the old behavior

Recommendations:

• Document the flag in help text and configuration docs
• Add validation: warn if flag is set with only one beacon node
• Add metrics to track selected beacon node
• Consider auto-enabling when multiple beacon nodes are configured

5. Block Slot Lookup Timeout May Accumulate (Line 440)

The 125ms timeout for block lookups is applied per-client in parallel, which is fine. However, if many beacon nodes are slow, this could impact overall performance. Consider:

• Caching block slot lookups (same block root will be requested by multiple clients)
• Making block lookup optional with fast-fail
• Documenting why 125ms was chosen

---

Code Quality Issues

6. Magic Number Constants Need Documentation (Lines 20-22)

The timeout constants lack justification:

const SOFT_TIMEOUT: Duration = Duration::from_millis(500);
const HARD_TIMEOUT: Duration = Duration::from_secs(1);
const BLOCK_SLOT_LOOKUP_TIMEOUT: Duration = Duration::from_millis(125);

Why 500ms? Why 1s? How do these relate to slot timing (12s)? Add documentation explaining:

• The rationale for these values
• How they were determined
• What happens if beacon nodes are consistently slower

7. Minor Race Condition in Response Counting (Line 259)

After processing a response, the code checks if succeeded + failed == num_clients to exit early. If exactly num_clients responses arrive between processing the last response and the next loop iteration, the code continues waiting unnecessarily until timeout. This is a minor efficiency issue, not a correctness bug.

Fix: Check the condition at the loop start:

loop {
    if succeeded + failed == num_clients {
        break;
    }
    tokio::select! { ... }
}

---

Security Considerations

8. No Checkpoint Consistency Validation

The code doesn't validate that:

• Source checkpoint epoch < target checkpoint epoch
• Checkpoints are reasonable (not far in the future)
• Multiple beacon nodes agree on checkpoints (mentioned in TODO)

Recommendation: Add validation in fetch_and_score() to reject obviously invalid attestation data.

---

Performance Considerations

9. Parallel Queries Are Good, But...

The parallel querying approach is a solid improvement over sequential fallback. However:

• No request cancellation: Once all futures are spawned, they all run to completion even after a winner is selected
• No circuit breaker: Repeatedly failing beacon nodes aren't temporarily excluded
• No adaptive timeouts: Timeouts are fixed regardless of observed beacon node performance

These aren't blockers, but worth considering for future improvements.

---

Documentation

10. Missing User Documentation

The PR doesn't include:

• Documentation of the new CLI flag
• Explanation of when to use parallel querying
• Guidance on timeout configuration
• Metrics documentation

Recommendation: Update relevant documentation files (README, configuration docs, etc.).

---

Comparison with Previous Review

The previous Claude review identified similar issues. Checking status:

• ✅ CLI flag added: Feature is now configurable
• ❌ Compilation error: Still present (line 193)
• ❌ Scoring algorithm issues: Not addressed
• ❌ No tests: Still missing
• ❌ Magic numbers: Still undocumented
• ⚠️ Error handling: Partially addressed (returns error if all fail)

---

Summary

Must Fix Before Merge:

1. Fix compilation error in futures mapping (line 193-199)
2. Fix scoring algorithm or document why it's acceptable
3. Add comprehensive test coverage

Should Fix Before Merge:
4. Document timeout constants
5. Add checkpoint validation
6. Add metrics for beacon node selection

Nice to Have:
7. Cache block slot lookups
8. Fix minor race condition
9. Add user documentation

The parallel querying approach is architecturally sound and addresses a real need. However, the implementation needs refinement—particularly the compilation fix, scoring algorithm, and test coverage—before it's production-ready.

I've added inline comments on the specific code sections that need attention. See the "Files changed" tab for detailed feedback on each issue.

---

[claude-code-actions-sigp]

Documentation Needed: These timeout constants need justification. Consider adding:

/// Soft timeout for beacon node responses. If we receive at least one valid
/// attestation data response by this timeout, we'll return early with the best
/// scored result. This is set to 500ms to balance responsiveness with giving
/// slower nodes a chance to respond.
const SOFT_TIMEOUT: Duration = Duration::from_millis(500);

/// Hard timeout for beacon node responses. After this timeout, we return
/// unconditionally with whatever responses we've received (or error if none).
/// Set to 1s to ensure we have time to fetch attestation data before the
/// attestation deadline (slot time is 12s).
const HARD_TIMEOUT: Duration = Duration::from_secs(1);

/// Timeout for looking up a block's slot number by root. This is an optional
/// enhancement to scoring, so we use a short timeout to avoid delaying the
/// overall attestation data fetch.
const BLOCK_SLOT_LOOKUP_TIMEOUT: Duration = Duration::from_millis(125);

Questions to address:

• Why 500ms soft / 1s hard? How were these values determined?
• What happens if beacon nodes are consistently slower than 500ms but faster than 1s?
• How do these relate to attestation deadline constraints?

[petarjuki7]

Timeouts taken inspiration from the same parts of code in the Go implementation of SSV.
They use:
Soft timeout = 2 seconds
Hard timeout = 5 seconds
Block slot lookup timeout = 1.25 seconds

I found that a bit too long so adjusted to 1 second, 3 seconds and 0.5 seconds respectively

[mergify]

Some required checks have failed. Could you please take a look @petarjuki7? 🙏