Global params cleanup #7947
Global params cleanup #7947
Conversation
| @@ -3212,20 +3212,20 @@ bool FindUndoPos(CValidationState &state, int nFile, CDiskBlockPos &pos, unsigne | |||
| return true; | |||
| } | |||
|
|
|||
| bool CheckBlockHeader(const CBlockHeader& block, CValidationState& state, bool fCheckPOW) | |||
| bool CheckBlockHeader(const CBlockHeader& block, CValidationState& state, const Consensus::Params& consensusParams, int64_t nTime, bool fCheckPOW) | |||
There was a problem hiding this comment.
I have never been sure on whether int64_t was the most appropriate type here or not.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
|
Trivial: Added explicit CChainParams& to LoadBlockIndexDB() and LoadBlockIndex() |
|
@hmel Does this compile and pass the test suites locally? |
|
I haven't tested the rpc tests. In this branch, unittests start failing with the second commit 9fc71c5 . 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 b9fb24f 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: 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 |
| @@ -2461,8 +2461,7 @@ enum FlushStateMode { | |||
| * if they're too large, if it's been a while since the last write, | |||
| * or always and in all cases if we're in prune mode and are deleting files. | |||
| */ | |||
| bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode) { | |||
| const CChainParams& chainparams = Params(); | |||
| bool static FlushStateToDisk(CValidationState &state, const CChainParams& chainparams, FlushStateMode mode) { | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, we already have that? Awesome!
|
Closing due to inactivity. |
|
@hmel Sorry that I didn't came back to you when you got travis erros. I got confused locally too. 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. |
|
Feel free to pick this up again! |
|
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). |
|
Be glad to. I'll see what I can do in the coming weeks. |
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()