/bitcoin

Redefine Dust and add a discard_rate #10817

Merged
merged 2 commits into from Jul 19, 2017
+46 −14

Conversation

Reviewers

@TheBlueMatt TheBlueMatt

@instagibbs instagibbs

@ryanofsky ryanofsky

@gmaxwell gmaxwell

Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
  0.15.0
8 participants
@morcos @instagibbs @TheBlueMatt @achow101 @gmaxwell @ryanofsky @fanquake @laanwj
@morcos
Contributor

morcos commented Jul 13, 2017 • edited

The definition of dust is redefined to remove the factor of 3.

Dust is redefined to be the value of an output such that it would
cost that value in fees to (create and) spend the output at the dust
relay rate. The previous definition was that it would cost 1/3 of the
value. The default dust relay rate is correspondingly increased to
3000 sat/kB so the actual default dust output value of 546 satoshis
for a non-segwit output remains unchanged. This commit is a refactor
only unless a dustrelayfee is passed on the commandline in which case
that number now needs to be increased by a factor of 3 to get the same
behavior. -dustrelayfee is a hidden command line option.

Note: It's not exactly a refactor due to edge case changes in rounding
as evidenced by the required change to the unit test.

A discard_rate is added which defaults to 10,000 sat/kB

Any change output which would be dust at the discard_rate you are
willing to discard completely and add to fee (as well as continuing to
pay the fee that would have been needed for creating the change)

This would be a nice addition for 0.15 and I think will remain useful for 0.16 with the new coin selection algorithms in discussion, but its not crucial.

It does add translation strings, but we could (should?) avoid that by hiding the option

fanquake added the Wallet label Jul 14, 2017

@gmaxwell

gmaxwell approved these changes Jul 17, 2017
View changes

ACK

@morcos
Contributor

morcos commented Jul 17, 2017

Clean rebase now that #10706 was merged
@morcos morcos Remove factor of 3 from definition of dust. 
This redefines dust to be the value of an output such that it would
cost that value in fees to (create and) spend the output at the dust
relay rate.  The previous definition was that it would cost 1/3 of the
value.  The default dust relay rate is correspondingly increased to
3000 sat/kB so the actual default dust output value of 546 satoshis
for a non-segwit output remains unchanged.  This commit is a refactor
only unless a dustrelayfee is passed on the commandline in which case
that number now needs to be increased by a factor of 3 to get the same
behavior.  -dustrelayfee is a hidden command line option.

Note: It's not exactly a refactor due to edge case changes in rounding
as evidenced by the required change to the unit test.
 b138585

morcos changed the title from Add a discard_rate to avoid small change to Redefine Dust and add a discard_rate Jul 17, 2017

@morcos
Contributor

morcos commented Jul 17, 2017

This PR has been changed to redefine dust and move the discard rate slightly lower. See new PR description.
@morcos
Contributor

morcos commented Jul 17, 2017

After discussion with @gmaxwell, just wanted to point out that the edge case rounding issues would only be with non-standard dust rates. Since the default dust rate was 1000 sat/kB and the division in question was by 1000, then there are no rounding issues regardless of the output in question.
@instagibbs
Member

instagibbs commented Jul 17, 2017

utACK 28da790
@ryanofsky

ryanofsky reviewed Jul 17, 2017
View changes

utACK 28da790. Much less confusing without the factor of 3.

src/wallet/wallet.cpp
+ unsigned int highest_target = estimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE);
+ CFeeRate discard_rate = estimator.estimateSmartFee(highest_target, nullptr /* FeeCalculation */, false /* conservative */);
+ discard_rate = std::min(discard_rate, CWallet::m_discard_rate);
+ discard_rate = std::max(discard_rate, ::dustRelayFee);
@ryanofsky

ryanofsky Jul 17, 2017

Contributor

In commit "Add a discard_rate"

Maybe it should be an init error to specify -dustrelayfee greater than -discardrate, because the -discardrate value is ignored in this case.

@morcos

morcos Jul 17, 2017

Contributor

I just informed in the option help rather than erroring..

src/wallet/wallet.cpp
+ return InitError(strprintf(_("Invalid amount for -discardfee=<amount>: '%s'"), GetArg("-discardfee", "")));
+ if (nFeePerK > HIGH_TX_FEE_PER_KB)
+ InitWarning(AmountHighWarn("-discardfee") + " " +
+ _("This is the transaction fee you may discard if change is smaller than dust at this level"));
@ryanofsky

ryanofsky Jul 17, 2017

Contributor

In commit "Add a discard_rate"

Pedantic, but maybe s/transaction fee/maximum transaction fee/

@gmaxwell

gmaxwell approved these changes Jul 17, 2017
View changes

re-ACK

laanwj added this to the 0.15.0 milestone Jul 17, 2017

@TheBlueMatt

TheBlueMatt reviewed Jul 17, 2017
View changes

This is awesome, though it would be nice to document the limits for discardfee (must be at least dust relay fee, will not go higher than estimate fee).

src/wallet/wallet.cpp
+static CFeeRate GetDiscardRate(const CBlockPolicyEstimator& estimator)
+{
+ unsigned int highest_target = estimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE);
+ CFeeRate discard_rate = estimator.estimateSmartFee(highest_target, nullptr /* FeeCalculation */, false /* conservative */);
@TheBlueMatt

TheBlueMatt Jul 17, 2017

Contributor

Dont you need to check that this wasnt an error return (ie 0)?

@instagibbs

instagibbs Jul 17, 2017

Member

i.e., failure to get a real estimate means we'll go to dustRelayFee instead of static discard rate

@morcos

morcos Jul 17, 2017

Contributor

Yes I suppose that would be a slight improvement. Stupid 0 as error value

@morcos morcos Add a discard_rate 
Any change output which would be dust at the discard_rate you are
willing to discard completely and add to fee (as well as continuing to
pay the fee that would have been needed for creating the change).
f4d00e6
@morcos
Contributor

morcos commented Jul 17, 2017

Addressed comments

(discard.ver2) -> f4d00e6 (discard.ver2.squash)
@TheBlueMatt
Contributor

TheBlueMatt commented Jul 17, 2017

utACK f4d00e6
@achow101
Contributor

achow101 commented Jul 17, 2017

utACK f4d00e6
@gmaxwell

gmaxwell approved these changes Jul 18, 2017
View changes

re-ACK

@ryanofsky

ryanofsky reviewed Jul 18, 2017
View changes

utACK f4d00e6. Has new safeguard against failing fee estimate & more comments and documentation.

@instagibbs
Member

instagibbs commented Jul 18, 2017

utACK with new safeguard logic

@laanwj laanwj merged commit f4d00e6 into bitcoin:master Jul 19, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@laanwj laanwj added a commit that referenced this pull request Jul 19, 2017

@laanwj laanwj Merge #10817: Redefine Dust and add a discard_rate 
f4d00e6 Add a discard_rate (Alex Morcos)
b138585 Remove factor of 3 from definition of dust. (Alex Morcos)

Pull request description:

  The definition of dust is redefined to remove the factor of 3.

  Dust is redefined to be the value of an output such that it would
  cost that value in fees to (create and) spend the output at the dust
  relay rate.  The previous definition was that it would cost 1/3 of the
  value.  The default dust relay rate is correspondingly increased to
  3000 sat/kB so the actual default dust output value of 546 satoshis
  for a non-segwit output remains unchanged.  This commit is a refactor
  only unless a dustrelayfee is passed on the commandline in which case
  that number now needs to be increased by a factor of 3 to get the same
  behavior.  -dustrelayfee is a hidden command line option.

  Note: It's not exactly a refactor due to edge case changes in rounding
  as evidenced by the required change to the unit test.

  A discard_rate is added which defaults to 10,000 sat/kB

  Any change output which would be dust at the discard_rate you are
  willing to discard completely and add to fee (as well as continuing to
  pay the fee that would have been needed for creating the change)

  This would be a nice addition for 0.15 and I think will remain useful for 0.16 with the new coin selection algorithms in discussion, but its not crucial.

  It does add translation strings, but we could (should?) avoid that by hiding the option

Tree-SHA512: 5b6f655354d0ab6b8b6cac1e8d1fe3136d10beb15c6d948fb15bfb105155a9d03684c6240624039b3eed6428b7e60e54216cc8b2f90c4600701e39f646284a9b
This commit was signed with a verified signature.
@laanwj laanwj Wladimir J. van der Laan
GPG key ID: 1E4AED62986CD25D Learn about signing commits
9022aa3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment