Remove GetAdjustedTime from init.cpp

MarcoFalke commented at 2:48 pm on November 30, 2021: member

It seems confusing to call GetAdjustedTime there, because no offset could have been retrieved from the network at this point. Even if connman was started, timedata needs at least 5 peer connections to calculate an offset.

Fix the confusion by replacing GetAdjustedTime with GetTime, which does not change behavior.

Also:

Replace magic number with MAX_FUTURE_BLOCK_TIME to clarify the context
Add test, which passes both on current master and this pull request
An unrelated refactoring commit, happy to drop

Replace addrman.h include with forward decl in net.h

Also, add missing addrman.h includes

fa815f8473

MarcoFalke added the label Refactoring on Nov 30, 2021

DrahtBot commented at 3:49 pm on November 30, 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:

#23280 (init: Coalesce Chainstate loading sequence between {,non-}unittest codepaths 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.

Coverage

Coverage	Change (pull 23636, 0386721afb7e984c01c284e79e52ba1a6563afc8)	Reference (master, ffdf8ee43e2440ab)
Lines	+0.0028 %	84.0014 %
Functions	+0.0000 %	76.2420 %
Branches	+0.0039 %	52.5420 %

Updated at: 2021-11-30T15:49:21.937703.

Remove GetAdjustedTime from init.cpp fa551b3bdd

MarcoFalke force-pushed on Nov 30, 2021

MarcoFalke commented at 4:22 pm on November 30, 2021: member

Before:

https://drahtbot.space/host_reports/DrahtBot/reports/coverage/bitcoin/bitcoin/ffdf8ee43e2440ab/total.coverage/src/init.cpp.gcov.html#1565

After:

https://drahtbot.space/host_reports/DrahtBot/reports/coverage/bitcoin/bitcoin/23636/total.coverage/src/init.cpp.gcov.html#1565

dongcarl commented at 5:13 pm on November 30, 2021: member

Code Review ACK fa551b3bdd380bcaa8fa929b378b3b6c81a6f65c, noticed the exact same thing here: https://github.com/bitcoin/bitcoin/pull/23280/commits/e073634c37f3a1e140920c6e5e3f2c1ae47cd293

mzumsande commented at 8:32 pm on November 30, 2021: member

Code Review ACK fa551b3bdd380bcaa8fa929b378b3b6c81a6f65c

in src/init.cpp:1564 in fa551b3bdd

1560@@ -1561,7 +1561,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1561 
1562                         const CBlockIndex* tip = chainstate->m_chain.Tip();
1563                         RPCNotifyBlockChange(tip);
1564-                        if (tip && tip->nTime > GetAdjustedTime() + 2 * 60 * 60) {
1565+                        if (tip && tip->nTime > GetTime() + MAX_FUTURE_BLOCK_TIME) {

luke-jr commented at 11:25 pm on November 30, 2021:

Should we have some extra tolerance here in case the adjusted time had been forward a bit?

MarcoFalke commented at 7:07 am on December 1, 2021:

This pull request is a refactor and I recommend to do any behavior changes in separate pull requests.

mzumsande commented at 11:27 am on December 1, 2021:

I don’t understand this: This PR argues that GetAdjustedTime() can’t differ from GetTime() at this point in init, so I can’t see how it could have been “forward a bit”. Or do you mean the former adjusted time at the point in time when we validated the tip?

shaavan approved

shaavan commented at 10:20 am on December 2, 2021: contributor

ACK fa551b3bdd380bcaa8fa929b378b3b6c81a6f65c

I confirmed that GetTime() and GetAdjustedTime() gives exactly same timestamp as output at this stage of init.cpp. So it makes sense to replace it with GetTime() to avoid unnecessary confusion.

Steps I took to confirm the above statement:

Apply following patch:

 0--- a/src/init.cpp
 1+++ b/src/init.cpp
 2@@ -1561,6 +1561,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 3
 4                         const CBlockIndex* tip = chainstate->m_chain.Tip();
 5                         RPCNotifyBlockChange(tip);
 6+                        LogPrintf("GetTime() value: %s\n", GetTime());
 7+                        LogPrintf("GetAdjustedTime() value: %s\n", GetAdjustedTime());
 8                         if (tip && tip->nTime > GetTime() + MAX_FUTURE_BLOCK_TIME) {
 9
10                             strLoadError = _("The block database contains a block which appears to be from the future. "
11                                     "This may be due to your computer's date and time being set incorrectly. "

Compile and run bitcoind
cat ~/.bitcoin/debug.log and search for GetTime() in the displayed text.

Here’s the screenshot of my debug.log file: Screenshot from 2021-12-02 15-28-45

The added test is logically correct and worked successfully on my PC for both the PR and Master branches.

jnewbery commented at 2:04 pm on December 2, 2021: member

Code review ACK fa551b3bdd380bcaa8fa929b378b3b6c81a6f65c

Connman hasn’t started at this point in init, so there’s no way that GetAdjustedTime() can return a different value than GetTime().

theStack approved

theStack commented at 2:09 pm on December 2, 2021: member

Code-review ACK fa551b3bdd380bcaa8fa929b378b3b6c81a6f65c

MarcoFalke referenced this in commit 26a1147ce5 on Dec 2, 2021

MarcoFalke closed this on Dec 2, 2021

MarcoFalke deleted the branch on Dec 2, 2021

sidhujag referenced this in commit a1443f8e0b on Dec 2, 2021

mzumsande commented at 6:16 pm on December 2, 2021: member

This was merged.

RandyMcMillan referenced this in commit 2f01af9c2b on Dec 23, 2021

Fabcien referenced this in commit 8406fa2d0d on Nov 21, 2022

DrahtBot locked this on Dec 2, 2022

Remove GetAdjustedTime from init.cpp #23636

Conflicts

Coverage