[avalanche] Rewind Avalanche finalization if a finalized block is found to be invalid
ClosedPublic
Actions

Authored by sdulfari on Jan 25 2023, 16:51.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC2380eb646f87: [avalanche] Rewind Avalanche finalization if a finalized block is found to be…

Summary

It is not expected for an invalid block to ever finalize but if it occurs due to a bug the node can partially recover by reverting the finalization. This leaves the door open for future Avalanche polling to switch to the correct chain.

Padding was added to BadBlock() so that the transaction passes checks in AcceptBlock and is added to the block index. There is no change in behavior for the other tests that call BadBlock().

Test Plan

ninja check check-functional

Diff Detail

Repository

rABC Bitcoin ABC

Branch

revert-ava-finalization

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 21875
Build 43385: Build Diff	build-without-wallet · build-clang-tidy · build-diff · lint-circular-dependencies · build-clang · build-debug
Build 43384: arc lint + arc unit

Event Timeline

sdulfari created this revision.Jan 25 2023, 16:51

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 25 2023, 16:51

sdulfari requested review of this revision.Jan 25 2023, 16:51

Harbormaster completed remote builds in B21872: Diff 37728.Jan 25 2023, 17:04

sdulfari mentioned this in D13058: Remove 10-block rolling finalization.Jan 25 2023, 18:41

sdulfari added a child revision: D13058: Remove 10-block rolling finalization.Jan 25 2023, 18:41

Fabien requested changes to this revision.Jan 25 2023, 19:32

Fabien added a subscriber: Fabien.

Fabien added inline comments.

src/test/validation_block_tests.cpp
256 ↗	(On Diff #37728)	DummyConfig is enough, isn't it ?
274 ↗	(On Diff #37728)	Factor the Assert(m_node.chainman) out
281 ↗	(On Diff #37728)	Is that needed ?

This revision now requires changes to proceed.Jan 25 2023, 19:32

Refactor ChainstateManager

sdulfari added inline comments.Jan 25 2023, 23:05

src/test/validation_block_tests.cpp
256 ↗	(On Diff #37728)	Apparently not. The unit test framework has not been completely refactored to accept any config. See GetConfig() call in `test/util/setup_common.cpp`. I didn't pin down the root cause but there is something in the validation code that breaks. Also note the other tests in this file all use GlobalConfig so we have work to do.
281 ↗	(On Diff #37728)	Yes. There is a safety check in AvalancheFinalizeBlock() that ensures the block being finalized is in the chain.

Harbormaster completed remote builds in B21875: Diff 37735.Jan 25 2023, 23:15

Fabien accepted this revision.Jan 26 2023, 07:59

Fabien added inline comments.

src/test/validation_block_tests.cpp
281 ↗	(On Diff #37728)	OK, please add a comment to explain that so it's not lost, since this is not really part of the test setup it's a bit confusing.