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

mariodian · 2017-04-13T13:45:38Z

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

TheBlueMatt · 2017-04-13T13:53:03Z

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

mariodian · 2017-04-13T14:03:33Z

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

TheBlueMatt · 2017-04-13T14:15:47Z

Doesnt matter either way, I think.

paveljanik · 2017-04-17T16:16:47Z

ACK 25660e9

jtimon · 2017-04-20T21:38:36Z

ACK 25660e9
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 · 2017-04-21T01:23:36Z

@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 · 2017-04-21T09:45:51Z

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 · 2017-04-21T16:18:39Z

ping @jtimon

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

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

25660e9 pass Consensus::Params& to ReceivedBlockTransactions() (Mario Dian) Tree-SHA512: d3a5b19d93313e4bda622b322bc9cbfb7e31486010eac40fca6eea9703f814f9667f778122ba7366bb304482a2c03e2e3325083beecac374751692361952e467

jtimon · 2017-04-21T17:49:56Z

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.

jtimon · 2017-04-21T17:51:37Z

src/validation.h

@@ -281,23 +281,6 @@ CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams);
 double GuessVerificationProgress(const ChainTxData& data, CBlockIndex* pindex);

 /**
- * Prune block and undo files (blk???.dat and undo???.dat) so that the disk space used is less than a user-defined target.


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

jtimon · 2017-04-21T17:53:10Z

src/validation.cpp

@@ -183,8 +183,8 @@ enum FlushStateMode {
 };

 // See definition for documentation
-bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode, int nManualPruneHeight=0);
-void FindFilesToPruneManual(std::set<int>& setFilesToPrune, int nManualPruneHeight);
+bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode, int nManualPruneHeight=0, const CChainParams& chainParams);


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 · 2017-04-21T18:46:37Z

@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 · 2017-04-21T19:17:41Z

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.

jtimon · 2017-04-27T14:38:38Z

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

mariodian · 2017-04-27T15:08:25Z

@jtimon I changed the title.

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

git checkout consensusparams-receivedblocktransactions
git rebase master
git checkout master
git merge consensusparams-receivedblocktransactions

Thank you.

jtimon · 2017-04-27T16:00:18Z

what I would usually do:

git checkout consensusparams-receivedblocktransactions
git fetch origin
git rebase -i origin/master
git 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):

[remote "origin"]
	url = github:bitcoin/bitcoin.git
	fetch = +refs/heads/*:refs/remotes/origin/*
[remote "jtimon"]
	url = github:jtimon/bitcoin.git
	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.

jtimon · 2017-05-19T04:59:26Z

Needs rebase

jtimon · 2017-06-01T23:53:12Z

needs rebase again

laanwj · 2017-06-05T15:26:58Z

src/validation.cpp

@@ -186,8 +186,11 @@ enum FlushStateMode {
 };

 // See definition for documentation
-bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode, int nManualPruneHeight=0);
-void FindFilesToPruneManual(std::set<int>& setFilesToPrune, int nManualPruneHeight);
+bool static FlushStateToDisk(const CChainParams& chainParams, CValidationState &state, FlushStateMode mode, int nManualPruneHeight=0);


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

I agree. Changed it.

Fix bugs as per PR comment Change bool static to static bool

…and make them static 24980a3 Make functions in validation.cpp static and pass chainparams (Mario Dian) Tree-SHA512: 2d1b7b0ffd851317ed522911c1b6592855376c5cbc65d71ec0f8aa507eb6caef21b0709b3692255f1d719662db7447579c10df02f6ef4cd35fcb0bdf2e653af6

…ions() 25660e9 pass Consensus::Params& to ReceivedBlockTransactions() (Mario Dian) Tree-SHA512: d3a5b19d93313e4bda622b322bc9cbfb7e31486010eac40fca6eea9703f814f9667f778122ba7366bb304482a2c03e2e3325083beecac374751692361952e467

…on.cpp and make them static 24980a3 Make functions in validation.cpp static and pass chainparams (Mario Dian) Tree-SHA512: 2d1b7b0ffd851317ed522911c1b6592855376c5cbc65d71ec0f8aa507eb6caef21b0709b3692255f1d719662db7447579c10df02f6ef4cd35fcb0bdf2e653af6

…on.cpp and make them static 24980a3 Make functions in validation.cpp static and pass chainparams (Mario Dian) Tree-SHA512: 2d1b7b0ffd851317ed522911c1b6592855376c5cbc65d71ec0f8aa507eb6caef21b0709b3692255f1d719662db7447579c10df02f6ef4cd35fcb0bdf2e653af6 fix Signed-off-by: Pasta <Pasta@dash.org> add param Signed-off-by: Pasta <Pasta@dash.org>

mariodian force-pushed the consensusparams-receivedblocktransactions branch from 5fbaec1 to 25660e9 Compare April 13, 2017 14:38

fanquake added the Validation label Apr 14, 2017

dcousens approved these changes Apr 17, 2017

View reviewed changes

paveljanik mentioned this pull request Apr 18, 2017

Make functions in validation.cpp static: #10227

Closed

jtimon mentioned this pull request Apr 20, 2017

Globals: TODO: Experiment: Kill "Params()" #7829

Closed

15 tasks

jtimon reviewed Apr 21, 2017

View reviewed changes

mariodian force-pushed the consensusparams-receivedblocktransactions branch 5 times, most recently from 8bc88d9 to a0a57e1 Compare April 21, 2017 18:45

mariodian force-pushed the consensusparams-receivedblocktransactions branch from 8b23dfd to a0a57e1 Compare April 21, 2017 20:08

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

mariodian force-pushed the consensusparams-receivedblocktransactions branch from a0a57e1 to cf8243b Compare April 27, 2017 18:18

mariodian force-pushed the consensusparams-receivedblocktransactions branch from af8be93 to 1dfdd81 Compare May 21, 2017 15:50

mariodian force-pushed the consensusparams-receivedblocktransactions branch 2 times, most recently from d5eba1b to 1fe4f4c Compare June 1, 2017 13:58

mariodian force-pushed the consensusparams-receivedblocktransactions branch from 1fe4f4c to aba9780 Compare June 2, 2017 06:20

laanwj reviewed Jun 5, 2017

View reviewed changes

Make functions in validation.cpp static and pass chainparams

24980a3

Fix bugs as per PR comment Change bool static to static bool

mariodian force-pushed the consensusparams-receivedblocktransactions branch from aba9780 to 24980a3 Compare June 6, 2017 14:21

laanwj merged commit 24980a3 into bitcoin:master Jun 6, 2017

mariodian deleted the consensusparams-receivedblocktransactions branch June 25, 2017 06:36

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

mariodian commented Apr 13, 2017

Uh oh!

TheBlueMatt commented Apr 13, 2017

Uh oh!

mariodian commented Apr 13, 2017

Uh oh!

TheBlueMatt commented Apr 13, 2017

Uh oh!

paveljanik commented Apr 17, 2017

Uh oh!

jtimon commented Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariodian commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtimon commented Apr 21, 2017

Uh oh!

mariodian commented Apr 21, 2017

Uh oh!

jtimon commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtimon Apr 21, 2017

Choose a reason for hiding this comment

Uh oh!

jtimon Apr 21, 2017

Choose a reason for hiding this comment

Uh oh!

mariodian Apr 21, 2017

Choose a reason for hiding this comment

Uh oh!

mariodian commented Apr 21, 2017

Uh oh!

jtimon commented Apr 21, 2017

Uh oh!

jtimon commented Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariodian commented Apr 27, 2017

Uh oh!

jtimon commented Apr 27, 2017

Uh oh!

jtimon commented May 19, 2017

Uh oh!

jtimon commented Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj Jun 5, 2017

Choose a reason for hiding this comment

Uh oh!

mariodian Jun 6, 2017

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtimon commented Apr 20, 2017 •

edited

Loading

mariodian commented Apr 21, 2017 •

edited

Loading

jtimon commented Apr 21, 2017 •

edited

Loading

jtimon commented Apr 27, 2017 •

edited

Loading

jtimon commented Jun 1, 2017 •

edited

Loading