p2p: Always serialize local timestamp for version msg #23695

1. 
   MarcoFalke commented at 4:01 pm on December 7, 2021: member

   Currently we serialize the local time when connecting to outbound connections and the “adjusted network” time when someone connects to us.

   I presume the reason is to avoid a fingerprint in case the local time is misconfigured. However, the fingerprint still exits when:

   • The local time goes out-of-sync after timedata is filled up, in which case the adjusted time is not adjusted. See comment in src/timedata.cpp. (In practise I expect no adjustment to happen after timedata is filled up by one entry more than half its size).
   • The local time is off by more than 70 minutes. See DEFAULT_MAX_TIME_ADJUSTMENT. While there is a warning in this case, the warning might be missed by the node operator.
   • The adjusted time is poisoned by an attacker. This is only a theoretical concern after commit e457513eb1bad11482f0820feb0f5810324a9d06.

   Using the adjusted time does help in a the case where the local time is off by a constant less than 70 minutes and the node quickly connects to 5 outbound peers to retrieve the adjusted time.

   Still, I think using GetAdjustedTime here gives a false sense of security. It will be better for node operators to instead set the correct time.

2. MarcoFalke added the label Brainstorming on Dec 7, 2021
3. MarcoFalke added the label P2P on Dec 7, 2021
4. 
   MarcoFalke commented at 8:14 pm on December 7, 2021: member

   This is the first step toward #4521 (comment) , but might even make sense on its own.
5. 
   mzumsande commented at 11:22 pm on December 7, 2021: member

   Concept ACK (to this change, not getting rid of AdjustedTime entirely)

   The timestamp a node sends to their inbounds will influence their adjusted time, and I think it makes sense to use the local time for that - this also counteracts non-local phenomena, where a wrong Adjusted Time is passed from A to B, affects B’s adjusted time and is passed along to B’s future peers and so on.

   I presume the reason is to avoid a fingerprint in case the local time is misconfigured.

   It seems impossible to know for sure (this inbound/outbound distinction seems to be a Satoshi thing that was never touched). But it makes little sense to me, for the reasons stated above. It seems much more easy to do fingerprinting if you can influence the adjusted time of your target, than if you would have to hope that the peer’s local time happens to be wrong in order to do anything. Maybe there is another reason?

   The local time is off by more than 70 minutes. See DEFAULT_MAX_TIME_ADJUSTMENT. While there is a warning in this case, the warning might be missed by the node operator.

   Also, according to the logic here the warning not even always issued in this case - only if we have no timedata sample within 5 minutes of our local time. Otherwise, we’d silently go back to using local time.

6. 
   MarcoFalke commented at 8:23 am on December 8, 2021: member

   It seems much more easy to do fingerprinting if you can influence the adjusted time of your target

   I don’t think this is trivially possible anymore after your commit e457513eb1bad11482f0820feb0f5810324a9d06. If someone controls most of the nodes’ outbound connections, any fingerprint concerns by the node operator should be nullified.

   Also, according to the logic here the warning not even always issued in this case - only if we have no timedata sample within 5 minutes of our local time. Otherwise, we’d silently go back to using local time.

   I think it should rarely happen in practise that one of your outbounds has misconfigured its time in the same way, so I didn’t mention this in OP. Also, this doesn’t affect the adjusted time. It only affects whether a warning is issues if the adjusted time is set back to the local time, right?

7. 
   DrahtBot commented at 8:46 am on December 8, 2021: member

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

   Conflicts

   No conflicts as of last run.

8. 
   mzumsande commented at 3:22 pm on December 8, 2021: member

   I don’t think this is trivially possible anymore after your commit e457513.

   Agree, I was just mentioning this because I doubt that avoiding fingerprinting really was the original motivation to add the inbound/outbound distinction - but then again, I don’t know what else.

   It only affects whether a warning is issues if the adjusted time is set back to the local time, right?

   Yes, that’s my understanding too.

9. MarcoFalke removed the label Brainstorming on Dec 13, 2021
10. 
    laanwj commented at 2:04 pm on December 13, 2021: member

    Concept ACK. Both on this and eventually getting rid of network-adjusted time completely. (after that, we can even stop sending our local time in the version message, also removing that fingerprinting vector)
11. 
    MarcoFalke commented at 2:10 pm on December 13, 2021: member

    (after that, we can even stop sending our local time in the version message, also removing that fingerprinting vector)

    Are you sure on that? I am thinking that this might drop previous releases of Bitcoin Core out of consensus?

12. 
    sipa commented at 4:01 pm on December 13, 2021: member

    @MarcoFalke I think @laanwj means that after dropping network-adjusted time completely, we could stop reporting local time to peers. I assume that’s a long time out.

    Concept ACK

13. 
    MarcoFalke commented at 4:09 pm on December 13, 2021: member

    Oh, I see. At that point we might also make the “cleanup version message” p2p change :sweat_smile:
14. p2p: Always serialize local timestamp for version msg fa1dc9b36a
15. DrahtBot added the label Needs rebase on Dec 14, 2021
16. MarcoFalke force-pushed on Dec 14, 2021
17. DrahtBot removed the label Needs rebase on Dec 14, 2021
18. 
    naumenkogs commented at 9:05 am on December 16, 2021: member

    ACK fa1dc9b36a0ccf96cbaf106c060336d91b54579e
19. 
    laanwj commented at 6:34 pm on December 16, 2021: member

    Are you sure on that? I am thinking that this might drop previous releases of Bitcoin Core out of consensus?

    Right. It’s too bad that it doesn’t explicitly ignore obviously invalid values like 0 for the timestamp. It still computes a timedelta and adds it. For some reason I thought it didn’t (It only compares the eventual delta median to -maxtimeadjustment, not individual contributions).

20. w0xlt approved
21. 
    w0xlt commented at 3:56 am on December 17, 2021: contributor

    crACK fa1dc9b

    This change makes the code simpler. I agree that it seems much more easier to do fingerprinting if the peers can influence the adjusted time of the target node and that the network-adjusted time should eventually be dropped.

22. 
    laanwj commented at 8:44 pm on December 17, 2021: member

    Code review ACK fa1dc9b36a0ccf96cbaf106c060336d91b54579e
23. laanwj merged this on Dec 17, 2021
24. laanwj closed this on Dec 17, 2021

    ---

25. sidhujag referenced this in commit 1fe88526ae on Dec 18, 2021
26. MarcoFalke deleted the branch on Dec 18, 2021
27. DrahtBot locked this on Dec 18, 2022

Contributors
MarcoFalke mzumsande DrahtBot laanwj sipa naumenkogs w0xlt

Labels
P2P