Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Safe desired targets call #12826

Merged
Ank4n merged 5 commits into from
Dec 10, 2022
Merged

Safe desired targets call #12826

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d93cb65
checked call for desired targets
Ank4n Nov 29, 2022
62d6461
fix compile
Ank4n Dec 2, 2022
7a689a2
fmt
Ank4n Dec 2, 2022
bbc807e
fix tests
Ank4n Dec 2, 2022
f672412
cleaner with and_then
Ank4n Dec 8, 2022
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
10 changes: 5 additions & 5 deletions frame/election-provider-multi-phase/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1409,12 +1409,12 @@ impl<T: Config> Pallet<T> {
return Err(ElectionError::DataProvider("Snapshot too big for submission."))
}

let mut desired_targets =
T::DataProvider::desired_targets().map_err(ElectionError::DataProvider)?;
let mut desired_targets = <Pallet<T> as ElectionProviderBase>::desired_targets_checked()
.map_err(|e| ElectionError::DataProvider(e))?;

// If `desired_targets` > `targets.len()`, cap `desired_targets` to that
// level and emit a warning
let max_desired_targets: u32 = (targets.len() as u32).min(T::MaxWinners::get());
// If `desired_targets` > `targets.len()`, cap `desired_targets` to that level and emit a
// warning
let max_desired_targets: u32 = targets.len() as u32;
if desired_targets > max_desired_targets {
log!(
warn,
Expand Down
10 changes: 6 additions & 4 deletions frame/election-provider-multi-phase/src/signed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,10 +594,12 @@ mod tests {
DesiredTargets::set(4);
MaxWinners::set(3);

let (_, _, actual_desired_targets) = MultiPhase::create_snapshot_external().unwrap();

// snapshot is created with min of desired_targets and MaxWinners
assert_eq!(actual_desired_targets, 3);
// snapshot not created because data provider returned an unexpected number of
// desired_targets
assert_noop!(
MultiPhase::create_snapshot_external(),
ElectionError::DataProvider("desired_targets must not be greater than MaxWinners."),
);
})
}

Expand Down
2 changes: 1 addition & 1 deletion frame/election-provider-support/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ pub trait ElectionProviderBase {
if desired_targets <= Self::MaxWinners::get() {
Ok(desired_targets)
} else {
Err("desired_targets should not be greater than MaxWinners")
Err("desired_targets must not be greater than MaxWinners.")
Copy link
Contributor Author

@Ank4n Ank4n Dec 5, 2022

Choose a reason for hiding this comment

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

I am leaning towards throwing an error if desired_targets < MaxWinners. Alternatively, we can cap the desired_targets to MaxWinners silently.

Copy link
Contributor

@gpestana gpestana Dec 8, 2022

Choose a reason for hiding this comment

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

My feeling is that error-ing the snapshot creation because desired_targets < MaxWinners is probably too harsh. I think that capping the desired_targets to MaxWinners would be a good option, but I'd do it when in fn snapshot_creation_external() when handling the error from this call. And add a warning too. Something along the lines of:

fn create_snapshot_external(
  // ...
  let mut desired_targets = <Pallet<T> as ElectionProviderBase>::desired_targets_checked().or_else(|e| 
     match e {
        Error::DesiredTargetsGreaterThanMaxWinners => {
             // warning: capping the desired targets
             Ok(<Pallet<T> as ElectionProviderBase>::MaxWinners::get())
          }
          _ => Err(ElectionError::DataProvider(e)),
  })?;

  // ...

Copy link
Contributor Author

@Ank4n Ank4n Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I have been split about this. We are already capping desired_targets to electable_targets so in that way it makes sense to cap desired_targets to MaxWinners as well.

But to present another perspective why this is a different case, electable_targets is a dynamic value that depends on total number of validator intentions while MaxWinners and desired_targets are static value configured in the runtime. If we consider this as a configuration issue, we should ideally fail till this configuration is fixed. We also have try_state checks to make sure we are not making this configuration error before it becomes part of the runtime.

Copy link
Contributor

@kianenigma kianenigma Dec 8, 2022

Choose a reason for hiding this comment

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

MaxWinners is the cap that should NEVER be reached, I think returning an error is better.

},
Err(e) => Err(e),
}
Expand Down