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

Trim compact solution for length during preparation #8317

Merged
gui1117 merged 28 commits into from
Apr 13, 2021
Merged

Trim compact solution for length during preparation #8317

gui1117 merged 28 commits into from
Apr 13, 2021

Conversation

@coriolinus
Copy link
Contributor

@coriolinus coriolinus commented Mar 10, 2021 •
edited
Loading

Closes #8315.

polkadot companion: paritytech/polkadot#2649

@coriolinus coriolinus added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Mar 10, 2021
@coriolinus coriolinus self-assigned this Mar 10, 2021
kianenigma
kianenigma reviewed Mar 15, 2021
View reviewed changes
kianenigma
kianenigma reviewed Mar 15, 2021
View reviewed changes
kianenigma
kianenigma reviewed Mar 15, 2021
View reviewed changes
kianenigma
kianenigma reviewed Mar 15, 2021
View reviewed changes
kianenigma
kianenigma reviewed Mar 15, 2021
View reviewed changes
kianenigma
kianenigma reviewed Mar 15, 2021
View reviewed changes
kianenigma
kianenigma reviewed Mar 15, 2021
View reviewed changes
kianenigma
kianenigma reviewed Mar 15, 2021
View reviewed changes
@coriolinus coriolinus mentioned this pull request Mar 15, 2021
Closed
@coriolinus coriolinus marked this pull request as ready for review March 15, 2021 14:04
@github-actions github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Mar 15, 2021
coriolinus added a commit to paritytech/polkadot that referenced this pull request Mar 19, 2021
@coriolinus
Companion for Trim compact solution for length during preparation
9bf5a47 
paritytech/substrate#8317
kianenigma
kianenigma reviewed Apr 1, 2021
View reviewed changes
@kianenigma kianenigma mentioned this pull request Apr 1, 2021
Closed
kianenigma
kianenigma reviewed Apr 1, 2021
View reviewed changes
frame/election-provider-multi-phase/src/unsigned.rs Outdated
///
/// Note that this solution is already computed, and winners are elected based on the merit of
/// the total stake in the system. Nevertheless, some of the voters may be removed here.
/// Sometimes, removing a voter can cause a validator to also be implicitely remoted, if
Copy link
Contributor

@kianenigma kianenigma Apr 1, 2021

Choose a reason for hiding this comment

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

Suggested change
/// Sometimes, removing a voter can cause a validator to also be implicitely remoted, if
/// Sometimes, removing a voter can cause a validator to also be implicitely removed, if

This is why you should never trust me.

Copy link
Contributor Author

@coriolinus coriolinus Apr 1, 2021

Choose a reason for hiding this comment

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

Hm, if we're correcting typos there, it should also be "implicitly".

Copy link
Contributor

@kianenigma kianenigma Apr 1, 2021

Choose a reason for hiding this comment

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

That as well 🤦‍♂️

kianenigma
kianenigma reviewed Apr 1, 2021
View reviewed changes
frame/election-provider-multi-phase/src/unsigned.rs
.map(|(who, stake, _)| (who.clone(), stake))
.collect::<Vec<_>>();
voters_sorted.sort_by_key(|(_, y)| *y);
voters_sorted.reverse();
Copy link
Contributor

@kianenigma kianenigma Apr 1, 2021

Choose a reason for hiding this comment

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

While we are waiting for audit, mind adding a test to ensure that we always remove based on least stake as well? i.e. say if we mess up the sorting order here, then we'd remove highest stake.

Copy link
Contributor Author

@coriolinus coriolinus Apr 1, 2021

Choose a reason for hiding this comment

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

Sure, will do.

Copy link
Contributor Author

@coriolinus coriolinus Apr 1, 2021

Choose a reason for hiding this comment

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

4c9f8d7

@kianenigma kianenigma added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Apr 13, 2021
@gui1117
Copy link
Contributor

gui1117 commented Apr 13, 2021

bot merge

@ghost
Copy link

ghost commented Apr 13, 2021

Merge aborted: Checks failed for 4c9f8d7

@gui1117
Copy link
Contributor

gui1117 commented Apr 13, 2021

needs to merge master to make CI happy, or manuall merge and then merge the companion

@gui1117 gui1117 merged commit 565078a into master Apr 13, 2021
@gui1117 gui1117 deleted the prgn-multi-phase-trim-length branch April 13, 2021 13:17
@kianenigma
Copy link
Contributor

kianenigma commented Apr 13, 2021

@coriolinus were you going to respond to the audit note in this branch or on a follow up?

@gui1117
Copy link
Contributor

gui1117 commented Apr 13, 2021

damn should I revert, when I see the green flag audit with thumb-up I didn't consider it was a failing audit

gui1117 added a commit that referenced this pull request Apr 13, 2021
@gui1117
Revert "Trim compact solution for length during preparation (#8317)"
326fdb8 
This reverts commit 565078a.
@gui1117 gui1117 mentioned this pull request Apr 13, 2021
Closed
@gui1117
Copy link
Contributor

gui1117 commented Apr 13, 2021 •
edited
Loading

I opened the revert #8613

I think we shouldn't mark D1-audited when the audit is not successful.
But from now on I'll be more careful about it anyway

@kianenigma
Copy link
Contributor

kianenigma commented Apr 13, 2021 •
edited
Loading

I think it is also okay to keep this merged and just add comments from audit in a follow up (if any).

@coriolinus
Copy link
Contributor Author

coriolinus commented Apr 13, 2021

I'd planned to handle the audit note here, but I don't mind moving it to a follow-up.

@kianenigma
Copy link
Contributor

kianenigma commented Apr 13, 2021

Yeah, so lets keep it in the follow-up for this one.

Note to self: don't mark audited if it is not your PR :p apologies for the inconvenience.

@gui1117
Copy link
Contributor

gui1117 commented Apr 13, 2021

I think it is more my bad not to have written the linked audit message

ghost pushed a commit to paritytech/polkadot that referenced this pull request Apr 13, 2021
@coriolinus @kianenigma @gui1117
Companion for Trim compact solution for length during preparation (#2649
a208449 
)

* Companion for Trim compact solution for length during preparation

paritytech/substrate#8317

* eliminate potential for overflow in OffchainSolutionLengthLimit

* Apply suggestions from code review

Co-authored-by: Guillaume Thiolliere <[email protected]>

* update substrate: cargo update -p sp-io

Co-authored-by: Kian Paimani <[email protected]>
Co-authored-by: Guillaume Thiolliere <[email protected]>
@joao-paulo-parity
Copy link
Contributor

joao-paulo-parity commented Apr 13, 2021

@thiolliere FWIW when CI is blocking the merge like in #8317 (comment) you can try bot merge force

hirschenberger pushed a commit to hirschenberger/substrate that referenced this pull request Apr 14, 2021
@coriolinus @gui1117
@kianenigma @hirschenberger
Trim compact solution for length during preparation (paritytech#8317)
a59bd63 
Co-authored-by: Guillaume Thiolliere <[email protected]>
Co-authored-by: Kian Paimani <[email protected]>
@coriolinus coriolinus mentioned this pull request Apr 16, 2021
Merged
@MRamanenkau MRamanenkau mentioned this pull request Sep 19, 2022
Merged
ggwpez pushed a commit to ggwpez/runtimes that referenced this pull request Mar 10, 2023
@coriolinus @kianenigma @gui1117
Companion for Trim compact solution for length during preparation (#2…
979491c 
…649)

* Companion for Trim compact solution for length during preparation

paritytech/substrate#8317

* eliminate potential for overflow in OffchainSolutionLengthLimit

* Apply suggestions from code review

Co-authored-by: Guillaume Thiolliere <[email protected]>

* update substrate: cargo update -p sp-io

Co-authored-by: Kian Paimani <[email protected]>
Co-authored-by: Guillaume Thiolliere <[email protected]>
ggwpez pushed a commit to ggwpez/runtimes that referenced this pull request Jul 13, 2023
@coriolinus @kianenigma @gui1117
Companion for Trim compact solution for length during preparation (#2…
faacce2 
…649)

* Companion for Trim compact solution for length during preparation

paritytech/substrate#8317

* eliminate potential for overflow in OffchainSolutionLengthLimit

* Apply suggestions from code review

Co-authored-by: Guillaume Thiolliere <[email protected]>

* update substrate: cargo update -p sp-io

Co-authored-by: Kian Paimani <[email protected]>
Co-authored-by: Guillaume Thiolliere <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@kianenigma kianenigma kianenigma approved these changes

@gui1117 gui1117 gui1117 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@coriolinus coriolinus

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited

Projects

(Nominated) Proof of Stake
Status: ✅ Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

MultiPhase Election: trim solution also by length.

7 participants

@coriolinus @gui1117 @kianenigma @joao-paulo-parity @gavofyork @paritytech-ci