Remove GetAdjustedTime from init.cpp by maflcko · Pull Request #23636 · bitcoin/bitcoin

maflcko · 2021-11-30T14:48:53Z

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

Also, add missing addrman.h includes

DrahtBot · 2021-11-30T15:49:22Z

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, `ffdf8ee`)
Lines	+0.0028 %	84.0014 %
Functions	+0.0000 %	76.2420 %
Branches	+0.0039 %	52.5420 %

^{Updated at: 2021-11-30T15:49:21.937703.}

maflcko · 2021-11-30T16:22:25Z

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 · 2021-11-30T17:13:41Z

Code Review ACK fa551b3, noticed the exact same thing here: e073634

mzumsande · 2021-11-30T20:32:34Z

Code Review ACK fa551b3

luke-jr · 2021-11-30T23:25:58Z

src/init.cpp

                        const CBlockIndex* tip = chainstate->m_chain.Tip();
                        RPCNotifyBlockChange(tip);
-                        if (tip && tip->nTime > GetAdjustedTime() + 2 * 60 * 60) {
+                        if (tip && tip->nTime > GetTime() + MAX_FUTURE_BLOCK_TIME) {


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

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

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

ACK fa551b3

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:

--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1561,6 +1561,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)

                         const CBlockIndex* tip = chainstate->m_chain.Tip();
                         RPCNotifyBlockChange(tip);
+                        LogPrintf("GetTime() value: %s\n", GetTime());
+                        LogPrintf("GetAdjustedTime() value: %s\n", GetAdjustedTime());
                         if (tip && tip->nTime > GetTime() + MAX_FUTURE_BLOCK_TIME) {

                             strLoadError = _("The block database contains a block which appears to be from the future. "
                                     "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:

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

jnewbery · 2021-12-02T14:04:56Z

Code review ACK fa551b3

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

theStack

Code-review ACK fa551b3

fa551b3 Remove GetAdjustedTime from init.cpp (MarcoFalke) fa815f8 Replace addrman.h include with forward decl in net.h (MarcoFalke) Pull request description: 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 ACKs for top commit: dongcarl: Code Review ACK fa551b3, noticed the exact same thing here: bitcoin/bitcoin@e073634 mzumsande: Code Review ACK fa551b3 jnewbery: Code review ACK fa551b3 shaavan: ACK fa551b3 theStack: Code-review ACK fa551b3 Tree-SHA512: 15807a0e943e3e8d8c5250c8f6d7b56afb26002b1e290bf93636a2c747f27e78f01f1de04ce1a83d6339e27284c69c43e077a8467545c4078746f4c1ecb1164d

fa551b3 Remove GetAdjustedTime from init.cpp (MarcoFalke) fa815f8 Replace addrman.h include with forward decl in net.h (MarcoFalke) Pull request description: 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 ACKs for top commit: dongcarl: Code Review ACK fa551b3, noticed the exact same thing here: bitcoin@e073634 mzumsande: Code Review ACK fa551b3 jnewbery: Code review ACK fa551b3 shaavan: ACK fa551b3 theStack: Code-review ACK fa551b3 Tree-SHA512: 15807a0e943e3e8d8c5250c8f6d7b56afb26002b1e290bf93636a2c747f27e78f01f1de04ce1a83d6339e27284c69c43e077a8467545c4078746f4c1ecb1164d

mzumsande · 2021-12-02T18:16:15Z

This was merged.

3bfde7d Remove GetAdjustedTime from init.cpp (MarcoFalke) ac31727 Replace addrman.h include with forward decl in net.h (MarcoFalke) Pull request description: 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 ACKs for top commit: dongcarl: Code Review ACK 3bfde7d, noticed the exact same thing here: bitcoin/bitcoin@e073634 mzumsande: Code Review ACK 3bfde7d jnewbery: Code review ACK 3bfde7d shaavan: ACK 3bfde7d theStack: Code-review ACK 3bfde7d Tree-SHA512: 15807a0e943e3e8d8c5250c8f6d7b56afb26002b1e290bf93636a2c747f27e78f01f1de04ce1a83d6339e27284c69c43e077a8467545c4078746f4c1ecb1164d

Summary: ``` 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 ``` Backport of [[bitcoin/bitcoin#23636 | core#23636]]. Depends on D12548 and D12552. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12554

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

fa815f8

Also, add missing addrman.h includes

maflcko added the Refactoring label Nov 30, 2021

Remove GetAdjustedTime from init.cpp

fa551b3

maflcko force-pushed the 2111-noTimeInit branch from faabea3 to fa551b3 Compare November 30, 2021 16:20

DrahtBot mentioned this pull request Nov 30, 2021

init: Coalesce Chainstate loading sequence between {,non-}unittest codepaths #23280

Merged

luke-jr reviewed Nov 30, 2021

View reviewed changes

shaavan approved these changes Dec 2, 2021

View reviewed changes

theStack approved these changes Dec 2, 2021

View reviewed changes

maflcko closed this Dec 2, 2021

maflcko deleted the 2111-noTimeInit branch December 2, 2021 14:31

bitcoin locked and limited conversation to collaborators Dec 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove GetAdjustedTime from init.cpp #23636

Remove GetAdjustedTime from init.cpp #23636
maflcko wants to merge 2 commits intobitcoin:masterfrom
maflcko:2111-noTimeInit

maflcko commented Nov 30, 2021

Uh oh!

DrahtBot commented Nov 30, 2021 •

edited

Loading

Uh oh!

maflcko commented Nov 30, 2021

Uh oh!

dongcarl commented Nov 30, 2021 •

edited

Loading

Uh oh!

mzumsande commented Nov 30, 2021

Uh oh!

luke-jr Nov 30, 2021

Uh oh!

maflcko Dec 1, 2021

Uh oh!

mzumsande Dec 1, 2021

Uh oh!

shaavan left a comment

Uh oh!

jnewbery commented Dec 2, 2021

Uh oh!

theStack left a comment

Uh oh!

mzumsande commented Dec 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Conversation

maflcko commented Nov 30, 2021

Uh oh!

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

Uh oh!

Conflicts

Coverage

Uh oh!

maflcko commented Nov 30, 2021

Uh oh!

dongcarl commented Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzumsande commented Nov 30, 2021

Uh oh!

luke-jr Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

mzumsande Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Dec 2, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande commented Dec 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented Nov 30, 2021 •

edited

Loading

dongcarl commented Nov 30, 2021 •

edited

Loading