Combining (non)-distributed selection proofs #8219
Conversation
Signed-off-by: Aliaksei Misiukevich <taberlick@gmail.com>
|
|
| } | ||
| } | ||
| for (validator_start_slot, duty) in pre_compute_duties { | ||
| if slot < *validator_start_slot { |
There was a problem hiding this comment.
doubting to keep this case and was wondering why it was only seen in distributed mode
@michaelsproul
There was a problem hiding this comment.
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
|
Some required checks have failed. Could you please take a look @nadtech-hub? 🙏 |
chong-he
left a comment
There was a problem hiding this comment.
Hi @nadtech-hub , can you update the target branch to unstable?
Some review comments below, thanks
| debug!( | ||
| period = sync_committee_period, | ||
| %current_slot, | ||
| %pre_compute_slot, | ||
| "Calculating sync selection proofs" | ||
| ); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
your comment is confusing, logging time in parallel mode is incorrect but the debug needs to be activated in parallel mode anyway
There was a problem hiding this comment.
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)
| if num_validators_updated > 0 { | ||
| debug!( | ||
| %slot, | ||
| updated_validators = num_validators_updated, | ||
| "Finished computing sync selection proofs" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Refer to the comment about calculating sync selection proofs, the same applies here
| 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"); |
There was a problem hiding this comment.
I know the original code has the double quote but it's not necessary to have that
| debug!("period" = sync_committee_period, "Missing sync duties"); | |
| debug!(period = sync_committee_period, "Missing sync duties"); |
| validator_index, | ||
| "slot" = %proof_slot, | ||
| "subcommittee_index" = *subnet_id, | ||
| // log full selection proof for debugging | ||
| "full selection proof" = ?proof, | ||
| "Validator is sync aggregator" |
There was a problem hiding this comment.
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
| 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" |
| 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!( |
There was a problem hiding this comment.
We can keep this part of the code until the end of this debug log
|
I'll update tonight, sorry for delay |
This one should close #7983