ninja check ./test/functional/test_runner.py abc-minimaldata-activation abc-schnorrmultisig-activation
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.
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.
|127 ↗||(On Diff #14901)|
confirm 609135 is first with mediantime >= 1573819200
|298 ↗||(On Diff #14901)|
confirm 1341711 is first with mediantime >= 1573819200
|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:
IsGravitonEnabled(pindexPrev) -> IsGravitonEnabledForNextBlock(pindex)
Anyway, fine to keep style for this diff, can be dealt with later.
|468 ↗||(On Diff #14901)|
Thanks, I missed removing this.
|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.
|58 ↗||(On Diff #14901)|
Hehe oops, I guess this class name really doesn't matter...
|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.
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.
|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 !?!!??!?