refactor / kernel: Move non-gArgs chainparams functionality to kernel

TheCharlatan commented at 3:42 pm on September 24, 2022: contributor

This pull request is part of the libbitcoinkernel project #24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its “Step 2: Decouple most non-consensus code from libbitcoinkernel”. dongcarl is the original author of this patchset, these commits were taken from https://github.com/dongcarl/bitcoin/tree/2022-03-libbitcoinkernel-chainparams-args-only.

Context

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.

Type	Reviewers
ACK	MarcoFalke, ryanofsky, ajtowns
Concept ACK	dongcarl, jonatack

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 marked this as ready for review on Sep 25, 2022

glozow added the label Refactoring on Sep 26, 2022

in src/chainparams.h:158 in 8c0a94f1d5 outdated

153+    const std::vector<uint8_t> challenge;
154+    std::vector<std::string> seeds;
155+};
156+
157+/**
158+ * Return SigNetOptions filled with default values.

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 9929969d3e outdated

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 c00f4f7599 outdated

27+        return std::make_unique<CBaseChainParams>("regtest", 18443, 18445);
28+    }
29+    throw std::runtime_error(strprintf("%s: Unknown chain %s.", __func__, chain));
30+}
31+
32+static std::unique_ptr<CBaseChainParams> globalChainBaseParams;

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.

ryanofsky commented at 5:03 pm on March 6, 2023:

re: #26177 (review)

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 9714fe9160 outdated

66@@ -67,10 +67,10 @@ static void SetupCliArgs(ArgsManager& argsman)
67 {
68     SetupHelpOptions(argsman);
69 
70-    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 9714fe9160 outdated

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 9714fe9160 outdated

171+};
172+
173+struct RegTestOptions {
174+    std::unordered_map<Consensus::DeploymentPos, VersionBitsParameters> version_bits_parameters{};
175+    std::unordered_map<Activations, int> activation_heights{};
176+    uint64_t prune_after_height{1000};

ajtowns commented at 1:47 am on September 27, 2022:

I would have expected bool fastprune{false}; here. I don’t really think we support any values other than 100 or 1000?

TheCharlatan commented at 2:14 pm on October 15, 2022:

This is corrected in 795b6fb83d8eee9a7f47629b274feca066fc17fb

in src/chainparams.cpp:300 in 8c0a94f1d5 outdated

306+    explicit SigNetParams(const SigNetOptions& options)
307+    {
308+        std::vector<uint8_t> bin{options.challenge};
309+        vSeeds = options.seeds;
310 
311+        if (bin == SIGNET_DEFAULT_CHALLENGE) {

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:

0    if (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:
0    if (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.

Some comments below; code implementing them, and some other ideas at https://github.com/ajtowns/bitcoin/commits/202209-kernelchainparams

in src/chainparams.cpp:403 in 8c0a94f1d5 outdated

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:

0struct SigNetOptions {
1    std::vector<uint8_t> challenge;
2    std::optional<std::vector<std::string>> seeds;
3}

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:

I think that is taking it a bit far. What I understood from @ryanofsky is something very similar to what was implemented in #25862 , where a default, constructor instantiated option struct is mutated with the values from the args - https://github.com/bitcoin/bitcoin/pull/25862/files#diff-4e268aeb074a176689aace31957a889c1f39c909e6678ca7be8707bda1a7be56 .

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)
4ca849d6bc2d617babdcb870ca1de4376917c11d Remove UpdateVersionBitsParameters (3/7)
5d70983a84c9e09d315bd19296a08c26570b3b9a Add factory functions for Main/Test chainparams (4/7)
c00f4f7599b89df8196287176e743a99da1de67f split non/kernel chainparamsbase (5/7)
e461bd23908802ad806cdb25a4cfe9e23149dc57 Split non/kernel chainparams (6/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 03468cbc37 outdated

291+                // Data from RPC: getchaintxstats 4096 000000d1a0e224fa4679d2fb2187ba55431c284fa1b74cbc8cfda866fd4d2c09
292+                .nTime    = 1661702566,
293+                .nTxCount = 1903567,
294+                .dTxRate  = 0.02336701143027275,
295+            };
296+            consensus.signet_challenge = ParseHex("512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430210359ef5021964fe22d6f8e05b2463c9540ce96883fe3b278760f048f5189f2e6c452ae");

ajtowns commented at 3:01 am on October 17, 2022:

0            consensus.signet_challenge = ParseHex("512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430210359ef5021964fe22d6f8e05b2463c9540ce96883fe3b278760f048f5189f2e6c452ae");
1            if (vSeeds.empty()) {
2                vSeeds.push_back("seed.signet.bitcoin.sprovoost.nl.");
3                vSeeds.push_back("178.128.221.177");
4                vSeeds.push_back("v7ajjeirttkbnt32wpy3c6w3emwnfr3fkla7hpxcfokr3ysd3kqtzmqd.onion:38333");
5            }

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.

ajtowns commented at 5:41 am on October 18, 2022:

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.

ajtowns commented at 7:43 am on October 18, 2022:

Err, somehow I missed that entirely? Whoops.

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:

0if (args.IsArgSet("-signetchallenge")) {
1    ...
2    options.challenge = ParseHex(signet_challenge[0]);
3} else {
4    options = GlobalDefaultSigNet;
5}
6if (args.IsArgSet("-signetseednode")) {
7    options.seeds = args.GetArgs("-signetseednode");
8}

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

Ping @TheCharlatan for a rebase here.

TheCharlatan commented at 2:17 pm on December 16, 2022: contributor

Ping @TheCharlatan for a rebase here.

Will get to it Monday.

TheCharlatan force-pushed on Dec 19, 2022

DrahtBot removed the label Needs rebase on Dec 19, 2022

TheCharlatan force-pushed on Dec 21, 2022

TheCharlatan force-pushed on Dec 22, 2022

TheCharlatan force-pushed on Jan 17, 2023

TheCharlatan commented at 9:44 pm on January 17, 2023: contributor

Rebased.

fanquake commented at 11:00 am on January 18, 2023: member

Looks like all three test failures are the same “Blockchain is too short for pruning”:

https://cirrus-ci.com/task/5072222193188864?logs=functional_tests#L2444

 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
 5                                       pruneheight_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__
 7                                       return_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)

https://cirrus-ci.com/task/5037121606516736?logs=ci#L3282

 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__
 9                                       return_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 src/chainparams.cpp:18 in de405bd493 outdated

377@@ -382,11 +378,25 @@ class SigNetParams : public CChainParams {
378     }
379 };
380 
381+void ReadSigNetArgs(const ArgsManager& args, CChainParams::SigNetOptions& options) {

ajtowns commented at 1:26 pm on February 16, 2023:

Seems more ergonomic to write:

0CChainParams::SigNetOptions ReadSigNetArgs(const ArgsManager& args)
1{
2    ...

but maybe this is just deliberately keeping with the same pattern as per https://github.com/bitcoin/bitcoin/pull/26883/files#r1068247937

TheCharlatan commented at 3:04 pm on February 16, 2023:

Yes, also keeping it the same as in: https://github.com/bitcoin/bitcoin/pull/25862/files#diff-4e268aeb074a176689aace31957a889c1f39c909e6678ca7be8707bda1a7be56R11

in src/chainparams.cpp:525 in 6f148a738d outdated

524 };
525 
526-static void MaybeUpdateHeights(const ArgsManager& args, Consensus::Params& consensus)
527-{
528-    for (const std::string& arg : args.GetArgs("-testactivationheight")) {
529+std::optional<Consensus::BuriedDeployment> name4namestr(const std::string& name_str) {

ajtowns commented at 1:48 pm on February 16, 2023:

Rename to GetBuriedDeployment() ? This function should be part of deploymentinfo.cpp, shouldn’t it?

in src/chainparams.cpp:444 in 6f148a738d outdated

441+        nPruneAfterHeight = opts.fastprune ? 100 : 1000;
442         m_assumed_blockchain_size = 0;
443         m_assumed_chain_state_size = 0;
444 
445-        UpdateActivationParametersFromArgs(args);
446+        for (const auto& [name, height] : opts.activation_heights) {

ajtowns commented at 1:49 pm on February 16, 2023:

name is really deployment_id (dep for short)?

TheCharlatan commented at 3:08 pm on February 16, 2023:

I see, I’ll use dep, because that’s what is used elsewhere in the code.

ajtowns commented at 1:57 pm on February 16, 2023: contributor

Approach ACK f54210f50ec0cd44f026988308d975397a948d8c

TheCharlatan force-pushed on Feb 16, 2023

TheCharlatan commented at 3:57 pm on February 16, 2023: contributor

Thanks for the review. diff

TheCharlatan requested review from ajtowns on Feb 16, 2023

TheCharlatan removed review request from ajtowns on Feb 16, 2023

TheCharlatan requested review from ryanofsky on Feb 16, 2023

TheCharlatan removed review request from ryanofsky on Feb 16, 2023

TheCharlatan requested review from ajtowns on Feb 16, 2023

DrahtBot added the label Needs rebase on Feb 23, 2023

TheCharlatan force-pushed on Feb 23, 2023

TheCharlatan commented at 10:36 pm on February 23, 2023: contributor

Rebased.

DrahtBot removed the label Needs rebase on Feb 24, 2023

TheCharlatan force-pushed on Feb 24, 2023

in src/deploymentinfo.cpp:38 in 1cfccc38dd outdated

33@@ -34,3 +34,18 @@ std::string DeploymentName(Consensus::BuriedDeployment dep)
34     } // no default case, so the compiler can warn about missing cases
35     return "";
36 }
37+
38+std::optional<Consensus::BuriedDeployment> GetBuriedDeployment(const std::string& deployment_name) {

ryanofsky commented at 2:35 pm on March 6, 2023:

In commit “Decouple RegTestChainParams from ArgsManager” (1cfccc38dd5d39a9e23a19b94d6ba1a7bc4c48dc)

string_view would work here, don’t need a full string

TheCharlatan commented at 8:30 pm on March 7, 2023:

Good point, updated. diff

in src/chainparams.cpp:547 in 1cfccc38dd outdated

565-}
566 
567-void CRegTestParams::UpdateActivationParametersFromArgs(const ArgsManager& args)
568-{
569-    MaybeUpdateHeights(args, consensus);
570+        options.activation_heights.insert_or_assign(maybe_dep.value(), height);

ryanofsky commented at 2:56 pm on March 6, 2023:

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 src/chainparams.cpp:444 in 1cfccc38dd outdated

441+        nPruneAfterHeight = opts.fastprune ? 100 : 1000;
442         m_assumed_blockchain_size = 0;
443         m_assumed_chain_state_size = 0;
444 
445-        UpdateActivationParametersFromArgs(args);
446+        for (const auto& [dep, height] : opts.activation_heights) {

ryanofsky commented at 3:36 pm on March 6, 2023:

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 src/chainparams.cpp:529 in 1cfccc38dd outdated

528-    for (const std::string& arg : args.GetArgs("-testactivationheight")) {
529+void ReadRegTestArgs(const ArgsManager& args, CChainParams::RegTestOptions& options) {
530+    options.fastprune = args.GetBoolArg("-fastprune", false);
531+
532+    for (const std::string& arg : args.GetArgs("-testactivationheight"))
533+    {

ryanofsky commented at 3:39 pm on March 6, 2023:

In commit “Decouple RegTestChainParams from ArgsManager” (1cfccc38dd5d39a9e23a19b94d6ba1a7bc4c48dc)

Unnecessary diff here, somehow brace after for got moved to the next line. Could try using clang-format-diff to fix.

TheCharlatan commented at 8:27 pm on March 7, 2023:

Good catch, updated. diff

in src/chainparams.cpp:585 in 2ec5cdb9e4 outdated

581@@ -582,20 +582,39 @@ const CChainParams &Params() {
582     return *globalChainParams;
583 }
584 
585+std::unique_ptr<const CChainParams> CChainParams::SigNet(const CChainParams::SigNetOptions& options) {

ryanofsky commented at 4:00 pm on March 6, 2023:

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 4820450688 outdated

50@@ -51,8 +51,9 @@ int main(int argc, char* argv[])
51 
52 
53     // SETUP: Misc Globals
54-    SelectParams(CBaseChainParams::MAIN);
55-    const CChainParams& chainparams = Params();
56+    SetGlobalBaseParams(CBaseChainParams::MAIN);
57+    auto chainparams_ptr = CChainParams::Main();
58+    const CChainParams& chainparams = *chainparams_ptr;

ryanofsky commented at 5:59 pm on March 6, 2023:

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:

Thanks, updated. Diff

ryanofsky commented at 9:39 pm on March 8, 2023:

re: #26177 (review)

Thanks, updated. Diff

This looks the same as before. I was suggesting dropping the chainparams_ptr variable and just writing *chainparams directly below.

TheCharlatan commented at 10:46 pm on March 8, 2023:

I dropped this by mistake. Included now.

in src/bitcoin-chainstate.cpp:54 in 4820450688 outdated

50@@ -51,8 +51,9 @@ int main(int argc, char* argv[])
51 
52 
53     // SETUP: Misc Globals
54-    SelectParams(CBaseChainParams::MAIN);
55-    const CChainParams& chainparams = Params();
56+    SetGlobalBaseParams(CBaseChainParams::MAIN);

ryanofsky commented at 6:02 pm on March 6, 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)
96e01ecb18f926355fdad4da3d30d48788641082 Remove UpdateVersionBitsParameters (3/7)
2ec5cdb9e4884b75623a0cc4328ac78bd5d075c9 Add factory functions for Main/Test/Sig/Reg chainparams (4/7)
2f2216dee7a683638e6a3e966142f50f5de7df78 split non/kernel chainparamsbase (5/7)
4820450688e371a99d66cfad24714fa6e041e43e Split non/kernel chainparams (6/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.

Updated a1db3966d1c4f4213d7bfd6be0ea7fb308fe4e11 -> 65d026b47270c2fb9d2dab7f3d448b4abc37b93a (tc/2022-09-libbitcoinkernel-chainparams-args_0 -> tc/2022-09-libbitcoinkernel-chainparams-args_1, compare) to address some of @ryanofsky ’s comments.

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”.

Updated 65d026b47270c2fb9d2dab7f3d448b4abc37b93a -> 067b727a9741ddabf44f6efd5125a9c8fbea0702 (tc/2022-09-libbitcoinkernel-chainparams-args_1 -> tc/2022-09-libbitcoinkernel-chainparams-args_2, compare) to reduce the refactor of chainparamsbase as suggested by @ryanofsky.

TheCharlatan force-pushed on Mar 8, 2023

TheCharlatan commented at 7:30 am on March 8, 2023: contributor

Updated 067b727a9741ddabf44f6efd5125a9c8fbea0702 -> f9e3c91f4d482b0c74ef899056d5a2c63e67e1fb (tc/2022-09-libbitcoinkernel-chainparams-args_2 -> tc/2022-09-libbitcoinkernel-chainparams-args_3, compare) to undo an unnecessary order change in the makefile and to re-add a newline at the end of a file.

TheCharlatan force-pushed on Mar 8, 2023

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 src/chainparams.cpp:526 in 66b1789418 outdated

524 };
525 
526-static void MaybeUpdateHeights(const ArgsManager& args, Consensus::Params& consensus)
527-{
528+void ReadRegTestArgs(const ArgsManager& args, CChainParams::RegTestOptions& options) {
529+    options.fastprune = args.GetBoolArg("-fastprune", false);

ryanofsky commented at 8:57 pm on March 8, 2023:

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 341be7fa68 outdated

 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

ryanofsky commented at 9:25 pm on March 8, 2023:

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

Previous comment about this was: #26177 (review)

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 ACK c44983faa02b4845cd7605abe99c332980a07af1 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)
2ea0e3a4266f1b7c3bddec5d68cae9347a9aad57 Remove UpdateVersionBitsParameters (3/7)
e634311723aa43ba18bc45e1cf7e83fc4add5861 Add factory functions for Main/Test/Sig/Reg chainparams (4/7)
341be7fa680e9dcdbaa46db0194833b5d0229394 split non/kernel chainparamsbase (5/7)
18099e9d28247002cc08407969feec4fada90b76 Split non/kernel chainparams (6/7)
c44983faa02b4845cd7605abe99c332980a07af1 refactor: Don’t use global chainparams in chainstatemanager method (7/7)

TheCharlatan force-pushed on Mar 8, 2023

TheCharlatan commented at 10:46 pm on March 8, 2023: contributor

Thanks for the review,

Updated c44983faa02b4845cd7605abe99c332980a07af1 -> a7afa98ca38fd66fc69b77b95a6a57d50207911b (tc/2022-09-libbitcoinkernel-chainparams-args_4 -> tc/2022-09-libbitcoinkernel-chainparams-args_5, compare) to address @ryanofsky ’s latest comments. Drops the commit splitting chainparamsbase as suggested and addresses the two remaining nits.

ryanofsky approved

ryanofsky commented at 12:44 pm on March 9, 2023: contributor

Code review ACK a7afa98ca38fd66fc69b77b95a6a57d50207911b. 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.

For reference, the current PR description is:

This pull request is mostly a code-move of chainparams related functionality into the kernel library. @dongcarl is the original author of this patchset, these commits were taken from https://github.com/dongcarl/bitcoin/tree/2022-03-libbitcoinkernel-chainparams-args-only.

Context

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

re: #26177#issue-1384719233

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

re: #26177#pullrequestreview-1332732743

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

Thank you for the suggestions. I cleaned the description.

fanquake referenced this in commit b175bdb9b2 on Mar 14, 2023

DrahtBot added the label Needs rebase on Mar 14, 2023

TheCharlatan force-pushed on Mar 14, 2023

TheCharlatan commented at 11:22 am on March 14, 2023: contributor

Rebased a7afa98ca38fd66fc69b77b95a6a57d50207911b -> 45048c0d7e0ee12210d226239fb5ce63d9ed4441 (tc/2022-09-libbitcoinkernel-chainparams-args_5 -> tc/2022-09-libbitcoinkernel-chainparams-args_6, compare) to resolve conflict.

DrahtBot removed the label Needs rebase on Mar 14, 2023

in src/bitcoin-chainstate.cpp:56 in 45048c0d7e outdated

52@@ -52,7 +53,7 @@ int main(int argc, char* argv[])
53 
54     // SETUP: Misc Globals
55     SelectParams(CBaseChainParams::MAIN);
56-    const CChainParams& chainparams = Params();
57+    auto chainparams = CChainParams::Main();

ajtowns commented at 2:46 pm on March 14, 2023:

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.

ryanofsky commented at 4:36 pm on March 14, 2023:

re: #26177 (review)

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 45048c0d7e outdated

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)

ajtowns commented at 2:50 pm on March 14, 2023:

Closer to a move-only commit if you didn’t add the CChainParams:: qualifier here (and for regtest)

ajtowns commented at 2:59 pm on March 14, 2023: contributor

ACK 45048c0d7e0ee12210d226239fb5ce63d9ed4441

Agree with ryanofsky – “This looks great, and much cleaner and simpler than the starting point.” but some nits anyway.

DrahtBot requested review from ryanofsky on Mar 14, 2023

TheCharlatan force-pushed on Mar 14, 2023

TheCharlatan commented at 3:43 pm on March 14, 2023: contributor

Updated 45048c0d7e0ee12210d226239fb5ce63d9ed4441 -> 600ab02bf58e073a93936438a7e884b3a7110f1c (tc/2022-09-libbitcoinkernel-chainparams-args_6 -> tc/2022-09-libbitcoinkernel-chainparams-args_7, compare) to address @ajtowns ’s comment #26177 (review) using consistent class member definition accessing and cleaning up the diff.

ryanofsky approved

ryanofsky commented at 4:39 pm on March 14, 2023: contributor

Code review ACK 600ab02bf58e073a93936438a7e884b3a7110f1c. 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

ACK 600ab02bf58e073a93936438a7e884b3a7110f1c

fanquake requested review from theuni on Mar 15, 2023

in src/chainparams.cpp:28 in f3225c9090 outdated

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]));

maflcko commented at 11:30 am on March 15, 2023:

unrelated in f3225c9090eaef8a2f9d711364ca5ff3fd799844: Could use TryParseHex?

maflcko commented at 3:30 pm on March 17, 2023:

To test, -signetchallenge=nonhex silently fails/passes

in src/chainparams.cpp:26 in f3225c9090 outdated

383+        options.seeds.emplace(args.GetArgs("-signetseednode"));
384+    }
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__));

maflcko commented at 11:31 am on March 15, 2023:

unrelated: Does __func__ add any value?

TheCharlatan commented at 2:21 pm on March 15, 2023:

This produces a log like:

0terminate called after throwing an instance of 'std::runtime_error'
1  what():  CreateChainParams: Unknown chain lol

So I think yes, it does add value.

maflcko commented at 3:29 pm on March 17, 2023:

what(): CreateChainParams: Unknown chain lol

I think you tested the wrong thing. For me it prints:

0Error: SigNetParams: -signetchallenge cannot be multiple values.

Which is confusing to an end-user, because it leaks internal implementation details that are irrelevant and redundant to the user.

in src/chainparams.h:131 in f3225c9090 outdated

126+    /**
127+     * SigNetOptions holds configurations for creating a signet CChainParams.
128+     */
129+    struct SigNetOptions {
130+        std::optional<std::vector<uint8_t>> challenge;
131+        std::optional<std::vector<std::string>> seeds;

maflcko commented at 11:33 am on March 15, 2023:

nit in https://github.com/bitcoin/bitcoin/commit/f3225c9090eaef8a2f9d711364ca5ff3fd799844: Use {} to document the (lack of a) default value, see also https://github.com/bitcoin/bitcoin/pull/26296

maflcko commented at 4:04 pm on March 15, 2023:

Thx, can be marked resolved

in src/deploymentinfo.cpp:40 in 1a691358b7 outdated

35@@ -34,3 +36,18 @@ std::string DeploymentName(Consensus::BuriedDeployment dep)
36     } // no default case, so the compiler can warn about missing cases
37     return "";
38 }
39+
40+std::optional<Consensus::BuriedDeployment> GetBuriedDeployment(const std::string_view& deployment_name) {

maflcko commented at 11:49 am on March 15, 2023:

nit in https://github.com/bitcoin/bitcoin/commit/1a691358b7b4d9c8bd6286c2f38481155775f9d5: string_view already is a pointer, so no need to wrap it into another pointer. Also, clang-format. Also, if the name was kept name, more code would be move-only. :sweat_smile:

0std::optional<Consensus::BuriedDeployment> GetBuriedDeployment(std::string_view name)
1{

maflcko commented at 4:04 pm on March 15, 2023:

Thx, can be marked resolved

in src/chainparams.h:15 in 1a691358b7 outdated

 7@@ -8,9 +8,11 @@
 8 
 9 #include <chainparamsbase.h>
10 #include <consensus/params.h>
11+#include <cstdint>
12 #include <netaddress.h>
13 #include <primitives/block.h>
14 #include <protocol.h>
15+#include <unordered_map>

maflcko commented at 11:52 am on March 15, 2023:

nit in https://github.com/bitcoin/bitcoin/commit/1a691358b7b4d9c8bd6286c2f38481155775f9d5: wrong section (see stdlib include section below)

maflcko commented at 4:05 pm on March 15, 2023:

Thx, can be marked resolved

in src/chainparams.h:174 in 6e5221c49e outdated

169+     */
170+    static std::unique_ptr<const CChainParams> Main();
171+
172+    /**
173+     * Creates and returns a std::unique_ptr<CChainParams> for testnet.
174+     * @return std::unique_ptr<const CChainParams>

maflcko commented at 12:22 pm on March 15, 2023:

? 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?

maflcko commented at 4:05 pm on March 15, 2023:

Thx, can be marked resolved

in src/chainparams.cpp:594 in 6e5221c49e outdated

589+    return std::make_unique<CRegTestParams>(options);
590+}
591+
592+std::unique_ptr<const CChainParams> CChainParams::Main()
593+{
594+    return std::unique_ptr<CChainParams>(new CMainParams());

maflcko commented at 12:26 pm on March 15, 2023:

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.)

maflcko commented at 4:05 pm on March 15, 2023:

Thx, can be marked resolved

in src/kernel/chainparams.cpp:12 in 1157bcde4b outdated

 7+
 8+#include <chainparamsseeds.h>
 9+#include <consensus/amount.h>
10+#include <consensus/merkle.h>
11+#include <consensus/params.h>
12+#include <hash.h> // for signet block challenge hash

maflcko commented at 12:36 pm on March 15, 2023:

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.

maflcko commented at 4:06 pm on March 15, 2023:

Thx, can be marked resolved

in src/kernel/chainparams.cpp:23 in 1157bcde4b outdated

18+#include <script/script.h>
19+#include <uint256.h>
20+#include <util/strencodings.h>
21+
22+#include <algorithm>
23+#include <assert.h>

maflcko commented at 12:37 pm on March 15, 2023:

nit in 1157bcde4b2f05521a0c0d58eb790d4c62572ea7: cassert

maflcko commented at 4:06 pm on March 15, 2023:

Thx, can be marked resolved

in src/kernel/chainparams.cpp:25 in 1157bcde4b outdated

20+#include <util/strencodings.h>
21+
22+#include <algorithm>
23+#include <assert.h>
24+#include <cstdint>
25+#include <string.h>

maflcko commented at 12:37 pm on March 15, 2023:

same

maflcko commented at 4:07 pm on March 15, 2023:

Thx, can be marked resolved

in src/kernel/chainparams.h:187 in 1157bcde4b outdated

201+    CCheckpointData checkpointData;
202+    MapAssumeutxo m_assumeutxo_data;
203+    ChainTxData chainTxData;
204+};
205+
206+#endif // BITCOIN_KERNEL_CHAINPARAMS_H

maflcko commented at 12:44 pm on March 15, 2023:

Same commit:

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

maflcko commented at 4:07 pm on March 15, 2023:

Thx, can be marked resolved

maflcko approved

maflcko commented at 12:45 pm on March 15, 2023: member

Nice. I left some non-blocking nits. Please advise maintainers if this should be merged and if you plan to address the nits (if any).

review ACK 600ab02bf58e073a93936438a7e884b3a7110f1 💒

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 600ab02bf58e073a93936438a7e884b3a7110f1 💒
3kzm7rHrxddPB+LjaomzYoCJON/6jVUm4ilbYIF+uPjDG2kXx6jB3bd9r+RtMuVEKCtA3jrp/uE0wa/yO1RytDA==

TheCharlatan commented at 2:12 pm on March 15, 2023: contributor

re #26177#pullrequestreview-1341271390

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

Thank you for the review!

Updated 600ab02bf58e073a93936438a7e884b3a7110f1c -> b3e78dc91d01e364b77aacd9fb9a2f88688ab8a6 (tc/2022-09-libbitcoinkernel-chainparams-args_7 -> tc/2022-09-libbitcoinkernel-chainparams-args_8, compare) to address @MarcoFalke’s nits.

maflcko commented at 4:10 pm on March 15, 2023: member

re-ACK b3e78dc91d01e364b77aacd9fb9a2f88688ab8a6 🛁

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK b3e78dc91d01e364b77aacd9fb9a2f88688ab8a6 🛁
3pGFBFXVOeOhFMH19Wl5XyydIloTKbVPWTgtwXZ9tuyT+RBzlOb30I4YoZ777kOwafST63JsYR/9m9ftyZqo1AA==

DrahtBot requested review from ajtowns on Mar 15, 2023

DrahtBot requested review from ryanofsky on Mar 15, 2023

ryanofsky approved

ryanofsky commented at 9:18 pm on March 15, 2023: contributor

Code review ACK b3e78dc91d01e364b77aacd9fb9a2f88688ab8a6. Only changes since last review were recent review suggestions.

ajtowns commented at 10:57 am on March 16, 2023: contributor

ACK b3e78dc91d01e364b77aacd9fb9a2f88688ab8a6

fanquake merged this on Mar 16, 2023

fanquake closed this on Mar 16, 2023

sidhujag referenced this in commit 83b49c647f on Mar 16, 2023

sidhujag referenced this in commit 2a3f87bc45 on Mar 16, 2023

fanquake referenced this in commit 369d4c03b7 on Apr 3, 2023

fanquake referenced this in commit fc06881f13 on May 9, 2023

ryanofsky referenced this in commit 153a6882f4 on Jun 9, 2023

bitcoin locked this on Mar 16, 2024

refactor / kernel: Move non-gArgs chainparams functionality to kernel #26177

Context

Changes

Reviews

Conflicts

Context

Changes

Currently Identified Drawbacks