Page MenuHomePhabricator

Reject headers building on invalid chains by tracking invalidity
ClosedPublic

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

Details

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; 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
jasonbcox edited the summary of this revision. (Show Details)Mar 4 2019, 23:47
jasonbcox edited the test plan for this revision. (Show Details)Mar 5 2019, 00:12
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.

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

bool cleanup + fixed clearing g_failed_blocks

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
jasonbcox updated this revision to Diff 7844.Mar 26 2019, 23:42

Added functional test

Fabien accepted this revision.Apr 3 2019, 07:46
deadalnix requested changes to this revision.Apr 4 2019, 19:48

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.

jasonbcox updated this revision to Diff 7963.Apr 5 2019, 16:55

Split out test + fix into separate diff

jasonbcox edited the summary of this revision. (Show Details)Apr 5 2019, 16:58
This revision now requires changes to proceed.Apr 6 2019, 12:57
jasonbcox planned changes to this revision.Apr 8 2019, 20:11

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

jasonbcox requested review of this revision.Jun 24 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.
deadalnix accepted this revision.Jun 25 2019, 23:17
This revision is now accepted and ready to land.Jun 25 2019, 23:17