Validator fixes: Removed dead code, devnet-2 feature flag leftover, moved tests to separate file. #63
Conversation
nsannn
commented
Jan 25, 2026
nsannn
commented
- Removed duplicated code.
- Removed not used devnet-2 feature flag.
- Moved unit tests from lib.rs to separate file.
… in validator/lib.rs and moved the unit_tests to separate folder and file
| .verify_aggregated_payload( | ||
| &validator_ids | ||
| .iter() | ||
| .map(|vid| validators.get(*vid).expect("validator must exist")) |
There was a problem hiding this comment.
don't panic here, throw proper error
| &attestation_data_root, | ||
| aggregated_attestation.data.slot.0 as u32, | ||
| ) | ||
| .expect("Attestation aggregated signature verification failed"); |
There was a problem hiding this comment.
same as before, don't panic
| if let Some(votes) = justifications.get(&target_root) { | ||
| let num_validators = self.validators.len_u64() as usize; | ||
| let count = votes.iter().filter(|&&v| v).count(); | ||
| let threshold = (2 * num_validators + 2) / 3; // ceil(2/3) |
There was a problem hiding this comment.
you can use .div_ceil instead
| tracing::info!( | ||
| target_slot = target_slot.0, | ||
| target_root = %format!("0x{:x}", target_root.0), | ||
| "JUSTIFICATION THRESHOLD REACHED!" |
There was a problem hiding this comment.
DONT SCREAM PLEASE! thank you :)
| leansig = { git = "https://github.com/leanEthereum/leanSig", rev = "73bedc26ed961b110df7ac2e234dc11361a4bf25" } | ||
| lean-multisig = { git = "https://github.com/leanEthereum/leanMultisig", rev = "e4474138487eeb1ed7c2e1013674fe80ac9f3165" } |
There was a problem hiding this comment.
nice! wanted to do the same thing 👍
| fn validate_attestation_data(store: &Store, data: &AttestationData) -> Result<(), String> { | ||
| // Cannot count a vote if we haven't seen the blocks involved | ||
| if !store.blocks.contains_key(&data.source.root) { | ||
| return Err(format!( | ||
| "Unknown source block: {:?}", | ||
| &data.source.root.0.as_bytes()[..8] | ||
| )); | ||
| } |
There was a problem hiding this comment.
you can use anyhow error instead of string, and immediately get lots of benefits:
- builtin backtraces
- builtin conversions from any type, implementing std::error::Error
- various nice helpers, like:
- custom error messages with
anyhow!macro; - assert-like non-panicking macro
ensure!; - nice wrapper
bail!for simplifyingreturn Err(anyhow!(...)).
- custom error messages with
The only shortcoming of anyhow is that you can't do pattern matching on error types. For those cases, thiserror exist.
So when using anyhow, your code can look a lot nicer:
| fn validate_attestation_data(store: &Store, data: &AttestationData) -> Result<(), String> { | |
| // Cannot count a vote if we haven't seen the blocks involved | |
| if !store.blocks.contains_key(&data.source.root) { | |
| return Err(format!( | |
| "Unknown source block: {:?}", | |
| &data.source.root.0.as_bytes()[..8] | |
| )); | |
| } | |
| fn validate_attestation_data(store: &Store, data: &AttestationData) -> Result<()> { | |
| // Cannot count a vote if we haven't seen the blocks involved | |
| ensure!(store.blocks.contains_key(&data.source.root), "Unknown source block: {:?}", &data.source.root.0.as_bytes()[..8]); |
Don't want to repeat this comment everywhere, but I think you got the idea, that all places, where you use String or Box<dyn std::error::Error> or something else as error type, must be changed to use anyhow's errors.
| let err_str = format!("{:?}", err); | ||
| if !err_str.contains("Duplicate") { | ||
| warn!(slot = slot, ?err, "Publish block with attestation failed"); | ||
| } |
There was a problem hiding this comment.
if duplicate errors are expected, then don't throw them at all. However, how do you distinguish between duplicate where you've received your own block, and duplicate where someone else attests twice? I think this place just doesn't make sense, and you've missed something during implementation, and warning is justified here.
| # docker-local: ./target/x86_64-unknown-linux-gnu/release/lean_client ./target/aarch64-unknown-linux-gnu/release/lean_client | ||
| docker-local: ./target/x86_64-unknown-linux-gnu/release/lean_client | ||
| @mkdir -p ./bin/amd64 | ||
| @cp ./target/x86_64-unknown-linux-gnu/release/lean_client ./bin/amd64/lean_client | ||
| @mkdir -p ./bin/arm64 | ||
| @cp ./target/aarch64-unknown-linux-gnu/release/lean_client ./bin/arm64/lean_client | ||
| # @mkdir -p ./bin/arm64 | ||
| # @cp ./target/aarch64-unknown-linux-gnu/release/lean_client ./bin/arm64/lean_client |
|
do this PR fix some test vectors? because that would be primary goal to achieve, apart from refactoring some parts |
|
let's merge it now, and fix all nitpicks later |
* feat: use ssz encoded validator key generation * fix: export validator keys to both ssz and json format for compatibility
