revamped TIER1 discovery protocol #8085
Conversation
pompon0
commented
Nov 18, 2022
pompon0
commented
- added some extra fields to AccountData, renamed a bunch of them for better readability. This PR is not backward compatible in a sense that the old binary won't accept new AccountData and vice versa (protobuf compatibility is preserved, however the custom validation logic will treat different fields as required). That's ok as the broadcasted data is not interpreted yet.
- changed identifier of AccountData from (EpochId,AccountId) to AccountKey, which simplifies a lot of stuff and generally makes more sense
- changed version ID of AccountData from an UTC timestamp to an integer: this allows to avoid bad impact of a wall clock time shift, which can be especially dangerous since we do not version AccountData by EpochId any more. The creation timestamp is still present in AccountData to be able to debug data freshness. We can additionally implement an expiration policy in the future based on the timestamp.
- rearranged the code related to broadcasting AccountData to align with the TIER1-related code which comes next after this PR
pompon0
requested review from
mm-near and
matklad
and removed request for
mm-near
| // Version of the AccountData. A node can override a previous version, | ||
| // by broadcasting a never version. | ||
| uint64 version = 7; | ||
| // Time of creation of this AccountData. | ||
| // TODO(gprusak): consider expiring the AccountData based on this field. | ||
| google.protobuf.Timestamp timestamp = 4; |
There was a problem hiding this comment.
Why do we need both version and timestamp? It seeems that the timestamp can double as version for the purposes of invalidation, no?
There was a problem hiding this comment.
timestamp is the UTC time at which the AccountData was created. There is no guarantee that UTC clock will be monotone, and we want to be able to override the previous version of AccountData even if UTC clock moved backwards in time.
There was a problem hiding this comment.
So we can do smth like max(prev_timestamp + 1, utc::now()), basically force monotonicity
There was a problem hiding this comment.
ok, look, I want to actually track the time at which the thing was signed, so that I can view it and debug.
And I don't want a situation in which somebody at some point produces a piece of data 100 years into the future and then suddenly the timestamp is broken for the next 100 years.
I also want to eventually implement expiration policy on these data and given that the clocks on different machines are skewed it will really make it hard to reason about this if we add custom behavior of max(prev_timestamp+1,now()) on top of that.
|
@matklad I've answered the questions inline. |
pompon0
requested review from
a team and
akhi3030
and removed request for
a team
* added some extra fields to AccountData, renamed a bunch of them for better readability. This PR is not backward compatible in a sense that the old binary won't accept new AccountData and vice versa (protobuf compatibility is preserved, however the custom validation logic will treat different fields as required). That's ok as the broadcasted data is not interpreted yet. * changed identifier of AccountData from (EpochId,AccountId) to AccountKey, which simplifies a lot of stuff and generally makes more sense * changed version ID of AccountData from an UTC timestamp to an integer: this allows to avoid bad impact of a wall clock time shift, which can be especially dangerous since we do not version AccountData by EpochId any more. The creation timestamp is still present in AccountData to be able to debug data freshness. We can additionally implement an expiration policy in the future based on the timestamp. * rearranged the code related to broadcasting AccountData to align with the TIER1-related code which comes next after this PR
