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

Add time since genesis to sysvar::clock #7289

Merged
merged 5 commits into from
Dec 12, 2019

Conversation

rob-solana
Copy link
Contributor

Problem

no notion of wallclock available to programs

Summary of Changes

add simple, good enough unix_timestamp to bank, sysvar::clock, based
on genesis creation timestamp

Fixes #

@mvines
Copy link
Member

mvines commented Dec 5, 2019

How do you intend to use this clock, and is Tyera's work in #7115 not good enough? We'll going from no clock to two clocks otherwise.

@rob-solana
Copy link
Contributor Author

How do you intend to use this clock, and is Tyera's work in #7115 not good enough? We'll going from no clock to two clocks otherwise.

I plan to use this clock for lockups that expire from some date vs. some number of epochs from genesis. The only reason I've decided not to calculate the epochs from when genesis ran (which is the same as this does) is because it'll be useful for vests later.

Tyera's clock is different, can be used as a stake-weighted wallclock, and it appears in the ledger. This one is just genesis unixtimestamp + slots (already calculated for consumers).

@codecov
Copy link

codecov bot commented Dec 5, 2019

Codecov Report

Merging #7289 into master will decrease coverage by 7.4%.
The diff coverage is 61.1%.

@@           Coverage Diff            @@
##           master   #7289     +/-   ##
========================================
- Coverage    71.8%   64.3%   -7.5%     
========================================
  Files         245     240      -5     
  Lines       54595   57157   +2562     
========================================
- Hits        39215   36806   -2409     
- Misses      15380   20351   +4971

@rob-solana rob-solana requested a review from garious December 5, 2019 16:52
@garious
Copy link
Contributor

garious commented Dec 5, 2019

I plan to use this clock for lockups that expire from some date vs. some number of epochs from genesis.

Is there such a requirement? There's a 6 month expiration after launch, but technically, genesis could be idling a long time before launch.

@mvines
Copy link
Member

mvines commented Dec 5, 2019

There's no concern about the timestamp here drifting if the cluster stalls for a time, or is reset?

@garious
Copy link
Contributor

garious commented Dec 5, 2019

There's no concern about the timestamp here drifting if the cluster stalls for a time, or is reset?

Drift should be okay. What's important is that all unlocking occurs at the same time.

@mvines
Copy link
Member

mvines commented Dec 5, 2019

But why not use Tyera's clock? We're building this for an external need regardless

@rob-solana
Copy link
Contributor Author

I plan to use this clock for lockups that expire from some date vs. some number of epochs from genesis.

Is there such a requirement? There's a 6 month expiration after launch, but technically, genesis could be idling a long time before launch.

The requirement comes and goes. It has most recently come again. Besides the network launch-based lockups, there are overlapping required holding periods. Stake lockups will have a semantic like: (clock.epoch > lockup.epoch) && (clock.unix_timestamp > lockup.unix_timestamp)

@rob-solana
Copy link
Contributor Author

But why not use Tyera's clock? We're building this for an external need regardless

Where is Tyera's clock exposed to programs? What if big stakers want to collude and get out of their lockups?

@rob-solana
Copy link
Contributor Author

There's no concern about the timestamp here drifting if the cluster stalls for a time, or is reset?

minimal concerns. we could allow custodian to change the lockups, too...

@mvines
Copy link
Member

mvines commented Dec 5, 2019

Where is Tyera's clock exposed to programs?

It's not exposed yet but it could be.

What if big stakers want to collude and get out of their lockups?

Ok that sounds like a decent argument to not use "Tyera's clock"! Thanks

@garious
Copy link
Contributor

garious commented Dec 5, 2019

The requirement comes and goes. It has most recently come again.

The requirement needs to be in the SSOT before we move forward here. Please send me a direct message showing where that's coming from.

@garious
Copy link
Contributor

garious commented Dec 5, 2019

Generally speaking, I like Solana-controlled date oracle accounts as the stopgap solution for all date issues.

@rob-solana
Copy link
Contributor Author

rob-solana commented Dec 8, 2019

Generally speaking, I like Solana-controlled date oracle accounts as the stopgap solution for all date issues.

for stake lockups, I think there are only 2 reasonable options:

  1. a runtime-defined value, not something Solana controls
  2. an oracle controlled by the creator of the stake, i.e. the entity that initialized the lockup (which, now that I think about it, is currently the staker)

this PR makes 1)

I'd have to define another "date oracle" field in StakeState to do 2)

I'm starting to think that we don't need stake_state::Lockup, we only need custodian, and Solana can enforce lockups operationally (which we're already planning) instead of trying to make the network know about wallclock time.

dropping lockup by time/date would also make genesis stakes more liquid/flexible. they could be split and withdrawn any time the custodian agrees

CC @aeyakovenko

@aeyakovenko
Copy link
Member

@rob-solana @garious solana controlled oracle puts to much control on us and has legal implications. A best effort slot approximation is what we want. We should be adjusting hashes per slot every epoch automatically.

@garious garious changed the title genesis timestamp Add time since genesis to sysvar::clock Dec 11, 2019
@rob-solana
Copy link
Contributor Author

@mvines this is ready for another once-over by you

@mvines

This comment has been minimized.

/// computed unix_timestamp at this slot height
pub fn unix_timestamp(&self) -> i64 {
self.genesis_creation_time
+ ((self.slot as u128 * self.ns_per_slot) / 1_000_000_000_000) as i64
Copy link
Contributor

Choose a reason for hiding this comment

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

Sure you have the right amount of zeros there? At a glance, it looks like 3 too many.

@mvines
Copy link
Member

mvines commented Dec 12, 2019

The PR contents itself looks good to me!

@rob-solana rob-solana merged commit 777ae3c into solana-labs:master Dec 12, 2019
@rob-solana rob-solana deleted the genesis-timestamp branch December 12, 2019 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants