Details

Reviewers

deadalnix
Fabien

Group Reviewers

Restricted Project

Commits

rABC40d52cfef267: Add functional tests for rejecting headers that build on invalid chains

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 5417
Build 8896: Bitcoin ABC Buildbot (legacy)
Build 8895: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Apr 5 2019, 16:57

Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 5 2019, 16:57

Herald added a subscriber: schancel. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5393: Diff 7964.Apr 5 2019, 17:13

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)Apr 5 2019, 17:26

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.

jasonbcox edited the test plan for this revision. (Show Details)Apr 5 2019, 20:44

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5397: Diff 7970.Apr 5 2019, 20:59

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)Apr 8 2019, 20:09

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

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

In D2777#65449, @deadalnix wrote:

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.

jasonbcox mentioned this in D2645: Reject headers building on invalid chains by tracking invalidity.Apr 8 2019, 20:11

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5417: Diff 8001.Apr 8 2019, 20:23

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.