Page MenuHomePhabricator

Use a modified size for transaction minimum fee calculation.
ClosedPublic

Authored by schancel on May 6 2018, 05:07.

Details

Summary

Currently, we use the full transaction size for a minimum fee calculation.
This commit changes the behavior to charge for the additional (minimum) of
179 bytes it would take to spend any newly created outputs.

Test Plan
make check && ./test/functional/test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
feesize
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 2576
Build 3269: Bitcoin ABC Buildbot (legacy)
Build 3268: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jasonbcox requested changes to this revision.May 6 2018, 19:01
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/primitives/transaction.cpp
130 ↗(On Diff #3792)

Can we name this in such a way that it better describes that this size is part of the transaction rather than the fee? In a vacuum, this looks like the size of the fee value itself. How about CalculateBillableSize?

131 ↗(On Diff #3792)

Is it feasible to calculate this value at compile time rather than hard coding it? I'm noticing a lot of magic numbers in this part of the code base, and it's going to make writing improvements in the future really inflexible.

133 ↗(On Diff #3792)

reasonbly -> reasonably

src/validation.cpp
901 ↗(On Diff #3792)

similar to the above comment, something like feeSize -> billableSize. The meaning of feeSize gets lost further down, since readers cannot see that this value was calculated from the transaction size.

src/wallet/wallet.cpp
2924 ↗(On Diff #3792)

should this be billableBytes? Or even billableSize to be consistent?

This revision now requires changes to proceed.May 6 2018, 19:01
src/primitives/transaction.cpp
131 ↗(On Diff #3792)

Not particularly. It just happens to be the minimum script that's reasonably spent. I intend to get rid of more of them as I go through the code. The priority stuff above is going to get killed.

src/wallet/wallet.cpp
2924 ↗(On Diff #3792)

Sure, but this stuff is not my immediate concern. I want to know why the tests are broken and I'd like some help with that.

deadalnix added inline comments.
src/primitives/transaction.h
375 ↗(On Diff #3792)

please fix

This cause the node to crash with:

bitcoind: ../../src/amount.cpp:29: Amount CFeeRate::GetFee(size_t) const: Assertion `nBytes_ <= uint64_t(std::numeric_limits<int64_t>::max())' failed.

There should be unit tests for this.

Fix assertion problems. Still in WIP mode.

There should be unit tests for this.

I have no idea what "this" is in context here. For the assert blowing up?

src/primitives/transaction.h
375 ↗(On Diff #3792)

I will. This is a WIP, I want to know why the tests fail.

deadalnix requested changes to this revision.May 24 2018, 07:18
deadalnix added inline comments.
src/primitives/transaction.cpp
140

This is false. Input are at minima 36 for the prevout + 1 for an empty script + 4 for the sequence = 41 byte for an input. In addition, this function doesn't take a transaction as input, which makes it a poor candidate for something that has an exposed API.

Instead of making assumptions, it would be preferable to make unit tests.

src/primitives/transaction.h
343

I don't quite understand why this is a standalone function as it is never used as such.

376

This is screwed up.

This revision now requires changes to proceed.May 24 2018, 07:18

Rebase, fix, add unit test

Fix mistaken update of this diff. Thanks arc!

test/functional/mempool_limit.py
24 ↗(On Diff #3987)

These changes seem to be necessary for the test to pass, but I cannot figure out why the change would have any impact on the mempool eviction on line 47.

Make unit test more exhaustive

test/functional/mempool_limit.py
41 ↗(On Diff #3988)

Tracked down why this is necessary. The funding of the first txn is now significantly larger because of the additional effective size which fundrawtransaction takes into account. However, all the mempool eviction uses lowest total fee/kb. The first 3 transactions are now below that.

src/test/transaction_tests.cpp
777 ↗(On Diff #3988)

There should be some test case that takes a transaction and verify pricing.

src/test/transaction_tests.cpp
777 ↗(On Diff #3988)

I need you to be more specific. This sounds like an RPC test.

deadalnix requested changes to this revision.May 29 2018, 14:36
deadalnix added inline comments.
src/test/transaction_tests.cpp
777 ↗(On Diff #3988)

No, that sounds like you added code that has no unit test for it. You changed the CTransaction class and no tes is using the CTransaction class, so clearly, there is something that is not covered.

This revision now requires changes to proceed.May 29 2018, 14:36
src/test/transaction_tests.cpp
777 ↗(On Diff #3988)

There's no fees in CTransaction. So I don't know how you expect me to verify pricing there. That's what I was asking about.

Remove free function, update tests

This revision is now accepted and ready to land.Jun 4 2018, 17:35
This revision was automatically updated to reflect the committed changes.