Deprecate main and test -chain params #23090

1. 
   katesalazar commented at 9:12 pm on September 24, 2021: contributor

   ‘Main’ and ’test’ is old terminology, with which it seems ‘signet’ as a name has broken with.

   Deprecate main and test -chain params in favor of mainnet and testnet, in line with signet.

   main and test shouldn’t be removed from the source code until most users are unaffected by the removal.

   This supersedes #23068.

2. 
   michaelfolkson commented at 9:25 pm on September 24, 2021: contributor

   Leaning towards Concept NACK as the only upside appears to be making it prettier (I don’t think this addresses any particular confusion) and don’t think that upside is worth the short term disruption and invasive change.
3. 
   katesalazar commented at 9:28 pm on September 24, 2021: contributor

   Leaning towards Concept NACK as the only upside appears to be making it prettier (I don’t think this addresses any particular confusion) and don’t think that upside is worth the short term disruption and invasive change.

   Mhnmyea, I wouldna tried this if I had known it would expand this big.

4. 
   michaelfolkson commented at 9:35 pm on September 24, 2021: contributor

   :) I think good first issues would be a better starting point until you understand the codebase/project better.
5. DrahtBot added the label GUI on Sep 24, 2021
6. DrahtBot added the label Utils/log/libs on Sep 24, 2021
7. DrahtBot added the label Validation on Sep 24, 2021
8. DrahtBot added the label Wallet on Sep 24, 2021
9. 
   katesalazar commented at 10:55 am on September 25, 2021: contributor

   Reading CI logs, I think at this point these changes spill out to bitcoin configuration files (in https://github.com/bitcoin/bitcoin/blame/16ccb3a1/doc/bitcoin-conf.md#L32 it’s rejecting [main] and [test]).

   I hadn’t intended that.

   I’m not sure about the choice between shrinking scope or keep pushing forward. I think it’s best to keep pushing forward and eventually break the PR into smaller pieces if it becomes too big to be handled comfortably, but some maintainer wisdom is naturally very welcome.

10. 
    DrahtBot commented at 3:01 pm on September 25, 2021: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #23157 (txmempool -/-> validation 1/2: improve performance of check() and remove dependency on validation by glozow)
    • #22677 (cut the validation <-> txmempool circular dependency 2/2 by glozow)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

11. 
    katesalazar commented at 4:29 pm on September 25, 2021: contributor

    test/functional/p2p_dos_header_tree.py failures are not very informative and test/functional/feature_backwards_compatibility.py seems to require some configuration efforts, so I expect no more progress here.
12. 
    katesalazar commented at 7:20 pm on September 26, 2021: contributor

    Currently pursuing locally a feeling that 22817 fixes test/functional/feature_backwards_compatibility.py in both my host and CI. 🤔
13. 
    Sjors commented at 8:58 am on September 29, 2021: member

    Concept NACK. This will create headaches for years for only cosmetic benefit. The code that processes config files is already rather complicated and full of unpleasant backward compatibility quirks. See e.g. #15934

    See also https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring

    Pull requests that refactor the code should not be made by new contributors. It requires a certain level of experience to know where the code belongs to and to understand the full ramification (including rebase effort of open pull requests).

14. Deprecate `main` and `test` `-chain` params

    Deprecate them in favor of `mainnet` and `testnet`, in line with
    `signet`.
    
    Expect plenty of CI protests on this change while I'm figuring it out.
    
    ---
    
    Rebase helpers (remove before merging)
    
        grep -r CBaseChainParams::MAIN src/ | sed s/MAINNET/XXX/ | \
        grep CBaseChainParams::MAIN | sed s/:.*//
    
        grep -r '"main"' src/ | grep -v Binary | sed s/:.*// | uniq

    38e9e9b015
15. katesalazar force-pushed on Oct 1, 2021
16. Break compile compatibility. 1d2baa4a64
17. 
    katesalazar commented at 8:30 pm on October 1, 2021: contributor

    Sjors: you are so right you almost convinced me. 😖
18. 
    meshcollider commented at 2:04 am on October 4, 2021: contributor

    Hi @katesalazar, thank you very much for contributing! As others have said, unfortunately this change is a bit too nuanced to be worth the cosmetic change and has already received NACKs, so I’m going to have to close it sorry.

    As michaelfolkson said, please do check out the good-first-issue tag for some good starting-point issues that you might like to help with 🙂 Look forward to further contributions in future!

19. meshcollider closed this on Oct 4, 2021

    ---

20. DrahtBot locked this on Oct 30, 2022

Contributors
katesalazar michaelfolkson DrahtBot Sjors meshcollider

Labels
GUI Wallet Validation Utils/log/libs