Make deployment options available on all networks in unit tests

[darosior]

Based on top of #90, this is the second (and last) preparatory PR to the BIP54 implementation.

We use deployment options to test soft forks. Some of the BIP54 mitigations are specific to the mainnet chain params, therefore the BIP54 implementation unit tests need to be able to set an active deployment even if the chain type is not regtest. This PR makes the -vbparams and -testactivationheight available on all networks in the unit tests framework.

I don't think those options are particularly more of a footgun than other options/commands already available to users, but logic was added to prevent using the startup options on mainnet (#98 (comment)).

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #94 (Allow configuring target block time for a signet by benthecarman)

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.

[Sjors]

Some of the BIP54 mitigations are specific to the mainnet chain params

Are referring to the different timewarp grace period between testnet4 and mainnet? If so, I would just ignore the testnet4 value.

[darosior]

I am talking mainly about difficulty adjustments. Regtest has none and the testnets have tweaks. Are you suggesting a different approach?

[Sjors]

Oh I see, we need difficulty adjustment to test the timewarp. I agree that the tweaks in testnet3 and testnet4 ruin that.

I think a better approach is to allow difficulty adjustment on regtest. It could be implemented by adding a -disablepowretargeting (for fPowNoRetargeting) that's only available on regtest (true by default). It could be expanded to signet (false by default).

This avoids the need to pre-mine mainnet transactions (like in bip54_timestamps.json‎), which makes tests more flexible.

[darosior]

I think that enabling difficulty adjustments on regtest would be a pretty invasive change, potentially lead to overflows, all that in consensus-critical code (gated to regtest of course, but still).

To be clear i'm not trying to avoid going through the whole process of generating the test vectors again, and updating them in the BIP, it just seems to me that your suggestion is not a good tradeoff compared to the current approach.

[Sjors]

I don't think it's invasive at all if you keep the default true for regtest. It would only impact the new test added for the timewarp. Why would things overflow?

in consensus-critical code (gated to regtest

It's regtest that's gated, the normal difficulty adjustment code runs on all other networks:

unsigned int CalculateNextWorkRequired(const CBlockIndex* pindexLast, int64_t nFirstBlockTime, const Consensus::Params& params)
{
    if (params.fPowNoRetargeting)
        return pindexLast->nBits;
...

[instagibbs]

@Sjors if you allow difficulty adjustments on regtest it's very easy to overflow the calculations causing much larger than 4x changes. I'm not sure it's worth the squeeze.

[darosior]

It's also not clear to me if you are suggesting to throw away the current BIP test vectors, or if you are suggesting this to improve the current Timewarp functional test provided with the BIP 54 implementation for Bitcoin Core. Also note other implementations may not have regtest, and being compatible with other implementations is a goal of the BIP test vectors. It's usually not an issue for BIPs that pertain to Script stuff since they can be applied to either, but here the test vectors would be based on a genesis block (header) that other Bitcoin clients may not have, for what appear to be very marginal gains to the Bitcoin Core implementation.

[Sjors]

Oh I see you used mainnet headers for the BIP54 test vectors. I guess you don't run into the minimum chain work check for these alternate histories, because the timewarp check will fail first?

I'm a bit worried that people are going to abuse -vbparams for UASF style actions, or other strange "use cases". Having them only available (for mainnet) from inside the test suite would fix that concern, though I don't see how you can do that with functional tests.

Another option could be to use a signet with OP_TRUE challenge (which allows omitting the block signature). IIUC it has the same genesis block, initial difficulty and adjustment rules. Unfortunately you still can't reuse the test vector because signets activate SegWit a height 1. You could regenerate the test vectors on Signet and use witness-stripped version for mainnet, but that seems overkill.

[darosior]

I'm a bit worried that people are going to abuse -vbparams for UASF style actions, or other strange "use cases".

We already expose functionalities that allow to do the same. We should avoid introducing footguns, and bigger ones already exist (invalidateblock much?), but if people want to fork themselves off they can always do so. Preventing that shouldn't (and can't) be a goal.

Having them only available (for mainnet) from inside the test suite would fix that concern

I can explicitly disable using the startup option on other network than regtest if you'd like. I didn't think it was worth doing because as i said we already expose bigger footguns (especially as undocumented commands and startup options). But if it can help address others' concerns i am happy to do that.

though I don't see how you can do that with functional tests.

How is this related to this discussion? Functional tests use regtest.

but that seems overkill.

Yes, and moreover Signet has different difficulty adjustment parameters (maximum target).

[Sjors]

Using invalidateblock requires a lot more coordination than saying "we all activate X at height 1 million, add this one line to bitcoin.conf".

Functional tests use regtest.

It can use any network, see mining_mainnet.py. But is has no privilege over the node, so can't use -vbparams on mainnet if the option isn't available.

I wrote:

Unfortunately you still can't reuse the test vector because signets activate SegWit a height 1.

But this can be changed if -vbparams is available on signet.

Signet has different difficulty adjustment parameters (maximum target).

Oh, the higher powLimit isn't mentioned in the BIP (see bitcoin#18267 (comment) and bitcoin#18267 (comment)). Although a test can stay clear of the limit it could cause confusion. That seems like a good reason to avoid signet.

[darosior]

If i prevented using -vbparams on mainnet outside of the unit tests, would that address your concerns?

[ajtowns]

Yes, and moreover Signet has different difficulty adjustment parameters (maximum target).

We've had people wanting to vary signet consensus params for other reasons (mutinynet's 1m block time eg); at least this would be a reason to do that that's directly related to improving mainnet.

[darosior]

Rebased on top of updated #90 and added a startup check to make sure deployment options may only be used on regtest (as per the existing documentation).

[Sjors]

#90 landed, so maybe give this a rebase so it looks less scary :-)

[darosior]

Rebased.

[darosior]

Actually it may be preferable to only prevent the argument from being used on mainnet, rather than making it regtest only, because we also have functional tests for other test networks. For instance feature_signet.py, for which -vbparams is set in #99.

[Sjors]

That seems fine, only mainnet needs seatbelts.

[darosior]

Updated to only prevent starting up with -vbparams/-testactivationheight on mainnet, not test networks.

[Sjors]

utACK 3a77a06

I briefly checked that (if you disable the "Disallow mainnet/testnet operation" guard) it rejects -testactivationheight=segwit@1 and -vbparams=testdummy:0:1 on mainnet.

[ajtowns]

Non-binding NACK f30d527 -- at this point, this PR is just a very small amount of setup work with no payoff and only one dependency, so I think we should just merge these commits as part of #99 without keeping them in a separate PR. Otherwise, apart from the TestNet4 vs TestNet issue below, looks good to me.

[darosior]

OK. Pushed one last time to address your review comment, and closing this to have the commits directly as part of #99 instead.