BIP8: allow some MUST_SIGNAL blocks to not signal

[ajtowns]

Based on #1020 since they touch neighbouring lines in the BIP.

Using the same threshold for MUST_SIGNAL as STARTED means that any chain
that would have resulted in activation with lockinontimeout=false will
also result in activation with lockinontimeout=true (and vice-versa).
This reduces the ways in which a consensus split can occur, and avoids
a way in which miners could attempt to discourage users from setting
lockinontimeout=true.

[ajtowns]

One thing to consider is behaviour when the parameters for a deployment are changed while it is in progress.

The "miners attempt to discourage users from setting lockinontimeout=true" scenario is one of those in a sense. In more detail: timeoutheight - 2016 is reached, then a block that does not signal is mined, and a few hours worth of additional blocks are then mined on top of that. Miners say "we don't want to throw away that work, so please just unset lockinontimeout and we'll promise to make sure it activates!" Then the last 100 blocks are all mined with the bit unset, and users either have to do a 2016 block reorg by setting lockinontimeout back to true, or have to start a new deployment attempt and try again. Reducing the threshold for MUST_SIGNAL from 100% down to the same as that for STARTED allows the code to cope with a small mistake like a single block failing to signal automatically.

Another parameter change case worth considering, is reducing the timeoutheight. If a node has timeoutheight = T, lockinontimeout=true then any valid chain will result in activation at some height A, and any other node with the same startheight and threshold but with timeoutheight > T -- will see that chain as valid, and also consider activation to occur at height A -- either because there was sufficient signalling prior and both nodes transition through STARTED, LOCKED_IN to ACTIVE at the same points, or because the first node transition from STARTED to MUST_SIGNAL while the second node remained in STARTED, but both nodes transition to LOCKED_IN and then ACTIVE at the same times, since a valid chain that passes through MUST_SIGNAL will also pass the threshold for a node to transition from STARTED to LOCKED_IN.

Decreasing the threshold parameter is not compatible, however. With BIP8 as it stands, if two nodes have the same startheight, timeoutheight but one node has a threshold = X and the other has threshold = Y, Y > X, then if there is a sequence of retarget periods that begin in STARTED, they will not be in consensus:

• X blocks signal; all blocks signal; no blocks signal
  • the first node will consider the statuses to be STARTED, LOCKED_IN, ACTIVE
  • the second node will consider blocks to be STARTED, STARTED, invalid due to lack of signalling during LOCKED_IN
• X blocks signal; Y blocks signal, all blocks signal
  • the first node will consider the statuses to be STARTED, LOCKED_IN, invalid due to lack of signalling during LOCKED_IN
  • the second node will consider blocks to be STARTED, STARTED, LOCKED_IN [then ACTIVE]

Without mandatory signalling during LOCKED_IN, the incompatibility changes -- activation for the first node by X blocks signalling does not affect the second node at all, so that the first node may consider the deployment to have activated while the second node does not, and this obviously becomes complicated if the second node has lockinontimeout=true. With MUST_SIGNAL requiring only threshold blocks to signal rather than all blocks, consensus divergence becomes possible in the final period as well.

In general if it is desirable to reduce the threshold for activation, I think taking the BIP-91 approach and instead introducing a new deployment with a reduced threshold that then triggers mandatory signalling of the original deployment at its original threshold seems a much safer approach.

[jonasnick]

This makes a lot of sense, because it "allows the code to cope with a small mistake like a single block failing to signal automatically" and otherwise there can be a chainsplit between { lioto=true, timeout = T } and { lioto = false, timeout = T } parameters despite the softfork being ACTIVE on both chains.

Fwiw, I implemented this BIP (in haskell, so it's a bit different than the pseudocode, EDIT: see here) and "quickcheck"ed that the following property holds for a large number of random chains:

1. if the chain is valid under { lioto = true, timeout = T }, it's valid under { lioto = false, timeout >= T }
2. if the chain is valid under { lioto = false, timeout = T } and the bip8 state is LockedIn or Active, it's valid under { lioto = true, timeout = T }

(only 2. wouldn't be true without this PR)

[rustyrussell]

Ack afe97b2

[ajtowns]

Removed draft marker; it's fine to review this PR. Note this PR incorporates the commit from #1020; if #1020 is merged that makes it a simple rebase; if it's rejected, this PR needs to be tweaked obviously.

[benthecarman]

ACK

[ProofOfKeags]

ACK

[setpill]

NACK makes the game theory significantly more complex/hard to reason about for marginal benefits.

ACK after @jonasnick pointed out it actually simplifies the game theory.

[dr-orlovsky]

concept NACK: Imo complexity+overengineering not worth the win. Added complexity may have some non-linear consequences and without the clear and strong need shouldn't be merged. PR seems to save some revenues for non-careful miners at the expense of higher complexity-driven potential risks for broader community...

[dr-orlovsky]

Following IRC discussion I tend to agree that the "serious win" here is to make UASF a bit more risky once we are there; however the current definition of the algorithm logic is very obscure and must be improved (modulo division + reverse counting with the forward counting in the same block - what can go wrong with this here when we are in UASF? Really seems to me like a non-totally predictable thing).

Removing concept NACK and replacing with "need to be re-worked" in the algorithm part and re-reviewed.

[lucasmoten]

ACK. Avoid divergence.

[hsjoberg]

ACK 9a119ce

[AbelLykens]

ACK

[luke-jr]

After the NACKs here, @jonasnick pointed out that without this, the chain could activate Taproot while lockinontimeout=true fail to follow that Taproot-activated chain. Therefore, I consider this a strict bugfix to BIP 8: ACK.

[achow101]

Post merge ACK afe97b2

[AlejandroDeLaTorre]

Post merge ACK