Page MenuHomePhabricator

Fix bug in UnwindBlock that could unpark or unfail a parked or failed chain
ClosedPublic

Authored by jasonbcox on Oct 29 2018, 23:18.

Details

Summary

Discovered while working on backports

Test Plan

test_runner.py abc-parkedchain
Fails without the fix and passes with it.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

deadalnix requested changes to this revision.Oct 29 2018, 23:36
deadalnix added inline comments.
src/validation.cpp
2731 ↗(On Diff #5575)

You don't need to pass true. It would also make sense to use a ternary, inline, without creating a whole sub function.

2746 ↗(On Diff #5575)

This is not correct, even thought it end up having the same result. It should be withFailedParent/.withParkedParent .

test/functional/abc-parkedchain.py
19 ↗(On Diff #5575)

Please do not number the tests, because it ends up making reordering/removing a test in the middle more tedious than needed.

48 ↗(On Diff #5575)

This needs to go to the final step of reconsidering the invalid chain, as well as other possible interleavings of:

  • park
  • invalidate
  • unpark
  • reconsider

There should be 4.

This revision now requires changes to proceed.Oct 29 2018, 23:36

Fixed everything according to feedback

deadalnix requested changes to this revision.Oct 30 2018, 00:06
deadalnix added inline comments.
test/functional/abc-parkedchain.py
47 ↗(On Diff #5581)

check the tip between the two

49 ↗(On Diff #5581)

You'd benefit also checking what happen after reconsiderblock here.

64 ↗(On Diff #5581)

And check unpark.

84 ↗(On Diff #5581)

This is redunddant witht he test cases above is requested changes are applied. However, these cases are missing:

  • invalidate
  • park
  • reconsider
  • unpark

and

  • park
  • invalidate
  • unpark
  • reconsider
This revision now requires changes to proceed.Oct 30 2018, 00:06
jasonbcox marked 4 inline comments as done.

More test cases + test cleanup

This revision is now accepted and ready to land.Oct 30 2018, 00:49
This revision was automatically updated to reflect the committed changes.