Page MenuHomePhabricator

Reject headers building on invalid chains by tracking invalidity
Needs RevisionPublic

Authored by jasonbcox on Mon, Mar 4, 23:45.

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Project
Summary

This tracks the set of all known invalid-themselves blocks (ie
blocks which we attempted to connect but which were found to be
invalid). This is used to cheaply check if new headers build on an
invalid chain.

While we're at it we also resolve an edge-case in invalidateblock
on pruned nodes which results in them needing a reindex if they
fail to reorg.

Partial backport of Core PR11531 (commit 015a525)
https://github.com/bitcoin/bitcoin/pull/11531/commits/015a5258adffb0cf394f387a95ac9c8afc34cfc3
Completes T456

Test Plan

make check
test_runner.py --extended

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D2645
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 5153
Build 8369: Bitcoin ABC Teamcity Staging
Build 8368: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Mon, Mar 4, 23:45
Herald added a reviewer: Restricted Project. · View Herald TranscriptMon, Mar 4, 23:45
Herald added a subscriber: schancel. · View Herald Transcript
jasonbcox edited the summary of this revision. (Show Details)Mon, Mar 4, 23:47
jasonbcox edited the test plan for this revision. (Show Details)Tue, Mar 5, 00:12
deadalnix requested changes to this revision.Tue, Mar 5, 16:00
deadalnix added inline comments.
src/validation.cpp
3163 ↗(On Diff #7589)

This doesn't looks like it is correct. github broke the feature to extend collapsed code so I have no way to check, but in the PR, this is not in the same bracket as pindexBestInvalid = nullptr;

3821 ↗(On Diff #7589)

You don't need to pass true.

4430 ↗(On Diff #7589)

dito

test/functional/p2p_unrequested_blocks.py
366 ↗(On Diff #7589)

Why is that changed here?

This revision now requires changes to proceed.Tue, Mar 5, 16:00
jasonbcox added inline comments.Tue, Mar 5, 18:07
src/validation.cpp
3163 ↗(On Diff #7589)

This and related functions have been heavily reworked by us. I think this part is close to correct but there's something missing. UpdateFlags() needs to know if descendants/ancestors should be removed from g_failed_blocks. I've updated the diff to cover them. I removed this line since it's redundant.

Perhaps this deserves its own set of tests? It's not covered very well in p2p_unrequested_blocks.

test/functional/p2p_unrequested_blocks.py
366 ↗(On Diff #7589)

Did you read the comment that was removed? The node now gets banned and this checks for that. It was in the original PR, but since I broke it up for review, this line was commented out until this diff was applied.

jasonbcox updated this revision to Diff 7606.Tue, Mar 5, 18:08

bool cleanup + fixed clearing g_failed_blocks

deadalnix requested changes to this revision.Tue, Mar 5, 21:52
deadalnix added inline comments.
src/validation.cpp
3163 ↗(On Diff #7589)

If the previous code was incorrect, yet was passing tests, it tells us that there are test cases missing.

This revision now requires changes to proceed.Tue, Mar 5, 21:52