p2p: Remove GetAdjustedTime() from AddrMan

w0xlt commented at 6:38 pm on December 17, 2021: contributor

This PR modifies AddrMan to use local time instead of GetAdjustedTime().

Motivation: Check discussion in #23695, #23631 and the issues related to GetAdjustedTime() reported in https://github.com/bitcoin/bitcoin/issues/4521

w0xlt force-pushed on Dec 17, 2021

laanwj added the label P2P on Dec 17, 2021

w0xlt renamed this:
~~Remove GetAdjustedTime() from AddrMan~~
p2p: Remove GetAdjustedTime() from AddrMan
on Dec 17, 2021

laanwj commented at 7:50 pm on December 17, 2021: member

Concept ACK Could for most part be a scripted-diff?

w0xlt force-pushed on Dec 17, 2021

w0xlt commented at 11:16 pm on December 17, 2021: contributor

@laanwj changed the last commit (f282ffb) to scripted-diff.

MarcoFalke commented at 8:59 am on December 18, 2021: member

I haven’t thought a lot about this, but I have two questions:

Is this going to cause any additional fingerprinting vectors? I guess not after commit 14ba286556faad794f288ef38493c540382897cb?

Also, could this break P2P address relay? I guess only for the local node?

laanwj commented at 11:39 am on December 18, 2021: member

Also, could this break P2P address relay? I guess only for the local node?

Yes, but so could a misadjusted time due to connecting to the wrong nodes. At the least it reduces variance between starts of bitcoind.

fanquake deleted a comment on Dec 18, 2021

mzumsande commented at 2:53 pm on December 18, 2021: member

Also, could this break P2P address relay?

This PR doesn’t affect address (gossip) relay, because that is independent from AddrMan: The decision whether to forward an address will still be on an adjusted time basis after this, see https://github.com/bitcoin/bitcoin/blob/cb27d60f966ad20c465e5db05ba1a2253bd4dab3/src/net_processing.cpp#L2887-L2888 and https://github.com/bitcoin/bitcoin/blob/cb27d60f966ad20c465e5db05ba1a2253bd4dab3/src/net_processing.cpp#L2933-L2936

Although it would probably make sense to change net_processing as well to be consistent.

naumenkogs commented at 7:56 am on December 20, 2021: member

Could you summarize the reasons for this change and put it in the OP post? It’s difficult to make sure everyone’s on the same page otherwise.

w0xlt commented at 2:26 pm on December 20, 2021: contributor

@naumenkogs The main reason is to remove the use of GetAdjustedTime.

As @mzumsande said, this change does not affect the network layer (or other layers), only the local AddrMan so it seems safer and easier to test and review.

I have a version of this PR that also removes GetAdjustedTime from src/node/miner.cpp, src/validation.cpp, src/net.cpp, src/rpc/net.cpp and src/net_processing.cpp. While this passes the unit and functional tests (without changing them), I’m not sure what behavioral changes this might cause.

If reviewers think it’s better, I can update the PR with this version.

in src/timedata.h:76 in f282ffb475 outdated

72@@ -73,6 +73,7 @@ class CMedianFilter
73 /** Functions to keep track of adjusted P2P time */
74 int64_t GetTimeOffset();
75 int64_t GetAdjustedTime();
76+int64_t GetLocalTime();

MarcoFalke commented at 2:49 pm on December 20, 2021:

what is this? I’d say please use GetTime or (if possible) GetTime<chrono>

w0xlt commented at 9:16 pm on December 22, 2021:

The idea of GetLocalTime was to encapsulate count_seconds(GetTime<std::chrono::seconds>() to avoid repetition.

I removed GetLocalTimefunction, butGetTimecan't be used withoutcount_secondssinceGetAdjustedTimereturnsint_64`.

MarcoFalke commented at 2:52 pm on December 20, 2021: member

If reviewers think it’s better, I can update the PR with this version.

If this really doesn’t change p2p behavior and is only a refactor (?), then I’d say to leave this pull as-is to simplify review. If this is not a refactor, it would be good to list all behavior changes.

DrahtBot commented at 3:36 am on December 21, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24543 (net processing: Move remaining globals into PeerManagerImpl by dergoegge)
#24171 (p2p: Sync chain more readily from inbound peers during IBD by sdaftuar)
#22564 (refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager by dongcarl)

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 9:38 am on December 21, 2021: member

@mzumsande I’m not sure this change doesn’t affect p2p

For example, we send addrs to our peers via ADDRv2 message, and those contain timestamps, which mean something for the peer. (Bitcoin Core might be using those very mildly, but anyway, it does look at them).
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) @w0xlt

And what’s the problem with GetAdjustedTime in addrman exactly?

MarcoFalke commented at 11:49 am on December 21, 2021: member

And what’s the problem with GetAdjustedTime in addrman exactly?

The problem is that adjusted time is:

controlled by “the network”
it is not guaranteed to be monotonic
it doesn’t even guarantee to be the correct time (or close to it), even if all your peers are well-behaved

Thus, it is an huge mental burden to think about any change to addrman that involves those timestamps. Also, it makes it hard to understand the existing logic, even if nothing is changed.

mzumsande commented at 1:56 pm on December 21, 2021: member

@mzumsande I’m not sure this change doesn’t affect p2p

I wasn’t trying to say that, just that it doesn’t really affect gossip relay, because self-announcements have already been changed to always use local time in #23695, and the mechanism of whether to forward these is located in net_processing , not in AddrMan.

One p2p aspect that the adjusted time in AddrMan can influence in theory are GETADDR responses:

Addrs for which IsTerrible() is true are not included in these, and the Adjusted Time influences IsTerrible() both directly as a parameter, and indirectly, by influencing nLastTry, nLastSuccess and nAttempts, nTime of an address, which are updated by functions using the adjusted time (such as Connected(), Good() etc.).

I’m not sure that this is a huge influence though, since IsTerrible() mostly operates on larger timescales (days instead of minutes), GETADDR responses are calculated once a day and cached - whereas the adjusted time would differ from the local time only slightly when they deviate.

I’d have to think about it a bit more, but I tend toward removing the Adjusted Time from all addr related code - not just AddrMan but also in net_processing.

naumenkogs commented at 11:30 am on December 22, 2021: member

@mzumsande

the adjusted time would differ from the local time only slightly when they deviate.

What is the biggest possible deviation? From what I see in the code, any, but we show warnings if it deviates more than 5 minutes? I thought the deviation from block header timestamps is limited, but I can’t find that.

I’d have to think about it a bit more, but I tend toward removing the Adjusted Time from all addr related code - not just AddrMan but also in net_processing.

I probably support the overall sentiment, I just think we should understand all implications and document it here. E.g., what do you think about the privacy leak I mentioned? Do you agree it exists, and consider it too mild to pose a threat?

MarcoFalke commented at 11:50 am on December 22, 2021: member

What is the biggest possible deviation?

Likely -maxtimeadjustment?

w0xlt force-pushed on Dec 22, 2021

w0xlt commented at 9:21 pm on December 22, 2021: contributor

If this really doesn’t change p2p behavior and is only a refactor (?),

This is not just a refactor because it changes the AddrMan behavior. But since @mzumsande suggests changing net_processing as well, I added this change.

If this is not a refactor, it would be good to list all behavior changes.

commit 3a141ff

AddrMan::Good() function will use (by default) the current time (instead of the adjusted time) to update the time when the node last connected to a peer (AddrInfo::nLastSuccess, AddrInfo::nLastTry and AddrManImpl::nLastGood) (addrman.h:92).
AddrMan::Attempt() function will use (by default) the current time (instead of the adjusted time) to update the AddrInfo::nLastTry (addrman.h:95).
AddrManImpl::AddSingle() will use the current time (instead of the adjusted time) to update CAddress::nTime periodically (addrman.cpp:559).
AddrManImpl::GetAddr_() will use the current time (instead of the adjusted time) as a filter for AddrInfo::IsTerrible() (addrman.cpp:785)
AddrManImpl::ResolveCollisions_() will use the current time (instead of the adjusted time) to compare AddrInfo::nLastSuccess or AddrInfo::nLastTry. Note that these fields are also updated with the current time in this PR (items 1 and 2). (addrman.cpp:871-892)
AddrInfo::IsTerrible() will use the current time (instead of the adjusted time) to determine whether the entry are bad enough to be deleted (addrman_impl:94).
AddrInfo::GetChance() will use the current time (instead of the adjusted time) to calculate nSinceLastTry to deprioritize very recent attempts (addrman_impl:97).

commit cb660ce

PeerManagerImpl::CanDirectFetch() will use the current time (instead of the adjusted time). This function is used to set the flag for initiating extra block-relay-only peer connections, to allow the creation of CMPCTBLOCK message and others. (net_processing:960).
PeerManagerImpl::ProcessMessage() will use the current time (instead of the adjusted time) to store the new addresses when receiving ADDR or ADDRV2 messages. (net_processing:2889)
PeerManagerImpl::MaybeSendAddr will use the current time (instead of the adjusted time) to calculate if the block time is close to today (net_processing:4599), the CNodeState::m_headers_sync_timeout (net_processing:4606) and to detect if a initial-headers-sync peer is unresponsive (net_processing:4640)

josibake commented at 3:07 pm on December 29, 2021: member

Concept ACK

I have found understanding GetAdjustedTime() to be a large stumbling block as I’ve been reading through AddrMan.

I’ll spend some time reading the linked threads and discussion on this PR before digging in a bit more

DrahtBot added the label Needs rebase on Jan 4, 2022

in src/addrman.cpp:892 in cb660ce885 outdated

894+                    Good_(info_new, false, count_seconds(GetTime<std::chrono::seconds>()));
895                     erase_collision = true;
896                 }
897             } else { // Collision is not actually a collision anymore
898-                Good_(info_new, false, GetAdjustedTime());
899+                Good_(info_new, false, count_seconds(GetTime<std::chrono::seconds>()));

MarcoFalke commented at 11:21 am on January 11, 2022:

count_seconds should only be used to serialize the time value in seconds. For example for RPC, the debug log, or the GUI, …

It shouldn’t be used for normal code. You can simply use the equivalent GetTime here. If you want to use std::chrono, you should update all related types, but this should be done in a separate commit. Ideally a separate pull request.

w0xlt commented at 8:52 am on January 16, 2022:

Changed to GetTime.

w0xlt force-pushed on Jan 16, 2022

Add time header to Addrman.h 67de6e4032

scripted-diff: Remove GetAdjustedTime() from AddrMan

-BEGIN VERIFY SCRIPT-
s() { sed -i -e 's/GetAdjustedTime()/GetTime()/g' $(git grep -l 'GetAdjustedTime' $1); }
s src/addrman.cpp
s src/addrman.h
s src/addrman_impl.h
s src/bench/addrman.cpp
s src/test/addrman_tests.cpp
-END VERIFY SCRIPT-

80d6047eee

scripted-diff: Remove GetAdjustedTime() from net_processing

-BEGIN VERIFY SCRIPT-
s() { sed -i -e 's/GetAdjustedTime()/GetTime()/g' $(git grep -l 'GetAdjustedTime' $1); }
s src/net_processing.cpp
-END VERIFY SCRIPT-

3445590aeb

w0xlt force-pushed on Jan 16, 2022

DrahtBot removed the label Needs rebase on Jan 16, 2022

w0xlt commented at 11:08 am on January 16, 2022: contributor

Rebased.

in src/net_processing.cpp:2894 in 3445590aeb

2890@@ -2891,7 +2891,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2891 
2892         // Store the new addresses
2893         std::vector<CAddress> vAddrOk;
2894-        int64_t nNow = GetAdjustedTime();
2895+        int64_t nNow = GetTime();

naumenkogs commented at 8:01 am on March 11, 2022:

One side effect I’m thinking of is if (addr.nTime <= 100000000 || addr.nTime > nNow + 10 * 60). If this is triggered (because our node lives in the past), we would mark some of the recent address (including all one-hop self-announcements) to be 5 days old, and deprioritize them?

in src/net_processing.cpp:966 in 3445590aeb

962@@ -963,7 +963,7 @@ bool PeerManagerImpl::TipMayBeStale()
963 
964 bool PeerManagerImpl::CanDirectFetch()
965 {
966-    return m_chainman.ActiveChain().Tip()->GetBlockTime() > GetAdjustedTime() - m_chainparams.GetConsensus().nPowTargetSpacing * 20;
967+    return m_chainman.ActiveChain().Tip()->GetBlockTime() > GetTime() - m_chainparams.GetConsensus().nPowTargetSpacing * 20;

MarcoFalke commented at 4:21 pm on March 16, 2022:

I don’t think this has anything to do with addrman

MarcoFalke commented at 4:22 pm on March 16, 2022: member

Looks like this is changing both too much (a few in net_processing) and too little (missing net.cpp).

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

Thanks, but due to inactivity I am closing this for now. I’ve picked it up in #24662.

MarcoFalke closed this on Mar 24, 2022

DrahtBot locked this on Mar 24, 2023

p2p: Remove GetAdjustedTime() from AddrMan #23807

Conflicts