Page MenuHomePhabricator

Cleanup graviton activation
ClosedPublic

Authored by Fabien on Dec 5 2019, 13:13.

Details

Summary

Move the graviton activation to use a block height and convert the
activation tests to be feature tests.

Depends on D4782.

Progress towards T653: Convert activation tests to feature tests.

Test Plan

Run IBD.

ninja check
./test/functional/test_runner.py abc-minimaldata-activation abc-schnorrmultisig-activation

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

deadalnix requested changes to this revision.Dec 6 2019, 11:06

It would be preferable to remove the tests along side the code that they do test. Doing this as this simply reduce test coverage.

This revision now requires changes to proceed.Dec 6 2019, 11:06

Presumably the next step after this is to set graviton activation to height=0 in regtest? It could be combined with this diff, since setting height=0 necessarily will force activation to be untestable. But I think separate diffs is good.

I think for these already-activated upgrades, it is totally fine to remove all activation functional tests, and rely on: 1) internal c++ tests that probe old flag combinations, 2) feature functional tests that are post-upgrade only, and finally 3) use IBD to catch any changes in old behaviour that were (for some reason) not covered in script tests.

It would be preferable to remove the tests along side the code that they do test. Doing this as this simply reduce test coverage.

I can't imagine any future diff that would be a better place to remove this code.

The only thing that will change is permanent insertion into mandatory flags (D4679 and D4673), which are definitely not the right places to remove the test of activation since we want to observe how the tx rejections change before upgrade. Nothing else will change about these features, they are now locked in.

Fabien retitled this revision from Cleanup graviton activation tests to Cleanup graviton activation.Dec 17 2019, 08:23
Fabien edited the summary of this revision. (Show Details)
Fabien edited the test plan for this revision. (Show Details)

Rebase on top of D4743.
Switch graviton activation to block height.

src/chainparams.cpp
127 ↗(On Diff #14901)

confirm 609135 is first with mediantime >= 1573819200

298 ↗(On Diff #14901)

confirm 1341711 is first with mediantime >= 1573819200

src/consensus/params.h
37 ↗(On Diff #14901)

I wonder if these comments might be confusing. To be clear, the first graviton-rules block was 609136, not 609135. All our BCH style activations are really "activates after", and this is at odds with the convention used for pre-BCH upgrades BIP16Height, BIP66Height, BIP65Height, CSVHeight -- those heights refer to the first new-rules block.

i.e., it might be better if it read as:
"Block height after which the graviton rules becomes active"

and

IsGravitonEnabled(pindexPrev) -> IsGravitonEnabledForNextBlock(pindex)

Anyway, fine to keep style for this diff, can be dealt with later.

src/validation.cpp
468 ↗(On Diff #14901)

Thanks, I missed removing this.

src/consensus/params.h
37 ↗(On Diff #14901)

I agree with you that this is confusing, I just copied the previous style here without thinking it more.

After each upgrade one should remember that there is an off-by-one to take into account, better naming would be helpful here; I'm all for changing this is in a later diff.

Please rename the file that is confusing phab, just for easier review for now. Everything other than that one looks good.

test/functional/abc-minimaldata.py
58 ↗(On Diff #14901)

Hehe oops, I guess this class name really doesn't matter...

test/functional/abc-schnorrmultisig.py
6 ↗(On Diff #14901)

hmm that's annoying, phab thinks this is renamed from minimaldata.

as a workaround we can keep the file named -activation for now.

This revision now requires changes to proceed.Dec 17 2019, 11:23
Fabien planned changes to this revision.Dec 17 2019, 12:16
Fabien added inline comments.
test/functional/abc-schnorrmultisig.py
6 ↗(On Diff #14901)

Meh, that's really annoying... I will keep the file names as suggested and update them in another diff to avoid this mess.

Don't rename functional tests to avoid confusing Phabricator.

src/validation.cpp
829 ↗(On Diff #14906)

Ah btw this should read something like "valid according to standard flags but invalid according to old consensus rules (during IBD)"

deadalnix requested changes to this revision.Dec 17 2019, 23:35

I think this went a bit sideways. The scope of the patch expanded, yet the problem wasn't really addressed. There is test for activation that is removed, yet the activation is still taking place (using height rather than timestamp, but that's not relevant to the fact that it happens or not).

So this is still reducing the test coverage of logic that still exist.

I should see some if (IsGravitonEnabled(...)) removed from somewhere to see these tests removed. Without it, then this is just removing test coverage from one side of the branch.

src/test/test_bitcoin_main.cpp
34 ↗(On Diff #14906)

lol, why is that a global? I won't request change for this because this is clearly out of scope, but seriously, WTF !?!!??!?

This revision now requires changes to proceed.Dec 17 2019, 23:35

Update graviton activation tests instead or removing them.
Make the unit tests supported arguments a local variable.

deadalnix added inline comments.
test/functional/abc-minimaldata-activation.py
54 ↗(On Diff #15023)

I think you can remove this.

This revision is now accepted and ready to land.Dec 20 2019, 15:24

Remove extra_args = [[]] which is already the default.

This revision was landed with ongoing or failed builds.Dec 20 2019, 19:21
This revision was automatically updated to reflect the committed changes.