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

pull pstratem wants to merge 1 commits into bitcoin:master from pstratem:2016-05-01-checkblock-header changing 2 files +11 −11

pstratem commented at 12:33 AM on May 2, 2016: contributor

Moves both functions closer to being pure.

I want this to improve my fuzzing results.
Add nAdjustedTime parameter to CheckBlock and CheckBlockHeader.
Moves both functions closer to being pure.
b3a9f85929
gmaxwell commented at 12:53 AM on May 2, 2016: contributor

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

utACK b3a9f85
btcdrak commented at 2:22 AM on May 2, 2016: contributor

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

utACK b3a9f8592952569da50c77b277f7ab7146b6cbea
jonasschnelli added the label Refactoring on May 2, 2016
jonasschnelli added the label Consensus on May 2, 2016
MarcoFalke commented at 11:01 AM on May 2, 2016: member

utACK b3a9f85
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)
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.
in src/main.cpp:None 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
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.
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.
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?
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.
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...
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).
MarcoFalke commented at 7:32 PM on June 1, 2016: member

#8077 has been merged.
MarcoFalke closed this on Jun 1, 2016
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Refactoring Consensus

Linked (view graph)

#8077 Consensus: Decouple from chainparams.o and timedata.o