Skip to content

Backport timestamp logging fix #5459

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
EgorPopelyaev merged 2 commits into from
Aug 27, 2024
Merged

Backport timestamp logging fix #5459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e10f25
pallet-timestamp: `UnixTime::now` implementation logs error only if c…
gpestana Jul 30, 2024
72bc45e
Update prdoc/pr_5055.prdoc
bkchr Aug 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions prdoc/pr_5055.prdoc
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@ordian ordian Aug 26, 2024

Choose a reason for hiding this comment

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

should it be renamed to pr_5459 so satisfy the prdoc check?

Copy link
Copy Markdown
Contributor

@EgorPopelyaev EgorPopelyaev Aug 26, 2024

Choose a reason for hiding this comment

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

Maybe, it is better to leave the original prdoc name, otherwise it leads to the conflicts, when the changes are backported back to master

Copy link
Copy Markdown
Member Author

@bkchr bkchr Aug 26, 2024

Choose a reason for hiding this comment

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

The change is already on master, so we don't need to backport it?

Copy link
Copy Markdown
Member Author

@bkchr bkchr Aug 26, 2024

Choose a reason for hiding this comment

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

But I also don't see any problem to link to the original pr.

Copy link
Copy Markdown
Contributor

@EgorPopelyaev EgorPopelyaev Aug 27, 2024

Choose a reason for hiding this comment

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

Not the change itself, I usually move the prdocs into release related folder like prdoc/1.15.1 so that those changes does not appear in the next release. And push it back to master with the version bumps.

Copy link
Copy Markdown
Contributor

@ordian ordian Aug 27, 2024

Choose a reason for hiding this comment

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

Then maybe we should disable the PRDoc check on backport (stable*) branches as it's expected to fail.

Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
title: Only log error in `UnixTime::now` call of the pallet-timestamp implementation if called at genesis

doc:
- audience: Runtime Dev
description: |
This minor patch re-introduces a check to ensure that the `UnixTime::now` implementation in the timestamp only
logs an error if called at the genesis block.

crates:
- name: pallet-timestamp
bump: patch
10 changes: 6 additions & 4 deletions substrate/frame/timestamp/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,10 +368,12 @@ impl<T: Config> UnixTime for Pallet<T> {
// `sp_timestamp::InherentDataProvider`.
let now = Now::<T>::get();

log::error!(
target: "runtime::timestamp",
"`pallet_timestamp::UnixTime::now` is called at genesis, invalid value returned: 0",
);
if now == T::Moment::zero() {
log::error!(
target: "runtime::timestamp",
"`pallet_timestamp::UnixTime::now` is called at genesis, invalid value returned: 0",
);
}

core::time::Duration::from_millis(now.saturated_into::<u64>())
}
Expand Down