pass Consensus::Params& to functions in validation.cpp and make them static

mariodian commented at 1:45 pm on April 13, 2017: contributor

Pass Consensus::Params& to ReceivedBlockTransactions() as requested in #7829

TheBlueMatt commented at 1:53 pm on April 13, 2017: member

utACK. Want to mark ReceivedBlockTransactions static while you’re at it?

mariodian commented at 2:03 pm on April 13, 2017: contributor

@TheBlueMatt sure. Should I soft reset the commit and add the new change within a single commit?

TheBlueMatt commented at 2:15 pm on April 13, 2017: member

Doesnt matter either way, I think.

mariodian force-pushed on Apr 13, 2017

fanquake added the label Validation on Apr 14, 2017

paveljanik commented at 4:16 pm on April 17, 2017: contributor

ACK https://github.com/bitcoin/bitcoin/pull/10201/commits/25660e91ef2d399d2db87edb2243122fd4093ede

dcousens approved

jtimon commented at 9:38 pm on April 20, 2017: contributor

ACK 25660e91ef2d399d2db87edb2243122fd4093ede Added to the list in #7829 If you feel like doing more functions in one go, maybe I can close #10227 (sorry I didn’t see this before opening it, you need to “ping @jtimon” to get the promised review faster). Just by equaling it I would be more than happy to close #10227, but it occurs to me that I didn’t do an exhaustive search on all the functions in validation.o that could be made static (independently on whether they use CChainParams or Consensus::Params or not). If you do that, I bet not only nobody will complain for increasing the scope of a PR with 3 acks but people will actually praise you (at least I will) for every new function that can be static you find apart from the 3 in #10227. I think it’s also a more interesting and rewarding task than what #7829 proposes alone.

EDIT: or maybe a separated PR for that makes more sense, I’m not sure

mariodian commented at 1:23 am on April 21, 2017: contributor

@jtimon this was the first time I was looking at a C++ code so my understanding of the language is next to zero. However, the similar pattern all the static functions seem to have is that they’re only called within the same file. If that’s the case (?) then I’ll be able to do it (perhaps in a separate PR as you suggested).

jtimon commented at 9:45 am on April 21, 2017: contributor

Yes, static functions are only called from the same file. So for searching more functions in validation.cpp that could be static, it’s more about grep than about C++ (and as said feel free to copy from #10227 ). Either way I already acked this PR as it is, but would be happy to review more.

mariodian commented at 4:18 pm on April 21, 2017: contributor

ping @jtimon

Made other methods static (not sure about FILE* OpenDiskFile() and FILE* OpenUndoFile() though).

Also copied #10227 and added parameter chainparams to FlushStateToDisk().

laanwj referenced this in commit a6548a47a5 on Apr 21, 2017

jtimon commented at 5:49 pm on April 21, 2017: contributor

Yeah, it seems OpenDiskFile and OpenUndoFile are only called from validation.cpp and can be static too.

EDIT: also it seems @laanwj added one commit here by mistake.

in src/validation.h:292 in 37254e7098 outdated

280@@ -281,23 +281,6 @@ CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams);
281 double GuessVerificationProgress(const ChainTxData& data, CBlockIndex* pindex);
282 
283 /**
284- * Prune block and undo files (blk???.dat and undo???.dat) so that the disk space used is less than a user-defined target.

jtimon commented at 5:51 pm on April 21, 2017:

The documentation you are removing from here, you could move to the cpp with the function definition

in src/validation.cpp:186 in 37254e7098 outdated

182@@ -183,8 +183,8 @@ enum FlushStateMode {
183 };
184 
185 // See definition for documentation
186-bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode, int nManualPruneHeight=0);
187-void FindFilesToPruneManual(std::set<int>& setFilesToPrune, int nManualPruneHeight);
188+bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode, int nManualPruneHeight=0, const CChainParams& chainParams);

jtimon commented at 5:53 pm on April 21, 2017:

Given that nManualPruneHeight is optional, the new CChainParams parameter (which is not) should come before it, for example, CChainParams can be the first parameter.

mariodian commented at 6:00 pm on April 21, 2017:

Right

mariodian force-pushed on Apr 21, 2017

mariodian commented at 6:46 pm on April 21, 2017: contributor

@jtimon oh, I “accidentally” soft reset my first commit instead of fixingup the last one so everything is in the single commit now. Sorry, I haven’t worked with git for quite a long time.

jtimon commented at 7:17 pm on April 21, 2017: contributor

Mhmm, squashing in one commit is fine, what I mean is you also added a commit that doesn’t belong here: “Merge branch ‘master’ into consensusparams-receivedblocktransactions” 8b23dfd Here’s what you can do to fix this: rebase -i origin/master and remove that commit from the list.

mariodian force-pushed on Apr 21, 2017

jtimon commented at 2:38 pm on April 27, 2017: contributor

Needs rebase. EDIT: Also, perhaps changing the tittle name now that it does more things?

mariodian renamed this:
~~pass Consensus::Params& to ReceivedBlockTransactions()~~
pass Consensus::Params& to functions in validation.cpp and make them static
on Apr 27, 2017

mariodian commented at 3:08 pm on April 27, 2017: contributor

@jtimon I changed the title.

Before I break anything while rebasing, is the following all that is needed?

0git checkout consensusparams-receivedblocktransactions
1git rebase master
2git checkout master
3git merge consensusparams-receivedblocktransactions

Thank you.

jtimon commented at 4:00 pm on April 27, 2017: contributor

what I would usually do:

0git checkout consensusparams-receivedblocktransactions
1git fetch origin
2git rebase -i origin/master
3git push jtimon +consensusparams-receivedblocktransactions

Instead of jtimon, you need to use something else pointing to you repository For example, in my bitcoin./git/config I have (among other things):

0[remote "origin"]
1	url = github:bitcoin/bitcoin.git
2	fetch = +refs/heads/*:refs/remotes/origin/*
3[remote "jtimon"]
4	url = github:jtimon/bitcoin.git
5	fetch = +refs/heads/*:refs/remotes/jtimon/*

Using -i (for interactive) it’s not very useful in this case because it is only one commit and the rebase will stop in it for the conflict anyway, but you can try to replace s/pick/edit/ or s/pick/reword/ to experiment with it. You can also use it for squash commits or remove them, I use interactive rebase all the time.

mariodian force-pushed on Apr 27, 2017

mariodian commented at 6:20 pm on April 27, 2017: contributor

@jtimon I just rebased it. Thanks so much for the help!

jtimon commented at 6:57 pm on April 27, 2017: contributor

You are welcomed, I’m happy to teach and you don’t seem afraid to learn.

Travis is already failing: https://travis-ci.org/bitcoin/bitcoin/jobs/226509712#L1865 You should compile locally before pushing (specially before force-pushing [the ‘+’ symbol before your branch name means force push]). Even better if you also run the unittests by doing “make check” (you can use several cores/hyperthreads to build faster with -j4 [example for 4 processes, but depends on you machine]). This PR shouldn’t really need it, but you can also run the python tests to be more sure your changes are correct, see https://github.com/bitcoin/bitcoin/tree/master/test (and by the way, writing functional tests is a good way to contribute and learn, plus python is easier to learn than C++).

At least one of your problems seems to be that you need a forward declaration for CheckInputs, but there may be more and you should be able to easily find them and fix them with the help of the compiler. I’m also jtimon on IRC freenode if you want to ask me questions there too, maybe in #bitcoin-core-dev too if I’m not around. That may be more convenient and also less disruptive to other reviewers.

mariodian force-pushed on Apr 27, 2017

jtimon commented at 0:54 am on April 28, 2017: contributor

Although it has been very useful for me to look only at the latest changes you have done to fix the problems separately and it is a nice practice to facilitate reviewers work (ie now I can approve 02cf6e1 independently, without having to re-approve cf8243b which I didn’t because it was wrong, but normally it would be just allowing to review the latest changes without re-reviewing anything they have already approved), this PR should not be merged as it is because not all the commits are correct individually.

Even travis is saying it, but even if we didn’t had Continuous Integration, we already figured out that cf8243b is incorrect locally. Please, squash again so that every commit is correct individually. People make mistakes, but it is always better for everyone to rewrite git history before it happens (is merged) than afterwards. Since we always maintain at least 2 major versions simultaneously, having a clean, and ideally always-correct history helps maintaining things.

Your only reserve for squashing could be invalidating previous approvals, but note that @paveljanik , @dcousens and me only approved 25660e9 , which this PR no longer contains. That’s my fault for asking you to increase the scope of the PR, but I really think also turning functions into static will be more interesting for reviewers than just reducing the dependency to the “Params()” function, which not everyone cares about. After you squash, another ACK shouldn’t take me much time, this looks good as it is for me.

But I’m also sorry to tell you, I don’t this will be the last time you will have to rebase. Changing the function signature is usually easy to write but often painful to maintain. Maybe not. Some PRs are merged in days, some after months without needing rebase, some need rebase all the time.

One way to solve potential future conflicts in advance is by making sure your branch could rebase cleanly (ie without any conflict/modification) on top of all the currently open PRs. That works for small projects, but not for this one, see https://github.com/bitcoin/bitcoin/pulls. You can wait, find something taking a look or just be warned: this conflicts with #10192 (in CheckInputs at least) and probably #10279 too, who knows what else. I see 3 options to advance further:

Don’t try to solve conflicts in advance, leave things as they are and rebase things as open conflicts get merged.
Avoid the conflicts by reducing the scope of your own PR.
Coordinate with the other PR to share preparation commits at the beginning of both or on another PR with only the common parts.

I really look for option 3, but 1 and 2 are usually easier.

mariodian force-pushed on Apr 28, 2017

mariodian commented at 3:57 pm on May 7, 2017: contributor

@jtimon for now, I’d go for the 1st option which makes it look way easier and less time-consuming. If I contribute in the future, I’d definitely make it less chaotic by reducing the scope of the PR and possibly coordinate with other PRs if necessary.

jtimon commented at 4:59 am on May 19, 2017: contributor

Needs rebase

mariodian force-pushed on May 21, 2017

mariodian force-pushed on Jun 1, 2017

jtimon commented at 11:53 pm on June 1, 2017: contributor

needs rebase again

mariodian force-pushed on Jun 2, 2017

Make functions in validation.cpp static and pass chainparams

Fix bugs as per PR comment

Change bool static to static bool

24980a3e40

in src/validation.cpp:189 in aba9780c79 outdated

185@@ -186,8 +186,11 @@ enum FlushStateMode {
186 };
187 
188 // See definition for documentation
189-bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode, int nManualPruneHeight=0);
190-void FindFilesToPruneManual(std::set<int>& setFilesToPrune, int nManualPruneHeight);
191+bool static FlushStateToDisk(const CChainParams& chainParams, CValidationState &state, FlushStateMode mode, int nManualPruneHeight=0);

laanwj commented at 3:26 pm on June 5, 2017:

static bool iso bool static would be more consistent with other uses of static in the code

mariodian commented at 2:22 pm on June 6, 2017:

I agree. Changed it.

mariodian force-pushed on Jun 6, 2017

laanwj merged this on Jun 6, 2017

laanwj closed this on Jun 6, 2017

laanwj referenced this in commit 1b708f2cf3 on Jun 6, 2017

mariodian deleted the branch on Jun 25, 2017

PastaPastaPasta referenced this in commit 5fc2073eaa on May 21, 2019

PastaPastaPasta referenced this in commit 35754bcf16 on May 22, 2019

PastaPastaPasta referenced this in commit 689127ab7e on May 22, 2019

PastaPastaPasta referenced this in commit 6cc54bec5a on May 22, 2019

PastaPastaPasta referenced this in commit ede6f0efcf on May 23, 2019

PastaPastaPasta referenced this in commit dca7d7e5b4 on May 23, 2019

PastaPastaPasta referenced this in commit f973d66270 on May 28, 2019

PastaPastaPasta referenced this in commit aa7d01dd4b on May 28, 2019

PastaPastaPasta referenced this in commit f60f02ab43 on May 28, 2019

PastaPastaPasta referenced this in commit 6fecb8612a on Jun 8, 2019

PastaPastaPasta referenced this in commit 90d5e7c5ec on Jun 8, 2019

PastaPastaPasta referenced this in commit 33544a4305 on Jun 10, 2019

PastaPastaPasta referenced this in commit e8bea7f85b on Jun 10, 2019

PastaPastaPasta referenced this in commit 3fcd95cdb1 on Jun 10, 2019

PastaPastaPasta referenced this in commit 131e94ef21 on Jun 11, 2019

PastaPastaPasta referenced this in commit cb5349cc47 on Jun 11, 2019

barrystyle referenced this in commit 0ad9445bbb on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021

pass Consensus::Params& to functions in validation.cpp and make them static #10201