Page MenuHomePhabricator

[ibd.sh] Add chainwork sanity check
AbandonedPublic

Authored by jasonbcox on Jun 28 2019, 00:23.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

This provides an additional guarantee that IBD reached its target chainwork.
These sorts of sanity checks are crucial for debugging since IBD takes a long time to complete.

In the future, we may want a more robust mechanism for fetching nMinimumChainWork, but fetching it from the logs will do for now.

Depends on D3452

Test Plan

IBD on TeamCity

Diff Detail

Repository
rABC Bitcoin ABC
Branch
ibd-chainparams
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6610
Build 11267: Bitcoin ABC Buildbot (legacy)
Build 11266: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Jun 28 2019, 00:23
jasonbcox planned changes to this revision.Jun 28 2019, 00:23
jasonbcox updated this revision to Diff 9774.Jun 28 2019, 16:06

Added missing datadir to bitcoin-cli calls

jasonbcox planned changes to this revision.Jun 28 2019, 16:06
jasonbcox requested review of this revision.Jun 29 2019, 15:30
deadalnix requested changes to this revision.Jun 30 2019, 00:00

The test plan doesn't test the feature. For all we know this could fail to extract the proper value for chain work and just copare to zero or something and you wouldn't know it.

This revision now requires changes to proceed.Jun 30 2019, 00:00
jasonbcox updated this revision to Diff 9892.Jul 1 2019, 23:39
  • Fixed a bug where chainparams wouldn't compare as expected.
  • Make the success of this check visually inspectable.
deadalnix requested changes to this revision.Jul 2 2019, 01:37
deadalnix added inline comments.
contrib/teamcity/ibd.sh
46

That is exactly why you need a dedicated header for this. You know you do, you even moved in that direction in another diff. So why this?

60

I'm not sure what this is supposed to achieve. Shouldn't this be a test in the software to make sure IBD is never marked as done if we haven't reached the minimum chain work?

If it is already tested, then this is redundant, and if it isn't then why test it here rather than there?

This revision now requires changes to proceed.Jul 2 2019, 01:37
jasonbcox abandoned this revision.Oct 1 2019, 20:25