Remove GetAdjustedTime from init.cpp #23636

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2111-noTimeInit changing 8 files +27 −7
  1. 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
  2. Replace addrman.h include with forward decl in net.h
    Also, add missing addrman.h includes
    fa815f8473
  3. MarcoFalke added the label Refactoring on Nov 30, 2021
  4. 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.

  5. Remove GetAdjustedTime from init.cpp fa551b3bdd
  6. MarcoFalke force-pushed on Nov 30, 2021
  7. 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
  8. mzumsande commented at 8:32 pm on November 30, 2021: member
    Code Review ACK fa551b3bdd380bcaa8fa929b378b3b6c81a6f65c
  9. 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?
  10. shaavan approved
  11. 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:

    1. 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. "
    
    1. Compile and run bitcoind
    2. 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.

  12. 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().

  13. theStack approved
  14. theStack commented at 2:09 pm on December 2, 2021: member
    Code-review ACK fa551b3bdd380bcaa8fa929b378b3b6c81a6f65c
  15. MarcoFalke referenced this in commit 26a1147ce5 on Dec 2, 2021
  16. MarcoFalke closed this on Dec 2, 2021

  17. MarcoFalke deleted the branch on Dec 2, 2021
  18. sidhujag referenced this in commit a1443f8e0b on Dec 2, 2021
  19. mzumsande commented at 6:16 pm on December 2, 2021: member
    This was merged.
  20. RandyMcMillan referenced this in commit 2f01af9c2b on Dec 23, 2021
  21. Fabcien referenced this in commit 8406fa2d0d on Nov 21, 2022
  22. DrahtBot locked this on Dec 2, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-19 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me