Following the main.cpp split, there are still some constants in the wrong places, eg net_processing constants in validation.h. Move them all to their rightful homes. At the same time, make them constexpr.
Also move all const declarations to the top of their files, and ensure that they all have doxygen comments.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
init. Just because the parsing code for most modules is thrown into init, I don’t think this is the right place long term. We might want to have modules weakly integrated into Bitcoin Core (like the wallet or the gui), in which case the parsing code should stay in those modules (along with the defaults)
We might want to have modules weakly integrated into Bitcoin Core (like the wallet or the gui), in which case the parsing code should stay in those modules (along with the defaults)
Concept ACK. I think it would make sense longer-term if modules managed their own arguments, but for now, that’s not the case so init.cpp is the right place for them. If you take a look at the constants I’ve moved init init.cpp, you’ll see:
DEFAULT_TXINDEX (was in validation but is part of the index module)DEFAULT_BLOCKFILTERINDEX (was in validation but is part of the index module)DEFAULT_PERSIST_MEMPOOL (was in validation. Is perhaps part of validation, but all the mempool loading/dumping code is in init.cpp)DEFAULT_PEERBLOOMFILTERS (was in net_processing, but is part of the net module)
Concept ACK
Could do s/const /constexpr /g on the touched lines where it makes sense? :)
Concept ACK
I don’t know if it makes sense to move the consts to init
Yes, I think there’s two ideas in conflict here. One is to move the defaults to implementation files (instead of having them clutter headers), which is a good thing in itself, and the other is to do parsing where the values are needed instead of having init.cpp be a busy hive of unrelated parsing (and merge conflicts).
I think it’s ok to do this first.
do parsing where the values are needed instead of having init.cpp be a busy hive of unrelated parsing (and merge conflicts).
Nicely put. Definite concept ACK on doing this (in future PRs)
do parsing where the values are needed instead of having init.cpp be a busy hive of unrelated parsing (and merge conflicts).
Nicely put. Definite concept ACK on doing this (in future PRs)
I thought the benefit of the current approach is to fail as soon as possible?
I thought the benefit of the current approach is to fail as soon as possible?
Moving the parsing code to another module doesn’t mean it can’t fail on init (see e.g. the wallet init parsing)
where the values are needed
Ah ok, my comment was because of this bit. Breaking the initialization sounds good to me.
Could do s/const /constexpr /g on the touched lines where it makes sense? :)
I don’t want scope to creep too much. It made sense to me to change const to constexpr when moving a constant from the header file to the implementation file, but for the simple moves within files I kept the definitions the same. (slightly arbitrary, but I had to draw the line somewhere)
Are we supposed to care about AppVeyor failures? Is there a way to rerun them when they fail?
I’ve restarted. There’s a Rebuild PR button there (maybe if you have permissions?).
Ah ok, my comment was because of this bit. Breaking the initialization sounds good to me.
Yesss I agree I didn’t literally mean where the values are needed, but the module where the values are needed, Of course it’s better to do as much of parsing upfront as possible. Failing on argument parsing when threads have already been spun up is inconvenient. It’s exactly what is wrong at the moment, why parsing can’t fail in ParseBool and ParseInt but have to return something no matter how little sense their input makes.
const is great and patch looks good :)
I’ve simplified this PR to remove any moves to init.cpp. I think the discussion above about start-up and separating components is interesting, but wasn’t really the point of this PR. This PR now only does the following:
I’ve also removed changing const to constexpr and adding doxygen comments.
Reviewing this PR should be trivial:
git diff --color-moved=zebra and observe that all moves are from header files to implementation files, or within implementation files.
Reviewing this PR should be trivial
I disagree that review of this is trivial. Let’s take a look at the first move (PING_TIMEOUT): ping and timeout are tightly coupled. (The timeout will start to count after the ping was sent). Ripping the constants away from each other is making the code harder to read.
Also, I don’t really understand the motivation to move compile time constants of trivial types from the header to the cpp file. This has no effect on compile time and it shouldn’t matter where they live.
Not to mention that this (potentially silently) conflicts with open pull requests.
Let’s take a look at the first move (PING_TIMEOUT): ping and timeout are tightly coupled.
No, the second value is a timeout on any socket activity, not just pings. It should live in net. PING is an application-level message and exists in net_processing.
Also, I don’t really understand the motivation to move compile time constants of trivial types from the header to the cpp file.
The header file is the interface. The .cpp file is the implementation. Constants that are only needed in the implementation should live in the .cpp file, and the interface should be kept as minimal as possible. I don’t see why this is controversial in any way.
Not to mention that this (potentially silently) conflicts with open pull requests.
One reason that we have merge conflicts is that there’s very tight coupling between files in this repo. If symbols were declared in the tightest possible scope there would be far fewer conflicts.
I picked this PR up again because I’m making a branch in net_processing and found myself touching files in validation and net. That shouldn’t happen!
Please read the other comments on this PR. You’re the only person who’s objected to this. I’ve pared this down and down to satisfy you by removing the changes in init.cpp, removing doxygen comments, and changing constexprs back to consts so this is now just move only.
ACK e9ea95a30d3c0f62b0df0b29744fb5d68687f97f 🚉
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK e9ea95a30d3c0f62b0df0b29744fb5d68687f97f 🚉
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUigiwv+NZXoqdH0Elf0l5k1w3q8pkNuTK5sx34HgU9JbfVzKrGUHh1Y7Qa3y5/F
81fxN4j/2C8TVXD1vy76FL0B6ABDxzxfX9OU+24h4md2Ih4KVWUj0voBgaPlQCIE0
9sb32Kzr1/BdNBaeeMerzh7YsgGv8ZPJSMGuuYFZwsdzKkDuJeSTSr/HMkeKhj29i
10DPQWxTdoqwtxB3VP0GuxtfuFERSOoDnc8yN2RXUBlZF9GB5Sbht+dJ5hfVjUsTyg
11QI7akwFnbYYORHVoAD4jp4sMDOhcjI28H+v/LhOM2vJCf8OhNK+ZgoylijYLSjn0
12sHSTJKQw8NtXJ4vRXHJ6iIRriwySh9TcrUJDZWGWHfc3e1ghdiynUfj74lnQ/2nN
13Ej81uTOmW2QbshOd0VAYGEz4ihz0aXUS5A2MZLffH6pPbL7QOB9/NkHhK1SJc27U
14L9ahCd6tJK3tho8UX+gza+K+6lDRzBKiz2hjAiPPdTrZoiH/3KU0Wso0NMWu/odt
153CQC3Zyb
16=aYki
17-----END PGP SIGNATURE-----
Timestamp of file with hash 374504941291a821e498997fb9108c5ce706230380ce58b924f1c892d5b70134 -