Concept ACK. This is the "one" way of "Never go to sea with two chronometers; take one or three.".

Yes, please change the default...

No need to call AddTimeData() at all if we trust system clock. Or you can collect time samples and warn user when his system clock is wrong "a lot"...

Concept ACK. But the default value should be set to false.

Concept NAK; especially with the default value.

As is, there is almost never any active monitoring it's quite common for systems even with network time configured to have totally incorrect time because the network time has simply silently stopped working. At RWC some persons from the google chrome team presented data collected from chrome users that showed very high rates of time inaccuracy on hosts (and also showed that a significant fraction of all SSL certificate errors are from clients with the wrong date).

IIRC Gnutella has the ability to query the local NTP daemon to determine if it believes that its in sync. It might be more reasonable to have an option to "use local time if and only if the local time server believes that it is in sync".

Of course all this ignores that the normal configuration of NTP is completely insecure, easily spoofed, will believe completely implausible information (e.g. that time is a decade before the release of the software), no effort made against sybil resistance, and there are already known incidents of malicious ntp servers in the public NTP pools... but as a default off option, perhaps that is less of a concern.

@paveljanik @wangchun OK, I changed the default to false. I pretty much expected that to be the first thing mentioned, thanks for confirming :) I only left it that way to keep merging easier for anyone that was already using it downstream.

@gmaxwell Thanks for your perspective. That point about clocks causing cert validation errors is interesting. I would not have guessed that. With the default changed to false now, does that make this more acceptable to you? The latest commit basically leaves people in the same boat as they are now, but it gives knowledgeable users that choose to override the ability to use only their local clock.

@mruddy Can you lease add warning to the user when his clock is too different to the network time?

@paveljanik The existing warning will still trigger for that scenario. Please see here https://github.com/mruddy/bitcoin/blob/a1a4d8a5de73e018485ec8fe67d3918b625ed0d0/src/timedata.cpp#L110.
This is because the P2P version message still calls AddTimeData. I didn't change any of that. This change leaves all that old time computation stuff, but ignores the offset that is calculated from it if the new command line switch is set. This is a super simple commit.

@mruddy Have you looked into making it conditional on the local NTP thinking its in sync?

@gmaxwell No, but not because I ignored the input.

The first reason is that I don't trust myself to not mess it up and add some kind of vector where a malformed response would allow node takeover etc... The extra attack surface makes me apprehensive.

Second, when I think of how this would be used, the first use-case that comes to mind is of a node operator that notices some other node(s) trying to jack up its perspective on time and (s)he wants to stop that. I envision that admin to be actively monitoring that his/her system time is within reason. The second use case is that of an operator that simply wants to trust the local clock as a standard operating procedure. Again, I envision that user as having made a conscious effort to commit to maintaining the node time through normal process and procedure.

Lastly, in the case where NTP has been spoofed/tampered, or is just not reliable (silent failure etc...), as you mentioned before, then the extra complexity does not seem to help by my way of thinking.

Indeed, detection would not help when someone is exploiting NTP's normal lack of security to change the system time; I didn't intend to suggest otherwise. Sorry.

I don't agree with your attack surface concern, after all-- it's all local-- portability may turn out to be more of an issue. But code to harden up this option to throw an error or refuse to operate when set to trust the local clock but the local clock is detectably undisciplined could be done as a separate PR.

Am I incorrect in my recollection that the getargument stuff is slow? Do we want that inside GetAdjustedTime?

Yep, you're absolutely right that portability might be the devil in the details for the NTP checking. Even being all local, it's still conditional logic that could be triggered under a remote attacker's influence. So, at that point it depends on your threat model whether an attacker being able to influence that exchange is in scope. If that's in scope, then possibly a concern that doesn't depend on a buffer overflow etc... would be where that could be used as a trigger for a previously installed trojan that listens where the NTP listener would have been and then does something bad. That's getting a little far fetched, but it's what came to mind.

I'll have to double check on the performance question and get back to you. I know when I first worked on this about six months ago or so, I did wonder that. We went with it, but I forget if that was just because it was the typical pattern and the code was simple, or if it just did not matter performance-wise.

off topic: Speaking of performance, I went to prune a current datadir today that had txindex on. Of course that caused me to have to reindex. I ran 0.12 to do that and was done soooo much faster thanks to sipa and your excellent work on libsecp256k1. Thank-you!

No problem with adding this, although I don't think it will be used a lot in practice. It should definitely not be enabled by default.

But I understand the use case where you have an expensive trusted time source, and you don't want bitcoind messing with it, even if it means well.

Am I incorrect in my recollection that the getargument stuff is slow? Do we want that inside GetAdjustedTime?

Yes - the preferred way to do this would be to set a global flag in AppInit2, then use that in the time function. Calling the argument parser every time someone asks for the time is not very elegant, at least.

Or possibly even better: have the option disable the time-offset logic, set the time offset to 0? That'd remove even need to change the GetAdjustedTime() function itself.

Nits:

• Add a DEFAULT_TRUST_SYSTEM_TIME constant as is done for other options instead of hardcode the default in the help message and GetBoolArg line.

Idea. This can even be extended. If I trust my time source, I can ban nodes with a huge time difference from my exact time to prevent network partition attacks on me (bad nodes trying to attack me on my node startup with a huge time difference bringing me off the main network). Not that I have seen such attack myself yet, but...

Thanks, I just pushed the updates mentioned by your feedback.

@laanwj I chose not to disable the time-offset logic entirely because I've gotten feedback about people liking to get the warning message "Please check that your computer's date and time are correct!" if none of the peers are within 5 minutes just in-case.

@paveljanik I also did not add any logic to ban nodes with largely differing times because I did not want to add side-effects that could increase P2P network partitioning. Right now, if the median offset is more than 70 minutes, the offset is set to zero and a warning is logged if none of the nodes are within five minutes. So, there is some protection against an attack that attempts to change your node's perspective on time a lot. If your node disagrees substantially with every other node, then you get a warning. But you'll continue to exchange transactions and blocks etc... If banning them turns out to be desirable, then a separate PR might be the way to go.

Please squash into one commit for easier review.

Sure thing, done.

utACK 1079b2c
This code change is nicely self-contained and localized. Let's not make any changes to the P2P code in this pull, also any proposal regarding automatic banning (for any reason) has to be very carefully evaluated for advantages and dangers, for the reasons already mentioned - partitioning risk.

@gmaxwell I am not convinced that the behaviour of not correcting at all is less harmful than correcting your clock based on what random peers tell you when chosen by someone who has confidence in their own clock configuration.

Perhaps don't think of it as someone "having confidence in their own clock configuration", but rather "wanting to have a consistent or specific perspective on time". For example, maybe a company has nodes in more than one geo location and wants to have a closely consistent perspective on time across all of its nodes. Or, maybe that company wants to have a cluster of nodes running on internally controlled time and a cluster running on network adaptive time. Perhaps there are attacker nodes that feed 69 minute time skews to peers based on user agent and those affected user agents don't want to have their ability to follow consensus affected. From a decentralized validation point of view, I don't see why it would be necessarily bad for a node to take a firm position, or have confidence in its own configuration, on what time it thought it was.

I've thought it over a bit and I still think GetTimeOffset should return 0 when -trustsystemclock is given. This will make the RPCs (getinfo, getnetworkinfo) give the correct effective time offset.

Sure, that makes sense too. Update made and rebased/squashed.

This seemed to be lingering, so, here's a commit with a little different approach.
Now, instead of adding -trustsystemclock (that effectively limited the max offset to zero), I added -maxtimeadjustment, that lets the user choose how much of an adjustment (in seconds) is acceptable.
Thus, -trustsystemclock and -maxtimeadjustment=0 give the same basic effect of not letting peers influence the local node's perspective of time.
But, this new way gives a superset of possible choices with less code and probably less runtime overhead because it does not add a condition to GetTimeOffset.

@laanwj @sipa Do you find this any more acceptable? If not, I'll close this and forget about it. No big deal to me either way. Thanks!

utACK, I like this better, you've un-magified a magic number (and you can still achieve the same as before, by setting it to 0).

I like this approach much better. But the implementation has an odd non-monotonicity. E.g. if you have 5 peers claiming ten seconds off, then six more connect claiming 80 minutes. You'll apply a correction of 0 instead of 10 seconds. Perhaps better to just fix in another PR, since it was an existing misbehavior (though more likely to be triggered when the time is cut down to small numbers).

@gmaxwell Interesting. True, and in that scenario a warning message also gets output when the median goes beyond the acceptable limit.

Maybe you were working towards the, perhaps more alarming, scenario where there are five nodes at 10 seconds off and then six more claim the well-known default acceptable limit of 4200 seconds.
That would cause a large effective change in offset with no other warning.
Maybe that could be the basis for another warning message (in a separate PR)?
Basically, the offset is within tolerance, but there might be something going on.
We'd need to add an option to set the threshold of change to alert that there might be "something going on".
This is different than simply seeing a large range between min and max offset seen because it actually changes our local offset.

Now that this value no longer a hard-coded constant, a possible mitigation would be to unpredictably pick a value between 0 and 4200 so that attacker nodes would run a higher risk of sending back offsets that are out of bounds.

Also, in your scenario, I'm not sure which set of nodes are attacker nodes (or maybe they're two sets of nodes working at cross-purposes).

For example, this is what's really on my mind:

I've been working on a CLTV-based unidirectional payment channel vulnerability assessment and two actual related threats that come to mind from that are:

• miners that want to get fees from time locked transactions by putting them in blocks "early" (will be mitigated by BIP113)
• senders that want to claim the channel's reserve value by getting a refund transaction mined before the receiver can spend a transaction allocating value to her

Both threats gain advantage by having more nodes accept into their mempool and relay time-locked transactions early.
Even with BIP113, if a sender can get a non-opt-in-RBF sequence value containing transaction propagated early, that inhibits the receiver's ability to even propagate an honest allocation transaction later.

So maybe the bad actors set up a bunch of passive nodes that, when connected to, return valid high time offsets.

Agreed, let's fix that to another pull.

There is no obligation for @mruddy to fix the peer time synchronization system, which is a herculean task, especially as it seems he doesn't want to use it at all. See #4521 for a collection of peer time related issues, will link there to this one too.