search

vacp2p / mvds

Minimal Viable Data Sync Implementation
https://specs.vac.dev/specs/mvds.html
MIT License
13 stars 3 forks source link

Set SendEpoch instead of cumulating it #86

Closed cammellos closed 3 years ago

cammellos commented 3 years ago

CalculateNextEpoch should set SendEpoch instead of cumulating it.

This resulted in a bug where new accounts would actually use mvds for re-transmission, while as time passed and epoch grew, messages would stop being transmitted as it would just double and double the epoch.

oskarth commented 3 years ago

Adding @staheri14 and @jm-clius as reviewer more as FYI to be aware of exposed to MVDS and Core syncing issues.

cammellos commented 3 years ago

Send Epoch and Send Count MUST be increased every time a record is retransmitted. Although no function is defined on how to increase Send Epoch, it SHOULD be exponentially increased until reaching an upper bound where it then goes back to a lower epoch in order to prevent a record’s Send Epoch’s from becoming too large.

Yes, that's still valid, as an observation because it's left to the application the calculation, MIGHT is probably better than SHOULD, as it's completely application dependent, some might want to retry in constant time, say if it's a bunch of high availability nodes talking to each other for example.

until reaching an upper bound where it then goes back to a lower epoch in order to prevent a record’s Send Epoch’s from becoming too large.

This I think it needs fixing, currently it's not an issue as we use I believe an int64 so it will never wrap at our pace (300 millisecond per epoch), but as a client of the library I would want a way to stop retransmission of a particular message, rather than have it re-transmitted forever (it becomes meaningless once it wraps around). But not an issue at the moment as it won't wrap around at any point, though it will still pollute the database.

Also, we are not currently at the latest release, we are at v0.0.23, and there's significant changes on v0.0.25 (breaking api change) so if that's ok with you I would create a release v0.0.23+hotfix.1 (or any naming/branch of your choosing) as we want to minimize risk and 0.0.25 brings changes that we won't be using at the moment.

cheers

cammellos commented 3 years ago

Would it be easier for you if mvds repo was transferred to status-im org?

We don't currently change it very often, so for now I think it's safe to stay here, but if we have to make more significant work it would make sense.

As for breaking change in latest version I don't remember or know what this is about, perhaps @decanus knows?

Yes, we added dependencies (it's a breaking change in the API, so it's just a matter of changing the code when pulling in the library, not that we have to reset database etc), just we would not use the new feature at the moment and it's safer for us to keep changes to a minimum.

The only request would be to raise specs issues/proposed changes in specs repo so we keep these in sync. Will do, thanks