[Consensus] Segwit does not activate on BTC regtest

[zeptin]

In Networks.cs the following constants are defined for segwit activation and timeout via BIP9:

            network.consensus.BIP9Deployments[BIP9Deployments.TestDummy] = new BIP9DeploymentsParameters(28, 0, 999999999);
            network.consensus.BIP9Deployments[BIP9Deployments.CSV] = new BIP9DeploymentsParameters(0, 0, 999999999);
            network.consensus.BIP9Deployments[BIP9Deployments.Segwit] = new BIP9DeploymentsParameters(1, 0, 999999999);

Unlike Main and TestNet, these constants denote a day in the year 2001. This means that segwit never activates on RegTest. More precisely, the BIP9 threshold state never transitions to Started.

It is probable that a similar error exists with the timeout constant for CSV activation. Both should be fixed.

There is a further error in ThresholdConditionCache.cs in the following code:

            if (pindexPrev != null)
            {
                pindexPrev = pindexPrev.GetAncestor(pindexPrev.Height - ((pindexPrev.Height + 1) % nPeriod));
            }

It is unclear whether there is a logical error in this code when the block height is very close to genesis, or if the introduction of the skip list affects the output of GetAncestor. Either way, this line does not appear to do what it is supposed to on regtest and the BIP9 state does not change from Defined to Started. Eventually the state changes to Failed and segwit never activates.

The expected behaviour (based on trials with a Core bitcoind) is that segwit should lock in at about block height 288, then be active by about block 432 on regtest.

There is a third apparent error. When the pindexPrev = pindexPrev.Get... line is commented out in the previous code sample, the BIP9 state does seem to correctly transition to Started. However, at consensus tip height 395 a BadWitnessNonceSize exception gets thrown in WitnessCommitmentsRule.cs. It has currently not been established why this is the case.

                    if ((witness.PushCount != 1) || (witness.Pushes.First().Length != 32))
                    {
                        this.Logger.LogTrace("(-)[BAD_WITNESS_NONCE_SIZE]");
                        ConsensusErrors.BadWitnessNonceSize.Throw();
                    }

[zeptin]

Note that in the scenarios above a Core bitcoind was used to generate 433+ blocks, thereafter SBFN was allowed to attempt to synchronise with it.

[mikedennis]

Very interesting - just to confirm the issues around ThreshholdConditionCache and WitnessCommitmentsRule are only appearing on RegTest but not on TestNet or MainNet?

[mikedennis]

Cursory investigation of logic looks same as core so not sure why behaviour is different. Equivalent Bitcoin Core sections:

Network: (logic looks same to me) https://github.com/bitcoin/bitcoin/blob/5961b23898ee7c0af2626c46d5d70e80136578d3/src/chainparams.cpp#L288-L296

ThresholdConditionCache: (logic looks same to me) https://github.com/bitcoin/bitcoin/blob/5961b23898ee7c0af2626c46d5d70e80136578d3/src/versionbits.cpp#L35-L38

WintnessCommitmentsRule: (logic looks same to me) https://github.com/bitcoin/bitcoin/blob/5961b23898ee7c0af2626c46d5d70e80136578d3/src/validation.cpp#L3163-L3189

[dangershony]

The bug might be here https://github.com/bitcoin/bitcoin/blob/5961b23898ee7c0af2626c46d5d70e80136578d3/src/consensus/params.h#L43

We initialise segwit with zero (instead of -1 = ALWAYS_ACTIVE ) https://github.com/stratisproject/StratisBitcoinFullNode/blob/master/src/NBitcoin/Networks.cs#L234

[mikedennis]

Yes maybe that's it! @zeptin do you have an integration/unit test started we can use to verify this?

[mikedennis]

Good catch @dangershony . There also seems to be a discrepancy in bitcoin mainnet segwit settings as well. I will fix that as well.

Bitcoin: https://github.com/bitcoin/bitcoin/blob/5961b23898ee7c0af2626c46d5d70e80136578d3/src/chainparams.cpp#L99-L102

Stratis Bitcoin: https://github.com/stratisproject/StratisBitcoinFullNode/blob/master/src/NBitcoin/Networks.cs#L68

[dangershony]

Ah good catch @mikedennis I assume @NicolasDorier already fixed it on NBitcoin.

[NicolasDorier]

NBitcoin does not have consensus rules into it so it is highly possible the bug is still here.

[dangershony]

I am reopening this to continue the discussion. @NicolasDorier it seems for regtest segwit is not activated on core (ver 15.1) when we use the RPC method generate The C# node has regtest activate segwit by default but the blocks that are generated in core fail the segwit validation, if we mine 432 blocks on core then core sends segwit blocks. Do you have any ideas?

[NicolasDorier]

segwit is planned to be activated by default on core on regtest for next version of core. You have a settings (vbparams) in core so you can make core activate earlier meanwhile.

[NicolasDorier]

-vbparams=segwit:0:0 should do the trick. For next version this is merged: https://github.com/bitcoin/bitcoin/pull/11389

[NicolasDorier]

Actually you should try -1 maybe. I have not tried.

[dangershony]

Thank you Nicolas, I guess we will for now just activate it by mining blocks and later fix to be always active. But it is a useful test however as at some point want to activate segwit on the stratis chain so activation tests will come in handy.