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.
Details
Details
- Reviewers
jasonbcox deadalnix - Group Reviewers
Restricted Project - Commits
- rSTAGINGb983ebdcaba0: Use a modified size for transaction minimum fee calculation.
rABCb983ebdcaba0: Use a modified size for transaction minimum fee calculation.
make check && ./test/functional/test_runner.py
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- feesize
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 2650 Build 3414: Bitcoin ABC Buildbot (legacy) Build 3413: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| 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? |
| 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. |
| src/primitives/transaction.h | ||
|---|---|---|
| 375 ↗ | (On Diff #3792) | please fix |
Comment Actions
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.
Comment Actions
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. |
| src/primitives/transaction.cpp | ||
|---|---|---|
| 140 ↗ | (On Diff #3872) | 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 ↗ | (On Diff #3872) | I don't quite understand why this is a standalone function as it is never used as such. |
| 376 ↗ | (On Diff #3872) | This is screwed up. |
| 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. |
| 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. |
| 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. |
| 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. |