Page MenuHomePhabricator

Add a generic flag to activate the next upgrade
Changes PlannedPublic

Authored by Fabien on Dec 17 2019, 07:44.

Details

Reviewers
deadalnix
jasonbcox
Group Reviewers
Restricted Project
Summary

This will allow to keep the various callsites working between every
upgrade without having to update them all.

Depends on D4742.

Test Plan
cmake -GNinja .. -DTEST_WITH_UPGRADE_ACTIVATED=ON
ninja check
./test/functional/test_runner.py --with-nextupgrade --extended

Run IBD.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
graviton_next_upgrade_flag
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8585
Build 15158: Default Diff Build & Tests
Build 15157: arc lint + arc unit

Event Timeline

It's worth pointing out there is a slight behavioural difference, in that using activatenextupgrade will activate for all blocks after genesis, whereas the MTP activation would only trigger sometimes:

  • on the third block (second block after genesis) if only genesis block is old
  • activate never if all blocks are old (some tests use old blocks, such as the cached chain which is all old blocks that are relatively empty)

I think that's fine... and maybe even better, since some of our functional tests fail to leave IBD on the ancient chain.

It's worth pointing out there is a slight behavioural difference, in that using activatenextupgrade will activate for all blocks after genesis, whereas the MTP activation would only trigger sometimes:

  • on the third block (second block after genesis) if only genesis block is old
  • activate never if all blocks are old (some tests use old blocks, such as the cached chain which is all old blocks that are relatively empty)

I think that's fine... and maybe even better, since some of our functional tests fail to leave IBD on the ancient chain.

Actually I should mention there is a side effect of this that can happen:

Suppose new flag B requires old flag A that has already activated -- in VerifyScript it might have if (flags & B) assert(flags & A); like we see with CLEANSTACK and P2SH. If you do this -activatenextupgrade then it moves the activation of B back to genesis, before flag A activated. Then the assert will fail.

This Diff passes fine since BIP16Height=0 and magneticAnomalyHeight=0 on regtest, i.e., p2sh always on since genesis (which doesn't really count) and cleanstack turns on at height=1. Well before any script could be executed.

I am not aware of any planned future flags that would incur this problem -- at most you might have restrict flags that require P2SH in order to be strict soft forks (like cleanstack), but I cant imagine any other dependencies.

jasonbcox requested changes to this revision.Dec 17 2019, 19:57
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/consensus/activation.cpp
69

I think making this checking logic more complex as well as complicating the associated test isn't the right way to do this. Setting -activatenextupgrade should be a proxy for setting -phononactivationtime (or whatever the next upgrade activation argument is) to current time - 1 second. This way, the -activatenextupgrade check is out of the code path for the actual upgrade (which IMO should be as simple as possible).

This revision now requires changes to proceed.Dec 17 2019, 19:57

It's worth pointing out there is a slight behavioural difference, in that using activatenextupgrade will activate for all blocks after genesis, whereas the MTP activation would only trigger sometimes:

  • on the third block (second block after genesis) if only genesis block is old
  • activate never if all blocks are old (some tests use old blocks, such as the cached chain which is all old blocks that are relatively empty)

I think that's fine... and maybe even better, since some of our functional tests fail to leave IBD on the ancient chain.

Actually I should mention there is a side effect of this that can happen:

Suppose new flag B requires old flag A that has already activated -- in VerifyScript it might have if (flags & B) assert(flags & A); like we see with CLEANSTACK and P2SH. If you do this -activatenextupgrade then it moves the activation of B back to genesis, before flag A activated. Then the assert will fail.

This Diff passes fine since BIP16Height=0 and magneticAnomalyHeight=0 on regtest, i.e., p2sh always on since genesis (which doesn't really count) and cleanstack turns on at height=1. Well before any script could be executed.

I am not aware of any planned future flags that would incur this problem -- at most you might have restrict flags that require P2SH in order to be strict soft forks (like cleanstack), but I cant imagine any other dependencies.

(as discussed in meeting today, this is a pretty minor issue and I don't think it should discount this approach)

Fabien planned changes to this revision.Dec 19 2019, 16:27

I will put that diff aside for the moment and try to find a better solution.

src/consensus/activation.cpp
69

There are a few problems with this approach:

  • Setting -activatenextupgrade implies setting -phononactivationtime, so this should be done during init, making it difficult to test (it requires a functional test that starts the node with the parameter, and a new RPC or similar to check the activation status, which defeats the maintenance reduction purpose of this diff).
  • Related to the issue above, this does not work for the unit tests (since there is no node startup).
  • Related to @markblundeberg comments, using MTP can lead to untested code on tests that do not build enough blocks.