[Consensus] Add nAdjustedTime parameter to CheckBlock and CheckBlockHeader. #7985

1. 
   pstratem commented at 0:33 am on May 2, 2016: contributor

   Moves both functions closer to being pure.

   I want this to improve my fuzzing results.

2. Add nAdjustedTime parameter to CheckBlock and CheckBlockHeader.

   Moves both functions closer to being pure.

   b3a9f85929
3. 
   gmaxwell commented at 0:53 am on May 2, 2016: contributor

   Concept ACK.
4. 
   dcousens commented at 1:27 am on May 2, 2016: contributor

   utACK b3a9f85
5. 
   btcdrak commented at 2:22 am on May 2, 2016: contributor

   utACK
6. 
   jonasschnelli commented at 6:49 am on May 2, 2016: contributor

   utACK b3a9f8592952569da50c77b277f7ab7146b6cbea
7. jonasschnelli added the label Refactoring on May 2, 2016
8. jonasschnelli added the label Consensus on May 2, 2016
9. 
   MarcoFalke commented at 11:01 am on May 2, 2016: member

   utACK b3a9f85
10. 
    laanwj commented at 10:50 am on May 5, 2016: member

    utACK https://github.com/bitcoin/bitcoin/commit/b3a9f8592952569da50c77b277f7ab7146b6cbea

    (maybe this could be taken even further, I’m not sure what you’re rationale is to stop at e.g. ConnectBlock but not TestBlockValidity, but it’s a good start)

11. 
    pstratem commented at 4:35 pm on May 5, 2016: contributor

    @laanwj I was trying to fuzz these two functions and was unable to do so easily.

    In the near future I’m certain I’ll have the same issue elsewhere.

12. in src/main.cpp: in b3a9f85929

    3211@@ -3212,20 +3212,20 @@ bool FindUndoPos(CValidationState &state, int nFile, CDiskBlockPos &pos, unsigne
    3212     return true;
    3213 }
    3214 
    3215-bool CheckBlockHeader(const CBlockHeader& block, CValidationState& state, bool fCheckPOW)
    3216+bool CheckBlockHeader(const CBlockHeader& block, CValidationState& state, int64_t nAdjustedTime, bool fCheckPOW)
    

    ---

    ---

    

    jtimon commented at 10:23 am on May 20, 2016:

    If it’s not too late for bikeshedding…all the times I’ve tried to do this, I’ve named the variable nTime instead of nAdjustedTime. Not important, but would make https://github.com/jtimon/bitcoin/commits/jt easier to rebase to 0.13…

    ---

    

    pstratem commented at 11:14 am on May 20, 2016:

    except… it’s actually the adjusted time

    ---

    

    jtimon commented at 11:37 am on May 20, 2016:

    Whatever…no big deal
13. 
    jtimon commented at 10:25 am on May 20, 2016: contributor

    Can you also pass Consensus::Params like in #7947 ? The diff shouldn’t grow much and it would save us from changing exactly the same lines twice. Should also be better for fuzzing.
14. 
    pstratem commented at 11:26 am on May 20, 2016: contributor

    @jtimon I made a decision not to pass Params() to keep the commit clean and obviously correct.
15. 
    jtimon commented at 11:40 am on May 20, 2016: contributor

    IMO the commit will remain clean and obviously correct. It’s currently +11 -11, and I predict it to become +15 -13 or less. Do you want me to provide a fixup! commit for you to decide whether to squash it or not?
16. 
    pstratem commented at 11:55 am on May 20, 2016: contributor

    @jtimon Nope I think this is as much change as I want to be in this PR.
17. 
    jtimon commented at 12:14 pm on May 20, 2016: contributor

    If we do the thing in 2 steps, it will be about 2 commits +11 -11. I guess I’ll open competing PR then…
18. 
    jtimon commented at 12:51 pm on May 20, 2016: contributor

    Opened #8077 which also does ContextualCheckBlockHeader (+16 -17). I believe that’s still clean and obviously correct (even if the scope is bigger).
19. 
    MarcoFalke commented at 7:32 pm on June 1, 2016: member

    #8077 has been merged.
20. MarcoFalke closed this on Jun 1, 2016

    ---

21. MarcoFalke locked this on Sep 8, 2021

Contributors
pstratem gmaxwell dcousens btcdrak jonasschnelli MarcoFalke laanwj jtimon

Labels
Refactoring Consensus