Page MenuHomePhabricator

Add functional tests for rejecting headers that build on invalid chains
ClosedPublic

Authored by jasonbcox on Apr 5 2019, 16:57.

Details

Summary

See title.

Co-authored-by: Jason B. Cox <contact@jasonbcox.com>
Co-authored-by: Nico Guiton <nico@bitframe.org>

Test Plan
make check
test_runner.py abc-invalid-chains

Diff Detail

Repository
rABC Bitcoin ABC
Branch
rejheaders-test
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5397
Build 8856: Bitcoin ABC Buildbot (legacy)
Build 8855: arc lint + arc unit

Event Timeline

To double-check my work, I tested the test by itself on master and realized it passes. Digging around, I found this: https://github.com/bitcoin/bitcoin/pull/13930 which made it more clear which case I was missing.

jasonbcox edited the test plan for this revision. (Show Details)

Added missing test case

Also, tested this on master and all test cases pass (expectation was that it would fail without D2645 applied). I suspect this is because our block
invalidation code (which was almost completely refactored) is more complete than Core's was at this stage.
Regardless, this should provide us with proper test coverage going forward.

deadalnix requested changes to this revision.Apr 6 2019, 12:56

The test case do not seem to be corresponding to the modified code (which is an assertion failing).

In D2645 , you state that there is a bug fix related to that test case, and that this is why the code isn't like in the backport but it is still unclear what the bug is, what fixes it and if the test is actually testing anything.

It shouldn't be difficult to have the bug fix coming with the test as one diff, or have an explanation as to why this isn't possible in the diff.

In the current form, there are no way to review this reliably.

This revision now requires changes to proceed.Apr 6 2019, 12:56

Removed assert which appears to be useless now.

jasonbcox edited the summary of this revision. (Show Details)

The test case do not seem to be corresponding to the modified code (which is an assertion failing).

In D2645 , you state that there is a bug fix related to that test case, and that this is why the code isn't like in the backport but it is still unclear what the bug is, what fixes it and if the test is actually testing anything.

It shouldn't be difficult to have the bug fix coming with the test as one diff, or have an explanation as to why this isn't possible in the diff.

In the current form, there are no way to review this reliably.

I've revisited this diff so that it's not intended to be test coverage for D2645, but rather a precursor to writing tests for it. Please review it as such.

Is that possible to remove the dependency from that test so that it test current behavior, then add the change required to the test to test the new behavior in D2645 ? This way it is very apparent what behavior is changed.

Rebase and test without D2645 as a dependency

This revision is now accepted and ready to land.Apr 9 2019, 16:40
This revision was automatically updated to reflect the committed changes.