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

Timestamp: set_timestamp sets DidUpdate #11960

Merged
merged 3 commits into from Aug 4, 2022
Merged

Timestamp: set_timestamp sets DidUpdate #11960

merged 3 commits into from Aug 4, 2022

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Aug 2, 2022

There exists the set_timestamp in the Timestamp pallet for setting the current timestamp. The
problem is that it doesn't set DidUpdate. This results in on_finalize panicking. There is no
real reason why the function doesn't also set DidUpdate.

Close: #11958

There exists the `set_timestamp` in the Timestamp pallet for setting the current timestamp. The
problem is that it doesn't set `DidUpdate`. This results in `on_finalize` panicking. There is no
real reason why the function doesn't also set `DidUpdate`.
@bkchr bkchr added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Aug 2, 2022
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Just looking at the timestamp pallet; there is a lot of unsafe math in it 🙈
I will fix it in a new MR.

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@bkchr
Copy link
Member Author

bkchr commented Aug 3, 2022

Just looking at the timestamp pallet; there is a lot of unsafe math in it see_no_evil I will fix it in a new MR.

Before we overflow u64, it will take some time :P

@ggwpez
Copy link
Member

ggwpez commented Aug 3, 2022

Before we overflow u64, it will take some time :P

Yea I just saw that it is an unsigned extrinsic anyway...

There is actually a test failure that looks related in report_equivocation_invalid_key_owner_proof: Timestamp slot must match CurrentSlot here https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1718072.

@bkchr
Copy link
Member Author

bkchr commented Aug 3, 2022

There is actually a test failure that looks related in report_equivocation_invalid_key_owner_proof: Timestamp slot must match CurrentSlot here https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1718072.

Yeah that is because of your changes :D I fix it :D

@bkchr bkchr requested a review from andresilva as a code owner August 3, 2022 13:11
@bkchr bkchr merged commit 9ece574 into master Aug 4, 2022
@bkchr bkchr deleted the bkchr-set-timestamp branch August 4, 2022 09:34
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
* Timestamp: `set_timestamp` sets `DidUpdate`

There exists the `set_timestamp` in the Timestamp pallet for setting the current timestamp. The
problem is that it doesn't set `DidUpdate`. This results in `on_finalize` panicking. There is no
real reason why the function doesn't also set `DidUpdate`.

* Update frame/timestamp/src/lib.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fix Babe tests

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Timestamp: `set_timestamp` sets `DidUpdate`

There exists the `set_timestamp` in the Timestamp pallet for setting the current timestamp. The
problem is that it doesn't set `DidUpdate`. This results in `on_finalize` panicking. There is no
real reason why the function doesn't also set `DidUpdate`.

* Update frame/timestamp/src/lib.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fix Babe tests

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Timestamp::set_timestamp does not set DidUpdate
3 participants