Page MenuHomePhabricator

Added abc-sync-chain test to ensure node doesn't seize while syncing many blocks
ClosedPublic

Authored by jasonbcox on Dec 13 2018, 00:18.

Details

Summary

As part of investigating test flakiness in abc-parkedchain.py, I noticed a
severe lack of test coverage for nodes syncing many out-of-order blocks, both in and
out of IBD. This test provides that explicit coverage so that further development
on parking blocks does not cause nodes to periodically seize while syncing.

Test Plan

Run multiple times:
for i in {1..100}; do ./test_runner.py abc-sync-chain; done

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.Dec 13 2018, 00:18
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 13 2018, 00:18
Herald added a subscriber: schancel. · View Herald Transcript
Fabien requested changes to this revision.Dec 13 2018, 14:55
Fabien added inline comments.
test/functional/abc-sync-chain.py
12 ↗(On Diff #6334)

Not required

15 ↗(On Diff #6334)

Could you whether remove the parenthesis or use it everywhere when there is no line break involved ?

43 ↗(On Diff #6334)

I would advise you to use the new format() method, like this:
"-minimumchainwork={:#x}".format(202 + 2 * NUM_IBD_BLOCKS)
which is intended to replace the % string format

44 ↗(On Diff #6334)

Shouldn't it be 200 + NUM_IBD_BLOCKS to have half the blocks during IBD and half the blocks outside of IBD ?

51 ↗(On Diff #6334)

This can be simplified:
node0conn.add_connection(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], node0conn))

81 ↗(On Diff #6334)

blocks[len(blocks) - 1] => blocks[-1]

This revision now requires changes to proceed.Dec 13 2018, 14:55
jasonbcox added inline comments.Dec 21 2018, 22:22
test/functional/abc-sync-chain.py
44 ↗(On Diff #6334)

I ran the test with NUM_IBD_BLOCKS set at 50 and got 404 chainwork at the end of the test. From this, I figured the chainwork per block was 2.

jasonbcox updated this revision to Diff 6405.Dec 21 2018, 22:47
jasonbcox marked 5 inline comments as done.

Updated according to feedback

Fabien accepted this revision.Dec 23 2018, 14:10
This revision is now accepted and ready to land.Dec 23 2018, 14:10
This revision was automatically updated to reflect the committed changes.