Skip to content

Commit

Permalink
Merge branch 'eichhorl/fix-share-counter' into 'master'
Browse files Browse the repository at this point in the history
fix(consensus): Don't increment random beacon & tape share counter for invalid shares

For performance reasons, https://gitlab.com/dfinity-lab/public/ic/-/merge_requests/8312 introduced a change with which we only ever validate at most `f+1` Random Beacon and Tape shares (which is the minimum number needed to create the aggregate).

To do this, when validating these shares, we initialize a counter with the current number of artifacts in the validated pool, and increment it whenever a change action for a new share is generated. Once the counter reaches the threshold, we stop validating shares.

However, incrementing the counter whenever a new change action is generated is wrong, since that change action could also be `HandleInvalid` (i.e. when the signature is invalid, see `compute_action_from_sig_verification`). In case we continuously receive shares with invalid signatures and comparatively small hashes (such that they are looked at first), this could lead to a replica no longer validating random beacon or tape shares.

With this MR we make sure to only increment the share counter if the change action is `MoveToValidated`. 

See merge request dfinity-lab/public/ic!12916
  • Loading branch information
eichhorl committed Jun 14, 2023
2 parents 0f5e385 + 12ce34b commit 3340e9d
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions rs/consensus/src/consensus/validator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1130,7 +1130,7 @@ impl Validator {
verification,
beacon.into_message(),
);
if ret.is_some() {
if let Some(&ChangeAction::MoveToValidated(_)) = ret.as_ref() {
existing_shares += 1;
}
ret
Expand Down Expand Up @@ -1258,7 +1258,7 @@ impl Validator {
tape.into_message(),
);
// increment counter of validated shares
if ret.is_some() {
if let Some(&ChangeAction::MoveToValidated(_)) = ret.as_ref() {
height_share_map.insert(height, existing_shares + 1);
}
ret
Expand Down

0 comments on commit 3340e9d

Please sign in to comment.