Redefine Dust and add a discard

morcos commented at 6:48 PM on July 13, 2017: member

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 label Wallet on Jul 14, 2017

morcos force-pushed on Jul 14, 2017

morcos force-pushed on Jul 15, 2017

gmaxwell approved

gmaxwell commented at 6:43 AM on July 17, 2017: contributor

ACK

morcos force-pushed on Jul 17, 2017

morcos commented at 10:02 AM on July 17, 2017: member

Clean rebase now that #10706 was merged

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.

b1385852ef

morcos force-pushed on Jul 17, 2017

morcos renamed this:
~~Add a discard_rate to avoid small change~~
Redefine Dust and add a discard_rate
on Jul 17, 2017

morcos commented at 11:11 AM on July 17, 2017: member

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

morcos commented at 11:56 AM on July 17, 2017: member

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 commented at 12:55 PM on July 17, 2017: member

utACK https://github.com/bitcoin/bitcoin/pull/10817/commits/28da790417d0a4d544988e12d5ce9033b0b97f87

in src/wallet/wallet.cpp:4140 in 28da790417 outdated

4132 | +        CAmount nFeePerK = 0;
4133 | +        if (!ParseMoney(GetArg("-discardfee", ""), nFeePerK))
4134 | +            return InitError(strprintf(_("Invalid amount for -discardfee=<amount>: '%s'"), GetArg("-discardfee", "")));
4135 | +        if (nFeePerK > HIGH_TX_FEE_PER_KB)
4136 | +            InitWarning(AmountHighWarn("-discardfee") + " " +
4137 | +                        _("This is the transaction fee you may discard if change is smaller than dust at this level"));

ryanofsky commented at 2:50 PM on July 17, 2017:

In commit "Add a discard_rate"

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

in src/wallet/wallet.cpp:2513 in 28da790417 outdated

2502 | @@ -2501,6 +2503,15 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
2503 |      return true;
2504 |  }
2505 |  
2506 | +static CFeeRate GetDiscardRate(const CBlockPolicyEstimator& estimator)
2507 | +{
2508 | +    unsigned int highest_target = estimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE);
2509 | +    CFeeRate discard_rate = estimator.estimateSmartFee(highest_target, nullptr /* FeeCalculation */, false /* conservative */);
2510 | +    discard_rate = std::min(discard_rate, CWallet::m_discard_rate);
2511 | +    discard_rate = std::max(discard_rate, ::dustRelayFee);

ryanofsky commented at 3:05 PM on July 17, 2017:

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 commented at 5:42 PM on July 17, 2017:

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

ryanofsky commented at 3:08 PM on July 17, 2017: member

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

gmaxwell approved

gmaxwell commented at 3:08 PM on July 17, 2017: contributor

re-ACK

laanwj added this to the milestone 0.15.0 on Jul 17, 2017

in src/wallet/wallet.cpp:2509 in 28da790417 outdated

2502 | @@ -2501,6 +2503,15 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
2503 |      return true;
2504 |  }
2505 |  
2506 | +static CFeeRate GetDiscardRate(const CBlockPolicyEstimator& estimator)
2507 | +{
2508 | +    unsigned int highest_target = estimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE);
2509 | +    CFeeRate discard_rate = estimator.estimateSmartFee(highest_target, nullptr /* FeeCalculation */, false /* conservative */);

TheBlueMatt commented at 4:11 PM on July 17, 2017:

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

instagibbs commented at 4:35 PM on July 17, 2017:

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

morcos commented at 5:21 PM on July 17, 2017:

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

TheBlueMatt commented at 4:11 PM on July 17, 2017: member

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).

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).

f4d00e63f7

morcos force-pushed on Jul 17, 2017

morcos commented at 5:42 PM on July 17, 2017: member

Addressed comments

(discard.ver2) -> f4d00e6 (discard.ver2.squash)

TheBlueMatt commented at 7:51 PM on July 17, 2017: member

utACK f4d00e63f7951469c840edede3f675d07249f62a

achow101 commented at 11:43 PM on July 17, 2017: member

utACK f4d00e63f7951469c840edede3f675d07249f62a

gmaxwell approved

gmaxwell commented at 5:13 PM on July 18, 2017: contributor

re-ACK

ryanofsky commented at 5:56 PM on July 18, 2017: member

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

instagibbs commented at 5:58 PM on July 18, 2017: member

utACK with new safeguard logic

laanwj merged this on Jul 19, 2017

laanwj closed this on Jul 19, 2017

laanwj referenced this in commit 9022aa3722 on Jul 19, 2017

PastaPastaPasta referenced this in commit ee18c4be02 on Sep 8, 2019

PastaPastaPasta referenced this in commit 1d5ec5d693 on Sep 8, 2019

PastaPastaPasta referenced this in commit 5f7280da1d on Sep 8, 2019

PastaPastaPasta referenced this in commit ec366d2595 on Sep 8, 2019

barrystyle referenced this in commit 6a2ddb62b3 on Jan 22, 2020

random-zebra referenced this in commit 2d50b6e3b9 on Jun 9, 2021

DrahtBot locked this on Sep 8, 2021

Redefine Dust and add a discard_rate #10817