Add --attestation-committee-count CLI parameter#141
Add --attestation-committee-count CLI parameter#141pablodeymo wants to merge 3 commits intofix-gossipfrom
Conversation
… attestation subnets configurable. Previously hardcoded to 1, this allows future devnets to use multiple committees. The value is passed to the P2P layer for subnet subscription (validator_id % committee_count).
🤖 Kimi Code ReviewReview SummaryThe PR makes 1. Division by Zero Risk (Critical)File: let subnet_id = validator_id.map(|vid| vid % attestation_committee_count);If if attestation_committee_count == 0 {
tracing::error!("attestation_committee_count must be > 0");
return;
}2. CLI Validation MissingFile: #[arg(long, default_value = "1", value_parser = clap::value_parser!(u64).range(1..))]
attestation_committee_count: u64,3. Default Value ConsistencyThe hardcoded constant was 4. Code StyleThe 5. DocumentationConsider adding documentation to the CLI argument explaining that this affects subnet calculation for validators. File: /// Number of attestation committees (subnets) per slot
/// Validators will be assigned to a subnet based on validator_id % attestation_committee_count
#[arg(long, default_value = "1", value_parser = clap::value_parser!(u64).range(1..))]
attestation_committee_count: u64,The changes are otherwise correct and follow Rust best practices. Automated review by Kimi (Moonshot AI) · custom prompt |
Greptile SummaryThis PR successfully makes the attestation committee count configurable via the Key changes:
Minor note:
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| bin/ethlambda/src/main.rs | Added --attestation-committee-count CLI parameter with proper validation (range 1..) to prevent division by zero. Parameter is correctly passed to start_p2p(). |
| crates/blockchain/src/lib.rs | Removed hardcoded ATTESTATION_COMMITTEE_COUNT constant as it's now configurable via CLI. Clean removal with no side effects. |
| crates/net/p2p/src/lib.rs | Updated start_p2p() to accept attestation_committee_count parameter and use it for subnet ID calculation. Properly replaces the hardcoded constant import. |
Last reviewed commit: ea37b6d
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // ATTESTATION_COMMITTEE_COUNT is 1 for devnet-3 but will increase in future devnets. | ||
| #[allow(clippy::modulo_one)] | ||
| let subnet_id = validator_id.map(|vid| vid % ATTESTATION_COMMITTEE_COUNT); | ||
| let subnet_id = validator_id.map(|vid| vid % attestation_committee_count); |
There was a problem hiding this comment.
modulo by zero will panic if attestation_committee_count is 0
| let subnet_id = validator_id.map(|vid| vid % attestation_committee_count); | |
| let subnet_id = validator_id.and_then(|vid| { | |
| if attestation_committee_count == 0 { | |
| None | |
| } else { | |
| Some(vid % attestation_committee_count) | |
| } | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/net/p2p/src/lib.rs
Line: 180
Comment:
modulo by zero will panic if `attestation_committee_count` is 0
```suggestion
let subnet_id = validator_id.and_then(|vid| {
if attestation_committee_count == 0 {
None
} else {
Some(vid % attestation_committee_count)
}
});
```
How can I resolve this? If you propose a fix, please make it concise.…ivision by zero in subnet calculation
|
@greptile-apps review it again please |
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 411
Comment:
outdated comment references `ATTESTATION_COMMITTEE_COUNT` constant which was removed
```suggestion
// When attestation_committee_count=1, all validators are in the same subnet.
```
How can I resolve this? If you propose a fix, please make it concise. |
… by CLI validation
1 participant
Motivation
ATTESTATION_COMMITTEE_COUNTwas hardcoded to1incrates/blockchain/src/lib.rs, meaning all validators shared a single attestation subnet. Future devnets will use multiple committees (other clients like Zeam and Lantern already support this parameter), so the value needs to be configurable at runtime.Description
Add a
--attestation-committee-countCLI argument (default1) that controls how many attestation subnets exist. Validators are assigned to subnets viavalidator_id % attestation_committee_count.Changes:
bin/ethlambda/src/main.rs: Add--attestation-committee-countCLI argument with default value1, pass it tostart_p2p()crates/blockchain/src/lib.rs: Remove theATTESTATION_COMMITTEE_COUNTconstant (no longer needed since the value comes from CLI)crates/net/p2p/src/lib.rs: Acceptattestation_committee_countas a function parameter instead of importing the constant, use it for subnet ID calculationThe hardcoded constant is removed entirely since clap provides the default. The blockchain crate doesn't store the value because it has no internal use for it — only the P2P layer needs it for subnet subscription.
How to test
--attestation-committee-count 1(should behave identically to before)1, same behavior)make lintandmake testpass with no warnings