Details

Reviewers

jasonbcox
deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGINGcd3c01418d9e: Use ceiling division for fees when funding transactions
rABCcd3c01418d9e: Use ceiling division for fees when funding transactions

Summary

Add a ceiling option to CFeeRate::GetFee. This ensure that
when calculating minimum fees, or funding transactions, we
can take the ceiling of division, instead of the floor.

This avoids potentially funding transactions below the minRelayFee.

Depends on D1670

Test Plan

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

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

schancel created this revision.Aug 20 2018, 21:17

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 20 2018, 21:17

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

jasonbcox requested changes to this revision.Aug 20 2018, 21:24

jasonbcox added a subscriber: jasonbcox.

jasonbcox added inline comments.

src/amount.cpp
27 ↗	(On Diff #4605)	Conceptually, I don't like the idea of splitting the implementation so that some use ceiling and others use floor, but I guess it makes sense to use different calculations when determining the exact fee rate for a given tx vs the minimum acceptable fee rate. If there's an alternate calculation that removes this split-path, I would prefer that.
39 ↗	(On Diff #4605)	Needs tests.

This revision now requires changes to proceed.Aug 20 2018, 21:24

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

Harbormaster completed remote builds in B3096: Diff 4605.Aug 20 2018, 21:56

Add tests like a good programmer

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

jasonbcox requested changes to this revision.Aug 21 2018, 17:59

jasonbcox added inline comments.

src/test/amount_tests.cpp
150 ↗	(On Diff #4616)	The test below this doesn't appear to truncate anything unless I'm missing something.
152 ↗	(On Diff #4616)	This comment doesn't appear to be true. Copy-paste?