Page MenuHomePhabricator
Differential D1561

[qa] change bip68 sequence to use calculate_fee
ClosedPublic
Actions

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

Details

Reviewers
deadalnix
jasonbcox
Group Reviewers
Restricted Project
Commits
rSTAGING1ab9230f5c56: [qa] change bip68 sequence to use calculate_fee
rABC1ab9230f5c56: [qa] change bip68 sequence to use calculate_fee
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 2904
Build 3912: Bitcoin ABC Buildbot (legacy)
Build 3911: arc lint + arc unit

Event Timeline

schancel created this revision.Jul 12 2018, 18:16
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 12 2018, 18:16
teamcity added a subscriber: teamcity.Jul 12 2018, 18:18

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Jul 12 2018, 18:59

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B2858: Diff 4290!Jul 12 2018, 19:00
schancel updated this revision to Diff 4292.Jul 12 2018, 22:18

Finally done

teamcity added a comment.Jul 12 2018, 22:21

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Jul 12 2018, 23:02

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B2860: Diff 4292.Jul 12 2018, 23:02
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
schancel updated this revision to Diff 4293.Jul 12 2018, 23:40

Remove extraneous log message

teamcity added a comment.Jul 12 2018, 23:42

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Jul 13 2018, 00:22

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B2861: Diff 4293.Jul 13 2018, 00:22
schancel planned changes to this revision.Jul 13 2018, 03:13

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

schancel added a comment.Jul 18 2018, 01:11

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 abandoned this revision.Jul 18 2018, 16:31
schancel reclaimed this revision.
schancel updated this revision to Diff 4351.Jul 18 2018, 17:11

Fix fee heuristics

teamcity added a comment.Jul 18 2018, 17:11

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

schancel added a child revision: D1486: Use a modified size for transaction minimum fee calculation..Jul 18 2018, 17:13
teamcity added a comment.Jul 18 2018, 17:50

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B2901: Diff 4351.Jul 18 2018, 17:50
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.

schancel updated this revision to Diff 4352.Jul 18 2018, 19:13

Address feedback

teamcity added a comment.Jul 18 2018, 19:15

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Jul 18 2018, 19:52

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B2902: Diff 4352.Jul 18 2018, 19:52
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
jasonbcox added inline comments.Jul 18 2018, 20:39
test/functional/test_framework/mininode.py
405 ↗(On Diff #4352)

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

schancel updated this revision to Diff 4354.Jul 18 2018, 20:59

Address feedback

teamcity added a comment.Jul 18 2018, 21:01

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

jasonbcox accepted this revision.Jul 18 2018, 21:40
teamcity added a comment.Jul 18 2018, 21:41

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B2904: Diff 4354.Jul 18 2018, 21:41
schancel updated this revision to Diff 4357.Jul 18 2018, 22:14

fuuuuuu

teamcity added a comment.Jul 18 2018, 22:14

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Jul 18 2018, 22:51

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B2907: Diff 4357.Jul 18 2018, 22:52
schancel added a parent revision: D1581: Update CTxMemPoolEntry and BlockAssembler to billing cutoffs.Jul 19 2018, 20:27
schancel removed a parent revision: D1581: Update CTxMemPoolEntry and BlockAssembler to billing cutoffs.
schancel updated this revision to Diff 4364.Jul 20 2018, 20:02

Fix double fee_multiplier

teamcity added a comment.Jul 20 2018, 20:04

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Jul 20 2018, 20:46

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B2913: Diff 4364!Jul 20 2018, 20:46
jasonbcox mentioned this in D1582: [qa] Use proper fee accounting in mempool_limit.Jul 20 2018, 20:59
schancel updated this revision to Diff 4419.Jul 25 2018, 18:41

Remove billable size function

teamcity added a comment.Jul 25 2018, 18:50

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Jul 25 2018, 19:34

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B2956: Diff 4419.Jul 25 2018, 19:34
deadalnix accepted this revision.Jul 26 2018, 12:28
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
schancel added inline comments.Jul 26 2018, 19:33
test/functional/bip68-sequence.py
242 ↗(On Diff #4419)

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

Closed by commit rABC1ab9230f5c56: [qa] change bip68 sequence to use calculate_fee (authored by schancel). · Explain WhyJul 26 2018, 19:37
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
test/
functional/
18 lines
test_framework/
12 lines

Diff 4354

View Options

test/functional/bip68-sequence.py

Loading...
View Options

test/functional/test_framework/mininode.py

Loading...