Study time synchronization issues

[TheQuantumPhysicist]

PR #1017 is an attempt at fixing time synchronization issues between nodes. However, this is not enough. Here are a few issues:

• Nodes can have clocks that drift over time
• A block far in the future shouldn't always result in banning the peer, but should instead try again later. Currently an invalid block (due to being a little in the future, more than the limit) will make the block invalid, but then will result in subsequent blocks to have no parent, and hence being orphan, resulting in banning the peer.
• There has to be an error with zero ban score for blocks not too far in the future (a few minutes) and a ban on blocks way too far in the future.
• The strategy implemented in Clock sync fixes #1017 is problematic because it ruins the usage of mocked time. Waiting isn't really the best wait to do this. The handle_header_list check that delays time should go eventually.

[pavel-kokolemin]

Some info about how other blockchains handle clock differences:

1. Ethereum:
   From https://github.com/ethereum/go-ethereum/issues/20498:

The discovery protocol relies on clock drift being smaller than 30 seconds. Currently you need to ensure that in order to run a node. We're working on a secondary discovery protocol based on DNS that should not have this requirement.

2. Bitcoin:
   No peer disconnects, the node adjusts the local time by taking the median of all known clock differences.

3. Cardano
   From https://github.com/input-output-hk/cardano-node/issues/2529:

As is well-known, clock skew causes the node to suspend its peers, because the blocks downloaded from them are "in the future" (see the error message in the next section).
Once all peers are suspended, the node chain degrades, and after a while it obtaining the ledger state.

And some interesting reading about Cardano's problems with blocks from the future - https://github.com/input-output-hk/ouroboros-network/issues/4251

[TheQuantumPhysicist]

It's helpful to take a look at how bitcoin deals with time offsets: https://github.com/bitcoin/bitcoin/blob/5bbf735defac20f58133bea95226e13a5d8209bc/src/timedata.cpp#L30

[iljakuklic]

Related discussions from bitcoin, in no particular order:

• AddTimeData will never update nTimeOffset past 199 samples bitcoin/bitcoin#4521
• P2P: add maxtimeadjustment command line option bitcoin/bitcoin#7573
• Change -maxtimeadjustment default from 70 minutes to 0 bitcoin/bitcoin#24606
• addrman: Use system time instead of adjusted network time bitcoin/bitcoin#24662
• p2p: remove adjusted time bitcoin/bitcoin#25908

A quick summary of some points raised:

• Bitcoin is much more tolerant to time drift than a PoS protocol, fair amount of discussion takes advantage of that.
• Time differences are sampled from outbound peers only. Makes it more difficult for an adversary to make majority of peers to report bogus time.
• Many projects with codebase derived from bitcoin (at least bitcoin cash and zcash) have set the max adjustment option default to 0.
• As a result of various historical code flukes, the time adjustment is in effect calculated as median of the first 199 outbound peer time diffs.
• The most common opinion regarding a sensible next step seems to be to gradually phase out time adjustment but keep a warning to notify the user when node's clock diverges too much from peers.
  • Difficult to automatically detect whether our clock is off or peers are trying to confuse us, so left to user intervention.
• Bitcoin syncs only when we connect to a peer (first 199 of them). Since it can tolerate up to ~2hrs of drift, it's assumed one-time adjustment is sufficient. We face much tighter tolerances so it should probably be done periodically.
• Points have also been raised against completely removing time adjustment:
  • Some existing users may rely on it (not applicable here)
  • It may be useful when casual users don't want to be bothered about extra configuration, i.e. less configuration for users to do. Having a user-friendly software helps decentralisation somewhat.

[TheQuantumPhysicist]

Due to prioritizing creating an RPC wallet, this will most likely not make it to mainnet. But it's still a priority. It's going to be removed from the mainnet milestone.