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

Introduce cancel_proposal to rid us of those pesky proposals #7111

Merged
31 commits merged into from
Sep 24, 2020

Conversation

gavofyork
Copy link
Member

@gavofyork gavofyork commented Sep 15, 2020

Also fixes the weight of propose and introduces a hard-core blacklist option to ensure a proposal hash never returns.

polkadot companion: paritytech/polkadot#1728

TODO:

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Sep 15, 2020
@gavofyork gavofyork added B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Sep 15, 2020
@gavofyork gavofyork added A0-please_review Pull request needs code review. and removed A7-needspolkadotpr A0-please_review Pull request needs code review. labels Sep 18, 2020
Copy link
Contributor

@joepetrowski joepetrowski left a comment

Choose a reason for hiding this comment

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

Breaks transaction_version in case that hasn't been updated for the next release.

frame/democracy/src/lib.rs Outdated Show resolved Hide resolved
@@ -544,6 +556,10 @@ decl_error! {
WrongUpperBound,
/// Maximum number of votes reached.
MaxVotesReached,
/// The provided witness data is wrong.
InvalidWitness,
Copy link
Contributor

Choose a reason for hiding this comment

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

self/random note that some other modules can generally use this terminology: Some data that is passed in for sanity/weight should be called witness data.

Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

Other than my comments, this looks good.

@gavofyork
Copy link
Member Author

@kianenigma @thiolliere @shawntabrizi ready for final review...

Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

looks good to me

frame/democracy/src/lib.rs Outdated Show resolved Hide resolved
frame/democracy/src/lib.rs Show resolved Hide resolved
@gavofyork
Copy link
Member Author

@kianenigma @shawntabrizi would be good to get a final review.

@shawntabrizi
Copy link
Member

on_initialize weights look borked when running benchmarks again:

// before
		(70826000 as Weight)
			.saturating_add((10716000 as Weight).saturating_mul(r as Weight))
			.saturating_add(T::DbWeight::get().reads(6 as Weight))
			.saturating_add(T::DbWeight::get().reads((2 as Weight).saturating_mul(r as Weight)))
			.saturating_add(T::DbWeight::get().writes(5 as Weight))
//after
		(12_357_000 as Weight)
			.saturating_add((9_372_000 as Weight).saturating_mul(r as Weight))
			.saturating_add(T::DbWeight::get().reads(5 as Weight))
			.saturating_add(T::DbWeight::get().reads((1 as Weight).saturating_mul(r as Weight)))

Missing reads and writes. I am investigating

Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

I pushed the latest Weights to the PR. Note that the only relevant changes was an additional read to the propose weight, caused by checking the blacklist, and a reduction in the on_initialize_base weight due to bad benchmarking code.

To clarify the intention, on_initialize_base is supposed to be the least amount of computation needed by the on_initialize code, so in this case (5 + R) Reads.

If the on_initialize code actually triggers any of the more complex paths, like launch_next or bake_referendum, we will assume that the computation should take a full block weight. This is hardcoded into the on_initialize logic.

Tracking Issue: https://github.com/paritytech/substrate/issues/7209

@gavofyork
Copy link
Member Author

bot merge

@ghost
Copy link

ghost commented Sep 24, 2020

Trying merge.

@ghost ghost merged commit 5ea2399 into master Sep 24, 2020
@ghost ghost deleted the gav-cancel-proposal branch September 24, 2020 21:33
@viniul viniul 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 22, 2021
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. 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
None yet
Development

Successfully merging this pull request may close these issues.

7 participants