BIP 8 - replace FAILING with MUST_SIGNAL #950

0x2... misses a 0 and the parantheses are wrong (there's at least one opening paranthesis missing). I tested this code a little bit and it seems correct. Nit: Could perhaps be a bit simplified by moving the break condition to the while conditions and moving the nonsignal + count + threshold > 2016 condition somewhere else because the LHS is constant across loop iterations. Perhaps something like

walk = block;
if (count + threshold >= 2016) {
    while (count > 0  && nonsignal + threshold <= 2016)  {
        ...
        if ... {
            ++nonsignal;
        }
    }
    if (nonsignal + threshold > 2016) {
        return Invalid;
    }
}

Generally the suggested changes look very reasonable but I can't speak to the p2p relay issues.

count decreases every iteration but nonsignal only increases sometimes, so the LHS is decreasing, which lets the loop exit early (if you've only seen 5 nonsignalling blocks, and there's only 95 more blocks to look at, you'll never get 101 nonsignalling blocks so no need to look any further). Not really very useful to include in the spec though. No real need to skip the loop if the current block signals either, I think which might make the spec a bit simpler.

Updated this

Hmm, not sure why I said the LHS would be constant. But what I meant is that you can leave count out of this as I did in my pseudocode.

Anyway, the new code passes my tests. So ACK that.

No real need to skip the loop if the current block signals either,

Fwiw, the updated code still seems to that with the else if block.

This drops mandatory signalling in the MASF case. Maybe that's okay, but I'm not sure yet. Can you move it to a separate PR?

Well, it drops mandatory signalling to 95% rather than 100%, but MASF won't happen without 95% signalling, so "MASF drops mandatory signalling" is technically saying "miners did 95% signalling to trigger activation, but they didn't do 95% signalling" which doesn't make sense to me... I'm not seeing what a 100% signal period buys over a 95% signal period (and it'd cost either miners or users having to immediately react to seeing it, or and extra month between lockin and activation, with states going STARTED -> PAUSE1 -> ALL_SIGNAL -> PAUSE2 -> ACTIVE assuming users are meant to react based on ALL_SIGNAL rather than the 95% signalling in STARTED)

The 95% is before LOCKED_IN, so fundamentally separate from the mandatory signalling during LOCKED_IN.

I'm not saying it should definitely be rejected, just that it should be separate from this PR and evaluated independently.

The way I'm looking at the fundamentals is that, at present:

• miners need to react instantly when they see 95% signalling during STARTED, and do 100% signalling during LOCKED_IN
• users need to react instantly when they see 100% signalling during FAILING/LOCKED_IN, and start enforcing soft fork rules
• with lockinontimeout, miners need to signal at 100% at a particular time

And after this patch:

• miners don't need to upgrade from 95% signalling to 100% signalling
• users have a retarget period to react when they see 95% signalling during STARTED/MUST_SIGNAL, and then need to start enforcing the soft fork rules
• with lockinontimeout, miners are required to signal at 95% at a particular time

The only reason I can see for "miners need to upgrade from 95% signalling to 100% signalling" is if users want to only react when they see 100% signalling (which would make it more plausible to reduce the activation threshold for a deployment I guess), but to preserve the "don't have to react immediately" that would look add another two retarget periods, something like:

 * STARTED -> THRESHOLD_MET (95% threshold)
 * THRESHOLD_MET -> SIGNAL_ALL (always)
 * SIGNAL_ALL -> LOCKED_IN (always)
 * LOCKED_IN -> ACTIVE (always)
 * STARTED -> LOCKED_IN (100% threshold)

 * all blocks must signal during SIGNAL_ALL
 * STARTED -> LOCKED_IN transition ensures compatibility with lock in via reduced threshold activation

But if you want to reduce the activation threshold you can just have another deployment like bip91 that signals on a different bit, so instead of STARTED -> THREDHOLD_MET -> SIGNAL_ALL -> LOCKED_IN -> ACTIVE you go (STARTED,STARTED) -> (STARTED,LOCKED_IN) -> (STARTED,ACTIVE) -> (LOCKED_IN,COMPLETE) -> (ACTIVE,COMPLETE) which takes the same amount of time, but doesn't complicate deployments that don't need their threshold reduced. And that approach works even if you change the period length as well as the threshold.

Again, I'm not saying we shouldn't make this change, only that it is logically independent from the rest of the changes here, and should be a separate PR.

I wonder if we should keep FAILING as an alternative here, for UX only (no logic difference from STARTED).

UX only should be in the code, not in the bip, I think?

Well, the UX side is simply conveying "deployment status". It might be cleaner to recognise a status of "others might be MUST_SIGNAL".

Buuut... the point of this change is to make it so that could be arbitrarily earlier. So maybe FAILING would be bad to reintroduce in the end.

Maybe ideally you'd somehow track "there's a lower work header chain where the fork has activated, and the tip is not terribly old" and alert based on that?

But even more importantly, nodes enforcing MUST_SIGNAL need to ensure they remain attached to other such nodes.

I think the details of that probably need to be left to the bips that want to deploy via bip-8; they could ensure they remain connected to other nodes by making sure lockinontimeout only happens when everybody is happy with it, in which case no special code is needed; or they could allocate a temporary service flag bit for it and preferentially connect to other nodes that way for some period.

Still worth mentioning, but nothing's coming to mind as to what to say exactly.

Added a note about this in a separate commit

Probably should rephrase this

...with STARTED (or MUST_SIGNAL) blocks...