Page MenuHomePhabricator

Fixed padding and tx ordering in bip68-sequence.py post-fork
ClosedPublic

Authored by jasonbcox on Oct 22 2018, 23:35.

Details

Summary

Continues T418
Depends on D1974

Co-authored-by: Dagur Valberg Johannsson <dagurval@pvv.ntnu.no>

Test Plan

./test_runner.py bip68-sequence

Diff Detail

Repository
rABC Bitcoin ABC
Branch
pad-bip68
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3666
Build 5407: Bitcoin ABC Buildbot (legacy)
Build 5406: arc lint + arc unit

Event Timeline

schancel requested changes to this revision.Oct 23 2018, 17:14
schancel added inline comments.
test/functional/bip68-sequence.py
392 ↗(On Diff #5509)

Something is seriously not right here. Why is sendtoaddress generating a TxID that is not valid?

This revision now requires changes to proceed.Oct 23 2018, 17:14
jasonbcox marked an inline comment as done.

Removed bad pad_tx call that could mask real errors in the future

test/functional/bip68-sequence.py
392 ↗(On Diff #5509)

Good catch. I added pad_tx() as a precaution rather than it failing the test. Since you brought this up, I realize that it can actually mask real errors and should *not* be applied. I've removed it.

deadalnix requested changes to this revision.Oct 23 2018, 22:57
deadalnix added inline comments.
test/functional/bip68-sequence.py
30

It doesn't seems like a great idea to stop testing for current behavior and only check for post fork behavior before the fork.

This revision now requires changes to proceed.Oct 23 2018, 22:57
test/functional/bip68-sequence.py
30

Is it preferable to duplicate this test and delete the pre-fork version after the fork?

test/functional/bip68-sequence.py
30

You can pass flags through to the test. See in test_runner.py

TEST_PARAMS = {
    # Some test can be run with additional parameters.
    # When a test is listed here, the it  will be run without parameters
    # as well as with additional parameters listed here.
    # This:
    #    example "testName" : [["--param1", "--param2"] , ["--param3"]]
    # will run the test 3 times:
    #    testName
    #    testName --param1 --param2
    #    testname --param3
    "txn_doublespend.py": [["--mineblock"]],
    "txn_clone.py": [["--mineblock"]]
}

Then use the flags to pass through to the daemon if running in --enable-magnetic-anomaly or something.

It might be worthwhile to make this pass through to all tests and pass through to the daemon? We could do this directly in the test framework.

jasonbcox marked an inline comment as done.

Test both before and after fork activation, with minimal changes and no duplicate tests

deadalnix requested changes to this revision.Oct 25 2018, 23:55
deadalnix added inline comments.
test/functional/bip68-sequence.py
35 ↗(On Diff #5540)

Maybe only set the activation time when the parameter is specified ?

test/functional/test_runner.py
77 ↗(On Diff #5540)

This should test pretty much the same as 0, which is the default.

This revision now requires changes to proceed.Oct 25 2018, 23:55
jasonbcox added inline comments.
test/functional/test_runner.py
77 ↗(On Diff #5540)

Fixed in the latest revision.

jasonbcox marked an inline comment as done.

Only set activation time when it's specified. Lo and behold it revealed a bug in the test, which is now fixed.

schancel added inline comments.
test/functional/bip68-sequence.py
28 ↗(On Diff #5569)

type="long", default=0

39 ↗(On Diff #5569)

"-magneticanomalyactivationtime={}".format(activation_time)

Fixed according to feedback

This revision is now accepted and ready to land.Nov 4 2018, 18:36

Rebase and update to argparse according to changes introduced in D2010

This revision was automatically updated to reflect the committed changes.