Page MenuHomePhabricator

[qa] change bip68 sequence to use calculate_fee
ClosedPublic

Authored by schancel on Jul 12 2018, 18:16.

Details

Summary

As per title

Test Plan
./test/functional/test_runner.py bip68-sequence

Diff Detail

Repository
rABC Bitcoin ABC
Branch
test-fees
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 2861
Build 3826: Bitcoin ABC Buildbot (legacy)
Build 3825: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Jul 12 2018, 23:26
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
test/functional/bip68-sequence.py
228 ↗(On Diff #4292)

Because CSV is height based, you just introduced an off by one error in all the subsequent tests.

This revision now requires changes to proceed.Jul 12 2018, 23:26

Remove extraneous log message

Something is still not right here. Trying to get to the bottom of this.

Okay, so the issue is the unsigned transaction is significantly smaller than the signed version. Adding a heuristic to get it to an equivalent or larger billable size when unsigned.

schancel reclaimed this revision.
jasonbcox requested changes to this revision.Jul 18 2018, 18:00
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
test/functional/bip68-sequence.py
238 ↗(On Diff #4351)

To help unfuck this slowly but surely, please init a variable so we can give this 10 a name (not sure what to call it... some sort of error margin or scale factor?). and use that variable in the other 3 instances below as well. This allows us to tweak the refactor later and gives more context where otherwise none exists.

This revision now requires changes to proceed.Jul 18 2018, 18:00
deadalnix requested changes to this revision.Jul 18 2018, 18:53
deadalnix added inline comments.
test/functional/bip68-sequence.py
228 ↗(On Diff #4351)

This is still breakign the test, as CSV is height based.

jasonbcox requested changes to this revision.Jul 18 2018, 20:38
jasonbcox added inline comments.
test/functional/bip68-sequence.py
242 ↗(On Diff #4352)

Use fee_multiplier here

261 ↗(On Diff #4352)

Use fee_multiplier here

282 ↗(On Diff #4352)

Use -fee_multiplier here

test/functional/test_framework/mininode.py
413 ↗(On Diff #4352)

Duplicate comment. Should read something like "Add actual size for vouts"

This revision now requires changes to proceed.Jul 18 2018, 20:38
test/functional/test_framework/mininode.py
405 ↗(On Diff #4352)

Looks like this might have been merged from another change? please take a look

Fix double fee_multiplier

Remove billable size function

deadalnix added inline comments.
test/functional/bip68-sequence.py
242 ↗(On Diff #4419)

It's kind of weird to initialize the value to 0 and then reinitialize it to something else. Why not compute the value, store it in a local variable and then construct the CTxOut ?

This revision is now accepted and ready to land.Jul 26 2018, 12:28
test/functional/bip68-sequence.py
242 ↗(On Diff #4419)

Because the existence of a vout changes the calculation of the fee.

This revision was automatically updated to reflect the committed changes.