Skip to content

Deprecate time iota ms#5792

Merged
cmwaters merged 9 commits intotendermint:masterfrom
sikkatech:deprecate_time_iota_ms
Dec 17, 2020
Merged

Deprecate time iota ms#5792
cmwaters merged 9 commits intotendermint:masterfrom
sikkatech:deprecate_time_iota_ms

Conversation

@ValarDragon
Copy link
Contributor

@ValarDragon ValarDragon commented Dec 13, 2020

Description

This PR removes the usage of time_iota_ms as a user-provided parameter, as its goal is to ensure time monotonicity. It hardcodes it in the code as 1ms, but still leaves the parameter available until the next breaking block version, so as to not break downstream users code.

Cref #2920

@codecov
Copy link

codecov bot commented Dec 13, 2020 •
edited
Loading

Codecov Report

Merging #5792 (2479946) into master (ebff8a9) will increase coverage by 0.13%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##           master    #5792      +/-   ##
==========================================
+ Coverage   59.77%   59.91%   +0.13%     
==========================================
  Files         262      262              
  Lines       23704    23704              
==========================================
+ Hits        14170    14202      +32     
+ Misses       8017     7993      -24     
+ Partials     1517     1509       -8
Impacted Files Coverage Δ
p2p/conn/connection.go 78.17% <75.00%> (ø)
config/config.go 78.31% <100.00%> (ø)
consensus/state.go 68.40% <100.00%> (+0.18%) ⬆️
p2p/switch.go 65.37% <100.00%> (-2.99%) ⬇️
rpc/jsonrpc/client/ws_client.go 57.79% <100.00%> (ø)
types/params.go 76.62% <100.00%> (ø)
p2p/transport_mconn.go 41.63% <0.00%> (-1.97%) ⬇️
blockchain/v0/pool.go 79.84% <0.00%> (-1.15%) ⬇️
consensus/replay.go 70.53% <0.00%> (-0.83%) ⬇️
... and 10 more

@ValarDragon
Copy link
Contributor Author

ValarDragon commented Dec 13, 2020

I don't think the tests (02) failure is related to this PR? Its a p2p test failure with nodes refusing to connect.
Also not sure whats going on w/ the Build & Push failure, do I need to change permissions on our sikka fork?

@tac0turtle
Copy link
Contributor

tac0turtle commented Dec 13, 2020

test (02) is non-deterministic due to some tests. I reran them, they may pass now. As for Build & Push this is because github prevents secrets from being used for forks. It is okay for it fail.

alexanderbez
alexanderbez reviewed Dec 13, 2020
View reviewed changes
melekes
melekes reviewed Dec 14, 2020
View reviewed changes
@ValarDragon @melekes
Update consensus/state.go
955114b 
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
melekes
melekes approved these changes Dec 14, 2020
View reviewed changes
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

cmwaters
cmwaters reviewed Dec 14, 2020
View reviewed changes
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

I would also remove the comments in config/config.go on lines 823 and 869 and add a note that the parameter is not used in proto/tendermint/types/params.proto#31 just to make sure there's no confusion. Other than that it LGTM

@tessr
Copy link
Contributor

tessr commented Dec 14, 2020

Nice. Also, whoever merges this - please make sure that the commit follows the style guide 🙏🏼

erikgrinaker
erikgrinaker reviewed Dec 14, 2020
View reviewed changes
@ValarDragon
Copy link
Contributor Author

ValarDragon commented Dec 16, 2020

Comments have been addressed, thanks for waiting!

@ValarDragon
Copy link
Contributor Author

ValarDragon commented Dec 16, 2020 •
edited
Loading

Apologies didn't realize theres a commit style guide. Here is a candidate one: (don't want to force push to update it)

consensus: Hardcode time_iota_ms to be 1ms, making the parameter unusde

time_iota_ms is intended to ensure that an honest validator always generates timestamps 
with time increasing monotonically. For this purpose, it always suffices to have this parameter
set to `1ms`. Allowing users to choose different numbers increases bug surface area.
Thus the code now ignores the user provided time_iota_ms parameter (marking it as unused), 
and uses 1ms internally.

erikgrinaker
erikgrinaker approved these changes Dec 16, 2020
View reviewed changes
Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

LGTM. We could consider logging a warning if set != 1ms, but from what I can tell that'd have to be on every block, which seems excessive.

cmwaters
cmwaters approved these changes Dec 16, 2020
View reviewed changes
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

Good stuff 👍 thanks

@cmwaters cmwaters self-assigned this Dec 17, 2020
cmwaters
cmwaters reviewed Dec 17, 2020
View reviewed changes
Copy link
Contributor

@cmwaters cmwaters left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey @ValarDragon, do you mind just making these changes (I feel we should make the deprecation aspect more explicit) and rebasing your branch so I can merge. Alternatively you can allow me to update your fork or if you're busy I can just copy this PR to a new one so that we can merge it.

@ValarDragon
Copy link
Contributor Author

ValarDragon commented Dec 17, 2020

I merged master in, and applied the suggested updates

@cmwaters cmwaters merged commit 8c0af72 into tendermint:master Dec 17, 2020
cmwaters pushed a commit that referenced this pull request Jan 4, 2021
@ValarDragon @cmwaters
consensus: deprecate time iota ms (#5792)
95826d5 
time_iota_ms is intended to ensure that an honest validator always generates timestamps 
with time increasing monotonically. For this purpose, it always suffices to have this parameter
set to `1ms`. Allowing users to choose different numbers increases bug surface area.
Thus the code now ignores the user provided time_iota_ms parameter (marking it as unused), 
and uses 1ms internally.
@tac0turtle
Copy link
Contributor

tac0turtle commented Jan 5, 2021

Will this PR be backported?

@alexanderbez
Copy link
Contributor

alexanderbez commented Jan 5, 2021

Will this PR be backported?

To what version(s)? That would constitute as an API breaking change I believe.

@tac0turtle
Copy link
Contributor

tac0turtle commented Jan 5, 2021

I believe the API stays the same. Working on spec updates/formatting and remembered this was done recently. I wanted to double-check if it would be backported since it was done in a non-breaking way.

@cmwaters
Copy link
Contributor

cmwaters commented Jan 5, 2021

It's non breaking but I think it was discussed in slack that because it constitutes a change in expected behaviour one might consider it still to be breaking. IMO, I would backport to 0.34.2

@tac0turtle
Copy link
Contributor

tac0turtle commented Jan 5, 2021 •
edited
Loading

Oh okay. I was gone the week this was merged. Makes sense!

@cmwaters cmwaters mentioned this pull request Mar 10, 2021
Closed
@cmwaters cmwaters mentioned this pull request Aug 19, 2022
Merged
@qinlz2 qinlz2 mentioned this pull request Oct 17, 2022
Closed
@jchappelow jchappelow mentioned this pull request Sep 13, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ebuchman ebuchman Awaiting requested review from ebuchman

@tac0turtle tac0turtle Awaiting requested review from tac0turtle

@tessr tessr Awaiting requested review from tessr

4 more reviewers

@alexanderbez alexanderbez alexanderbez left review comments

@cmwaters cmwaters cmwaters approved these changes

@erikgrinaker erikgrinaker erikgrinaker approved these changes

@melekes melekes melekes approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@cmwaters cmwaters

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@ValarDragon @tac0turtle @tessr @alexanderbez @cmwaters @erikgrinaker @melekes