bip 8: simplify MUST_SIGNAL check #1063

pull ajtowns wants to merge 1 commits into bitcoin:master from ajtowns:202102-bip8-simplify-mustsignal-check changing 1 files +5 −5

ajtowns commented at 2:43 AM on February 4, 2021: contributor

Simplify the validity check that at least threshold blocks are signalling during the MUST_SIGNAL phase for BIP 8.
ajtowns commented at 2:44 AM on February 4, 2021: contributor

cc @jonasnick @dr-orlovsky @luke-jr -- see also #1021 (comment)
bip 8: simplify MUST_SIGNAL check 63d2800fab

in bip-0008.mediawiki:193 in 3a9f7d98e9 outdated

 192 | -                if (nonsignal + threshold > 2016) {
 193 | +                if (nonsignal > 2016 - threshold) {
 194 |                      return state.Invalid(BlockValidationResult::RECENT_CONSENSUS_CHANGE, "bad-version-bip8-must-signal");
 195 |                  }
 196 |              } else if (nonsignal == 0) {
 197 | +                // the new block signalled, and its parent was valid, so we don't need to keep searching

ajtowns commented at 2:46 AM on February 4, 2021:

Could get rid of the nonsignal == 0 branch as well, that's really just an optimisation.

ajtowns force-pushed on Feb 4, 2021
dr-orlovsky approved
dr-orlovsky commented at 5:29 PM on February 5, 2021: contributor

LGTM. Propose to include it into the next taproot activation meeting for the review
benthecarman approved
benthecarman commented at 12:49 PM on February 6, 2021: contributor

ACK 63d2800fabe4393382f699ba1e41260ab7b01727
jonasnick commented at 4:12 PM on February 8, 2021: contributor

ACK 63d2800fabe4393382f699ba1e41260ab7b01727

Sanity checked that there's no nasty off-by-one by adjusting my previous implementation according to this PR and running the same tests against it. The nonsignal == 0 was a neat optimization but fine to leave that up to the implementor.
luke-jr merged this on Feb 8, 2021
luke-jr closed this on Feb 8, 2021

Contributors

Linked (view graph)

#1021 BIP8: allow some MUST_SIGNAL blocks to not signal