addrman: Use system time instead of adjusted network time

MarcoFalke commented at 6:26 pm on March 24, 2022: member

This changes addrman to use system time for address relay instead of the network adjusted time.

This is an improvement, because network time has multiple issues:

It is non-monotonic, even if the system time is monotonic.
It may be wrong, even if the system time is correct.
It may be wrong, if the system time is wrong. For example, when the node has limited number of connections (4), or the system time is wrong by too much (more than +-70 minutes), or the system time only got wrong after timedata collected more than half of the entries while the time was correct, …)

This may slightly degrade addr relay for nodes where timedata successfully adjusted the time. Addr relay can already deal with minor offsets of up to 10 minutes. Offsets larger than this should still allow addr relay and not result in a DoS.

MarcoFalke added the label P2P on Mar 24, 2022

laanwj commented at 6:47 pm on March 24, 2022: member

Concept ACK

MarcoFalke force-pushed on Mar 24, 2022

MarcoFalke closed this on Mar 24, 2022

MarcoFalke reopened this on Mar 24, 2022

MarcoFalke force-pushed on Mar 24, 2022

fanquake requested review from mzumsande on Mar 24, 2022

fanquake requested review from naumenkogs on Mar 24, 2022

fanquake requested review from sdaftuar on Mar 24, 2022

in src/net.cpp:2083 in fafd77b298 outdated

2079@@ -2080,7 +2080,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
2080 
2081         addrman.ResolveCollisions();
2082 
2083-        int64_t nANow = GetAdjustedTime();
2084+        int64_t nANow = GetTime();

mzumsande commented at 0:04 am on March 25, 2022:

maybe rename this variable, I think the “A” in nANow was for “Adjusted”.

MarcoFalke commented at 4:02 pm on March 28, 2022:

Thx, done in a later commit

mzumsande commented at 1:06 am on March 25, 2022: contributor

Concept ACK

I think that this line in net_processing should be changed too, otherwise we’d still use adjusted time for addr relay.

However, addr relay can already deal with minor offsets of up to 60 minutes

Where does the 60 minute number come from?

I can think of the following relevant times: When receiving addr messages (relevant code):

If nTime is >10 minutes in the future relative to our time, we backdate nTime to (OurTime - 5days) for addrman and don’t relay it further.
If nTime is >10 minutes in the past, we don’t relay it further (but accept it to addrman).

This means that addr gossip relay is relatively sensitive to what we think is the right time. If our time is off by more than 10 minutes, we will most likely not take part in it and not forward addresses received from others. That would not prevent us from accepting addrs to addrman / finding outbound peers. But we wouldn’t help the network and our own advertised address would be off by 10 minutes and not relayed by others as well, so we’d likely have a harder time getting inbound peers.

For GETADDR, we treat addresses as Terrible and don’t include them in a GETADDR response if:

If nTime is more than 10 minutes in the future (unlikely to happen, we would have backdated nTime if it was in the future when receiving it in the first place)
If nTime is older than 30 days

So I think that GETADDR responses are not very sensitive at the time scales of AdjustedTime (minutes), especially since they are only calculated only every ~24h (GETADDR caching). @naumenkogs mentioned here the possibility of fingerprinting:

Also, making it further from mainstream (not using adjusted time) might add more fingerprinting capabilities by looking at those timestamps, for nodes with broken clocks (and mildly for the rest, as a consequence)

I think that’s an interesting question, there are probably two aspects:

Active, targeted fingerprinting by trying to adjust the time of a peer is made impossible by this.
Passive fingerprinting of nodes that happen to be a few seconds off (which could have been corrected by the network before) could become easier.

MarcoFalke marked this as a draft on Mar 25, 2022

DrahtBot commented at 10:15 pm on March 25, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25619 (net: avoid overriding non-virtual ToString() in CService and use better naming by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

naumenkogs commented at 8:08 am on March 28, 2022: member

Concept ACK.

This means that addr gossip relay is relatively sensitive to what we think is the right time. If our time is off by more than 10 minutes, we will most likely not take part in it and not forward addresses received from others. That would not prevent us from accepting addrs to addrman / finding outbound peers. But we wouldn’t help the network and our own advertised address would be off by 10 minutes and not relayed by others as well, so we’d likely have a harder time getting inbound peers.

I was thinking more about our own peer selection in this case, especially deprioritizing in the context of 10+ minutes in the future get 5 days of penalty.

I don’t think this PR significantly worsens any existing behavior or opens any new attack surface.

For the “natural” behavior, it doesn’t matter much because addr relay freshness has a lag of hours, not minutes (maybe an issue with immediate self-announcements, but those are already connected). For the attacks, an attacker could always manipulate the addr timestamps how they wanted, and this PR doesn’t make it easier.

Re fingerprinting, I think that would be cool to investigate even for the current master.

Otherwise I agree with the summary of @mzumsande.

MarcoFalke force-pushed on Mar 28, 2022

mzumsande commented at 10:58 am on March 28, 2022: contributor

I was thinking more about our own peer selection in this case, especially deprioritizing in the context of 10+ minutes in the future get 5 days of penalty.

If I’m not missing something, we don’t deprioritize based on nTime: In GetChance() which is called from Addrman::Select_(), there is logic to deprioritize based on (possibly failed) earlier attempts to connect, but I didn’t find any influence of the timestamp on our own peer selection.

MarcoFalke renamed this:
~~addrman: Use sytem time instead of adjusted network time~~
addrman: Use system time instead of adjusted network time
on Mar 28, 2022

MarcoFalke commented at 4:01 pm on March 28, 2022: member

I think that this line in net_processing should be changed too, otherwise we’d still use adjusted time for addr relay.

Good catch, fixed.

Where does the 60 minute number come from?

Thanks, fixed up. I think I incorrectly interpreted 600 as 60 minutes, whereas it is 10 minutes.

Passive fingerprinting of nodes that happen to be a few seconds off (which could have been corrected by the network before) could become easier.

Bitcoin Core will already send it’s system time in the version message. See commit 14ba286556faad794f288ef38493c540382897cb, so I don’t think fingerprinting gets worse by this?

MarcoFalke commented at 4:08 pm on March 28, 2022: member

An alternative idea to this pull (not sure if serious):

Since we “know” the time offset for each peer, at least incoming ones (and outgoing ones after commit 14ba286556faad794f288ef38493c540382897cb), an alternative might also be to adjust the timestamp of each addr message on receipt by the peer-specific offset. This would get rid of adjusted time, but presumably limit the slight negative effects of our clock being wrong on addr relay?

MarcoFalke force-pushed on Mar 29, 2022

laanwj commented at 9:22 am on April 6, 2022: member

An alternative idea to this pull (not sure if serious):

It’s an intriguing idea, but I don’t think it’s worth it. Peers that have time offsets large enough for this to matter are probably unreliable anyway, and if our own time is wrong for even a short while, when it gets corrected the entire addrman db has wrong lastseen timestamps. It’s also quite some extra complexity to test.

MarcoFalke commented at 9:29 am on April 6, 2022: member

Right, this is probably not worth to look into due to the complexity and questionable benefit.

I’ll rebase the changes here once #24697 is merged.

MarcoFalke force-pushed on Apr 7, 2022

fanquake commented at 12:58 pm on April 7, 2022: member

Concept ACK

jonatack commented at 1:24 pm on April 7, 2022: contributor

Concept ACK on the refactorings, modulo “addrman: Use system time instead of adjusted network time” that I think it would be wiser to hold off on for the time being (pun intended) (edit: or maybe code it in a way that is easily reversible).

DrahtBot added the label Needs rebase on May 24, 2022

fanquake referenced this in commit 9ba73758c9 on Jul 27, 2022

sidhujag referenced this in commit 7b36c8f266 on Jul 27, 2022

MarcoFalke force-pushed on Jul 29, 2022

DrahtBot removed the label Needs rebase on Jul 29, 2022

addrman: Use system time instead of adjusted network time fadd8b2676

MarcoFalke force-pushed on Jul 30, 2022

MarcoFalke marked this as ready for review on Jul 30, 2022

fanquake requested review from mzumsande on Jul 30, 2022

fanquake requested review from dergoegge on Jul 30, 2022

dergoegge commented at 10:51 am on August 4, 2022: member

Code review ACK fadd8b2676f6d68ec87189871461c9a6a6aa3cac

fanquake merged this on Aug 5, 2022

fanquake closed this on Aug 5, 2022

MarcoFalke deleted the branch on Aug 5, 2022

sidhujag referenced this in commit 8a8c7580a4 on Aug 5, 2022

bitcoin locked this on Aug 5, 2023

addrman: Use system time instead of adjusted network time #24662

Conflicts