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

maflcko · 2022-03-24T18:26:33Z

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.

laanwj · 2022-03-24T18:47:56Z

Concept ACK

mzumsande

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.

src/net.cpp

DrahtBot · 2022-03-25T22:15:47Z

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 · 2022-03-28T08:08:22Z

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.

mzumsande · 2022-03-28T10:58:41Z

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.

maflcko · 2022-03-28T16:01:49Z

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 14ba286, so I don't think fingerprinting gets worse by this?

maflcko · 2022-03-28T16:08:13Z

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 14ba286), 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?

laanwj · 2022-04-06T09:22:36Z

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.

maflcko · 2022-04-06T09:29:13Z

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.

fanquake · 2022-04-07T12:58:25Z

Concept ACK

jonatack

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).

fa64dd6 refactor: Use type-safe std::chrono for addrman time (MarcoFalke) fa2ae37 Add type-safe AdjustedTime() getter to timedata (MarcoFalke) fa5103a Add ChronoFormatter to serialize (MarcoFalke) fa253d3 util: Add HoursDouble (MarcoFalke) fa21fc6 scripted-diff: Rename addrman time symbols (MarcoFalke) fa9284c refactor: Remove not needed std::max (MacroFake) Pull request description: Those refactors are overlapping with, but otherwise largely unrelated to #24662. ACKs for top commit: naumenkogs: utACK fa64dd6 dergoegge: Code review ACK fa64dd6 Tree-SHA512: a50625e78036e7220a11997e6d9b6c6b317cb38ce02b1835fb41cbee2d8bfb1faf29b29d8990be78d6b5e15e9a9d8dec33bf25fa439b47610ef708950969724b

fa64dd6 refactor: Use type-safe std::chrono for addrman time (MarcoFalke) fa2ae37 Add type-safe AdjustedTime() getter to timedata (MarcoFalke) fa5103a Add ChronoFormatter to serialize (MarcoFalke) fa253d3 util: Add HoursDouble (MarcoFalke) fa21fc6 scripted-diff: Rename addrman time symbols (MarcoFalke) fa9284c refactor: Remove not needed std::max (MacroFake) Pull request description: Those refactors are overlapping with, but otherwise largely unrelated to bitcoin#24662. ACKs for top commit: naumenkogs: utACK fa64dd6 dergoegge: Code review ACK fa64dd6 Tree-SHA512: a50625e78036e7220a11997e6d9b6c6b317cb38ce02b1835fb41cbee2d8bfb1faf29b29d8990be78d6b5e15e9a9d8dec33bf25fa439b47610ef708950969724b

dergoegge · 2022-08-04T10:51:26Z

Code review ACK fadd8b2

…work time fadd8b2 addrman: Use system time instead of adjusted network time (MarcoFalke) Pull request description: 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. ACKs for top commit: dergoegge: Code review ACK fadd8b2 Tree-SHA512: b6c178fa01161544e5bc76c4cb23e11bcc30391f7b7a64accce864923766647bcfce2e8ae21d36fb1ffc1afa07bc46415aca612405bd8d4cc1f319c92a08498f

maflcko mentioned this pull request Mar 24, 2022

p2p: Remove GetAdjustedTime() from AddrMan #23807

Closed

maflcko added the P2P label Mar 24, 2022

maflcko force-pushed the 2203-add- branch from face168 to fa51329 Compare March 24, 2022 18:48

maflcko closed this Mar 24, 2022

maflcko reopened this Mar 24, 2022

maflcko force-pushed the 2203-add- branch 2 times, most recently from faf47e5 to fafd77b Compare March 24, 2022 19:05

fanquake requested review from mzumsande, naumenkogs and sdaftuar March 24, 2022 20:11

mzumsande reviewed Mar 25, 2022

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

maflcko marked this pull request as draft March 25, 2022 16:20

DrahtBot mentioned this pull request Mar 26, 2022

logging: Add severity level to logs #24464

Merged

maflcko force-pushed the 2203-add- branch from fafd77b to fa94f2d Compare March 28, 2022 08:19

maflcko changed the title ~~addrman: Use sytem time instead of adjusted network time~~ addrman: Use system time instead of adjusted network time Mar 28, 2022

maflcko mentioned this pull request Mar 28, 2022

refactor address relay time #24697

Merged

maflcko force-pushed the 2203-add- branch from faab10f to b5d6569 Compare March 29, 2022 08:33

DrahtBot mentioned this pull request Mar 30, 2022

refactor: use braced init for integer literals instead of c style casts #23829

Merged

maflcko force-pushed the 2203-add- branch from b5d6569 to 8fe66f8 Compare April 7, 2022 12:29

fanquake mentioned this pull request Apr 7, 2022

Change -maxtimeadjustment default from 70 minutes to 0 #24606

Closed

jonatack reviewed Apr 7, 2022

View reviewed changes

DrahtBot mentioned this pull request Apr 14, 2022

p2p, rpc: Manual block-relay-only connections with addnode #24170

Closed

DrahtBot added the Needs rebase label May 24, 2022

fanquake mentioned this pull request May 27, 2022

Improve CMedianFilter algorithm #25221

Closed

maflcko force-pushed the 2203-add- branch from 8fe66f8 to fa96140 Compare July 29, 2022 15:36

DrahtBot removed the Needs rebase label Jul 29, 2022

DrahtBot mentioned this pull request Jul 29, 2022

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

Merged

addrman: Use system time instead of adjusted network time

fadd8b2

maflcko force-pushed the 2203-add- branch from fa96140 to fadd8b2 Compare July 30, 2022 09:04

maflcko marked this pull request as ready for review July 30, 2022 09:04

fanquake requested review from mzumsande and dergoegge July 30, 2022 09:33

fanquake merged commit e038605 into bitcoin:master Aug 5, 2022

maflcko deleted the 2203-add-🔍 branch August 5, 2022 08:10

bitcoin locked and limited conversation to collaborators Aug 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

maflcko commented Mar 24, 2022 •

edited

Loading

Uh oh!

laanwj commented Mar 24, 2022

Uh oh!

mzumsande left a comment

Uh oh!

Uh oh!

DrahtBot commented Mar 25, 2022 •

edited

Loading

Uh oh!

naumenkogs commented Mar 28, 2022 •

edited

Loading

Uh oh!

mzumsande commented Mar 28, 2022

Uh oh!

maflcko commented Mar 28, 2022

Uh oh!

maflcko commented Mar 28, 2022

Uh oh!

laanwj commented Apr 6, 2022 •

edited

Loading

Uh oh!

maflcko commented Apr 6, 2022

Uh oh!

fanquake commented Apr 7, 2022

Uh oh!

jonatack left a comment •

edited

Loading

Uh oh!

dergoegge commented Aug 4, 2022

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

maflcko commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Mar 24, 2022

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

naumenkogs commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzumsande commented Mar 28, 2022

Uh oh!

maflcko commented Mar 28, 2022

Uh oh!

maflcko commented Mar 28, 2022

Uh oh!

laanwj commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Apr 6, 2022

Uh oh!

fanquake commented Apr 7, 2022

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dergoegge commented Aug 4, 2022

Uh oh!

Uh oh!

maflcko commented Mar 24, 2022 •

edited

Loading

DrahtBot commented Mar 25, 2022 •

edited

Loading

naumenkogs commented Mar 28, 2022 •

edited

Loading

laanwj commented Apr 6, 2022 •

edited

Loading

jonatack left a comment •

edited

Loading