Page MenuHomePhabricator

Merge #11773: [tests] Change feature_block.py to use BitcoinTestFramework
ClosedPublic

Authored by markblundeberg on Jul 2 2019, 06:34.

Details

Summary

PR11773 backport https://github.com/bitcoin/bitcoin/pull/11773/files
265d7c4 [tests] Improve assert message when wait_until() fails (John Newbery)
ebf053a [tests] Change feature_block.py to use BitcoinTestFramework (John Newbery)
fc02c12 [tests] Add logging to feature_block.py (John Newbery)
3898c4f [tests] Tidy up feature_block.py (John Newbery)
5cd01d2 [tests] Fix flake8 warnings in feature_block.py (John Newbery)

Pull request description:

Next step in #10603.

- first three commits tidy up feature_block.py
- fourth commit removes usage of ComparisonTestFramework

Longer term, it would be better to separate net_processing testing from validation testing, but I think this is still a useful PR, since it moves us away from the comparison test framework.
Test Plan

test_runner.py feature_block.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR11773
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6635
Build 11317: Bitcoin ABC Teamcity Staging
Build 11316: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Jul 2 2019, 06:34
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 2 2019, 06:34
markblundeberg planned changes to this revision.Jul 2 2019, 06:37

linter introduced syntax errors

markblundeberg updated this revision to Diff 9902.Jul 2 2019, 06:45

fix lint-induced syntax errors and remove lint-identified unused imports

This was, hands down, the most enjoyable backport ever.

(Now excuse me while I go cry myself to sleep...)

markblundeberg planned changes to this revision.Jul 2 2019, 19:29

Self review the next morning shows only minor issues. Pointing out some of the changes that happened due to CTOR and the padding rule below, and some minor things to change...

test/functional/feature_block.py
172 ↗(On Diff #9902)

Not clear to me why this was needed... (from D1907)
It has been taken out of the new next_block

175 ↗(On Diff #9902)

move these back

307 ↗(On Diff #9902)

(Note: we do not have the b24/b25 too-large block rejection here.)

545 ↗(On Diff #9902)

this is different from CTOR

655 ↗(On Diff #9902)

note our error message here differs...

658 ↗(On Diff #9902)

Note to self: remove this comment! Our test here is different in function.

The duplicate check we perform here is for CTOR blocks only. Core's duplicate check is just checking merkle malleation.

750 ↗(On Diff #9902)

Our tail repeat here is different from CTOR as well...

833 ↗(On Diff #9902)

linter did this ... I guess it's nice to have

890 ↗(On Diff #9902)

Note with our large block limit, this test unfortunately does not do exactly what we want. To be fully mimicking Core here, we should make a 32 MB block that goes over with non-canonical varint.

923 ↗(On Diff #9902)

(b66 test skipped here, since we don't have TTOR it doesn't apply)

931 ↗(On Diff #9902)

typo 'creted'

1009 ↗(On Diff #9902)

different from CTOR

1018 ↗(On Diff #9902)

(this is unnecessary, right?)

1063 ↗(On Diff #9902)

This is a fascinating and scary consensus rule... makes me hate sigops even more :-D

1203 ↗(On Diff #9902)

this vout_offset business comes from pad_tx adding an extra output during create_tx

Note to self: also backport #13048 after this

markblundeberg added inline comments.Jul 2 2019, 19:59
test/functional/feature_block.py
179 ↗(On Diff #9902)

This is also unnecessary. A block with only one non-coinbase transaction is always canonically ordered.

markblundeberg updated this revision to Diff 9928.Jul 2 2019, 20:14

minor changes

markblundeberg updated this revision to Diff 9930.Jul 2 2019, 20:27

just rebase

markblundeberg updated this revision to Diff 9935.Jul 2 2019, 20:54

rebase again :/

deadalnix accepted this revision.Jul 3 2019, 13:40
This revision is now accepted and ready to land.Jul 3 2019, 13:40