Globals: TODO: Experiment: Kill “Params()” #7829

pull jtimon wants to merge 7 commits into bitcoin:master from jtimon:0.12.99-todo-globals-chainparams changing 14 files +92 −77
  1. jtimon commented at 8:29 pm on April 6, 2016: contributor

    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

    • Trivial: Globals: Explicitly pass const CChainParams& to ProcessMessage() #7828

    • Explicitly pass CChainParams& to DisconnectTip() #7916 (does UpdateTip() DisconnectTip() and InvalidateBlock() at once)

    • Consensus: Decouple from chainparams.o and timedata.o #8077 [CheckBlockHeader, CheckBlock, ContextualCheckBlockHeader]

    • Trivial: pass Consensus::Params& instead of CChainParams& in ContextualCheckBlock #8413

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

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

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

    • Globals: Pass Consensus::Params through CBlockTreeDB::LoadBlockIndexGuts() #9176 [0.13-chainparams-loadblockindexguts]

    • ConnectBlock()

    • FlushStateToDisk()

    • LoadMempool()

    • ProcessMessages() [not trivial]

    • SendMessages() [not trivial]

    • IsInitialBlockDownload() (disruptive)

    • Explicitly pass const CChainParams and Consensus::Params() in net.cpp (several functions)

    “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:

    1. 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).
    2. You are interested in my PROMISES*
    3. 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.
    4. 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.

  2. achow101 commented at 9:12 pm on April 6, 2016: member
    1. @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.

  3. jtimon renamed this:
    TODO: Kill "Params()"
    TODO: Experiment: Kill "Params()"
    on Apr 6, 2016
  4. kirkalx commented at 0:29 am on April 7, 2016: contributor
    subscribing, concept ACK
  5. laanwj added the label Refactoring on Apr 11, 2016
  6. jtimon force-pushed on Apr 14, 2016
  7. jtimon commented at 1:36 pm on April 14, 2016: contributor

    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.

  8. jtimon commented at 1:36 pm on April 20, 2016: contributor
    Updated the dependency list: replaced 2 TODO items with #7916.
  9. dcousens commented at 3:48 am on April 21, 2016: contributor
    concept ACK
  10. jtimon force-pushed on May 20, 2016
  11. jtimon commented at 10:45 am on May 20, 2016: contributor
    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.
  12. jtimon force-pushed on Jul 18, 2016
  13. jtimon commented at 4:21 pm on July 18, 2016: contributor
    Rebased and updated to TODO list. Added AcceptToMemoryPoolWorker() and ReceivedBlockTransactions(). Also replaced CheckBlockHeader, CheckBlock and ContextualCheckBlockHeader with a single line for #8077 (merged).
  14. MarcoFalke added the label Easy to implement on Jul 18, 2016
  15. jtimon force-pushed on Jul 27, 2016
  16. jtimon commented at 10:12 pm on July 27, 2016: contributor
    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.
  17. NicolasDorier commented at 3:15 am on July 28, 2016: contributor
    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.
  18. jtimon commented at 12:44 pm on July 28, 2016: contributor
    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.
  19. jtimon force-pushed on Aug 30, 2016
  20. jtimon commented at 1:32 pm on August 30, 2016: contributor
    Needed rebase. Also added a couple of new commits.
  21. jtimon force-pushed on Oct 19, 2016
  22. jtimon commented at 5:24 pm on October 19, 2016: contributor
    Rebased, added #8975
  23. gtsui commented at 2:51 pm on October 25, 2016: contributor

    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!

  24. jtimon commented at 3:27 pm on October 25, 2016: contributor
    Thank you! I reviewed your PR and updated the list so that other people don’t work on the same one.
  25. jtimon force-pushed on Nov 3, 2016
  26. jtimon commented at 11:03 pm on November 3, 2016: contributor
    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 !
  27. jtimon force-pushed on Nov 17, 2016
  28. jtimon commented at 6:48 am on November 17, 2016: contributor
    Added #9176 to the list and rebased
  29. jtimon renamed this:
    TODO: Experiment: Kill "Params()"
    Globals: TODO: Experiment: Kill "Params()"
    on Nov 17, 2016
  30. jtimon force-pushed on Nov 17, 2016
  31. jtimon commented at 8:04 am on December 3, 2016: contributor
    Needs to rebase on top of #9176 but the TODO comments are still there.
  32. jtimon force-pushed on Jan 24, 2017
  33. jtimon force-pushed on Apr 5, 2017
  34. jtimon commented at 8:15 pm on April 5, 2017: contributor
    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.
  35. Make functions in validation.cpp static: 
    - ReceivedBlockTransactions
- AcceptToMemoryPoolWorker
- AcceptToMemoryPoolWithTime

also pass Consensus::Params to them directly
    46fa880f18
  36. Globals: Pass Consensus::Params through CBlockTreeDB::LoadBlockIndexGuts() 8a76f57261
  37. Wallet: Pass Consensus::Params and ChainTxData to: 
    - CWallet::ScanForWalletTransactions
- CWallet::CreateWalletFromFile and InitLoadWallet

also make CWallet::CreateWalletFromFile private
    4fc1544309
  38. Pass CChainParams to AcceptToMemoryPool (both) 
    Both validation::AcceptToMemoryPool() and CMerkleTx::AcceptToMemoryPool
    557fc7bb82
  39. Pass CChainParams to CWallet::ReacceptWalletTransactions d859f3f62a
  40. TODO: grep Params() in main.cpp TODO list 0ec8546c12
  41. TODO: s/Params()/chainparams/ in net.cpp 9b00b070de
  42. jtimon force-pushed on Apr 18, 2017
  43. jtimon commented at 5:28 pm on May 30, 2017: contributor
    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.
  44. jtimon closed this on May 30, 2017
  45. NicolasDorier commented at 11:50 am on June 5, 2017: contributor
    I think this is a good way to go, but better done in smaller, fast to review PR.
  46. MarcoFalke locked this on Sep 8, 2021


jtimon achow101 kirkalx dcousens NicolasDorier gtsui

Labels
Refactoring good first issue

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-05 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me