Page MenuHomePhabricator

Prevent auto-finalization from moving backward the finalization point
ClosedPublic

Authored by Fabien on Fri, Nov 30, 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Fri, Nov 30, 15:39
Herald added a reviewer: Restricted Project. · View Herald TranscriptFri, Nov 30, 15:39
Herald added a subscriber: schancel. · View Herald Transcript
Fabien updated this revision to Diff 6201.Fri, Nov 30, 17:11

Update functional test to be in accordance with the new behavior

deadalnix requested changes to this revision.Fri, Nov 30, 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.Fri, Nov 30, 18:18
deadalnix added inline comments.Fri, Nov 30, 18:20
src/validation.cpp
2325 ↗(On Diff #6201)

Incidentally, this is what IsBlockFinalized already does.

Fabien updated this revision to Diff 6204.Fri, Nov 30, 20:30

Update as per review

Fabien updated this revision to Diff 6206.Fri, Nov 30, 20:45

Rebase

Fabien updated this revision to Diff 6231.Sun, Dec 2, 21:26

Rebase

deadalnix requested changes to this revision.Sun, Dec 2, 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.Sun, Dec 2, 21:32
Fabien updated this revision to Diff 6232.Sun, Dec 2, 21:39

Update as per review

deadalnix accepted this revision.Sun, Dec 2, 22:20
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.Sun, Dec 2, 22:20
Fabien marked an inline comment as done.Sun, Dec 2, 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)

Fabien updated this revision to Diff 6234.Sun, Dec 2, 22:28

Add an assertion

Fabien updated this revision to Diff 6278.Tue, Dec 4, 16:46

Rebase

This revision was automatically updated to reflect the committed changes.