-
Notifications
You must be signed in to change notification settings - Fork 136
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
Change default value for origin_time #3229
base: develop
Are you sure you want to change the base?
Conversation
f385c69
to
d0aceac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are other important implications to this. One is that origin_time is a useful lower bound on the range of valid timestamps, and if we have an origin time going all the way back to the 70's (or even back to last year), that's just more wiggle room for bad actors to try something.
More importantly, the origin_time is fundamental to the way we do gossip and is the basis of a lot of other future features we want to add. In particular the further back from the current moment the origin_time is, the more "regions" we need to use to represent that history with. The number of regions we send at each gossip round is logarithmically proportional to the total length of history since origin_time.
I loaded this into my brain again and I might not have it perfectly but I'll try to show the math and get to the result: we need to send an average of something like
bytes at each gossip round just to figure out how up to date we are.
So one way to say this is, every time
A lot of this could be tuned differently, and we could even put in a threshold so we stop logarithmically growing the space we need to represent the distant past up to a point which would make the origin time less important. But this is currently how things stand.
Also, if we do this, Timestamp::ZERO is not actually the UNIX epoch, it's much further back. We could use HOLOCHAIN_EPOCH
though, which is Jan 1 2022. That would start us at around the 6.5KB per round mark which seems reasonable.
A question is, do the benefits outweigh the cost? Maybe so, since the cost is quite low right now. But there is a cost.
Thanks for the details, I hadn't realised it was quite that much extra data if this default gets used. I would like to see this field set by each app and the default not used ideally because I don't think there's an ideal default value. What I'm after is just a value that doesn't move on different installs because that means new nodes joining a network expecting to see existing data will quietly not see it. So if the default value is a fixed point in time and usually overridden that'd be good. Alternatively, this could be a required field and we could make |
Oh I didn't realize that was your impetus. If different people install the same app, I'm pretty sure they're not all going to get different origin times. The default is just on the Builder, which I think only gets used in a handful of places, and only in tests. The place where now() is used for real DNAs is in I can't vouch for the origin time not changing when updating coordinator zomes, but any time you have a DNA manifest at hand, and use it to do anything, those values will not be changed automatically. Pretty sure the only time defaults are invoked is when you're creating a manifest from scratch, which you only typically do at the very beginning of a development project, once. |
Ahh okay, I ran into this in a Kitsune test where I thought I ended up with different timestamps but maybe I did something there that isn't possible on code paths from Holochain, or maybe something else was wrong. I'll do some more testing with this and come back with some evidence or maybe this can just be closed :) |
This item has been open for 30 days with no activity. |
This item has been open for 30 days with no activity. |
@abe-njama I think it's OK to let stalebot do its thing and close stale PRs like this one. We all get notified when it goes stale after a month, and if nobody does anything about that, it's probably safe to let it fade away |
This item has been open for 30 days with no activity. |
Summary
I'm proposing this change because it's caught me out a couple of times and I think it's a confusing default.
The pros for changing it:
Timestamp::now()
moves so that the gossip range for each install is different. This is valid from Kitsune's point of view but probably not what the author intended.The cons for changing it:
Timestamp::now()
if somebody does want this behavior.I'm inclined to say that if happ devs want to control visibility of existing data when new users join a network then that should be app behavior and not Kitsune skipping gossiping. If users of a happ want to skip syncing a lot of historical data because they don't care about it, they're free to set a recent
origin_time
when they install the happ.So I think with this change all the behavior we want is maintained, but with a less surprising default value for this field.
TODO: