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

jtimon · 2016-04-06T20:29:53Z

Dependencies (and list of things TODO):

~~- [ ] Globals: Explicitly pass const CChainParams& to UpdateTip() #7876~~

~~- [ ] Globals: DisconnectTip() requires the full CChainParams, not just Consensus::Params (depends on #7876)~~

~~- [ ] Make functions in validation.cpp static: #10227~~

"I opened this PR so that newcomers to the Bitcoin Core project could make simple changes to get familiar with contributing to Bitcoin Core. I have marked a number of TODO comments in the source code of this pull request. The changes listed here are not critical to the project but they are good introductory material. Each TODO is relatively simple, and more experienced developers are busy doing other tasks, making this an excellent chance for you to get feedback from me on basic contributions to Bitcoin Core. Hopefully this will help you streamline submissions to Core, please let me know how I can help."

We can slowly turn the global pCurrentParams (hidden behind the function "Params()" for no good reason) into a CChainParams& parameter wherever is needed. This will take very long and I won't be done by one single person. I'm pretty convinced that everybody agrees with this in general, so it should be relatively easy to get your based-on-this-PR merged. But I'm here, just ask.

You may be interested in this PR because:

You are a developer, but you're not familiar with git (or mercurial or similar), or you are intimidated about contributing to Bitcoin Core for whatever reason. I've done it. I'm trying to be here to show you you can too (without coma between 2 "yous", if that's allowed in enligsh).
You are interested in my PROMISES*
You just want to get one commit merged in github/bitcoin/bitcoin because you have an interview in the evening, I don't care if you help me.
rustyrussell told me nobody would ever read the forth point in a numbered list.

What would you have to do:

Simple: force me to remove TODO lines from this PR, one function at a time if possible.

PROMISES:

If you force me to replace any TODO line in this PR, and you're open for nits in your own PR, I promise:

I will review your PR and nack soon or nit until I can give a no-test-needed ACK (or an utACK [the former is better]).
Please ping me on this PR, or on your own for anything related to this.

Once a month (at least):

collect nits and solve them
I will incorporate open PRs that remove TODOs from this mission.
I will rebase anything that's needed if the maintainer can't do it from my base to my-next-month-base, every month
I will try to attract more review to your PR.
You will just need patience some times.

@laanwj please, take this as an open issue that is going to last for long. Just with code [but, still, never to merge no matter how much I change it].

Continues #6173 #6163 #6024 (closed) #5054 and friends.

achow101 · 2016-04-06T21:12:46Z

@rustyrussell told me nobody would ever read the forth point in a numbered list.

Well I just did :)

I'll take a crack at these in a few weeks (if no one else beats me to it) once I get more time.

kirkalx · 2016-04-07T00:29:11Z

subscribing, concept ACK

jtimon · 2016-04-14T13:36:31Z

Thank you @achow101 and @kirkalx for showing interest!

As predicted, #7828 has been rapidly merged. Please subscribe to that
PR, somebody may add comments to that PR later. And, of course, if you
have any questions about that particular PR, please try to ask them
there instead of here.

It was also the first dependency in the list, so now I can cross it from the list, great! this PR/issue gets closer to get closed without being merged.
I also created #7876, and updated the list with more things that need to be done.

I expect you pick one of them (I suggest ConnectBlock() as the next easiest one), create your PR for it and say it here so that I link to your PR in the dependency list for that one.
This way we can avoid that two people work on the same function.

If somebody wants to do more than one, that's fine, I'm not worried about running out of ideas to tell other people to do yet.

But I suggest waiting for the first you open to be merged before opening another one. These changes are generally good and likely to be merged, but they're not a priority. So if you try to do too much of this at once you may end up suffering what is known as a "rebase mouse-wheel" (forgive me, I can't remember who I heard this term from). If you don't mind needing to rebase more times, fine.
But my advice is to start with little and progressively increase your load as you see fit.

jtimon · 2016-04-20T13:36:59Z

Updated the dependency list: replaced 2 TODO items with #7916.

dcousens · 2016-04-21T03:48:18Z

concept ACK

jtimon · 2016-05-20T10:45:52Z

Updated dependencies after #7916 has been merged.
#7947 and #7985 are related to this (but I won't put any of them in the dependencies until one of them is merged first).
Rebased.

jtimon · 2016-07-18T16:21:25Z

Rebased and updated to TODO list. Added AcceptToMemoryPoolWorker() and ReceivedBlockTransactions(). Also replaced CheckBlockHeader, CheckBlock and ContextualCheckBlockHeader with a single line for #8077 (merged).

jtimon · 2016-07-27T22:12:55Z

Rebased on top of #8413 which is very easy to review (hint to new contributors: reviewing is important too both for the project and for learning yourself).
Also fixed some compilation errors I had introduced in the last push.

NicolasDorier · 2016-07-28T03:15:00Z

Concept ACK, but I think that for functions which are not part of consensus, there is not a lot to earn by removing Params() call.

jtimon · 2016-07-28T12:44:10Z

This is not to be merged, but for new devs to get ideas from (and I promise review. Outside of consensus...Params () is still equivalent to using the global it's hiding directly. Globals are bad.

jtimon · 2016-08-30T13:32:41Z

Needed rebase. Also added a couple of new commits.

jtimon · 2016-10-19T17:24:30Z

Rebased, added #8975

gtsui · 2016-10-25T14:51:17Z

Hey there! I just made a pull request #9013 for LoadBlockIndexDB() on your TODO list.

I'm new to contributing so please let me know if there's anything I've done incorrectly!

jtimon · 2016-10-25T15:27:54Z

Thank you!
I reviewed your PR and updated the list so that other people don't work on the same one.

jtimon · 2016-11-03T23:03:44Z

Rebased after #8975 and #9013 were merged. The TODOs in the code are still up to date for both main.cpp and net.cpp (but feel free to take one somewhere else, just grep "Params()" to look for more).
Thanks again @gtsui !

jtimon · 2016-11-17T06:48:16Z

Added #9176 to the list and rebased

jtimon · 2016-12-03T08:04:45Z

Needs to rebase on top of #9176 but the TODO comments are still there.

jtimon · 2017-04-05T20:15:08Z

Rebased and added more ideas. Not sure if maybe adding some of them to #9176 which doesn't seem to be interesting enough on its own. As the only reviewer, thoughts @mrbandrews ?
Well, anybody else reading is welcomed to share his thoughts too.

- ReceivedBlockTransactions - AcceptToMemoryPoolWorker - AcceptToMemoryPoolWithTime also pass Consensus::Params to them directly

…uts()

- CWallet::ScanForWalletTransactions - CWallet::CreateWalletFromFile and InitLoadWallet also make CWallet::CreateWalletFromFile private

Both validation::AcceptToMemoryPool() and CMerkleTx::AcceptToMemoryPool

jtimon · 2017-05-30T17:28:17Z

Thanks for all participants in this experiment, I hope you learned from it. Anyone feel free to ping me for review for more things in here or similar simple things for people to start contributing, but the interest has been decaying and there's no point on keep rebasing this or keeping it open. Closing.

NicolasDorier · 2017-06-05T11:50:19Z

I think this is a good way to go, but better done in smaller, fast to review PR.

jtimon changed the title ~~TODO: Kill "Params()"~~ TODO: Experiment: Kill "Params()" Apr 6, 2016

laanwj added the Refactoring label Apr 11, 2016

jtimon force-pushed the 0.12.99-todo-globals-chainparams branch from d486a45 to 3bbfb65 Compare April 14, 2016 13:12

jtimon mentioned this pull request Apr 14, 2016

Globals: Explicitly pass const CChainParams& to UpdateTip() #7876

Closed

hmel mentioned this pull request Apr 20, 2016

Explicitly pass CChainParams& to DisconnectTip() #7916

Merged

hmel mentioned this pull request Apr 26, 2016

Global params cleanup #7947

Closed

jtimon force-pushed the 0.12.99-todo-globals-chainparams branch from 3bbfb65 to d3711c9 Compare May 20, 2016 10:44

jtimon mentioned this pull request May 20, 2016

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

Merged

jtimon force-pushed the 0.12.99-todo-globals-chainparams branch from d3711c9 to e908d34 Compare July 18, 2016 16:17

maflcko added the good first issue label Jul 18, 2016

jtimon force-pushed the 0.12.99-todo-globals-chainparams branch from e908d34 to ee29a60 Compare July 27, 2016 22:10

jtimon force-pushed the 0.12.99-todo-globals-chainparams branch from ee29a60 to eb8b082 Compare August 30, 2016 13:31

jtimon mentioned this pull request Oct 19, 2016

Chainparams: Trivial: In AppInit2(), s/Params()/chainparams/ #8975

Merged

jtimon force-pushed the 0.12.99-todo-globals-chainparams branch from eb8b082 to f03df4a Compare October 19, 2016 17:20

gtsui mentioned this pull request Oct 25, 2016

Trivial: Explicitly pass const CChainParams& to LoadBlockIndexDB() #9013

Merged

jtimon force-pushed the 0.12.99-todo-globals-chainparams branch from f03df4a to b06cc58 Compare November 3, 2016 22:59

jtimon force-pushed the 0.12.99-todo-globals-chainparams branch from b06cc58 to 7cb4880 Compare November 17, 2016 06:44

jtimon mentioned this pull request Nov 17, 2016

Globals: Pass Consensus::Params through CBlockTreeDB::LoadBlockIndexGuts() #9176

Merged

jtimon changed the title ~~TODO: Experiment: Kill "Params()"~~ Globals: TODO: Experiment: Kill "Params()" Nov 17, 2016

jtimon force-pushed the 0.12.99-todo-globals-chainparams branch from 7cb4880 to 7827573 Compare November 17, 2016 07:38

jtimon force-pushed the 0.12.99-todo-globals-chainparams branch from 7827573 to 9776919 Compare January 24, 2017 21:24

jtimon force-pushed the 0.12.99-todo-globals-chainparams branch from 9776919 to 93d038d Compare April 5, 2017 04:22

mariodian mentioned this pull request Apr 13, 2017

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

Merged

ghost mentioned this pull request Apr 13, 2017

Chainparams: In ReceivedBlockTransactions() #10206

Closed

jtimon added 7 commits April 18, 2017 19:07

Make functions in validation.cpp static:

46fa880

- ReceivedBlockTransactions - AcceptToMemoryPoolWorker - AcceptToMemoryPoolWithTime also pass Consensus::Params to them directly

Globals: Pass Consensus::Params through CBlockTreeDB::LoadBlockIndexG…

8a76f57

…uts()

Wallet: Pass Consensus::Params and ChainTxData to:

4fc1544

- CWallet::ScanForWalletTransactions - CWallet::CreateWalletFromFile and InitLoadWallet also make CWallet::CreateWalletFromFile private

Pass CChainParams to AcceptToMemoryPool (both)

557fc7b

Both validation::AcceptToMemoryPool() and CMerkleTx::AcceptToMemoryPool

Pass CChainParams to CWallet::ReacceptWalletTransactions

d859f3f

TODO: grep Params() in main.cpp TODO list

0ec8546

TODO: s/Params()/chainparams/ in net.cpp

9b00b07

jtimon force-pushed the 0.12.99-todo-globals-chainparams branch from 93d038d to 9b00b07 Compare April 18, 2017 17:16

jtimon mentioned this pull request Apr 18, 2017

Make functions in validation.cpp static: #10227

Closed

jtimon closed this May 30, 2017

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

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

achow101 commented Apr 6, 2016

Uh oh!

kirkalx commented Apr 7, 2016

Uh oh!

jtimon commented Apr 14, 2016

Uh oh!

jtimon commented Apr 20, 2016

Uh oh!

dcousens commented Apr 21, 2016

Uh oh!

jtimon commented May 20, 2016

Uh oh!

jtimon commented Jul 18, 2016

Uh oh!

jtimon commented Jul 27, 2016

Uh oh!

NicolasDorier commented Jul 28, 2016

Uh oh!

jtimon commented Jul 28, 2016

Uh oh!

jtimon commented Aug 30, 2016

Uh oh!

jtimon commented Oct 19, 2016

Uh oh!

gtsui commented Oct 25, 2016

Uh oh!

jtimon commented Oct 25, 2016

Uh oh!

jtimon commented Nov 3, 2016

Uh oh!

jtimon commented Nov 17, 2016

Uh oh!

jtimon commented Dec 3, 2016

Uh oh!

jtimon commented Apr 5, 2017

Uh oh!

jtimon commented May 30, 2017

Uh oh!

NicolasDorier commented Jun 5, 2017

Uh oh!

Uh oh!

jtimon commented Apr 6, 2016 •

edited

Loading