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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Dec 5 2019, 13:13
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 5 2019, 13:13
Fabien edited the summary of this revision. (Show Details)Dec 6 2019, 06:54
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
markblundeberg added a comment.EditedDec 12 2019, 23:12

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)
Fabien updated this revision to Diff 14901.Dec 17 2019, 08:25

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

markblundeberg added inline comments.Dec 17 2019, 08:53
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.

Fabien added inline comments.Dec 17 2019, 09:09
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.

markblundeberg requested changes to this revision.Dec 17 2019, 11:23

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.

Fabien updated this revision to Diff 14906.Dec 17 2019, 12:52

Don't rename functional tests to avoid confusing Phabricator.

Fabien edited the test plan for this revision. (Show Details)Dec 17 2019, 12:56
markblundeberg added inline comments.Dec 17 2019, 20:41
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
Fabien updated this revision to Diff 15023.Dec 20 2019, 11:31

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

deadalnix accepted this revision.Dec 20 2019, 15:24
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
Fabien updated this revision to Diff 15044.Dec 20 2019, 19:18

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.