[amount] Extend GetFee() by optional flag ceil #7660

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:Mf1603-amountCeil changing 4 files +69 −11

MarcoFalke commented at 3:23 PM on March 9, 2016: member

This flag causes rounding up to the next satoshi instead of just truncating (default behavior)
This reverts the hard-to-read and buggy code introduced in d88af560111863c3e9c1ae855dcc287f04dffb02 (#4465)

master happily produces the following results:

    feeRate = CFeeRate(123);
    BOOST_CHECK_EQUAL(feeRate.GetFee(0), 123);   // TODO: Should be 0
    BOOST_CHECK_EQUAL(feeRate.GetFee(8), 123);   // TODO: Should be 1
    BOOST_CHECK_EQUAL(feeRate.GetFee(9), 1);     // TODO: Should be 2
    BOOST_CHECK_EQUAL(feeRate.GetFee(121), 14);  // TODO: Should be 15
    BOOST_CHECK_EQUAL(feeRate.GetFee(122), 15);  // TODO: Should be 16
    BOOST_CHECK_EQUAL(feeRate.GetFee(999), 122); // TODO: Should be 123

[amount] Extend GetFee() by optional flag ceil

* This flag causes rounding up to the next satoshi instead of just
  truncating (default behavior)

* This reverts the hard-to-read and buggy code introduced in
  d88af560111863c3e9c1ae855dcc287f04dffb02

faccb1818c

jonasschnelli added the label Refactoring on Mar 9, 2016
MarcoFalke cross-referenced this on Mar 9, 2016 from issue [wallet] Round up to the next satoshi on odd fee rates by MarcoFalke
MarcoFalke commented at 5:49 PM on March 9, 2016: member

@morcos suggested an alternative would be to use std::ceil in all cases. This also changes the dust value by a small amount (fa03771), so I am not sure if this really is preferred?
jtimon commented at 4:47 PM on March 16, 2016: contributor

I don't know what is the expected usage, but wouldn't it be simpler to just add a CFeeRate::GetFeeSatoshis(size) method ? Maybe that would be too disruptive for the indended use, I don't really know.

EDIT: Although it may look unrelated, maybe this would also a good opportunity to move CFeeRate, CTxOut::GetDustThreshold() and CTxOut::IsDust() finally out of libconsensus. Something like https://github.com/jtimon/bitcoin/commit/dc1b96d368d471ea5242ca021148f9f900f69661 or #5114 but maybe not based on #6068, since #6068 seems extremely hard for reasons I still don't understand. I guess I just leave it there in case someone wants to rebase or rewrite such a thing. Sorry, I cannot help remembering every time I review changes in amount.o.
MarcoFalke commented at 8:05 PM on March 16, 2016: member

Closing this per IRC discussion.
MarcoFalke closed this on Mar 16, 2016
MarcoFalke deleted the branch on Mar 16, 2016
jtimon cross-referenced this on Mar 21, 2016 from issue Discussion: By "more precision", I don't mean using rational numbers in CFeeRate by jtimon
bitcoin locked this on Sep 8, 2021

Contributors

MarcoFalke

jtimon

Labels

Refactoring

Linked (view graph)

#4465 Fee fixes #5114 Policy: Move CTxOut::IsDust() to policy.o #6068 Policy: Create CPolicy interface and CStandardPolicy class implementing it #7661 [wallet] Round up to the next satoshi on odd fee rates #7731 Discussion: By "more precision", I don't mean using rational numbers in CFeeRate