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

FRAME: Fix the Referenda confirming alarm #13704

Merged
merged 2 commits into from
Mar 27, 2023
Merged

Conversation

gavofyork
Copy link
Member

@gavofyork gavofyork commented Mar 24, 2023

We were doing a straight mul to get the block number from a Perbill, which will round down if the fractional part is half or less. We should always be rounding up, though, to ensure that when the alarm goes off the confirmation period is definitely ready to begin.

It would be good to write a fuzzer to ensure the issue manifested itself prior and no longer manifests after these changes. It is also possible that the issue was fixed by #13660, so if a fuzzer doesn't find this has fixed anything we might want to try testing with the commit immediately before.

Possible fix for paritytech/polkadot-sdk#200

@gavofyork gavofyork added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B1-note_worthy Changes should be noted in the release notes T1-runtime This PR/Issue is related to the topic “runtime”. labels Mar 24, 2023
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

A test?

This fails on bf395c8 since the
downwards rounding voids the curve delay.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@gavofyork gavofyork changed the title Fix the Referenda confirming alarm FRAME: Fix the Referenda confirming alarm Mar 27, 2023
@gavofyork
Copy link
Member Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit bede15d into master Mar 27, 2023
@paritytech-processbot paritytech-processbot bot deleted the gav-fix-alarm branch March 27, 2023 12:04
pgherveou pushed a commit that referenced this pull request Apr 4, 2023
* Fix the Referenda confirming alarm

* Add minimal regression test

This fails on bf395c8 since the
downwards rounding voids the curve delay.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
gpestana pushed a commit that referenced this pull request Apr 23, 2023
* Fix the Referenda confirming alarm

* Add minimal regression test

This fails on bf395c8 since the
downwards rounding voids the curve delay.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* Fix the Referenda confirming alarm

* Add minimal regression test

This fails on bf395c8 since the
downwards rounding voids the curve delay.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
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. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants