Skip to content
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.

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

Create or integrate linter to check time.Now use #10329

Closed
2 of 4 tasks
robert-zaremba opened this issue Oct 8, 2021 · 6 comments
Closed
2 of 4 tasks

Create or integrate linter to check time.Now use #10329

robert-zaremba opened this issue Oct 8, 2021 · 6 comments

Comments

@robert-zaremba
Copy link
Collaborator

robert-zaremba commented Oct 8, 2021

Summary

time.Now is not deterministic in the network and must not be used.

Problem Definition

Using time.Now in a context of protobuf Msg service causes consensus problems and must not be used.

Proposal

Create (or find) and integrate a linter to check that we don't use time.Now in all Go code which is related to the state transition. We can use time.Now, in:

  • **/cli/**
  • Query service (there can be a trap if Msg service will use Query service)
  • *_test.go files
  • telemetry

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@robert-zaremba robert-zaremba added this to the v0.45 milestone Oct 8, 2021
@ValarDragon
Copy link
Contributor

It should also exempt telemetry, and then we should move all time.Now() calls for telemetry into a method within telemetry. (A good idea separately, for #10245 ). e.g. doing telemetry.StartTimer() instead of time.Now() as one of the args.

@waggonerjake
Copy link
Contributor

Just to be extra clear here, time.Now() calls can exist in the above mentioned places (i.e. **/cli/**, *_test.go and the Query Service)?

You say "DONT use time.Now() in all Go code which is NOT in ..." which sounds like do use them in those 3 pieces... would like some clarification.

@alexanderbez
Copy link
Contributor

Yes, subjective time can be used anywhere except the state-machine, i.e. module business logic.

@robert-zaremba
Copy link
Collaborator Author

I rephrase the description to make it easier to read.

@tomtau
Copy link
Contributor

tomtau commented Nov 3, 2021

This pattern should be captured in https://github.com/crypto-com/cosmos-sdk-codeql/blob/main/src/system-time.ql -- it will ignore CLI, tests and telemetry.
It doesn't ignore those in query services, because msg services may use query services -- the false positives could be then individually dismissed in GitHub UI: https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/managing-code-scanning-alerts-for-your-repository#dismissing-or-deleting-alerts

This issue can be closed after #10488 is merged.

@robert-zaremba
Copy link
Collaborator Author

Thank you @tomtau
Closed by #10488

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants