Details
- Reviewers
deadalnix markblundeberg - Group Reviewers
Restricted Project - Maniphest Tasks
- T653: Clean up past upgrades
- Commits
- rSTAGINGb6ef051607ca: Cleanup graviton activation
rABCb6ef051607ca: Cleanup graviton activation
Run IBD.
ninja check ./test/functional/test_runner.py abc-minimaldata-activation abc-schnorrmultisig-activation
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- graviton_remove_activation_tests
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 8588 Build 15164: Default Diff Build & Tests Build 15163: arc lint + arc unit
Event Timeline
It would be preferable to remove the tests along side the code that they do test. Doing this as this simply reduce test coverage.
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.
| src/chainparams.cpp | ||
|---|---|---|
| 127 | confirm 609135 is first with mediantime >= 1573819200 | |
| 298 | confirm 1341711 is first with mediantime >= 1573819200 | |
| src/consensus/params.h | ||
| 37 | 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: and IsGravitonEnabled(pindexPrev) -> IsGravitonEnabledForNextBlock(pindex) Anyway, fine to keep style for this diff, can be dealt with later. | |
| src/validation.cpp | ||
| 468 | Thanks, I missed removing this. | |
| src/consensus/params.h | ||
|---|---|---|
| 37 | 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 | Hehe oops, I guess this class name really doesn't matter... | |
| test/functional/abc-schnorrmultisig.py | ||
| 6 | hmm that's annoying, phab thinks this is renamed from minimaldata. as a workaround we can keep the file named -activation for now. | |
| test/functional/abc-schnorrmultisig.py | ||
|---|---|---|
| 6 | Meh, that's really annoying... I will keep the file names as suggested and update them in another diff to avoid this mess. | |
| 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)" |
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 !?!!??!? |
Update graviton activation tests instead or removing them.
Make the unit tests supported arguments a local variable.
| test/functional/abc-minimaldata-activation.py | ||
|---|---|---|
| 54 ↗ | (On Diff #15023) | I think you can remove this. |