p2p: Remove GetAdjustedTime() from AddrMan by w0xlt · Pull Request #23807 · bitcoin/bitcoin

w0xlt · 2021-12-17T18:38:51Z

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 #4521

laanwj · 2021-12-17T19:50:36Z

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

w0xlt · 2021-12-17T23:16:11Z

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

maflcko · 2021-12-18T08:59:31Z

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

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

laanwj · 2021-12-18T11:39:24Z

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.

mzumsande · 2021-12-18T14:53:12Z

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

bitcoin/src/net_processing.cpp

Lines 2887 to 2888 in cb27d60

    
           int64_t nNow = GetAdjustedTime(); 
        
           int64_t nSince = nNow - 10 * 60;

and

bitcoin/src/net_processing.cpp

Lines 2933 to 2936 in cb27d60

    
           if (addr.nTime > nSince && !peer->m_getaddr_sent && vAddr.size() <= 10 && addr.IsRoutable()) { 
        
               // Relay to a limited number of other nodes 
        
               RelayAddress(pfrom.GetId(), addr, fReachable); 
        
           }

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

naumenkogs · 2021-12-20T07:56:55Z

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 · 2021-12-20T14:26:00Z

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

src/timedata.h

maflcko · 2021-12-20T14:52:36Z

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 · 2021-12-21T03:36:48Z

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 · 2021-12-21T09:38:30Z

@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?

maflcko · 2021-12-21T11:49:35Z

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 · 2021-12-21T13:56:58Z

@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 · 2021-12-22T11:30:35Z

@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?

maflcko · 2021-12-22T11:50:10Z

What is the biggest possible deviation?

Likely -maxtimeadjustment?

w0xlt · 2021-12-22T21:21:19Z

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 · 2021-12-29T15:07:39Z

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

src/addrman.cpp

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

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

w0xlt · 2022-01-16T11:08:30Z

Rebased.

naumenkogs · 2022-03-11T08:01:03Z

src/net_processing.cpp

        // Store the new addresses
        std::vector<CAddress> vAddrOk;
-        int64_t nNow = GetAdjustedTime();
+        int64_t nNow = GetTime();


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?

maflcko

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

maflcko · 2022-03-16T16:21:29Z

src/net_processing.cpp

 bool PeerManagerImpl::CanDirectFetch()
 {
-    return m_chainman.ActiveChain().Tip()->GetBlockTime() > GetAdjustedTime() - m_chainparams.GetConsensus().nPowTargetSpacing * 20;
+    return m_chainman.ActiveChain().Tip()->GetBlockTime() > GetTime() - m_chainparams.GetConsensus().nPowTargetSpacing * 20;


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

maflcko · 2022-03-24T18:27:17Z

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

w0xlt force-pushed the remove_adjusted_time_addrman branch from 64c2492 to 14b5d2b Compare December 17, 2021 18:44

laanwj added the P2P label Dec 17, 2021

w0xlt changed the title ~~Remove GetAdjustedTime() from AddrMan~~ p2p: Remove GetAdjustedTime() from AddrMan Dec 17, 2021

w0xlt force-pushed the remove_adjusted_time_addrman branch 3 times, most recently from 765e71b to f282ffb Compare December 17, 2021 23:05

w0xlt mentioned this pull request Dec 18, 2021

ci, error: p2p_timeouts.py failed #23800

Closed

bitcoin deleted a comment Dec 18, 2021

maflcko reviewed Dec 20, 2021

View reviewed changes

src/timedata.h Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Dec 21, 2021

test: Make AddrMan unit tests use public interface, extend coverage #23826

Merged

w0xlt force-pushed the remove_adjusted_time_addrman branch from f282ffb to cb660ce Compare December 22, 2021 20:54

DrahtBot mentioned this pull request Dec 23, 2021

net: Encapsulate asmap in NetGroupManager #22910

Merged

DrahtBot added the Needs rebase label Jan 4, 2022

maflcko reviewed Jan 11, 2022

View reviewed changes

src/addrman.cpp Outdated Show resolved Hide resolved

w0xlt force-pushed the remove_adjusted_time_addrman branch from cb660ce to a27fab1 Compare January 16, 2022 04:22

w0xlt added 3 commits January 16, 2022 05:07

Add time header to Addrman.h

67de6e4

scripted-diff: Remove GetAdjustedTime() from net_processing

3445590

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

w0xlt force-pushed the remove_adjusted_time_addrman branch from a27fab1 to 3445590 Compare January 16, 2022 08:50

DrahtBot removed the Needs rebase label Jan 16, 2022

DrahtBot mentioned this pull request Jan 26, 2022

p2p: Sync chain more readily from inbound peers during IBD #24171

Merged

This was referenced Feb 24, 2022

refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager #22564

Merged

blockman: Properly guard blockfile members #24456

Closed

naumenkogs reviewed Mar 11, 2022

View reviewed changes

DrahtBot mentioned this pull request Mar 12, 2022

net processing: Move remaining globals into PeerManagerImpl #24543

Merged

maflcko reviewed Mar 16, 2022

View reviewed changes

maflcko closed this Mar 24, 2022

naumenkogs mentioned this pull request Mar 28, 2022

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

Merged

bitcoin locked and limited conversation to collaborators Mar 24, 2023

Conversation

w0xlt commented Dec 17, 2021

Uh oh!

laanwj commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

w0xlt commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Dec 18, 2021

Uh oh!

laanwj commented Dec 18, 2021

Uh oh!

mzumsande commented Dec 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

naumenkogs commented Dec 20, 2021

Uh oh!

w0xlt commented Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

maflcko commented Dec 20, 2021

Uh oh!

DrahtBot commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

naumenkogs commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Dec 21, 2021

Uh oh!

mzumsande commented Dec 21, 2021

Uh oh!

naumenkogs commented Dec 22, 2021

Uh oh!

maflcko commented Dec 22, 2021

Uh oh!

w0xlt commented Dec 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josibake commented Dec 29, 2021

Uh oh!

Uh oh!

w0xlt commented Jan 16, 2022

Uh oh!

naumenkogs Mar 11, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 16, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

laanwj commented Dec 17, 2021 •

edited

Loading

w0xlt commented Dec 17, 2021 •

edited

Loading

mzumsande commented Dec 18, 2021 •

edited

Loading

w0xlt commented Dec 20, 2021 •

edited

Loading

DrahtBot commented Dec 21, 2021 •

edited

Loading

naumenkogs commented Dec 21, 2021 •

edited

Loading

w0xlt commented Dec 22, 2021 •

edited

Loading