Page MenuHomePhabricator

Fix parent<->child mixup in UnwindBlock
ClosedPublic

Authored by markblundeberg on Jan 22 2020, 14:37.

Details

Summary

D4929 faithfully backported this line from Core, however the original
code has this part backwards.

In this if-statement, the idea is to check that the tip has not
changed from previous loop iteration. If so then actually
to_mark_failed_or_parked is the parent block of invalid_walk_tip,
not the other way around.

The code as-is would end up marking all disconnected blocks as fully
failed (or parked) where the intention was to mark all but the lowest
as failedparent (parkedparent). This doesn't change much in practice since
the reconsiderblock / unparkblock will clear all children anyway, and
the automatic unparking also doesn't care whether things are parked or
parkedparent.

A side note:
This whole function is in fact a bit crazy now as it's attempting a very
slippery task by trying to invalidate a chain while the tip might be
changing on every iteration, due to other threads. If the tip actually
does end up changing for some reason, then the function doesn't really
handle it that well anyway. Backporting 16849
(to ensure tip doesn't change, via cs_chainstate lock) will bring back
some sanity by preventing this slippery thing.

Test Plan
ninja check-all

As mentioned in summary, there is little noticeable behaviour change
so I don't have a test for this, at least not now.

Diff Detail

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

Event Timeline

src/validation.cpp
3093 ↗(On Diff #15734)

see here ^ where to_mark_failed_or_parked is set on the previous iter.

markblundeberg added a child revision: Restricted Differential Revision.Jan 22 2020, 14:44

If that was a bug, a test would really be beneficial.

This revision is now accepted and ready to land.Jan 22 2020, 15:39