Details

Reviewers

deadalnix
Fabien

Group Reviewers

Restricted Project

Commits

rSTAGINGcc69e5760082: Reject headers building on invalid chains by tracking invalidity
rABCcc69e5760082: Reject headers building on invalid chains by tracking invalidity

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
Progress towards T456

Test Plan

make check
test_runner.py --extended

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jasonbcox created this revision.Mar 4 2019, 23:45

Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 4 2019, 23:45

Herald added a subscriber: schancel. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

jasonbcox edited the summary of this revision. (Show Details)Mar 4 2019, 23:47

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5139: Diff 7589.Mar 4 2019, 23:59

jasonbcox edited the test plan for this revision. (Show Details)Mar 5 2019, 00:12

jasonbcox mentioned this in D1977: Merge #11531: Check that new headers are not a descendant of an invalid block (more effeciently).Mar 5 2019, 00:36

deadalnix requested changes to this revision.Mar 5 2019, 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.Mar 5 2019, 16:00

jasonbcox added inline comments.Mar 5 2019, 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.

bool cleanup + fixed clearing g_failed_blocks

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Harbormaster completed remote builds in B5153: Diff 7606.Mar 5 2019, 18:23

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

deadalnix requested changes to this revision.Mar 5 2019, 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.Mar 5 2019, 21:52

Added functional test

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Harbormaster completed remote builds in B5313: Diff 7844.Mar 26 2019, 23:57

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Fabien accepted this revision.Apr 3 2019, 07:46

The backports looks good, however the next test+bugfix really belongs in its own diff.

src/validation.cpp
3823 ↗	(On Diff #7844)	There are variation of that logic in many places in that file. It's a miracle there is no CVE related to this yet...
test/functional/abc-invalid-chains.py
145 ↗	(On Diff #7844)	That+the fix that goes with it should be its own patch.

This revision now requires changes to proceed.Apr 4 2019, 19:48

The backports looks good, however the next test+bugfix really belongs in its own diff.

If I separate them, I'm not landing this diff until the test is approved, just FYI. I think the risk is too high to do otherwise.

Split out test + fix into separate diff

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

jasonbcox added a child revision: D2777: Add functional tests for rejecting headers that build on invalid chains.Apr 5 2019, 16:57

jasonbcox edited the summary of this revision. (Show Details)Apr 5 2019, 16:58

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5392: Diff 7963.Apr 5 2019, 17:12

jasonbcox mentioned this in D2777: Add functional tests for rejecting headers that build on invalid chains.Apr 5 2019, 20:44

See https://reviews.bitcoinabc.org/D2777#65449

This revision now requires changes to proceed.Apr 6 2019, 12:57

D2777 doesn't adequately provide test coverage for this change. I will need to continue investigating to see why this is the case.

schancel added a child revision: D1972: [Part 5 of 5] Add a CChainState class to clarify internal interfaces.Apr 9 2019, 03:24

jasonbcox removed a child revision: D2777: Add functional tests for rejecting headers that build on invalid chains.Apr 9 2019, 16:30

jasonbcox mentioned this in D2818: Remove direct bitcoin calls from qt/clientmodel.cpp.Apr 17 2019, 18:28

Re-ran some tests and I believe the current state of this patch is adequately tested:

p2p_unrequested_blocks test #9 tests a specific case that triggers the new codepath in AcceptBlockHeader(). As long as this test case continues to be maintained, we don't need duplicate test coverage of this case in abc-invalid-chains.
Reverting the bugfix here: https://reviews.bitcoinabc.org/D2645?vs=7589&id=7606#toc fails the new tests introduced in D2777 (abc-invalid-chains). So while D2777 doesn't explicitly provide additional test coverage of the intended behavior for this patch, it *does* provide test coverage against unintended behavior.