feat(network): add dynamic peer count based on active subnets#726
feat(network): add dynamic peer count based on active subnets#726mergify[bot] merged 12 commits intosigp:release-v1.1.0from
Conversation
|
Claude finished @petarjuki7's task —— View job Code Review CompleteTasks
SummaryThis PR implements dynamic peer target calculation based on active subnets. The implementation is straightforward and follows the stated requirements (base 60 + 3 per subnet, capped at 150). However, there are several areas that need attention. Issues Found1. Logic Issue: User-provided value override is incompleteLocation: The PR description states: "If the user provides more than the calculated if config.network.target_peers < dyn_peers {
config.network.target_peers = dyn_peers;
}This means:
The current implementation actually means: "Use the maximum of user-provided and calculated values." If this is the intended behavior, the PR description should be corrected. If not, the logic needs adjustment. 2. Missing DocumentationLocation: The dynamic peer calculation block lacks any comments explaining:
Recommendation: Add a comment block explaining the logic: // Calculate dynamic peer target based on active subnets.
// Base: 60 peers, add 3 per unique subnet, cap at 150.
// If the user configured a lower target_peers value, override it with the calculated value.3. Testing Coverage MissingThere are no tests for this new functionality. Consider adding tests for:
Note: I did not find existing test files in 4. Performance: Unnecessary Scoping BlockLocation: The code uses an extra scoping block {
let state = database.state();
// ... calculation ...
}While this isn't harmful, it adds visual noise. The 5. Minor: Default Value Change DocumentationLocation: The default
Code Quality Observations✅ Good:
Security & Performance
RecommendationSuggested actions before merging:
Despite these issues, the core implementation is sound and the feature itself is valuable. With the clarifications above, this will be a solid addition. |
dknopik
left a comment
dknopik
left a comment
There was a problem hiding this comment.
This does not dynamically adjust the target peers if we subscribe or unsubscribe to a subnet while the node is running.
Shouldn't the value provided by the user be used no matter what? |
![claude-code-actions-sigp[bot]](https://avatars.githubusercontent.com/in/2108028?s=60&v=4){kind=small}
anchor/client/src/lib.rs
Outdated
| { | ||
| let state = database.state(); | ||
| let mut unique_subnets = HashSet::new(); | ||
| for cluster_id in state.get_own_clusters() { | ||
| if let Some(cluster) = state.clusters().get_by(cluster_id) { | ||
| let subnet = SubnetId::from_committee(cluster.committee_id(), SUBNET_COUNT); | ||
| unique_subnets.insert(subnet); | ||
| } | ||
| } | ||
| let dyn_peers = min(60 + unique_subnets.len() * 3, 150); | ||
| if config.network.target_peers < dyn_peers { | ||
| config.network.target_peers = dyn_peers; | ||
| } |
There was a problem hiding this comment.
Clarification needed: The PR description states "If the user provides more than the calculated target_peers, then that value is used." However, this logic only updates target_peers when the user's value is less than the calculated value.
Current behavior: Uses max(user_value, calculated_value)
Is this the intended behavior? If so, please update the PR description. If not, the logic needs adjustment.
Also consider:
- Adding a comment explaining this calculation (base 60 + 3 per subnet, capped at 150)
- Using a more descriptive variable name than
dyn_peers(e.g.,calculated_target_peers) - Removing the unnecessary scoping block or adding a comment explaining why it's needed
There was a problem hiding this comment.
@claude can we calculate this in the network crate or in another place where the number of subnets is known?
|
@diegomrsantos @dknopik |
anchor/client/src/lib.rs
Outdated
| if dynamic_target_peers { | ||
| let state = database.state(); | ||
| let mut unique_subnets = HashSet::new(); | ||
| for cluster_id in state.get_own_clusters() { | ||
| if let Some(cluster) = state.clusters().get_by(cluster_id) { | ||
| let subnet = SubnetId::from_committee(cluster.committee_id(), SUBNET_COUNT); | ||
| unique_subnets.insert(subnet); | ||
| } | ||
| } | ||
| let dyn_peers = min(60 + unique_subnets.len() * 3, 150); | ||
|
|
||
| config.network.target_peers = Some(dyn_peers); | ||
| } |
There was a problem hiding this comment.
Can we calculate this in the network or in another place where we have the number of subnets?
There was a problem hiding this comment.
It could probably be done in the network, my logic was to keep it here for all the startup / config code to stay in the client crate. ConnectionManager needs the target_peers in its constructor.
There was a problem hiding this comment.
can you e.g. update_dynamic_target_peers on network creation instead of having the duplicated logic here?
|
@claude reply to this #726 (comment) |
|
Claude finished @diegomrsantos's task —— View job Analyzing Calculation Location
|
Fixes workflow not completing tasks and hanging indefinitely. 1. **Fix Configuration Format**: Updated workflow arguments to use multi-line YAML format (required by the action) 2. **Add Timeout**: Added 15-minute timeout to prevent infinite hangs Co-Authored-By: diego <diego@sigmaprime.io>
|
@claude reply to this #726 (comment) |
|
Claude finished @diegomrsantos's task —— View job Response to Calculation Location QuestionResponding to this review comment: AnalysisAfter reviewing the codebase, I can see that the dynamic peer calculation is currently done in the client crate ( Current StateGood news: The PR already implements dynamic updates in the network crate! Looking at the code:
The only remaining issue is the initial calculation at startup, which happens in the client crate. Recommendation: Move Initial Calculation to NetworkYes, the initial calculation should be moved to the network crate. Here's why: Benefits:
Implementation Approach:Rather than calculating in Option 1: Calculate in ConnectionManager constructor impl ConnectionManager {
pub fn new(config: &Config, active_subnets: HashSet<SubnetId>) -> Self {
let target_peers = config.target_peers.unwrap_or_else(|| {
// Calculate based on active subnets
std::cmp::min(60 + active_subnets.len() * 3, 150)
});
// ... rest of constructor
}
}Option 2: Initialize with default, then update after construction // In client crate, after creating the network:
if dynamic_target_peers {
network.update_target_peers_from_subnets(active_subnets);
}Addressing @petarjuki7's Concern
This is valid, but I'd argue that:
Current State AssessmentThe PR is already 90% there - it correctly updates dynamically at runtime. The remaining 10% is just moving the initial calculation from client to network for consistency. Summary: While the current implementation works, moving the initial calculation to the network crate would improve code organization and maintain a single source of truth for the dynamic peer logic. The network crate already has all the machinery needed; it just needs to handle the initial calculation as well as the runtime updates. |
| )) | ||
| .with_max_established_per_peer(Some(1)); | ||
|
|
||
| self.connection_limits = connection_limits::Behaviour::new(limits); |
There was a problem hiding this comment.
I think it would be cleaner to update the limits using limits_mut on the existing behaviour
There was a problem hiding this comment.
As far as I can tell, limits_mut gives me a &mut ConnectionLimits, but all the methods for updating values (.with_max_established_incoming() etc.) take self (ConnectionLimits). And also the fields of the ConnectionLimits struct are private so I can't just update the fields through &mut ConnectionLimits.
Or did you have in mind to just do something like:
*self.connection_limits.limits_mut() =
ConnectionLimits::default()
.with_max_pending_incoming(Some(5))
.with_max_pending_outgoing(Some(16))
.with_max_established_incoming(Some(...))
anchor/client/src/lib.rs
Outdated
| if dynamic_target_peers { | ||
| let state = database.state(); | ||
| let mut unique_subnets = HashSet::new(); | ||
| for cluster_id in state.get_own_clusters() { | ||
| if let Some(cluster) = state.clusters().get_by(cluster_id) { | ||
| let subnet = SubnetId::from_committee(cluster.committee_id(), SUBNET_COUNT); | ||
| unique_subnets.insert(subnet); | ||
| } | ||
| } | ||
| let dyn_peers = min(60 + unique_subnets.len() * 3, 150); | ||
|
|
||
| config.network.target_peers = Some(dyn_peers); | ||
| } |
There was a problem hiding this comment.
can you e.g. update_dynamic_target_peers on network creation instead of having the duplicated logic here?
jxs
left a comment
jxs
left a comment
There was a problem hiding this comment.
Hi Petar, overall looks good to me, left some comments
anchor/client/src/lib.rs
Outdated
| let dynamic_target_peers = config.network.target_peers.is_none(); | ||
| if dynamic_target_peers { |
There was a problem hiding this comment.
nitpick:
| let dynamic_target_peers = config.network.target_peers.is_none(); | |
| if dynamic_target_peers { | |
| let target_peers = match config.network.target_peers { | |
| Some(target_peers) => target_peers, | |
| None => {...} |
anchor/client/src/lib.rs
Outdated
| for cluster_id in state.get_own_clusters() { | ||
| if let Some(cluster) = state.clusters().get_by(cluster_id) { | ||
| let subnet = SubnetId::from_committee(cluster.committee_id(), SUBNET_COUNT); | ||
| unique_subnets.insert(subnet); | ||
| } | ||
| } | ||
| let dyn_peers = min(60 + unique_subnets.len() * 3, 150); |
There was a problem hiding this comment.
we can simplify this by counting the unique subnets using itertools unique (which we already depend transitively:
| for cluster_id in state.get_own_clusters() { | |
| if let Some(cluster) = state.clusters().get_by(cluster_id) { | |
| let subnet = SubnetId::from_committee(cluster.committee_id(), SUBNET_COUNT); | |
| unique_subnets.insert(subnet); | |
| } | |
| } | |
| let dyn_peers = min(60 + unique_subnets.len() * 3, 150); | |
| let unique_subnets = state | |
| .get_own_clusters() | |
| .iter() | |
| .filter_map(|id| state.clusters().get_by(id)) | |
| .map(|s| SubnetId::from_committee(s.committee_id(), SUBNET_COUNT)) | |
| .unique() | |
| .count(); | |
| let dyn_peers = min(60 + unique_subnets.len() * 3, 150); |
anchor/network/src/network.rs
Outdated
| outcome_rx: mpsc::Receiver<Outcome>, | ||
| executor: TaskExecutor, | ||
| spec: Arc<ChainSpec>, | ||
| dynamic_target_peers: bool, |
There was a problem hiding this comment.
| dynamic_target_peers: bool, | |
| target_peers: bool, |
they can also be static right? If they are user provided
anchor/client/src/lib.rs
Outdated
| } | ||
| let dyn_peers = min(60 + unique_subnets.len() * 3, 150); | ||
|
|
||
| config.network.target_peers = Some(dyn_peers); |
There was a problem hiding this comment.
are we using the value in dyn_peers anywhere else? If not we can avoid declaring config as mut in the input parameters
This commit refactors the dynamic peer target calculation to improve code organization, maintainability, and clarity. Key improvements: - Extract calculation formula to single method in ConnectionManager - Remove panic risk by handling Option<usize> gracefully - Improve variable naming (is_dynamic_target_peers) - Add comprehensive documentation explaining formula and behavior - Add tests for calculation formula correctness - Simplify subnet counting with iterator chain - Remove unnecessary mutability from config parameter The calculation logic (base 60 + 3 per subnet, capped at 150) is now centralized in ConnectionManager::calculate_target_peers(), used for both startup initialization and runtime updates. When config.target_peers is None, the calculation is applied automatically. When Some, the user's value is used as a static override.
diegomrsantos
force-pushed
the
dynamic_target_peers
branch
from
355c6d3 to
ca7dbf1
Compare
Moves calculate_target_peers from ConnectionManager to PeerManager to properly separate policy (peer targeting strategy) from mechanism (connection management). PeerManager now calculates the target based on subnet count and passes it to ConnectionManager via the renamed set_target_peers method. This improves separation of concerns and eliminates the awkward pattern where PeerManager had to call ConnectionManager::calculate_target_peers before instantiating the ConnectionManager.
|
Claude finished @petarjuki7's task —— View job Code Review CompleteTasks
SummaryThis PR implements dynamic peer target calculation based on active subnets. The implementation follows the formula: base 60 + 3 per subnet, capped at 150. The recent refactoring (commit Overall AssessmentStrengths:
Issues Found: 1. Missing User Override DocumentationSeverity: Minor The PR description states: "If the user provides more than the calculated However, the actual implementation uses the user's value regardless of whether it's higher or lower than the calculated value: // anchor/network/src/peer_manager/mod.rs:67-69
let target_peers = config
.target_peers
.unwrap_or_else(|| Self::calculate_target_peers(initial_subnet_count));Recommendation: Update the PR description to accurately reflect the behavior: 2. Type Change Not DocumentedSeverity: Minor The Recommendation: Add a note in the PR description under "Additional Info" mentioning this type change and its implications for any code that directly accesses this field. 3. Code Quality: Redundant VariableSeverity: Trivial The Current: let is_dynamic_target_peers = config.network.target_peers.is_none();
// Start the p2p network
let mut network = Network::try_new::<E>(
// ...
is_dynamic_target_peers,
)Suggestion: // Start the p2p network
let mut network = Network::try_new::<E>(
// ...
config.network.target_peers.is_none(),
)Or keep the variable but add a comment explaining its purpose for clarity. 4. Potential for Configuration ConfusionSeverity: Minor The behavior where
Code Quality Observations✅ Good Practices:
|
Address code review feedback by: - Extracting magic numbers (60, 3, 150) into named constants - Adding clarifying comments about dynamic vs static peer targeting - Documenting that values are copied from go-ssv for network compatibility - Updating tests to reference constants instead of hardcoded values This improves code maintainability and makes the rationale for the calculation values more explicit.
anchor/client/src/lib.rs
Outdated
| // Calculate initial subnet count for dynamic peer target calculation. | ||
| // If the user provided a static target_peers value, this count is still used | ||
| // by the network layer but the user's value takes precedence. | ||
| let initial_subnet_count = { | ||
| let state = database.state(); | ||
| state | ||
| .get_own_clusters() | ||
| .iter() | ||
| .filter_map(|id| state.clusters().get_by(id)) | ||
| .map(|cluster| SubnetId::from_committee(cluster.committee_id(), SUBNET_COUNT)) | ||
| .collect::<HashSet<_>>() | ||
| .len() | ||
| }; |
There was a problem hiding this comment.
is this necessary?
as soon as the first subnet join event comes in, the target will be changed again. So why not start with a target of 60 to avoid adding this here?
anchor/client/src/lib.rs
Outdated
| // Determine if we should dynamically adjust target_peers when subnets change. | ||
| // If the user specified a target_peers value (Some), we keep it static throughout runtime. | ||
| // If not specified (None), we calculate and adjust target_peers dynamically based on the | ||
| // number of active subnets as validators join/leave subnet duties. | ||
| let is_dynamic_target_peers = config.network.target_peers.is_none(); |
There was a problem hiding this comment.
we pass the whole network config below. why do we pass this flag additionally seperately?
The previous implementation had a logic bug where subnets were counted twice: 1. At startup from DB clusters -> calculated initial target_peers 2. During startup as subnet_service emitted join events -> recalculated repeatedly This caused target_peers to thrash during startup as subnet joins fired, incrementing the target repeatedly until all initial subnets were joined. Solution: Remove initial_subnet_count entirely. When dynamic peer calculation is enabled, target_peers now: - Starts at BASE_PEER_COUNT - Gets updated naturally as subnet_service reports joins/leaves - Uses subnet_service as single source of truth Additional cleanup: Moved is_dynamic_target_peers calculation inside Network::try_new since config is already passed in - no need for the caller to derive and pass this flag separately. This is cleaner because: - Subnet_service is the runtime source of truth, not the DB - Self-correcting if DB and runtime state diverge - No state synchronization complexity - Brief startup adjustment has no real-world impact (peer dialing takes much longer than subnet join events)
diegomrsantos
force-pushed
the
dynamic_target_peers
branch
from
ce55d40 to
3831b9e
Compare
diegomrsantos
force-pushed
the
dynamic_target_peers
branch
from
3831b9e to
52811e8
Compare
diegomrsantos
force-pushed
the
dynamic_target_peers
branch
from
52811e8 to
7879620
Compare
|
Claude finished @petarjuki7's task —— View job Code Review CompleteTasks
SummaryThis PR implements dynamic peer target calculation based on active subnets, following the formula from go-ssv: base 60 + 3 per subnet, capped at 150. The implementation has gone through several iterations addressing previous review feedback, with the most recent refactoring successfully moving calculation logic from ConnectionManager to PeerManager. Overall AssessmentThe implementation is well-structured and production-ready with good architectural decisions. The code quality is high, with proper separation of concerns, clear documentation, and comprehensive test coverage for the calculation logic. Code Quality✅ Strengths
|
dknopik
left a comment
dknopik
left a comment
There was a problem hiding this comment.
Two nits :)
Can we also adjust the --target-peers help text to describe the behaviour if it is omitted?
All done |
Address review feedback: - Update CLI help text for --target-peers to explain dynamic calculation when omitted - Update target_peers field documentation to explain dynamic calculation behavior - Add debug log in set_target_peers to show old/new values when updating - Remove unnecessary clippy::too_many_arguments allow attribute
diegomrsantos
force-pushed
the
dynamic_target_peers
branch
from
efce118 to
dd8f4f4
Compare
4 participants
Proposed Changes
Implements dynamic peer target calculation similar to go-ssv:
Additional Info
Breaking Change
target_peersconfig field changed fromusizetoOption<usize>Behavior
--target-peers <N>: That static value is always used, regardless of subnet count--target-peers: Target peers are calculated dynamically based on active subnets (60 + subnets × 3, capped at 150)Implementation Notes
The dynamic calculation formula is copied from the go-ssv implementation for compatibility with the existing SSV network. The values are now defined as named constants (
BASE_PEER_COUNT,PEERS_PER_SUBNET,MAX_PEER_COUNT) for maintainability.