Skip to content

Combining (non)-distributed selection proofs #8219

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nadtech-hub wants to merge 5 commits into
base: unstable
Choose a base branch
from
Open

Combining (non)-distributed selection proofs #8219

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5ced929
proof selection cases combined (#7983)
nadtech-hub Oct 16, 2025
f856f6f
Merge branch 'stable' into feat/de-dup-sync-selection-proof
chong-he Dec 17, 2025
139e891
Merge branch 'unstable' into feat/de-dup-sync-selection-proof
chong-he Jan 23, 2026
a8486d4
Remove http-api tests, and test on `unstable` by default, with option…
jimmygchen Jan 12, 2026
f856c05
logs restored
nadtech-hub Jan 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
215 changes: 54 additions & 161 deletions validator_client/validator_services/src/sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use crate::duties_service::{DutiesService, Error, SelectionProofConfig};
use eth2::types::SyncCommitteeSelection;
use futures::future::join_all;
use futures::stream::{FuturesUnordered, StreamExt};
use logging::crit;
use parking_lot::{MappedRwLockReadGuard, RwLock, RwLockReadGuard, RwLockWriteGuard};
Expand Down Expand Up @@ -607,187 +606,81 @@ pub async fn fill_in_aggregation_proofs<S: ValidatorStore, T: SlotClock + 'stati

// Generate selection proofs for each validator at each slot, one slot at a time.
for slot in (start_slot..=pre_compute_slot.as_u64()).map(Slot::new) {
// For distributed mode
if duties_service
.sync_duties
.selection_proof_config
.parallel_sign
{
let mut futures_unordered = FuturesUnordered::new();

for (_, duty) in pre_compute_duties {
let subnet_ids = match duty.subnet_ids::<S::E>() {
Ok(subnet_ids) => subnet_ids,
Err(e) => {
crit!(
"error" = ?e,
"Arithmetic error computing subnet IDs"
);
continue;
}
};

// Construct proof for prior slot.
let proof_slot = slot - 1;

// Calling the make_sync_selection_proof will return a full selection proof
for &subnet_id in &subnet_ids {
let duties_service = duties_service.clone();
futures_unordered.push(async move {
let result =
make_sync_selection_proof(&duties_service, duty, proof_slot, subnet_id)
.await;

result.map(|proof| (duty.validator_index, proof_slot, subnet_id, proof))
});
}
}

while let Some(result) = futures_unordered.next().await {
let Some((validator_index, proof_slot, subnet_id, proof)) = result else {
continue;
};
let sync_map = duties_service.sync_duties.committees.read();
let Some(committee_duties) = sync_map.get(&sync_committee_period) else {
debug!("period" = sync_committee_period, "Missing sync duties");
continue;
};
let mut futures_unordered = FuturesUnordered::new();

let validators = committee_duties.validators.read();

// Check if the validator is an aggregator
match proof.is_aggregator::<S::E>() {
Ok(true) => {
if let Some(Some(duty)) = validators.get(&validator_index) {
debug!(
validator_index,
"slot" = %proof_slot,
"subcommittee_index" = *subnet_id,
// log full selection proof for debugging
"full selection proof" = ?proof,
"Validator is sync aggregator"
);

// Store the proof
duty.aggregation_duties
.proofs
.write()
.insert((proof_slot, subnet_id), proof);
}
}
Ok(false) => {} // Not an aggregator
Err(e) => {
warn!(
validator_index,
%slot,
"error" = ?e,
"Error determining is_aggregator"
);
}
}
for (validator_start_slot, duty) in pre_compute_duties {
if slot < *validator_start_slot {
Copy link
Author

@nadtech-hub nadtech-hub Oct 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doubting to keep this case and was wondering why it was only seen in distributed mode
@michaelsproul

Copy link
Member

@chong-he chong-he Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only in the non-distributed mode now, I can't recall why I didn't have that in the distributed mode, but I think we can keep this

continue;
}
} else {
// For non-distributed mode
debug!(
period = sync_committee_period,
%current_slot,
%pre_compute_slot,
"Calculating sync selection proofs"
);
Comment on lines 692 to 697
Copy link
Member

@chong-he chong-he Dec 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This log is completely removed, but we would want this log in the normal/non-distributed case, together with the log about Finished computing sync selection proofs, so that we can know how long it took to compute the selection proofs.

But for distributed case, this will not be accurate because it involves sending the request to the middleware and waiting to hear from them. What we could do is to add a condition that under distributed/parallel_sign case, we don't log this

Copy link
Author

@nadtech-hub nadtech-hub Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your comment is confusing, logging time in parallel mode is incorrect but the debug needs to be activated in parallel mode anyway

Copy link
Member

@chong-he chong-he Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, I miss a "don't" in the comment, edited as above. What I mean is, under parallel_sign case, we don't log this; and under normal case, we log it (which is what it does currently)


let mut validator_proofs = vec![];
for (validator_start_slot, duty) in pre_compute_duties {
// Proofs are already known at this slot for this validator.
if slot < *validator_start_slot {
let subnet_ids = match duty.subnet_ids::<S::E>() {
Ok(subnet_ids) => subnet_ids,
Err(e) => {
crit!(
"error" = ?e,
"Arithmetic error computing subnet IDs"
);
continue;
}
};

let subnet_ids = match duty.subnet_ids::<S::E>() {
Ok(subnet_ids) => subnet_ids,
Err(e) => {
crit!(
error = ?e,
"Arithmetic error computing subnet IDs"
);
continue;
}
};

// Create futures to produce proofs.
let duties_service_ref = &duties_service;
let futures = subnet_ids.iter().map(|subnet_id| async move {
// Construct proof for prior slot.
let proof_slot = slot - 1;
// Construct proof for prior slot.
let proof_slot = slot - 1;

let proof =
make_sync_selection_proof(duties_service_ref, duty, proof_slot, *subnet_id)
// Calling the make_sync_selection_proof will return a full selection proof
for &subnet_id in &subnet_ids {
let duties_service = duties_service.clone();
futures_unordered.push(async move {
let result =
make_sync_selection_proof(&duties_service, duty, proof_slot, subnet_id)
.await;

match proof {
Some(proof) => match proof.is_aggregator::<S::E>() {
Ok(true) => {
debug!(
validator_index = duty.validator_index,
slot = %proof_slot,
%subnet_id,
"Validator is sync aggregator"
);
Some(((proof_slot, *subnet_id), proof))
}
Ok(false) => None,
Err(e) => {
warn!(
pubkey = ?duty.pubkey,
slot = %proof_slot,
error = ?e,
"Error determining is_aggregator"
);
None
}
},

None => None,
}
result.map(|proof| (duty.validator_index, proof_slot, subnet_id, proof))
});

// Execute all the futures in parallel, collecting any successful results.
let proofs = join_all(futures)
.await
.into_iter()
.flatten()
.collect::<Vec<_>>();

validator_proofs.push((duty.validator_index, proofs));
}
}

// Add to global storage (we add regularly so the proofs can be used ASAP).
while let Some(result) = futures_unordered.next().await {
let Some((validator_index, proof_slot, subnet_id, proof)) = result else {
continue;
};
let sync_map = duties_service.sync_duties.committees.read();
let Some(committee_duties) = sync_map.get(&sync_committee_period) else {
debug!(period = sync_committee_period, "Missing sync duties");
debug!("period" = sync_committee_period, "Missing sync duties");
Copy link
Member

@chong-he chong-he Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the original code has the double quote but it's not necessary to have that

Suggested change
debug!("period" = sync_committee_period, "Missing sync duties");
debug!(period = sync_committee_period, "Missing sync duties");

continue;
};

let validators = committee_duties.validators.read();
let num_validators_updated = validator_proofs.len();

for (validator_index, proofs) in validator_proofs {
if let Some(Some(duty)) = validators.get(&validator_index) {
duty.aggregation_duties.proofs.write().extend(proofs);
} else {
debug!(
Comment on lines -771 to -777
Copy link
Member

@chong-he chong-he Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can keep this part of the code until the end of this debug log

Copy link
Author

@nadtech-hub nadtech-hub Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't quite understand what you meant to restore

// Check if the validator is an aggregator
match proof.is_aggregator::<S::E>() {
Ok(true) => {
if let Some(Some(duty)) = validators.get(&validator_index) {
debug!(
validator_index,
"slot" = %proof_slot,
"subcommittee_index" = *subnet_id,
// log full selection proof for debugging
"full selection proof" = ?proof,
"Validator is sync aggregator"
Comment on lines +672 to +677
Copy link
Member

@chong-he chong-he Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the interest of combining both normal and distributed cases, the "full selection proof" will make it confusing, I suggest we just stick to selection_proof, and keep the comment to indicate it is a full selection proof

Also we can remove the double quotes in the fields, I am not sure why I put them there in the past

Suggested change
validator_index,
"slot" = %proof_slot,
"subcommittee_index" = *subnet_id,
// log full selection proof for debugging
"full selection proof" = ?proof,
"Validator is sync aggregator"
validator_index,
slot = %proof_slot,
subcommittee_index = *subnet_id,
// In distributed mode, this is the full selection proof
selection_proof = ?proof,
"Validator is sync aggregator"

);

// Store the proof
duty.aggregation_duties
.proofs
.write()
.insert((proof_slot, subnet_id), proof);
}
}
Ok(false) => {} // Not an aggregator
Err(e) => {
warn!(
validator_index,
period = sync_committee_period,
"Missing sync duty to update"
%slot,
"error" = ?e,
"Error determining is_aggregator"
);
}
}

if num_validators_updated > 0 {
debug!(
%slot,
updated_validators = num_validators_updated,
"Finished computing sync selection proofs"
);
}
Comment on lines -785 to -791
Copy link
Member

@chong-he chong-he Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refer to the comment about calculating sync selection proofs, the same applies here

Copy link
Author

@nadtech-hub nadtech-hub Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this log seems useless cuz vector may contains 1 item at most

}
}
}