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
Branch
arcpatch-D2645
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5153
Build 8369: Bitcoin ABC Buildbot (legacy)
Build 8368: arc lint + arc unit

Event Timeline

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

Why is that changed here?

This revision now requires changes to proceed.Mar 5 2019, 16:00
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

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

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
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.

Split out test + fix into separate diff

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.

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.
This revision is now accepted and ready to land.Jun 25 2019, 23:17