Add a generic flag to activate the next upgrade
Changes PlannedPublic
Actions

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 Passed

Unit

No Test Coverage

Build Status

Buildable 8585
Build 15158: Default Diff Build & Tests
Build 15157: arc lint + arc unit

Event Timeline

Fabien created this revision.Dec 17 2019, 07:44

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 17 2019, 07:44

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8585: Diff 14898.Dec 17 2019, 08:02

Fabien added a child revision: D4642: Cleanup graviton activation.Dec 17 2019, 08:23

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.

Fabien edited the test plan for this revision. (Show Details)Dec 17 2019, 09:19

In D4743#114268, @markblundeberg wrote:

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

In D4743#114323, @markblundeberg wrote:

In D4743#114268, @markblundeberg wrote:

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.