Page MenuHomePhabricator

Prevent auto-finalization from moving backward the finalization point
ClosedPublic

Authored by Fabien on Nov 30 2018, 15:39.

Details

Summary

This fix an issue where the auto-finalization behaviour could cause
unexpected rewind of the finalization chain by an amount of
maxreorgdepth blocks.

Depends on D2130

Test Plan
./test/functional/abc-finalize-block.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
fix_finalization_block
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4267
Build 6599: Bitcoin ABC Buildbot (legacy)
Build 6598: arc lint + arc unit

Event Timeline

Update functional test to be in accordance with the new behavior

deadalnix requested changes to this revision.Nov 30 2018, 18:18
deadalnix added inline comments.
src/validation.cpp
2325 ↗(On Diff #6201)
if (pindexFinalized && pindexFinalized ->GetAncestor(pindex->nHeight) == pindex) {
  // The block is already finalized.
  return true;
}

// We have a new block to finalize.
pindexFinalized = pindex;
retunr true;
test/functional/abc-finalize-block.py
47 ↗(On Diff #6201)

It would be preferable to designate this block by something that actually identify it. There is something special about this block, and it is not that it is at height 209.

124 ↗(On Diff #6201)

I commented on these tests a million times. They do not check anything very interesting. The node could be on any branch the test would pass, but if we add another test case that shift the hight, then we got to modify everything in the test.

Please test what's relevant.

158 ↗(On Diff #6201)

dito

181 ↗(On Diff #6201)

Reformat so it fit on one line.

212 ↗(On Diff #6201)

dito

This revision now requires changes to proceed.Nov 30 2018, 18:18
src/validation.cpp
2325 ↗(On Diff #6201)

Incidentally, this is what IsBlockFinalized already does.

deadalnix requested changes to this revision.Dec 2 2018, 21:32

A few small changes, but LGTM.

test/functional/abc-finalize-block.py
125 ↗(On Diff #6231)

This check is redundant.

149 ↗(On Diff #6231)
assert_equal(alt_node.getfinalizedblockhash(), fork_block)
This revision now requires changes to proceed.Dec 2 2018, 21:32
deadalnix added inline comments.
test/functional/abc-finalize-block.py
171 ↗(On Diff #6232)

Maybe change this into

assert_equal(node.getfinalizedblockhash(), tip)

instead of removing it.

This revision is now accepted and ready to land.Dec 2 2018, 22:20
Fabien marked an inline comment as done.Dec 2 2018, 22:22
Fabien added inline comments.
test/functional/abc-finalize-block.py
171 ↗(On Diff #6232)

It has been removed because it is a duplicate, already tested at line 162 (164 in new version)

This revision was automatically updated to reflect the committed changes.