Refactor: Remove m_is_test

devtimnbr commented at 5:06 pm on August 31, 2023: contributor

Remove the m_is_test_chain bool Compiled and run tests locally

#28376

Refactor: Remove m_is_test_chain 27b4084e16

DrahtBot commented at 5:06 pm on August 31, 2023: 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, ajtowns
Concept NACK	luke-jr
Concept ACK	theStack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label Refactoring on Aug 31, 2023

in src/kernel/chainparams.h:108 in 27b4084e16 outdated

102@@ -103,7 +103,7 @@ class CChainParams
103     /** Default value for -checkmempool and -checkblockindex argument */
104     bool DefaultConsistencyChecks() const { return fDefaultConsistencyChecks; }
105     /** If this chain is exclusively used for testing */
106-    bool IsTestChain() const { return m_is_test_chain; }
107+    bool IsTestChain() const { return m_chain_type != ChainType::MAIN; }
108     /** If this chain allows time to be mocked */
109     bool IsMockableChain() const { return m_is_mockable_chain; }

maflcko commented at 7:21 am on September 1, 2023:

Looks like it would be cleaner to remove this method completely? This is only used to check that the chain type is regtest, which can be seen in the error strings. See:

0$ git grep -A1 IsMockableChain  src/rpc/
1src/rpc/mempool.cpp:            if (!Params().IsMockableChain()) {
2src/rpc/mempool.cpp-                throw std::runtime_error("submitpackage is for regression testing (-regtest mode) only");
3--
4src/rpc/node.cpp:    if (!Params().IsMockableChain()) {
5src/rpc/node.cpp-        throw std::runtime_error("setmocktime is for regression testing (-regtest mode) only");
6--
7src/rpc/node.cpp:    if (!Params().IsMockableChain()) {
8src/rpc/node.cpp-        throw std::runtime_error("mockscheduler is for regression testing (-regtest mode) only");

The rpc code would be easier to read if the chain type check was inlined and this method be removed, I’d say. For example, submitpackage has nothing to do with whether the time can be mocked.

maflcko commented at 9:22 am on September 1, 2023:

For clarity, my comment is on IsMockableChain, not on IsTestChain.

devtimnbr commented at 9:43 am on September 1, 2023:

I see. So keep IsTestChain and remove IsMockableChain method and replace all occurrences with inline checks like != ChainType::REGTEST?

maflcko commented at 10:08 am on September 1, 2023:

Yes, I think it makes sense to keep the IsTestChain alias, because it is just one line of code and it makes code that uses it easier to read.

However, the IsMockableChain helper is used only in contexts to check for regtest, so I think it makes code easier to read to use a regtest chain-type check directly.

ajtowns commented at 2:56 pm on September 1, 2023:

“mockable time” isn’t a concept with libbitcoinkernel, so I think it makes sense to move IsMockableChain out of the kernel module. IsTestChain otoh seems like an important enough concept to still have in the kernel and be something we can ask about any chain we might have.

maflcko commented at 7:35 am on September 1, 2023: member

lgtm ACK 27b4084e16a1cb210ce27119416ee34625781052

Looks good, with or without the other suggestion.

devtimnbr commented at 8:54 am on September 1, 2023: contributor

Thanks for the suggestion. I’ve removed the IsTestChain method and replaced all occurrences with inline checks of the ChainType.

devtimnbr force-pushed on Sep 1, 2023

theStack commented at 2:06 pm on September 1, 2023: contributor

Concept ACK

in src/rpc/mempool.cpp:865 in 4a1cd56df6 outdated

861@@ -862,7 +862,7 @@ static RPCHelpMan submitpackage()
862         },
863         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
864         {
865-            if (!Params().IsMockableChain()) {
866+            if (Params().GetChainType() != ChainType::REGTEST) {

ajtowns commented at 2:39 pm on September 1, 2023:

This change makes sense independently; submitpackage has no relationship to mocktime.

in src/rpc/node.cpp:45 in 4a1cd56df6 outdated

41@@ -42,7 +42,7 @@ static RPCHelpMan setmocktime()
42         RPCExamples{""},
43         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
44 {
45-    if (!Params().IsMockableChain()) {
46+    if (Params().GetChainType() != ChainType::REGTEST) {

ajtowns commented at 2:52 pm on September 1, 2023:

Not really convinced this makes sense though; having the IsMockable condition in one place (via an easily greppable name), rather than two places seems sensible. OTOH, could easily have that code just be in rpc/node.cpp rather than kernel/chainparams.h, eg

0static bool IsMockable(const CChainParams& params)
1{
2    return params.GetChainType() == ChainType::REGTEST;
3}
4
5if (!IsMockable(Params())) { error...; }

May also make sense to add an error in init.cpp if -mocktime is specified with a different chain (in which case mocktime activates but you can’t ever change it, so things will break pretty quickly), in which case perhaps putting IsMockable into src/chainparams.h (rather than kernel/chainparams.h) or similar would make sense.

May want to also replace Params() with EnsureAnyChainman(request.context).GetParams() to reduce global accesses?

maflcko commented at 3:08 pm on September 1, 2023:

If the function is kept, I’d prefer to keep it in the place it is now. Doesn’t seem worth it to move a single function into another file (like src/chainparams.h). I guess this can be done in the future, if there is more than one function.

Keeping the location as-is should also make it a smaller diff and less controversial :)

ajtowns commented at 2:53 pm on September 1, 2023: contributor

Concept ACK

DrahtBot added the label CI failed on Sep 3, 2023

DrahtBot removed the label CI failed on Sep 5, 2023

luke-jr commented at 9:06 pm on September 5, 2023: member

Concept NACK.

maflcko commented at 6:27 am on September 6, 2023: member

As for the second commit, I think it can be reduced down to just #28379 (review) and I think everyone agrees to keep IsMockableChain, because it is useful.

maflcko commented at 5:25 pm on September 14, 2023: member

Please squash your last 3 commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

You should end up with a total of 2 commits in the end.

Refactor: Replace 'isMockableChain' with inline 'ChainType' check for 'submitpackage' 78c2707b2a

devtimnbr force-pushed on Sep 15, 2023

devtimnbr commented at 2:40 pm on September 15, 2023: contributor

Please squash your last 3 commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

You should end up with a total of 2 commits in the end.

Thank you Marco. I wasn’t quite sure if you wanted to keep the history due to the discussion.

maflcko commented at 3:15 pm on September 15, 2023: member

re-ACK 78c2707b2aefa9e0aee5ddceaeab31997085a241

only change since previous review is adding one commit

maflcko approved

maflcko requested review from ajtowns on Sep 21, 2023

ajtowns commented at 2:53 pm on September 21, 2023: contributor

ACK 78c2707b2aefa9e0aee5ddceaeab31997085a241

DrahtBot removed review request from ajtowns on Sep 21, 2023

fanquake merged this on Sep 21, 2023

fanquake closed this on Sep 21, 2023

Frank-GER referenced this in commit f59610d8b5 on Sep 25, 2023

sidhujag referenced this in commit 56c1574a94 on Sep 26, 2023

bitcoin locked this on Sep 20, 2024

Refactor: Remove m_is_test_chain #28379

Reviews

Conflicts