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

Refactor, this shouldn't change functionality.

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

Rename related to https://github.com/bitcoin/bitcoin/pull/16526

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

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.

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

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.

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

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

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.

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.

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

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?

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.

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)

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

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.

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

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

There's a label that says "waiting for author". I don't understand, waiting for author to do what?

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.

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

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?

Closing due to lack of interest.

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

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.