p2p: Don't use timestamps from inbound peers for Adjusted Time

[mzumsande]

GetAdjustedTime() (used e.g. in validation and addrman) returns a time with an offset that is influenced by timestamps that our peers have sent us in their version message.

Currently, timestamps from all peers are used for this.
However, I think that it would make sense to ignore the timedata samples from inbound peers, making it much harder for others to influence the Adjusted Time in a targeted way.
With the extra feeler connections (every 2 minutes on average) and extra block-relay-only connections (every 5 minutes on average) there are also now plenty of opportunities to gather a meaningful number of timedata samples from outbound peers.

There are some measures in place to prevent abuse: the -maxtimeadjustment parameter with a default of 70 minutes, warnings in cases of large deviations, only using the first 200 samples (explanation), but I think that only using samples from outbound connections in the first place would be an additional safety measure that would make sense.

See also issue #4521 for further context and links: There have been several discussions in the past about replacing or abolishing the existing timedata system.

[sipa]

Concept ACK

[maflcko]

Is this fixing #6071 ?

[mzumsande]

Is this fixing #6071 ?

Yes I think so, with SPV clients being inbound peers.

[vasild]

Concept ACK

I have been thinking "what could go wrong with this?" but can't find a realistic adverse scenario. Need to stare a bit more at the surrounding code before full ACK.

What about this - a node fails to create outgoing connections while incoming connections arrive just fine (could happen due to a firewall or some other network mishap (intentional or not) or addrman filled with junk). Many incoming connections arrive from honest nodes with correct times, which are ignored. How realistic is that? Is it worth considering an extension of this patch that starts collecting time samples from incoming connections if none or very few samples have been collected from outgoing connections for a long time after startup?

[mzumsande]

What about this - a node fails to create outgoing connections while incoming connections arrive just fine (could happen due to a firewall or some other network mishap (intentional or not) or addrman filled with junk). Many incoming connections arrive from honest nodes with correct times, which are ignored. How realistic is that?

I've never heard of someone in this situation this before, maybe others have?
It seems hard to get into this, after all outbound connections must have worked at some point in the past to broadcast our address to the network before others would connect to us. Plus, the lack of timedata samples could only cause potential problems if on top of all this, our system time is seriously off.

Is it worth considering an extension of this patch that starts collecting time samples from incoming connections if none or very few samples have been collected from outgoing connections for a long time after startup?

My first reaction was that this situation is so unlikely that it wouldn't be worth the extra complexity, but I'd love to hear other opinions!

[darosior]

tentative Concept ACK. The motivation sounds good to me although i'm not very familiar with this code.

[jnewbery]

Concept and code review ACK 0c85dc3

[vasild]

ACK 0c85dc3

Is it worth considering an extension of this patch...

Probably not.

[vasild]

nit, feel free to ignore: there is already if (!pfrom.IsInboundConn()) { just a few lines above and the nTimeOffset local variable seems unnecessary.

dont click here

--- i/src/net_processing.cpp
+++ w/src/net_processing.cpp
@@ -2657,47 +2657,48 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         if (!pfrom.IsInboundConn()) {
             // For non-inbound connections, we update the addrman to record
             // connection success so that addrman will have an up-to-date
             // notion of which peers are online and available.
             //
             // While we strive to not leak information about block-relay-only
             // connections via the addrman, not moving an address to the tried
             // table is also potentially detrimental because new-table entries
             // are subject to eviction in the event of addrman collisions.  We
             // mitigate the information-leak by never calling
             // AddrMan::Connected() on block-relay-only peers; see
             // FinalizeNode().
             //
             // This moves an address from New to Tried table in Addrman,
             // resolves tried-table collisions, etc.
             m_addrman.Good(pfrom.addr);
+
+            // Don't use timedata samples from inbound peers to make it
+            // harder for others to tamper with our adjusted time.
+            pfrom.nTimeOffset = nTime - GetTime();
+            AddTimeData(pfrom.addr, pfrom.nTimeOffset);
         }

         std::string remoteAddr;
         if (fLogIPs)
             remoteAddr = ", peeraddr=" + pfrom.addr.ToString();

         LogPrint(BCLog::NET, "receive version message: %s: version %d, blocks=%d, us=%s, txrelay=%d, peer=%d%s\n",
                   cleanSubVer, pfrom.nVersion,
                   peer->m_starting_height, addrMe.ToString(), fRelay, pfrom.GetId(),
                   remoteAddr);

-        int64_t nTimeOffset = nTime - GetTime();
-        pfrom.nTimeOffset = nTimeOffset;
-        AddTimeData(pfrom.addr, nTimeOffset);
-
         // If the peer is old enough to have the old alert system, send it the final alert.

[jnewbery]

That suggested patch would leave pfrom.nTimeOffset unset for inbound peers, which in turn would mean that timeoffset is unset in the node stats.

[vasild]

Right!

[naumenkogs]

I think the justification makes sense and the code is good. This is an improvement.
ACK 0c85dc3

What about this - a node fails to create outgoing connections while incoming connections arrive just fine (could happen due to a firewall or some other network mishap (intentional or not) or addrman filled with junk). Many incoming connections arrive from honest nodes with correct times, which are ignored. How realistic is that? Is it worth considering an extension of this patch that starts collecting time samples from incoming connections if none or very few samples have been collected from outgoing connections for a long time after startup?

Yeah, it's hard for me to imagine this, I can only see this happen if Addrman is attacked in a very severe way sometime during node uptime. And even in that case i don't think this current approach can help much.

[jonatack]

Posthumous ACK 0c85dc3

-maxtimeadjustment should be updated with this change; done in #24609.