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

Improve complexity of CompactAssignments::unique_targets #8314

Merged
3 commits merged into from
Mar 17, 2021

Conversation

coriolinus
Copy link
Contributor

Original implementation was O(n**2). Current impl is O(n log n).

Avoided the original proposed mitigation because it does not retain
the de-duplicating property present in the original implementation.
This implementation does a little more work, but retains that property.

Mitigates https://github.com/paritytech/srlabs_findings/issues/58.

Original implementation was O(n**2). Current impl is O(n log n).

Avoided the original proposed mitigation because it does not retain
the de-duplicating property present in the original implementation.
This implementation does a little more work, but retains that property.
@coriolinus coriolinus added 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. labels Mar 10, 2021
@coriolinus coriolinus requested a review from kianenigma March 10, 2021 12:47
@coriolinus coriolinus self-assigned this Mar 10, 2021
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

LGTM!

@kianenigma
Copy link
Contributor

Avoided the original proposed mitigation because it does not retain
the de-duplicating property present in the original implementation.

The deduplication is absolutely important, and I believe we should have tests for it. If not, this is the time to add one.

I see that while using BTreeSet we maintain the fact that the results are sorted. This is totally optional. If there is a way that makes the complexity better, but is unsorted, let's do that (off the top of my head, there isn't one).

@coriolinus
Copy link
Contributor Author

If we had access to a deterministic HashSet in sp_std, then we could get this from n log n to just n, preserving deduplication but not sorting. However, we still haven't written that.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Was wrong, the imports need to be sorted.

Ensures that the macro still works if someone uses it in a context
in which sp_std is not imported or is renamed.
let mut all_targets: Vec<Self::Target> = Vec::with_capacity(self.average_edge_count());
use _npos::sp_std::collections::btree_set::BTreeSet;

let mut all_targets: BTreeSet<Self::Target> = BTreeSet::new();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember, we do have proper test for this unique_targets, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, AFAICT this crate's macros don't generate any test functions, and we don't test them with an example case. Would you like me to add that?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm let me see. There must be some tests in the root level crate, otherwise I've been a very bad coder.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

There are some instances of uniqueue_targets there, I presume that back then I found them enough, but would be good if you ensure they are safe and sound again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh cool! I'll look over them there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO unique_targets_len_edge_count_works sufficiently exercises unique_targets: in two different instances, it provides non-unique inputs, and validate that unique_targets unifies them.

@coriolinus coriolinus added the D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. label Mar 15, 2021
@coriolinus
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Mar 17, 2021

Trying merge.

@ghost ghost merged commit 319d244 into master Mar 17, 2021
@ghost ghost deleted the prgn-compactassignments-unique_targets-quadratic branch March 17, 2021 08:13
@kianenigma
Copy link
Contributor

What was the audit conclusion here?

@coriolinus
Copy link
Contributor Author

AFAIU you wrote the best formulation of the policy: "generally it should be audited again, except such cases where the change is trivial as well." The heart of this PR is +5/-6 which just swaps a vector for a BTreeSet; I think that counts as trivial.

@kianenigma kianenigma added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Apr 13, 2021
hirschenberger pushed a commit to hirschenberger/substrate that referenced this pull request Apr 14, 2021
…8314)

* Improve complexity of CompactAssignments::unique_targets

Original implementation was O(n**2). Current impl is O(n log n).

Avoided the original proposed mitigation because it does not retain
the de-duplicating property present in the original implementation.
This implementation does a little more work, but retains that property.

* Explicitly choose sp_std Vec and BTreeSet

Ensures that the macro still works if someone uses it in a context
in which sp_std is not imported or is renamed.

* explicitly use sp_std vectors throughout compact macro
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants