Global params cleanup #7947

hmel commented at 12:14 PM on April 26, 2016: contributor

Some more work on global Params() cleanup as part of #7829 Changed FlushStateToDisk(), CheckBlockHeader(), CheckBlock() and ContextualCheckBlockHeader() to accept relevant ChainParams instead of calling global Params()

Explicitly pass CChainParams& to FlushStateToDisk() 190370b1c8

Explicitly pass Consensus::Params& to ContextualCheckBlockHeader() 9fc71c544e

Explicitly pass consensus params and adjusted time to CheckBlockHeader() b9fb24fc0f

in src/main.cpp:None in b9fb24fc0f outdated

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, const Consensus::Params& consensusParams, int64_t nTime, bool fCheckPOW)

jtimon commented at 12:18 PM on April 26, 2016:

I have never been sure on whether int64_t was the most appropriate type here or not.

sipa commented at 12:21 PM on April 26, 2016:

It is. Only "block time" is sometimes a 32-bit integer as we can't just change the block headers. Otherwise, always use int64_t for time.

jtimon commented at 12:25 PM on April 26, 2016:

Yeah, precisely for that (because CBlockHeader::nTime uses 32 bit) I thought that maybe it would make more sense to use 32 bit in the code that is intended to become part of libconsensus. But if doesn't make sense passing it like this is fine, thanks for the fast answer.

in src/main.h:None in b9fb24fc0f outdated

 445 | @@ -446,13 +446,13 @@ bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus
 446 |  /** Functions for validating blocks and updating the block tree */
 447 |  
 448 |  /** Context-independent validity checks */
 449 | -bool CheckBlockHeader(const CBlockHeader& block, CValidationState& state, bool fCheckPOW = true);
 450 | -bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW = true, bool fCheckMerkleRoot = true);
 451 | +bool CheckBlockHeader(const CBlockHeader& block, CValidationState& state, const Consensus::Params& consensusParams, int64_t nTime, bool fCheckPOW = true);
 452 | +bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true, bool fCheckMerkleRoot = true);

jtimon commented at 12:21 PM on April 26, 2016:

If it was me, I would put CheckBlockHeader, CheckBlock and ContextualCheckBlockHeader in the Consensus namespace at this point (otherwise causing equivalent disruption again later only for the namespace seems unlikely to ever happen). Not that this is very important, just want to point it out to hear what others think.

jtimon commented at 12:21 PM on April 26, 2016: contributor

utACK b9fb24f

Explicitly pass CChainParams& to LoadBlockIndexDB() 418d9627de

hmel commented at 2:22 PM on April 26, 2016: contributor

Trivial: Added explicit CChainParams& to LoadBlockIndexDB() and LoadBlockIndex()

MarcoFalke commented at 9:07 AM on April 27, 2016: member

@hmel Does this compile and pass the test suites locally?

MarcoFalke added the label Refactoring on Apr 27, 2016

jtimon commented at 5:33 PM on April 29, 2016: contributor

I haven't tested the rpc tests. In this branch, unittests start failing with the second commit https://github.com/bitcoin/bitcoin/pull/7947/commits/9fc71c544e592f6c51c3007030ceaafc1f7b9737 . But I'm pretty sure that commit is correct, and if you put it before the "FlushStateToDisk()" one both pass the unitests, and then the next commit fails. After tweaking the order a little bit, it seems the commit that is causing the error is https://github.com/bitcoin/bitcoin/pull/7947/commits/b9fb24fc0f047072bc1d14a07de222645fdf2ab5

Even if you put it the first (the rest of the commits pass unittests when alone and combined), you get:

test_bitcoin: main.cpp:1720: void InvalidChainFound(CBlockIndex*): Assertion `tip' failed.

I'm not really sure what is going on here, though. @hmel Every commit in a PR should pass the unittests, one by one. One way to guarantee this is edit all the commits with an interactive and make check them one by one

Another slightly less complete option is running:

git bisect start HEAD base
git bisect run make [distcheck] check -j6
git bisect reset
(distcheck optional, assuming 6 cores/hyperthreads)

This will just binary search for the first commit that fails, assuming that once it fails it will fail in all the following commits too, that's what I mean by "less complete".

Apart from make check, you can run more tests locally with

python ./qa/pull-tester/rpc-tests.py
python ./qa/pull-tester/rpc-tests.py -extended (this is slow)
python ./qa/pull-tester/rpc-tests.py mempool_limit (if you just want to run a single rpc test like mempool_limit.py)

in src/main.cpp:None in 418d9627de

2460 | @@ -2461,8 +2461,7 @@ enum FlushStateMode {
2461 |   * if they're too large, if it's been a while since the last write,
2462 |   * or always and in all cases if we're in prune mode and are deleting files.
2463 |   */
2464 | -bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode) {
2465 | -    const CChainParams& chainparams = Params();
2466 | +bool static FlushStateToDisk(CValidationState &state, const CChainParams& chainparams, FlushStateMode mode) {

jtimon commented at 5:38 PM on April 29, 2016:

Syle nit: If I remember correctly, https://github.com/bitcoin/bitcoin/blob/master/src/.clang-format says to break after function or method headers in definitions (ie, move the opening bracket to the next line), but I can't find the line that says so (may just default to true).

MarcoFalke commented at 10:00 PM on April 30, 2016:

If you don't want to do that manually (or remember the exact "rules"), you can just do $ git diff HEAD~ -U0|cfd. Where cfd is an alias for ./contrib/devtools/clang-format-diff.py -p1 -i -v.

jtimon commented at 10:32 PM on April 30, 2016:

Oh, we already have that? Awesome!

in src/main.cpp:None in 418d9627de

3219 |      if (fCheckPOW && !CheckProofOfWork(block.GetHash(), block.nBits, Params().GetConsensus()))
3220 |          return state.DoS(50, false, REJECT_INVALID, "high-hash", false, "proof of work failed");
3221 |  
3222 |      // Check timestamp
3223 | -    if (block.GetBlockTime() > GetAdjustedTime() + 2 * 60 * 60)
3224 | +    if (block.GetBlockTime() > nTime + 2 * 60 * 60)

MarcoFalke commented at 5:45 PM on April 29, 2016:

Is this worth the refactoring?

jtimon commented at 6:14 PM on April 29, 2016:

If we want this code to ever get into the consensus module/package, yes, then the function shouldn't depend on timedata.h (just like it shouldn't depend on chainparams.h).

in src/main.cpp:None in 418d9627de

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, const Consensus::Params& consensusParams, int64_t nTime, bool fCheckPOW)
3217 |  {
3218 |      // Check proof of work matches claimed amount
3219 |      if (fCheckPOW && !CheckProofOfWork(block.GetHash(), block.nBits, Params().GetConsensus()))

jtimon commented at 6:16 PM on April 29, 2016:

s/Params().GetConsensus()/consensusParams

dcousens commented at 1:21 AM on May 2, 2016: contributor

utACk b9fb24f, agreed with nits by @jtimon

Overlaps #7985

MarcoFalke commented at 7:29 PM on June 1, 2016: member

Closing due to inactivity.

MarcoFalke closed this on Jun 1, 2016

jtimon commented at 10:02 PM on June 1, 2016: contributor

@hmel Sorry that I didn't came back to you when you got travis erros. I got confused locally too. In any case, #8077 has been merged which contained some of what this had. But you had more than that and I hope you're open to rebase that and keep merging things.

I just cared too much about those few lines to let @pstratem do something different from what I wanted without listening to me for a while first. I will update #7829 but please don't feel like you have to wait for it. Please feel free to rebase and reopen this, open another fresh new PR (probably that is preferable) or whatever. Ping me and I will try to help.

sipa commented at 10:07 PM on June 1, 2016: member

Feel free to pick this up again!

jtimon commented at 4:27 PM on July 18, 2016: contributor

Now that 0.13 has branched, refactor changes are more likely to get merged for a while. Maybe this is a good time to pick this up again, or something else from #7829 (the list is now up to date).

hmel commented at 1:20 PM on July 19, 2016: contributor

Be glad to. I'll see what I can do in the coming weeks.

hmel deleted the branch on Mar 27, 2018

MarcoFalke locked this on Sep 8, 2021