Skip to content

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

Closed
maflcko wants to merge 1 commit intobitcoin:masterfrom
maflcko:Mf1603-amountCeil
Closed

[amount] Extend GetFee() by optional flag ceil#7660
maflcko wants to merge 1 commit intobitcoin:masterfrom
maflcko:Mf1603-amountCeil

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Mar 9, 2016

  • 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
    d88af56 (Fee fixes #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
faccb18 
* 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
  d88af56
@maflcko maflcko mentioned this pull request Mar 9, 2016
Closed
@maflcko
Copy link
Member Author

maflcko commented Mar 9, 2016

@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
Copy link
Contributor

jtimon commented Mar 16, 2016

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 jtimon@dc1b96d 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.

@maflcko
Copy link
Member Author

maflcko commented Mar 16, 2016

Closing this per IRC discussion.

@maflcko maflcko closed this Mar 16, 2016
@maflcko maflcko deleted the Mf1603-amountCeil branch March 16, 2016 20:06
@jtimon jtimon mentioned this pull request Mar 21, 2016
Closed
1 task
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@maflcko @jtimon @jonasschnelli