refactor / kernel: Move non-gArgs chainparams functionality to kernel#26177
pull
TheCharlatan
wants to merge
6
commits into
bitcoin:master
from
TheCharlatan:tc/2022-09-libbitcoinkernel-chainparams-args
changing
12
files
+792−636
The bitcoin kernel library currently relies on code containing user configurations through the ArgsManager. This is not optimal, since as a stand-alone library it should not rely on bitcoind’s argument parsing logic. Instead, its interfaces should accept control and options structs that control the kernel library’s desired configuration.
Similar work towards decoupling the ArgsManager from the kernel has been done in
#25290, #25487, #25527 and #25862.
Changes
By moving the CChainParams class definition into the kernel and giving it new factory functions CChainParams::{RegTest,SigNet,Main,TestNet}it can be constructed without an ArgsManager reference, unlike the current factory function CreateChainParams.
The first few commits remove uses of ArgsManager within CChainParams. Then the CChainParams definition is moved to a new file in the kernel/ subdirectory.
TheCharlatan force-pushed
on Sep 24, 2022
DrahtBot
commented at 4:21 pm on September 24, 2022:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviews
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
Reviewers, this pull request conflicts with the following ones:
#26201 (Remove Taproot activation height by Sjors)
#25725 (consensus: Remove mainnet checkpoints by sdaftuar)
#17783 (util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)
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.
TheCharlatan force-pushed
on Sep 24, 2022
TheCharlatan force-pushed
on Sep 24, 2022
TheCharlatan force-pushed
on Sep 24, 2022
TheCharlatan force-pushed
on Sep 24, 2022
TheCharlatan force-pushed
on Sep 24, 2022
TheCharlatan marked this as ready for review
on Sep 25, 2022
glozow added the label
Refactoring
on Sep 26, 2022
dongcarl
commented at 7:01 pm on September 26, 2022:
I think it’s not really “SigNetOptions filled with default values” but rather “Options for the default signet”. I remember @theuni made a good point offline that it almost never makes sense to override some of the options for the default signet.
ryanofsky
commented at 3:40 pm on September 27, 2022:
In commit “Decouple SigNetChainParams from ArgsManager” (8c0a94f1d56345c45304319e72b7f78626e47061)
Agree but I think having this function at all is a footgun and source of unnecessary complexity and it would be better to drop. Just make a default-constructed SigNetOptions struct give default behavior as suggested above so this function doesn’t need to exist.
in
src/chainparams.cpp:415
in
9929969d3eoutdated
414@@ -415,7 +415,8 @@ std::unique_ptr<const CChainParams> CreateSignetChainParams(const ArgsManager& a
415 class CRegTestParams : public CChainParams
dongcarl
commented at 7:04 pm on September 26, 2022:
Ping @ajtowns, could you take a peek? 9929969d3e95197499f2c81c01702f11aa52ff0d
in
src/kernel/chainparamsbase.cpp:34
in
c00f4f7599outdated
dongcarl
commented at 7:10 pm on September 26, 2022:
Hmmm… Been a while since I last took a look but how hard would it be to have this as a member of kernel::Context? What does the caller graph look like?
It might be the case that this is a situation where it’d be too intrusive to have the N I C E C O D E as discussed here: #25065#pullrequestreview-985450940
TheCharlatan
commented at 9:30 am on September 28, 2022:
The caller graph is kind of annoying, it boils down to users of SelectBaseParams (like bitcoin-cli), and more importantly SelectParams, which is used all over the place (I count 23 separate files as call site). If I understand the current usage of kernel::Context correctly, this would mean that we’d have to construct the context for nearly each call site separately, and many in places where it was previously not required. I currently tend towards handling this in a separate refactor attempt to get rid of these param globals in general.
ajtowns
commented at 4:58 pm on September 28, 2022:
You could maybe split it up? Have chainparams provide SelectParams(ArgsManager&) and use that instead of SelectParams(args.GetChainName()) which covers all the cases outside of test/ and test/fuzz/. Then most of the test or test/fuzz cases already hardcode the network, and could be replaced by kernel::SelectParams(RegTestBase(), RegTest(regtestopts)) that just initializes the globals?
TheCharlatan
commented at 2:24 pm on October 15, 2022:
I’ve had no luck coming up with a nice pattern to get this split done without blowing everything up :/
I might try it again at a later point in time by first create an ugly solution and then see if I can mold it into something nice.
TheCharlatan
commented at 1:17 pm on February 16, 2023:
I looked into this again. The underlying problem seems to be that the kernel library still depends on the ArgsManager, which in turn depends on the global. I’d rather not make this change devolve into changing ArgsManager internals and behaviour, so I’ll defer refactoring this global to once the ArgsManager is removed from the kernel library.
In commit “split non/kernel chainparamsbase” (2f2216dee7a683638e6a3e966142f50f5de7df78)
I don’t think it makes sense to have BaseParams in the kernel at all. BaseParams contains things like RPC port which are at a higher level than the kernel, and are used by things like bitcoin-cli code and ArgsManager code which shouldn’t depend on the kernel (see https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md#dependencies). Probably BaseParams code should stay where it is, or move to libbitcoin_common if it does move.
It does make sense to move CChainParams to the kernel, though.
Also, just in general reference to the discussion above it should be trivial to move any global like globalChainParams or globalChainBaseParams to the kernel::Context struct. Just add a global kernel::Context* g_kernel_dontusethis variable, change old code to use it, and avoid using it in new code. When the last usage of g_kernel_dontusethis is deleted it can go away. Existing uses of globalChainParams would mechanically change to g_kernel_dontusethis->params or whatever the new member is called.
in
src/bitcoin-cli.cpp:73
in
9714fe9160outdated
66@@ -67,10 +67,10 @@ static void SetupCliArgs(ArgsManager& argsman)
67 {
68 SetupHelpOptions(argsman);
6970- const auto defaultBaseParams = CreateBaseChainParams(CBaseChainParams::MAIN);
71- const auto testnetBaseParams = CreateBaseChainParams(CBaseChainParams::TESTNET);
72- const auto signetBaseParams = CreateBaseChainParams(CBaseChainParams::SIGNET);
73- const auto regtestBaseParams = CreateBaseChainParams(CBaseChainParams::REGTEST);
74+ const auto defaultBaseParams = kernel::CreateBaseChainParams(CBaseChainParams::MAIN);
dongcarl
commented at 7:11 pm on September 26, 2022:
nit: Could use using declarations in .cpp files (if you want)
TheCharlatan
commented at 2:55 pm on October 2, 2022:
Seems to me like using declarations are not used for internal namespaces in the code so far, so I chose not to.
dongcarl
commented at 7:18 pm on September 26, 2022:
contributor
Concept ACK
Did a first-pass review. Perhaps @ajtowns and @kallewoof can help w/re the activation/chainparams/signet bits!
in
src/chainparams.h:29
in
9714fe9160outdated
66-struct ChainTxData {
67- int64_t nTime; //!< UNIX timestamp of last known number of transactions
68- int64_t nTxCount; //!< total number of transactions between genesis and that timestamp
69- double dTxRate; //!< estimated number of transactions per second after that timestamp
70-};
71+std::unique_ptr<const CChainParams> CreateSignetChainParams(const ArgsManager& args);
ajtowns
commented at 0:06 am on September 27, 2022:
I would have expected a SignetOptions GetSignetOptions(const ArgsManager& args) function, and then just calling CreateSignetChainParams(GetSignetOptions(args));. Makes it a bit clearer that all that’s going on is that the args are being translated to a SignetOptions structure, and doesn’t tempt anyone to put any clever logic outside of the kernel.
You could even call it operator SigNetOptions(const ArgsManager& args) to have it be implicit, I guess?
(Be nice to be consistent calling it “SigNet” rather than “Signet” in some places and “SigNet” in others. Apologies if I introduced any of the “Signet” instances :)
in
src/kernel/chainparams.h:176
in
9714fe9160outdated
ajtowns
commented at 2:45 am on September 27, 2022:
I think if you wanted exactly the same behaviour as current, it would be better to write:
0if (options.challenge.empty()) { /* use defaults */ } else { /* use values from options */ }
ryanofsky
commented at 3:35 pm on September 27, 2022:
In commit “Decouple SigNetChainParams from ArgsManager” (8c0a94f1d56345c45304319e72b7f78626e47061)
I think if you wanted exactly the same behaviour as current, it would be better to write:
0if (options.challenge.empty()) { /* use defaults */ } else { /* use values from options */ }
I agree with this suggestion. I think a good design goal for most options structs is to make them default constructible, and make default behavior when you pass an empty options struct the same as what would happen if you launched bitcoind without the corresponding arguments. Don’t introduce a new set of default behaviors, don’t require callers to initialize options structs if they are not overriding something, and avoid complications like having some struct members be const and other be non-const. If code is implemented like the suggestion above and accepts empty options, this all happens easily and naturally.
TheCharlatan
commented at 2:14 pm on October 15, 2022:
Done in 7d7900edeff6af18c3bb2c11475ac26414ed78d2
ajtowns
commented at 2:50 am on September 27, 2022:
contributor
Concept ACK. Having simple options structs as a buffer between code and gArgs is great IMO.
398+ throw std::runtime_error(strprintf("%s: -signetchallenge cannot be multiple values.", __func__));
399+ }
400+ return CreateSignetChainParams(SigNetOptions{ParseHex(signet_challenge[0]), seeds});
401+ } else {
402+ auto opts = GetDefaultSignetOptions();
403+ if (!seeds.empty()) {
ryanofsky
commented at 3:45 pm on September 27, 2022:
In commit “Decouple SigNetChainParams from ArgsManager” (8c0a94f1d56345c45304319e72b7f78626e47061)
This looks like a change in behavior if -nosignetseednode is specified. Previously that would clear the seeds. Now it will use default seeds. I don’t know if this is good or bad but I think it would be better not change here.
I would suggest following aj’s suggestion above #26177 (review) and just have a literal 1:1 correspondence between arguments and options. Make unset arguments map to unset options. Fill options only when arguments are present. Don’t try to be clever and fill options when there are no arguments.
The definition of the SigNetOptions struct then would be:
The default constructed struct would result in default behavior. -signetchallenge and -signetseednode would override these defaults. -nosignetchallenge would be an error like it is currently with no change in behavior. -nosignetseednode would set an empty vector and add no seed nodes, which seems to be current behavior.
ajtowns
commented at 0:00 am on September 28, 2022:
… just have a literal 1:1 correspondence between arguments and options.
I think there’s two conflicting goals here? It seems like one idea is that the kernel should be just given the actual data (eg a map from buried deployment ids via enum to integer heights) that’s needed – the kernel’s meant to be used by software, so why translate into text in the meantime?
But if that were really the case, why not just pass a fully fleshed out const CChainParams& to the kernel in the first place?
In that case, kernel/chainparams.h could be a headers only module, with the mainnet/signet/etc setup code staying in chainparams.cpp. I’m not sure if I’m missing a design goal here though.
TheCharlatan
commented at 1:52 pm on September 28, 2022:
ryanofsky
commented at 3:04 pm on September 28, 2022:
I think that is taking it a bit far.
Right I’m just talking about a minor change to the SigNetOptions struct that would simplify the changes here.
The only thing this PR really needs to do is get rid of ArgsManager usage in this part of the code, replacing ArgsManager lookups with SigNetOptions member accesses. I suggested a slightly different version of the SigNetOptions struct above that would let it provide the same information ArgsManager provides, in a 1:1 way so less code has to change, and so the struct is easier for callers to fill and customize.
But if that were really the case, why not just pass a fully fleshed out const CChainParams& to the kernel in the first place?
This is fine, and I think it basically already happens later after CChainParams are initialized. I’m just looking at the earlier code used to initialize CChainParams. I do think alternate approaches are possible. Like instead of SigNetOptions being used to initialize CChainParams and have CChainParams contain dynamic settings determined at runtime, CChainParams could just contain static hardcoded settings, and code that uses CChainParams could use combine it with runtime settings. This already happens with chain parameters like nMinimumChainWork that can be overridden on the command line.
In that case, kernel/chainparams.h could be a headers only module, with the mainnet/signet/etc setup code staying in chainparams.cpp. I’m not sure if I’m missing a design goal here though.
I’m not sure exactly what this would look like but it does sound fine. Maybe similar to what I was describing above making CChainParams not vary depending on runtime options.
These alternate approaches are just bigger than changes I was suggesting above. All I was suggesting above was replacing ArgsManager uses with SigNetOptions in a 1:1 way instead of doing more complicated mappings and creating another set of defaults and making it harder to initialize.
ajtowns
commented at 5:00 pm on September 28, 2022:
I think that is taking it a bit far.
Yeah – I think I agree, I just don’t know why I agree? I was more hoping to understand why that would be taking it a bit far?
ryanofsky
commented at 6:48 pm on September 28, 2022:
Rereading I think we might be talking past each other. You mentioned things like “translating to text” which I’m not suggesting. I’m literally just agreeing with your initial suggestion if (options.challenge.empty()) { /* use defaults */ } else { /* use values from options */ } and saying how you can tweak the options struct definition so the same approach works for seeds as well, and the options struct does not need to contain any new or different information than what ArgsManager provides.
Later you mentioned passing “a fully fleshed out const CChainParams&” and having a “headers only module” and I think these changes sound ok, but maybe involve getting rid of the options struct or not using it here, and I’m just proposing tweaking the struct a little.
ajtowns
commented at 7:51 pm on September 28, 2022:
@ryanofsky Yeah, I understand what you were saying, and tweaking the approach in this PR feels sensible to me too; I just don’t feel like I intellectually understand the tradeoffs here, so was hoping thinking about more extreme alternatives might be revealing.
Three approaches:
kernel::RegTestOptions just contains the exact results from gArgs (so a vector of strings, not a map of enums etc)
what’s implemented in this PR, with some tweaks
keep all the mainnet/testnet/etc defaults out of kernel, and just give it a prefilled CChainParams
I think this PR is better than the first option, because text is annoying, and the tweaks make it even better. I don’t have a handle on the tradeoffs for the third approach, though.
ryanofsky
commented at 3:31 pm on September 29, 2022:
I think there is still a misunderstanding. I’m not suggesting using text instead of enums or using hex strings instead of binary strings in the Options struct.
So I don’t think I’m suggesting any of the approaches you listed above. I’m suggesting a fourth approach: kernel::RegTestOptions contains a 1:1 representation of information in gArgs. If a gArgs option is unset, corresponding Options parameter is unset or empty. If gArgs option is valid, corresponding options member contains the parsed option value. No special calls are needed to fill the options struct with defaults, because it just does the default thing by default and can be easily customized if desired. bitcoind behavior is unchanged and less code has to change because the options struct can be used as a straightforward replacement for ArgsManager calls.
I think if you want to go further and remove more code from the kernel, it could be good or bad, and the tradeoffs you are thinking about come into play. But I think the only thing we actually want to remove right now is ArgsManager. I don’t think we need to something like removing preset chain parameters.
ajtowns
commented at 5:23 pm on September 29, 2022:
Right – I’m describing your (and my) suggestions as “some tweaks” to this PR. (They seem like obviously good ideas to me, so I was glossing over them in favour of the things I’m confused about)
ryanofsky
commented at 7:06 pm on September 29, 2022:
Makes sense!
ryanofsky
commented at 4:04 pm on September 27, 2022:
contributor
Just reviewed the first commit so far, but already I think some things are a little overcomplicated and potentially buggy. I would suggest having a 1:1 correspondence between arguments and options, treating empty arguments as unset options, and keeping PR a pure refactoring which does not change behavior.
Started review (will update list below with progress).
8c0a94f1d56345c45304319e72b7f78626e47061 Decouple SigNetChainParams from ArgsManager (1/7)
9929969d3e95197499f2c81c01702f11aa52ff0d Decouple RegTestChainParams from ArgsManager (2/7)
9714fe9160282dc36269b1d16d57dfdd48d0a1f1 Wrap kernel chainparamsbase functions in kernel namespace (7/7)
TheCharlatan force-pushed
on Oct 15, 2022
TheCharlatan
commented at 2:20 pm on October 15, 2022:
contributor
Thank you all for the reviews, I reworked my commits to include your suggestions.
@ajtowns I molded most of your suggested code into the existing commits. I did not include the complete refactor of the Consensus::BuriedDeployment though.
@ryanofsky the options structs are now instantiated with default values and read from the ArgsManager in dedicated functions. Since I decided to move the options structs into CChainParams, I did not move these dedicated functions into a separate file.
in
src/kernel/chainparams.cpp:296
in
03468cbc37outdated
along with defaulting options.seeds to empty. Otherwise just overriding options.challenge will have you connecting to the default seeds which would be annoying.
TheCharlatan
commented at 7:22 pm on October 17, 2022:
Thanks for review, I pushed another potential solution using constants. Let me know if you like something more akin to your solution, or fully using defaults better.
Currently, if you runbitcoind -signet -signetchallenge=51 then you start up with no seed nodes; with your current patch, you’ll instead connect to the default signet seednodes, and then reject them for having the wrong network magic. That just seems buggy to me? Putting these defaults into the code rather than the structure definition seems like the easiest way of making that work sensibly to me.
TheCharlatan
commented at 7:27 am on October 18, 2022:
I thought I changed the behaviour now: It does not populate the seeds if the passed in option seeds are default and the signet challenge is non-default. This is still kind of confusing in the code though, so please let me know if you find that inferior.
It’s not the way I’d do it, but as long as it works, I don’t really mind.
Another approach would be to have SigNetOptions default to an empty challenge and empty seed vector, but define a const SigNetOptions GlobalDefaultSigNet { DEFAULT_CHALLENGE, DEFAULT_SEEDS }; with the global defaults, then make ReadSigNetArgs do:
then you don’t have to special case challenge/seeds in SigNetParams(options), though you’d still be special casing min chain work, assume valid, etc.
ryanofsky
commented at 12:13 pm on October 18, 2022:
In commit “Decouple SigNetChainParams from ArgsManager” 9d3500c2d35f7375aadba4a10d50dbc9a6b82251:
This is better than previous attempt, but new code is still not equivalent to existing code. Previous code was checking !args.IsArgSet("-signetchallenge") new code is checking options.challenge == DEFAULT_SIGNET_CHALLENGE and these cases are not equivalent if default signetchallenge value is hardcoded in the config file. Same comment applies to signetseednode.
I think we should keep this a pure refactoring and have a smaller diff and leave the seeds where they and make the options struct a 1:1 substitute for ArgsManager. If we want to make changes to behavior later, or make code cleanups later, that’s great, but I don’t think those changes belong here.
There is a simple way to do what I’m requesting: change the type of challenge from std::vector<uint8_t> to std::optional<std::vector<uint8_t>> and change the type of seeds from std::vector<std::string> to std::optional<std::vector<std::string>>. If the arguments are set, populate the fields. If the arguments are unset, leave the fields set to nullopt. Leave the logic in chainparams.cpp unchanged and just swap out ArgsManager::IsArgSet calls for std::optional::has_value calls.
TheCharlatan
commented at 11:14 am on December 22, 2022:
Commit “Decouple SigNetChainParams from ArgsManager” e893056ed68d02ef94f7e94fc6287217610723a4 now uses a struct with optionals as proposed here.
I tried implementing similar optionals for the RegTestOptions in commit “Douple RegTestChainParams from ArgsManager” 6d57a29df85277964edb21ee251b395e4eae1ef6, but it felt un-ergonomic.
ajtowns
commented at 1:55 pm on February 16, 2023:
Using optionals for signet but not regtest makes sense to me.
TheCharlatan force-pushed
on Oct 17, 2022
DrahtBot added the label
Needs rebase
on Dec 7, 2022
jamesob
commented at 1:56 pm on December 16, 2022:
member
0 test 2023-01-17T22:29:12.633000Z TestFramework (ERROR): JSONRPC error
1 Traceback (most recent call last):
2 File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_framework.py", line 134, in main
3 self.run_test() 4 File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\feature_index_prune.py", line 73, in run_test
5pruneheight_new= node.pruneblockchain(400) 6 File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\coverage.py", line 49, in __call__
7return_val= self.auth_service_proxy_instance.__call__(*args, **kwargs) 8 File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\authproxy.py", line 146, in __call__
9 raise JSONRPCException(response['error'], status)10 test_framework.authproxy.JSONRPCException: Blockchain is too short for pruning. (-1)
0 test 2023-01-17T22:14:21.962000Z TestFramework (ERROR): JSONRPC error
1 Traceback (most recent call last):
2 File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 134, in main
3 self.run_test() 4 File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/interface_usdt_utxocache.py", line 150, in run_test
5 self.test_flush() 6 File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/interface_usdt_utxocache.py", line 397, in test_flush
7 self.nodes[0].pruneblockchain(315) 8 File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/coverage.py", line 49, in __call__
9return_val= self.auth_service_proxy_instance.__call__(*args, **kwargs)10 File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 146, in __call__
11 raise JSONRPCException(response['error'], status)12 test_framework.authproxy.JSONRPCException: Blockchain is too short for pruning. (-1)
TheCharlatan force-pushed
on Jan 18, 2023
TheCharlatan
commented at 11:35 pm on January 18, 2023:
contributor
Thanks for the heads up fanquake, there was a bug that is now fixed in 1ad1c68. I’m not sure about the one remaining test failure.
In commit “Decouple RegTestChainParams from ArgsManager” (1cfccc38dd5d39a9e23a19b94d6ba1a7bc4c48dc)
I think this could say:
0options.activation_heights[*maybe_dep] = height;
Would also suggest renaming maybe_dep to buried_deployment.
These are just style comments, but in general this the code in the first few commits of this PR seems a little bogged down in mechanics of optional and map types instead of fluently expressing application logic. This is c++ in 2023, not java in 1996, so we can use the [] operator to access map elements and we can write *value and value-> and if (value) to show that a value can be null without having to call it maybe_vlu and obscure the meaning of the variable. Verbosity is good for calling attention to special cases, but basic code accessing standard data structures should be concise and follow the natural patterns of the language.
TheCharlatan
commented at 8:30 pm on March 7, 2023:
Thank you for taking the time to get into this. I hope the update I pushed is more in style. diff
In commit “Decouple RegTestChainParams from ArgsManager” (1cfccc38dd5d39a9e23a19b94d6ba1a7bc4c48dc)
I think it would be better to change existing UpdateActivationParametersFromArgs(args) line to UpdateActivationParametersFromOptions(options) and have that function do this stuff, instead of moving more code into this giant constructor, especially when none of the other code in the constructor touches the options struct.
TheCharlatan
commented at 7:58 pm on March 7, 2023:
I’ll keep this as is, to me this reads easier. We also do use the options struct before to read the fastprune flag.
In commit “Add factory functions for Main/Test/Sig/Reg chainparams” (2ec5cdb9e4884b75623a0cc4328ac78bd5d075c9)
I think this commit makes sense, but isn’t explained very well. If the goal is to replace uses of new with std::make_unique you could do that without adding a bunch of factory methods. The reason new factory methods are needed is that the existing CreateChainParams factory method takes an ArgsManager parameter, so it can’t be called by libbitcoin_kernel code. By constrast, the new factory methods don’t require ArgsManager so they provide a way for code using libbitcoin_kernel to construct CMainParams, CRegTestParams, etc. classes indirectly. It can’t construct the classes directly because they are private to the chainparams.cpp file.
TheCharlatan
commented at 8:26 pm on March 7, 2023:
Thank you for spelling this out, I updated the description.
in
src/bitcoin-chainstate.cpp:58
in
4820450688outdated
In commit “Split non/kernel chainparams” (4820450688e371a99d66cfad24714fa6e041e43e)
Existing chainparams variable is used exactly one place below so would suggest just writing auto chainparams = CChainParams::Main(); and using it directly instead of having two similar variables.
TheCharlatan
commented at 11:26 pm on March 7, 2023:
In commit “Split non/kernel chainparams” (4820450688e371a99d66cfad24714fa6e041e43e)
Is calling SetGlobalBaseParams() actually necessary here? I’m guessing something breaks if isn’t added but it’s not clear what. It’d be good drop the call or change the generic comment above “// SETUP: Misc Globals” into a specific comment about why it’s needed currently, and maybe how it can go away in the future.
TheCharlatan
commented at 11:26 pm on March 7, 2023:
This is no longer relevant.
ryanofsky
commented at 6:08 pm on March 6, 2023:
contributor
Partial code review ACK for 2ec5cdb9e4884b75623a0cc4328ac78bd5d075c9, which is the fourth commit. The first 4 commits resolve all the previous issues that were discussed and look ready to merge. I did leave some new comments about code style, but they aren’t important and can be ignored.
The last 3 commits seem more half-baked to me, and might be better to split off. They require a different type of review anyway since they are moves and renames, not actual code changes. I left a more detailed comment below but basically I think the chainparams commits make sense and the chainparamsbase commits don’t. So an alternative to splitting up this PR could be to drop the 5th and 7th commits.
2795ff9cc40383f74223736c3b96515faea37410 Decouple SigNetChainParams from ArgsManager (1/7)
1cfccc38dd5d39a9e23a19b94d6ba1a7bc4c48dc Decouple RegTestChainParams from ArgsManager (2/7)
a1db3966d1c4f4213d7bfd6be0ea7fb308fe4e11 Wrap kernel chainparamsbase functions in kernel namespace (7/7)
TheCharlatan
commented at 8:25 pm on March 7, 2023:
contributor
Thank you for the review! These are valuable and I hope they allow me to improve future commits. I’m pushing changes addressing comments up to the 4th commit now.
The last 3 commits seem more half-baked to me, and might be better to split off.
I’ll now move to assessing how to proceed here now. I agree though that chainparamsbase could go into another PR altogether. The current approach is kind of ugly.
TheCharlatan force-pushed
on Mar 7, 2023
jonatack
commented at 8:54 pm on March 7, 2023:
contributor
Concept ACK, thanks for working on this.
TheCharlatan force-pushed
on Mar 7, 2023
TheCharlatan
commented at 11:26 pm on March 7, 2023:
contributor
The last 3 commits seem more half-baked to me, and might be better to split off.
Ok, I kept the 5th commit, “split non/kernel chainparamsbase”, but limited the rest to just moving the declaration of the CBaseChainParams class to the kernel. I also dropped the 7th commit “wrap kernel chainparamsbase functions in kernel namespace”.
TheCharlatan
commented at 10:35 am on March 8, 2023:
contributor
Updated f9e3c91f4d482b0c74ef899056d5a2c63e67e1fb -> c44983faa02b4845cd7605abe99c332980a07af1 (tc/2022-09-libbitcoinkernel-chainparams-args_3 -> tc/2022-09-libbitcoinkernel-chainparams-args_4, compare) to rebase on latest master for fixing an include issue causing CI build failures, add a commit to fix the regression introducing said include issue, and update the copyright header on kernel/chainparamsbase.h.
In commit “Decouple RegTestChainParams from ArgsManager” (66b17894185e0cda3ea7cbec77cc771a7ba41d56)
Probably better to merge the option instead of overwriting it:
0if (auto value = args.GetBoolArg("-fastprune")) options.fastprune =*value;
This would avoid needing to hardcode the default false value here, and would be more more consistent with version_bits_parameters and activation_heights handling below which keep preexisting values if arguments are unset.
Alternately, if you don’t want this function to keep preexisting values, it would make sense to add version_bits_parameters.clear() and activation_height.clear() calls.
TheCharlatan
commented at 10:46 pm on March 8, 2023:
Merging with the option seems more in tune with how the options are handled in other places in the code, so I went with that.
in
src/kernel/chainparamsbase.h:14
in
341be7fa68outdated
9+
10+/**
11+ * CBaseChainParams defines the base parameters (shared between bitcoin-cli and bitcoind)
12+ * of a given instance of the Bitcoin system.
13+ */
14+class CBaseChainParams
In commit “split non/kernel chainparamsbase” (341be7fa680e9dcdbaa46db0194833b5d0229394)
I still don’t think it makes sense to have CBaseChainParams in the kernel. Like the comment above says, the point of the CBaseChainParams class and the reason it is separate from CChainParams is that it is shared between bitcoin-cli and bitcoind. I think of CBaseChainParams as client parameters, and CChainParams as server parameters. Practically speaking, bitcoin-cli or other clients that needs basic information like network datadir locations or RPC port numbers should not be required to link against the kernel.
The justification for this change given in the commit message about allowing “kernel chainparams to be instantiated” also does not make sense to me, because chainparamsbase is part of libbitcoin_util, and it is fine for kernel code to depend on libbitcoin_util if we are going by https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md#dependencies
TheCharlatan
commented at 10:46 pm on March 8, 2023:
This makes sense, I wasn’t connecting the fact that the util library includes chainparamsbase anyway with segregating the kernel code. I dropped the commit.
ryanofsky
commented at 9:46 pm on March 8, 2023:
contributor
Partial code review ACKc44983faa02b4845cd7605abe99c332980a07af1 without the 5th commit, because I don’t think CBaseChainParams should move to the kernel. Comment about that below, but feel free to ignore the other comments.
Overall good changes, and nice that this is getting progressively simpler.
b56f0555fd2b048bb1d37474cda706efece49ed5 Decouple SigNetChainParams from ArgsManager (1/7)
66b17894185e0cda3ea7cbec77cc771a7ba41d56 Decouple RegTestChainParams from ArgsManager (2/7)
ryanofsky
commented at 12:44 pm on March 9, 2023:
contributor
Code review ACKa7afa98ca38fd66fc69b77b95a6a57d50207911b. This looks great, and much cleaner and simpler than the starting point. I’d encourage @ajtowns and anybody else who looked at this previously to have another look.
I think that one remaining drawback is that the PR description is very verbose. I think the PR could be described in a few sentences as:
Move CChainParams class definition into the kernel, and give it new factory functions CChainParams::{RegTest,Signet,Main,TestNet} that allow it to be constructed without an ArgsManager reference, unlike the current factory function CreateChainParams.
This allows external code using libbitcoinkernel to access to chain parameters and set chain options and call validation code without having to invoke bitcoind’s argument parsing logic.
Doing this required removing uses of ArgsManager within the CChainParams class itself, so the first few commits do that refactoring.
The bitcoin kernel library currently relies on code containing user configurations through the ArgsManager. This is not optimal, since as a stand-alone library it should not rely on most of the user-defined configuration. Instead, its interfaces should accept control and options structs that control the kernel library’s desired configuration.
A lot of tweakable parameters are defined within CChainParams and depend on access on the global ArgsManager gArgs instance.
Changes
This PR splits the chainparams code of chainparams.cpp/h and chainparamsbase.cpp/h into parts depending on gArgs access and parts that don’t. These are then moved into separate files within the kernel/ directory.
Though a lot of code had to be moved and reshuffled to accommodate to the validation of both gArgs and options structs, the behavior should not change by the hand of this PR.
Similar work towards decoupling the ArgsManager from the kernel has been done in #25290, #25487 and #25527. Similar PR’s are open in #25862 and #25152.
Currently Identified Drawbacks
The patches touch a large amount of consensus critical code. I tried to keep the change sets as incremental and easy to review as possible, but a lot of manual comparison of diffs will have to be done.
There is no patch done yet introducing the kernel/chainparams.cpp/h into the kernel namespace. Such a change would be very intrusive to the entire codebase and hard to review as part of this patch set. Due to its intrusiveness, I am not sure whether this is the correct path forward or not.
The globalChainBaseParams global had to be moved into the kernel chainparamsbase.cpp in order to allow calls to BaseParams() from contexts without access to gArgs. This is not ideal, but was the only way forward I could think of so far without basing this pull request on more prior work decoupling gArgs from the various call sites of BaseParams().EDIT: chainparamsbase.cpp is left untouched now. Only the class declaration of CBaseChainParams is moved to the kernel directory. EDIT 2: chainparamsbase.* is left untouched. See #26177 (comment) for reasoning.
ryanofsky
commented at 12:57 pm on March 9, 2023:
contributor
There is no patch done yet introducing the kernel/chainparams.cpp/h into the kernel namespace. Such a change would be very intrusive to the entire codebase and hard to review as part of this patch set. Due to its intrusiveness, I am not sure whether this is the correct path forward or not.
I think this PR is doing the right thing. If you move code into the kernel incrementally, and then set the namespace at the end after the code is moved, that avoids the need in incremental commits to add kernel:: in code before it is moved, and then remove kernel:: after moving it, which would just be a lot of noise.
TheCharlatan
commented at 2:54 pm on March 9, 2023:
contributor
I don’t follow why this is being changed when SelectParams() is still used – SelectParams already relies on gArgs and creates its own unique_ptr<CChainParams> so this is just creating two copies of the same object?
TheCharlatan
commented at 3:15 pm on March 14, 2023:
This is a demo binary, so I think the point is to show that the chainman can now be instantiated with a chainparams that is itself instantiated without directly relying on gArgs. I’m not sure if this really makes sense though.
This is a demo binary, so I think the point is to show that the chainman can now be instantiated with a chainparams that is itself instantiated without directly relying on gArgs. I’m not sure if this really makes sense though.
IMO, it would be a little clearer if the vague “SETUP: Misc Globals” comment was changed to say specifically that SelectParams() call was needed to set some globals used by validation code and could be removed when the globals were no longer needed.
in
src/kernel/chainparams.cpp:290
in
45048c0d7eoutdated
285+/**
286+ * Signet: test network with an additional consensus parameter (see BIP325).
287+ */
288+class SigNetParams : public CChainParams {
289+public:
290+ explicit SigNetParams(const CChainParams::SigNetOptions& options)
ryanofsky
commented at 4:39 pm on March 14, 2023:
contributor
Code review ACK600ab02bf58e073a93936438a7e884b3a7110f1c. Only changes since last review were rebase, and making suggested changes to make the split commit move only
DrahtBot requested review from ajtowns
on Mar 14, 2023
ajtowns approved
ajtowns
commented at 2:55 am on March 15, 2023:
contributor
ACK600ab02bf58e073a93936438a7e884b3a7110f1c
fanquake requested review from theuni
on Mar 15, 2023
in
src/chainparams.cpp:28
in
f3225c9090outdated
385+ if (args.IsArgSet("-signetchallenge")) {
386+ const auto signet_challenge = args.GetArgs("-signetchallenge");
387+ if (signet_challenge.size() != 1) {
388+ throw std::runtime_error(strprintf("%s: -signetchallenge cannot be multiple values.", __func__));
389+ }
390+ options.challenge.emplace(ParseHex(signet_challenge[0]));
? in 6e5221c49ef0e259a3109b51cbc68474f587be58: Seems pretty useless to waste LOCs with the only purpose to add redundancy and manual maintenance burden to repeat the return type verbatim twice?
Seems fine to remove all those docs, unless there is something non-trivial and worthy to mention?
nit in 6e5221c49ef0e259a3109b51cbc68474f587be58: Might as well make this make_unique as well? ( I guess you wanted to keep this move-only, but the other make_unique are no longer move-only anyway, as you removed the const.)
nit in 1157bcde4b2f05521a0c0d58eb790d4c62572ea7: I think the comment here can be removed? We have iwyu to produce these if anyone wants them, and all other includes in this file don’t have them either.
0node/mempool_args.cpp should add these lines:
1#include "kernel/chainparams.h" // for CChainParams
2node/mempool_args.cpp should remove these lines:
3- #include <chainparams.h> // lines 10-10
Please advise maintainers if this should be merged and if you plan to address the nits (if any).
Yes, will address the nits. It think it’s worth another round.
maflcko
commented at 2:21 pm on March 15, 2023:
member
Ok, but please don’t address those that are marked “unrelated”, as that would not be refactoring changes, but behavior changes/bugfixes.
Decouple SigNetChainParams from ArgsManager
SigNet chain params can now be initialized by configuring a
SigNetOptions struct, or with ArgsManager. This offers an interface for
creating SigNetChainParams without a gArgs object.
76cd4e7c96
Decouple RegTestChainParams from ArgsManager
RegTest chain params can now be initialized by configuring a
RegTestOptions struct, or with ArgsManager. This offers an interface for
creating RegTestChainParams without a gArgs object.
84b85786f0
Remove UpdateVersionBitsParameters
Moves setting struct member fields from a function to its call site.
This improves readability by surfacing the code.
d938098398
Add factory functions for Main/Test/Sig/Reg chainparams
This normalizes the behavior of initializing Main/Test/Sig/Reg
chainparams with RegTest/SigNet chainparams. These factory functions can
also easily be used from a context without an instantiated ArgsManager,
e.g. from libbitcoin kernel code, unlike the existing CreateChainParams
method.
edabbc78a3
Split non/kernel chainparams
Moves chainparams code not using the ArgsManager to the kernel.
Subsequently use the kernel chainparams header now where possible in
order to further decouple chainparams call sites from gArgs.
382b692a50
refactor: Don't use global chainparams in chainstatemanager method
The chainstatemanager m_options.chainparams member variable gets its
value from the global chainparams in init.cpp. This allows
validation.cpp to only include the the kernel chainparams file.
b3e78dc91d
TheCharlatan force-pushed
on Mar 15, 2023
TheCharlatan
commented at 3:49 pm on March 15, 2023:
contributor
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: 2025-06-03 09:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me