Chainparams: Rename IsTestChain() to AllowAcceptNonstd()

jtimon commented at 3:18 pm on August 31, 2019: contributor

Refactor, this shouldn’t change functionality.

This is a step to get rid of Params().IsTestChain() which is currently used for 2 unrelated things.

fanquake added the label Refactoring on Aug 31, 2019

DrahtBot commented at 5:01 pm on August 31, 2019: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17032 (Tests: Chainparams: Make regtest almost fully customizable by jtimon)
#16224 (gui: Bilingual GUI error messages by hebasto)

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.

instagibbs commented at 5:33 pm on September 12, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/16770/commits/e1960b127cdd6cbd9e54ea70989c417ad9fd0c4d

in src/chainparams.h:67 in e1960b127c outdated

63@@ -64,6 +64,8 @@ class CChainParams
64     const CBlock& GenesisBlock() const { return genesis; }
65     /** Default value for -checkmempool and -checkblockindex argument */
66     bool DefaultConsistencyChecks() const { return fDefaultConsistencyChecks; }
67+    /** If it is allowed to set -acceptnonstdtxn=1 for this chain or not */
68+    bool AllowAcceptNonstd() const { return m_allow_accept_nonstd; }
69     /** Policy: Filter transactions that do not match well-defined patterns */

MarcoFalke commented at 3:04 am on September 13, 2019:

Could add a commit to mention this is the default value and not the value used in validation?

MarcoFalke commented at 3:06 am on September 13, 2019:

Something like

0    /** Default for -acceptnonstdtxn */

jtimon commented at 5:15 pm on September 13, 2019:

well, I was leaving that for another PR: #16526 See also #16527 which is another option.

jtimon force-pushed on Oct 2, 2019

jtimon renamed this:
~~Chainparams: Decouple AllowAcceptNonstd() from IsTestChain()~~
Chainparams: Rename IsTestChain() to AllowAcceptNonstd()
on Oct 2, 2019

jtimon commented at 10:41 pm on October 2, 2019: contributor

After #16524 has been merged, this doesn’t decouple anything anymore, it’s just a rename. Which means we can also remove the old names: updated.

MarcoFalke commented at 10:57 pm on October 2, 2019: member

I know this has been discussed in the meeting and everyone was meh or +1 for this and I was the only one against it. But to be consistent, shouldn’t you update the if (!Params().MineBlocksOnDemand()) in setmocktime as well? Pretty much any MineBlocksOnDemand that is not used in the miner is a candidate for IsTestChain (according to me) or for a separate flag in the chainparams (according to everyone that is not me).

jtimon commented at 0:47 am on October 3, 2019: contributor

I asumed MineBlocksOnDemand() was renamed to the equivalent for fPowNoRetargeting against my deepest bike-shedding wishes, but, no, not the getter, only the internal variable. What was the rationale?

Why do we even have “candidates for IsTestChain”? Is that documented anywhere?

I understand the desire to simplify chainparams, but I don’t think the best path is to group them and couple them but to try to document them so it gets more obvious which ones aren’t really needed.

For example, IMO we don’t need Params().RequireStandard() as per #16527 Or, as you seem to agree, we didn’t need Params().IsFallbackFeeEnabled() as per #16402 and #16524

MarcoFalke commented at 3:06 pm on October 3, 2019: member

I understand the desire to simplify chainparams, but I don’t think the best path is to group them and couple them but to try to document them so it gets more obvious which ones aren’t really needed.

Those checks are there to prevent users from shooting themselves in the foot. Non std txs could mean they accidentally created an malformed tx, making all their funds go to miners. Incorrect mocktime means they get out of consensus (among other things, like braking tx relay). Same goes for a fallbackfee default, …

However the question “can you shoot yourself in the foot on regtest or testnet?” can always be answered with “no” for a test chain.

jtimon commented at 5:57 pm on October 3, 2019: contributor

Sorry, I don’t see how that says anything against what I’m saying. The checks and functionalities can be the same independently of them being documented in different chainparams fields or grouped and coupled into a single field named IsTestChain despite being unrelated functionalities. In any case, if I can read more about discussions around “candidates for IsTestChain()”, that would be awesome.

MarcoFalke commented at 6:05 pm on October 3, 2019: member

Right, the only difference is the amount of code needed in chainparams

MarcoFalke commented at 6:08 pm on October 3, 2019: member

Long-term I don’t care which option we pick, but we should be consistent. That means that the MineBlocksOnDemand() check in setmocktime should be either replaced with IsTestChain() or something like CanSetMockTime(), no?

jtimon commented at 8:04 pm on October 3, 2019: contributor

That means that the MineBlocksOnDemand() check in setmocktime should be either replaced with IsTestChain() or something like CanSetMockTime(), no?

Perhaps CanSetMockTime(), but not IsTestChain, I think. That should be false for testnet, no? I mean, even if we wanted to group unrelated things under IsTestChain(), unless I’m missing something.

MarcoFalke commented at 8:07 pm on October 3, 2019: member

Perhaps CanSetMockTime(), but not IsTestChain, I think.

Fine with me.

That should be false for testnet, no?

Fine with me. (off-topic: While I think there is no risk to enable it for testnet)

MarcoFalke added the label Waiting for author on Oct 8, 2019

MarcoFalke commented at 2:06 pm on October 8, 2019: member

@jtimon Are you working on the CanSetMockTime or nah? #16770 (comment)

jtimon commented at 7:03 pm on October 8, 2019: contributor

No, I’m not working on CanSetMockTime. I’m still not sure what do you want to do about it or what’s the relation to this PR.

MarcoFalke commented at 7:37 pm on October 8, 2019: member

What is the high level goal of this pull request then?

jtimon commented at 7:52 pm on October 8, 2019: contributor

Get rid of IsTestChain renaming it to something that describes what it does better.

jtimon commented at 1:45 pm on October 9, 2019: contributor

There’s a label that says “waiting for author”. I don’t understand, waiting for author to do what?

MarcoFalke removed the label Waiting for author on Oct 9, 2019

jtimon commented at 4:00 am on October 11, 2019: contributor

I think @MarcoFalke and I agreed on IRC his nits would be solved with something like this:

#17106

Therefore we can focus on what’s at hand here, which is much less. @instagibbs you gave your utACK before #16524 when in my opinion it was more important, (because there was a functional coupling right there) it was actually decoupling rather than a simple rename, so feel free to re-utACK or meh or -0 or nack or whatever.

jtimon commented at 11:40 am on February 4, 2020: contributor

@MarcoFalke Do you agree with what I said or did I misunderstood our chat discussion?

DrahtBot added the label Needs rebase on Feb 18, 2020

Chainparams: Rename IsTestChain() to AllowAcceptNonstd() 698c6e6d4c

jtimon force-pushed on Feb 19, 2020

jtimon commented at 8:13 pm on February 19, 2020: contributor

Rebased. @MarcoFalke note it seems #18037 decouples whether the chain is mockable or not from MineBlocksOnDemand like #17106 (and it does more things), so if I understood you well, your concerns should be solved, right?

DrahtBot removed the label Needs rebase on Feb 19, 2020

jtimon commented at 1:33 pm on May 6, 2020: contributor

Closing due to lack of interest.

jtimon closed this on May 6, 2020

MarcoFalke commented at 2:37 pm on May 6, 2020: member

I think there was agreement to do this in the meeting when the topic was discussed, but I think no one had interest to review.

I hereby commit to reviewing this if at least one other person reviews this :)

jtimon commented at 8:27 pm on May 6, 2020: contributor

I think it takes very little time to review this, probably even less time than in answering my last question to you. I mean, how much time does it get to give an utACK or even a concept ACK to this? 3 minutes?

But I’m not interested in this myself anymore, so don’t bother. I would be more interested in getting review for #17037 or even #17032 But even for those, I’m not even motivated to rebase #17037 or to answer/fix nits for #17032 So I guess don’t bother either. Use your time in something that has more chances of getting merged.

Thanks anyway.

MarcoFalke added the label Up for grabs on May 6, 2020

bitcoin locked this on Feb 15, 2022

MarcoFalke removed the label Up for grabs on Aug 29, 2023

Chainparams: Rename IsTestChain() to AllowAcceptNonstd() #16770

Conflicts