p2p: Always serialize local timestamp for version msg #23695
Conversation
|
This is the first step toward #4521 (comment) , but might even make sense on its own. |
|
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.
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.
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 don't think this is trivially possible anymore after your commit e457513. If someone controls most of the nodes' outbound connections, any fingerprint concerns by the node operator should be nullified.
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? |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
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.
Yes, that's my understanding too. |
|
Concept ACK. Both on this and eventually getting rid of network-adjusted time completely. |
Are you sure on that? I am thinking that this might drop previous releases of Bitcoin Core out of consensus? |
|
@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 |
|
Oh, I see. At that point we might also make the "cleanup version message" p2p change 😅 |
fa92bb3 to
fa1dc9b
Compare
|
ACK fa1dc9b |
Right. It's too bad that it doesn't explicitly ignore obviously invalid values like |
There was a problem hiding this comment.
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.
|
Code review ACK fa1dc9b |
…n msg fa1dc9b p2p: Always serialize local timestamp for version msg (MarcoFalke) Pull request description: 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 e457513. 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. ACKs for top commit: naumenkogs: ACK fa1dc9b laanwj: Code review ACK fa1dc9b w0xlt: crACK fa1dc9b Tree-SHA512: 70a0f4ab3500e6ddcde291620e35273018cefd1d9e94b91ad333e360139ed18862718bb1a9854af2bf79990bf74b05d95492f77d0747c7b9bdd276c020116dcb
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:
src/timedata.cpp. (In practise I expect no adjustment to happen after timedata is filled up by one entry more than half its size).DEFAULT_MAX_TIME_ADJUSTMENT. While there is a warning in this case, the warning might be missed by the node operator.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
GetAdjustedTimehere gives a false sense of security. It will be better for node operators to instead set the correct time.