[FEATURE REQUEST]: disable DEX features if system clock is not in sync

[cipig]

Somebody running AtomicDex Desktop 0.5.2-beta | 2.1.4315_mm2.1_9fe6e9402_Windows_NT_Release as maker is failing all swaps with taker_swap:821] Started_at time_dif over 60 61 because his system clock is wrong.
There is no banning for this, so no way to swap the coins if his offer is the cheapest in orderbook.
Please add system clock check on app startup and don't start it if clock is not in sync.

[tonymorony]

Since it might be actual on all interfaces probably is worth checking on the API level instead of a particular GUI. What do you think @artemii235 ?
full app startup blocking might be an overkill, just dex functionality disabling and placed orders canceling with the relevant message might be enough imo.

[artemii235]

I agree that preventing the app from starting at all is overkill.

just dex functionality disabling and placed orders canceling

Sounds like a good idea. The only question is: what source can we consider as a reliable time provider?

[tonymorony]

I agree that preventing the app from starting at all is overkill.

just dex functionality disabling and placed orders canceling

Sounds like a good idea. The only question is: what source can we consider as a reliable time provider?

We might use various ntp servers, e.g. https://gist.github.com/mutin-sa/eea1c396b1e610a2da1e5550d94b0453

[Milerius]

Also https://github.com/ripple/rippled/blob/develop/src/ripple/core/impl/SNTPClock.cpp

[artemii235]

Note for the implementation - we should also deny orders kickstart after a restart. It would be also nice to have a time_sync_status RPC, which GUIs will call to notify users about invalid local time.

[onur-ozkan]

We can calculate utc timestamp on user side and compare it with utc timestamp from the ntp servers. If diff is more than we expected, we can warn the user and disable operations which might cause problems.

And this should be performed in a loop with worker thread. We don't need to request ntp servers all the time for the comparison, one request on the mm2 initialization is enough. If the initial check works(which means diff is not higher than ~20-30 or x seconds), then we can use std::time::Duration to check if time is changed later manually.

[artemii235]

@ozkanonur Thanks for your comment! I would like to note one thing: if the time is fixed while MM2 is still running, we should likely enable DEX operations back. Or would it be easier to just ask the user to fix the time and restart the app?

[onur-ozkan]

@ozkanonur Thanks for your comment! I would like to note one thing: if the time is fixed while MM2 is still running, we should likely enable DEX operations back. Or would it be easier to just ask the user to fix the time and restart the app?

Since we have a loop already, I believe it can be done without restart

[shamardy]

if the time is fixed while MM2 is still running, we should likely enable DEX operations back.

We can do a check at the dispatcher level, this way we can decide which APIs should and shouldn't work when time is not in sync. We don't have to get the time from the ntp servers with each API call, the app start time and the duration @ozkanonur mentioned can be saved in the ctx and shared between threads.

So to summarize, with the checks @artemii235 and @ozkanonur mentioned, there should be 3 checks:

1. When starting the app, if time is not in sync, we shouldn't kickstart orders/swaps until it's fixed.
2. If the time is changed while the app is running, this should be detected using the worker thread loop. We should cancel unmatched orders. For matched orders and active swaps, I am not sure if we should fail those or just stop the operation until the time is fixed. What do you think @artemii235 @ozkanonur?
3. For APIs that require the time to be in sync, they should return an error when called if the time is not in sync.

[onur-ozkan]

For matched orders and active swaps, I am not sure if we should fail those or just stop the operation until the time is fixed. What do you think @artemii235 @ozkanonur?

What comes to my mind is freezing the operations until time gets synced back. But this might not be possible for swap operations because of timelocks. Maybe we should not get time values in middle of the swap operations. So if the operation starts, it won't get effected even if you change time manually.

[ca333]

"system clock" and "time" as such isn't a reliable metric - we will very likely use blockchain data to source the actual time (which showed to be more reliable than "system clock") as it would match across all users and the entire DEX-network. But we won't disable (DEX) features based on some (local/central) timestamps.

[cipig]

"system clock" and "time" as such isn't a reliable metric - we will very likely use blockchain data to source the actual time (which showed to be more reliable than "system clock") as it would match across all users and the entire DEX-network. But we won't disable (DEX) features based on some (local/central) timestamps.

Swaps are failing if the time is not in sync. Why not compare the time of the device with something from internet like https://time.is/ or NTP and not allow swaps if it doesn't match? It's actually very simple.

[onur-ozkan]

NTP implementation(which I explained above) was quite solid and reliable way to overcome this problem, so I haven't look for any other alternative ways(like the one @ca333 mentioned).

[onur-ozkan]

"system clock" and "time" as such isn't a reliable metric - we will very likely use blockchain data to source the actual time (which showed to be more reliable than "system clock") as it would match across all users and the entire DEX-network. But we won't disable (DEX) features based on some (local/central) timestamps.

I guess you want to get timestamp from the latest block of blockchain. If so, what if latest block was created sometime ago(like 40-50 seconds, or even more than 1 minute)? Also, Pairs would get race condition while asking for a latest block. To overcome race condition, one have to share the height of block with other pair, which I think becomes an overkill.

[cipig]

we could also just make the error message more informative... atm it's just taker_swap:821] Started_at time_dif over 60 61 and users don't seem to understand that, since i saw dozens of such failed swaps in a row from same pubkey, so user tried again and again without realizing what the problem is and that it won't simply go away by retrying, no matter how often

[shamardy]

we will very likely use blockchain data to source the actual time (which showed to be more reliable than "system clock") as it would match across all users and the entire DEX-network

I agree, this is also how it's handled in lightning/rust-lightning to calculate if an invoice has expired or not by saving the blockchain highest_seen_timestamp and comparing it with the expiry time. For this to be completely reliable, we need to validate the headers first as well so this depends on SPV implementation #1612.

[cipig]

needs to be precise though... we are talking about seconds, not minutes... mm2 fails swaps when time diff between taker and maker is bigger than 60s

[onur-ozkan]

I agree, this is also how it's handled in lightning/rust-lightning to calculate if an invoice has expired or not by saving the blockchain highest_seen_timestamp and comparing it with the expiry time. For this to be completely reliable, we need to validate the headers first as well so this depends on SPV implementation #1612.

If we don't overcome the race condition with sharing block height for timestamp, there will be potential swap failure.

Like:

Alice and Bob wants to read timestamp from latest block.

--> Alice wants to read timestamp from latest block(height is 555)
--> Bob wants to read timestamp from latest block (556 which is created couple ms after Alice's request)
--> if 556.timestamp - 555.timestamp is higher than 60 seconds, swap fails.

I know this can't happen on most of the chains specially eth, btc ones. But since the possibility is not %0 for all the chains we are supporting or we will support in the future, it's better to handle this in the implementation.

ps: This is only worth if we implement this just for swap timestamps, other than that the cost will be too much for using timestamps in mm2. It's like interacting with blockchain every single time when we need timestamp.

[shamardy]

we could also just make the error message more informative... atm it's just taker_swap:821] Started_at time_dif over 60 61 and users don't seem to understand that

This can be fixed easily, will open a PR for that.

needs to be precise though... we are talking about seconds, not minutes... mm2 fails swaps when time diff between taker and maker is bigger than 60s

Well, this part should be changed when we rely on blockchain timestamps.

If we don't overcome the race condition with sharing block height for timestamp, there will be potential swap failure.

We can add a buffer time until both get the latest block header, maybe 60 seconds.

ps: This is only worth if we implement this just for swap timestamps, other than that the cost will be too much for using timestamps in mm2. It's like interacting with blockchain every single time when we need timestamp.

If spv is enabled, block headers are downloaded whenever there is a new one and saved to the DB, so this will only require getting the latest header timestamp from DB. Or even saving it in a struct as it's done in highest_seen_timestamp.

[cipig]

needs to be precise though... we are talking about seconds, not minutes... mm2 fails swaps when time diff between taker and maker is bigger than 60s

Well, this part should be changed when we rely on blockchain timestamps.

i guess that, if you increase the threshold, some other things will fail, like the refund in case of failed swaps

[shamardy]

i guess that, if you increase the threshold, some other things will fail, like the refund in case of failed swaps

Swaps/timelocks already depend on block timestamps since the refund path is activated when a block with a timestamp is mined that surpasses the timelock.

[cipig]

i don't see how fetching the timestamp of latest block of some chains will solve this issue
that timestamp will be the same, no matter what your clock is set to
it just shows when block was mined (according to the miner)... it has nothing to do with the system clock

[shamardy]

i don't see how fetching the timestamp of latest block of some chains will solve this issue

It's a single source of truth for time, which was the intention, intervals are in blocktime instead of seconds though (10 minutes is too much for an interval for instance). I understand that this will bring some complexity that we are not currently aware of, but it's an idea worth considering.

[cipig]

look at block explorers and you will see times from the future, (in a few seconds) like this from https://kmd.explorer.dexstats.info/

because the timestamp is set by the miner
and it can also be "in 3 minutes"

[onur-ozkan]

If spv is enabled

We don't have spv proof implementation for all the coins we have and it can be disabled by the coins configuration

i don't see how fetching the timestamp of latest block of some chains will solve this issue

We don't really care about if the time is correct in real world or not, we want to sync the pairs so time calculations will be more precise.

IMO, ntp solves time problem in better way and general not just for swaps, and it's cost is much less than reading time from the chains which sounds little bit problematic for less popular coins. Ntp will sync pairs in exact time no matter which chain they are going to use or not. I also like the blockchain idea, but I don't see how it can be better than ntp one. And it can fail the swaps as I demonstrated an example above. Not every chain has couple of seconds as average block time.

[cipig]

i also think that ntp or something similar is the best option
wrong clocks have lots of other negative effects, not just for swaps on ADEX... eg if miners have a wrong clock
so getting users to fix their clock will benefit other things too
but if that is too complicated, i am fine with a better error message too... so that they at least don't try/retry like crazy to make swaps that will all fail anyway

[shamardy]

IMO, ntp solves time problem in better way and general not just for swaps, and it's cost is much less than reading time from the chains which sounds little bit problematic for less popular coins.

It's the best solution of course, I think the only problem with NTPs is that they are a centralized service. I guess the ntp pool is at least a distributed network with thousand of devices, so this should be considered too.

[onur-ozkan]

look at block explorers and you will see times from the future, (in a few seconds) like this from https://kmd.explorer.dexstats.info/ because the timestamp is set by the miner and it can also be "in 3 minutes"

according to that ss, kmd swap can fail very likely due to race condition.

[shamardy]

according to that ss, kmd swap can fail very likely due to race condition.

locktimes are chosen to be long enough to avoid this, also waiting for maker payment confirmations at the maker side is 2/5 of the taker locktime.

[ca333]

glad to see the input kickstarted this thriving discussion - thanks for all inputs!

NTP implementation(which I explained above) was quite solid and reliable way to overcome this problem, so I haven't look for any other alternative ways(like the one @ca333 mentioned).

This is definitely a (quick) option. Ripple & Co. use this as well as referenced above.

As a clean solution we could also very simply achieve a "decentralised clock" (which is preferred) where the ADEX protocol ensures that clients "agree on time" without using system clocks or such centralised layers at all (using a relative starting point / as described below).

We could also execute the "overkill" project (not too hard actually) and implement a type of decentralised NTP protocol where all our seeds and majority of network peers fetch NTP time from different randomised NTP servers, sign it, share it with each other and have a broadcast each second where they all agree on a specific tolerance level with a retrospective validation to track avg. time-offset, etc. This basically extends ADEX into a DNTP.

"system clock" and "time" as such isn't a reliable metric - we will very likely use blockchain data to source the actual time (which showed to be more reliable than "system clock") as it would match across all users and the entire DEX-network. But we won't disable (DEX) features based on some (local/central) timestamps.

Swaps are failing if the time is not in sync. Why not compare the time of the device with something from internet like https://time.is/ or NTP and not allow swaps if it doesn't match? It's actually very simple.

closing this ticket was mainly ref. the request to disable certain features if system clock is off while we can just ensure that the ADEX client side "agree on the time-factor" without being impacted by the system-clock. Will open a [bug] swaps failing if system clock time wrong instead and ref. to this one.

"system clock" and "time" as such isn't a reliable metric - we will very likely use blockchain data to source the actual time (which showed to be more reliable than "system clock") as it would match across all users and the entire DEX-network. But we won't disable (DEX) features based on some (local/central) timestamps.

I guess you want to get timestamp from the latest block of blockchain. If so, what if latest block was created sometime ago(like 40-50 seconds, or even more than 1 minute)? Also, Pairs would get race condition while asking for a latest block. To overcome race condition, one have to share the height of block with other pair, which I think becomes an overkill.

yes - but not necessarily since we have multiple time sources within a blockchain on-chain layer but also blockchain off-chain channels which allow us to "cryptographically agree with each other / across the network on a time" (I say "a time" since it possibly becomes a relative value in this regard of the order matching process).
what this means as an example: the order matching procedure is a real-time operation so we can measure time from when something starts / specific event. In our case e.g. a ACK package (relative from the moment of time we started network commz / both clients completed handshake, etc.), et cetera towards the completion of the swap (can use "blockchain timeouts" for the swap process with both, block-times or block-heights and always using that starting point as our relative "begin of time") . This is like a "noob proof" method to not rely on sys clocks / NTPs, etc. at all. However this approach will require additional thoughts and a bit of work.

Ref. the actual blockchain time I am not too concerned - there's ETH2.0 with around 6 blocks per minute and XRP (admitted, not the most "decentralized solution" & we d indirectly use NTP) with ~3 second blocks. And many more chains suitable to become our "dClock".

we will very likely use blockchain data to source the actual time (which showed to be more reliable than "system clock") as it would match across all users and the entire DEX-network

I agree, this is also how it's handled in lightning/rust-lightning to calculate if an invoice has expired or not by saving the blockchain highest_seen_timestamp and comparing it with the expiry time. For this to be completely reliable, we need to validate the headers first as well so this depends on SPV implementation #1612.

yes, as it makes a lot of sense for DeFi protocols to use such an approach of "sourcing time from the blockchain layer" or other "decentralised layers" to specifically remove dependency on (sys cock) times.. My inspiration came from this angle - and we faced a relevant issue in the past.

needs to be precise though... we are talking about seconds, not minutes... mm2 fails swaps when time diff between taker and maker is bigger than 60s

block-spacing shouldn't be an issue - we pretty much reached real-time network capabilities across many leading protocols and on various technology stacks

I agree, this is also how it's handled in lightning/rust-lightning to calculate if an invoice has expired or not by saving the blockchain highest_seen_timestamp and comparing it with the expiry time. For this to be completely reliable, we need to validate the headers first as well so this depends on SPV implementation #1612.

If we don't overcome the race condition with sharing block height for timestamp, there will be potential swap failure.

Like:

Alice and Bob wants to read timestamp from latest block.

--> Alice wants to read timestamp from latest block(height is 555)
--> Bob wants to read timestamp from latest block (556 which is created couple ms after Alice's request)
--> if 556.timestamp - 555.timestamp is higher than 60 seconds, swap fails.

I know this can't happen on most of the chains specially eth, btc ones. But since the possibility is not %0 for all the chains we are supporting or we will support in the future, it's better to handle this in the implementation.

ps: This is only worth if we implement this just for swap timestamps, other than that the cost will be too much for using timestamps in mm2. It's like interacting with blockchain every single time when we need timestamp.

if alice read/used timestamp from 555 she could just let bob know about this detail - i.e. this could be our "start of time" - and this way it could simply prevent such a race condition on a protocol level. They would both use the same timestamp in this case for starting point and ensure they agree on this.
This does mean if we decide to use a blockchain time source we would use just one (fast) chain (+ cross validation layer) independently of the trading pair - since we are only interested in using it as our "decentralized clock" - nothing else. And ref the cost will be too much I think it should be quite cheap and we shouldn't have to fetch the time from chain each time the user interacts with the DEX order matching process and assume that at some point there will be an event stream (i.e. mm2 getting notified from SPV layer) for blockchain updates, etc. so as Omar also outlined, we can persist the data into the planned caching layer or even mm2 runtime scope et cetera.

Furthermore we could also go by pure height check:
Alice sends order request at DNTP_chain.height 777
Bob send accept/match pkg at DNTP_chain.height 778
`if bob.DNTP_chain.height - alice.DNTP_chain.height < 3 then success (or whatever "time rel" we think is acceptable)

*DNTP_chain refers to our "decentralized clock" (= a robust/reliable and fast chain).

look at block explorers and you will see times from the future, (in a few seconds) like this from https://kmd.explorer.dexstats.info/ because the timestamp is set by the miner and it can also be "in 3 minutes"

yes, KMD obv wouldn't be suitable for this use-case.

If spv is enabled

We don't have spv proof implementation for all the coins we have and it can be disabled by the coins configuration

i don't see how fetching the timestamp of latest block of some chains will solve this issue

We don't really care about if the time is correct in real world or not, we want to sync the pairs so time calculations will be more precise.

IMO, ntp solves time problem in better way and general not just for swaps, and it's cost is much less than reading time from the chains which sounds little bit problematic for less popular coins. Ntp will sync pairs in exact time no matter which chain they are going to use or not. I also like the blockchain idea, but I don't see how it can be better than ntp one. And it can fail the swaps as I demonstrated an example above. Not every chain has couple of seconds as average block time.

it does solve it faster (also more comfy / simpler from a dev perspective) - I don't think better/cleaner though. Also extends on threat model.

We don't really care about if the time is correct in real world or not, - exactly, thus we could use a "relative approach" and still use blockchain as its source as outlined above. We already attach to SPV - technically such path will not be more work than a multi-NTP endpoint integration.

Not every chain has couple of seconds as average block time. - wouldn't have to rely on many chains for this as outlined above.

As a wrap up, summary & options / possible action item:

(0) general check/evaluation: make order matching process "entirely independent from a sys time / don't compare client provided timestamps / use diff. metric / source"

actual solutions:

(i) Use NTP to sync time & create time consensus between all peers. Use a NTP pool and have peers pick randomly. This method does the job and seems sufficient. (optionally - use means of cryptographic validation)
SoW: we set up a NTP-src-endpoint with get_ntp_time() capability which rotates through a list of NTPs which our team maintains OR we optionally integrate this directly into mm2. Afterwards when clients aware of this "global consensus time" we can have a simple check on network commz basis and add client_time to request header or payload.

(ii) Use specific blockchain source and either (A) block-time or (B) block-height checks (or hybrid option) - which is bit more elegant.
SoW: we just extract additional metric (time or height) from blockchain stack (already within mm2 runtime scope) and use that as outlined above.

(iii) implement DNTP as outlined above - very elegant but probably overkill.
SoW: this would be a step 2 to option (i) - now as all clients/seeds are able to fetch a accurate time from our NTP-src-endpoint they sign a msg and interval-broadcast across network with the above outlined schematic which expands mm2 into a "decentralized clock with p2p/decentralized consensus" as the network participants could agree on time-offset to even ban misbehaving "timetravelers", et cetera.

my 2 or 3 satoshis on this matter - feel free to tweak suggestions and to propose diff. solutions!

references [WIP]:
https://eprint.iacr.org/2019/1348.pdf

[onur-ozkan]

An alternative solution would be to implement a logic that compares the client's time with the time of the currently connected seed nodes. If the client isn't synced with any seed node, we can assume the client's clock is out of sync and abort the KDF runtime with a message saying "Your time isn't synced. Please sync it before running KDF." This way, we only rely on the seed nodes of our network without trusting/needing any external sources.