Redefine Dust and add a discard_rate #10817

morcos · 2017-07-13T18:48:31Z

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

gmaxwell

ACK

morcos · 2017-07-17T10:02:35Z

Clean rebase now that #10706 was merged

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.

morcos · 2017-07-17T11:11:56Z

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

morcos · 2017-07-17T11:56:31Z

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 · 2017-07-17T12:55:44Z

utACK 28da790

ryanofsky

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

ryanofsky · 2017-07-17T14:50:57Z

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"));


In commit "Add a discard_rate"

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

ryanofsky · 2017-07-17T15:05:28Z

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


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.

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

gmaxwell

re-ACK

TheBlueMatt

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

TheBlueMatt · 2017-07-17T16:11:20Z

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 */);


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

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

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

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

morcos · 2017-07-17T17:42:07Z

Addressed comments

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

TheBlueMatt · 2017-07-17T19:51:12Z

utACK f4d00e6

achow101 · 2017-07-17T23:43:34Z

utACK f4d00e6

gmaxwell

re-ACK

ryanofsky

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

instagibbs · 2017-07-18T17:58:26Z

utACK with new safeguard logic

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

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 Signed-off-by: Pasta <pasta@dashboost.org>

d5526bd Wrap dumpwallet warning and note scripts aren't dumped (MeshCollider) 3711c6a Add wallet backup text to import*, add* and dumpwallet RPCs (MeshCollider) dbda874 [Wallet] always show help-line of wallet encryption calls (Jonas Schnelli) 20c269b Avoid opening copied wallet databases simultaneously (Russell Yanofsky) e411b70 [wallet] Fix leak in CDB constructor (João Barbosa) f15aeea Change getmininginfo errors field to warnings (Andrew Chow) c04390b Unify help text for GetWarnings output in get*info RPCs (random-zebra) 1d966ce Add warnings field to getblockchaininfo (Andrew Chow) ffcd781 [Trivial] Cleanup after MOVE-ONLY commits (random-zebra) e067235 MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp (random-zebra) e947eec MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp (random-zebra) 2188c3e Move some static functions out of wallet.h/cpp (random-zebra) f49acf7 [wallet] [moveonly] Move CAffectedKeysVisitor (random-zebra) 8bd979f [wallet] Specify wallet name in wallet loading errors (random-zebra) 900bbfa Reject invalid wallet files (João Barbosa) a1f4e2a Reject duplicate wallet filenames (random-zebra) ee52c2e Fix misleading "Method not found" multiwallet errors (Russell Yanofsky) ce35e1e [Qt] Use wallet 0 in rpc console if running with multiple wallets (random-zebra) 37089d1 [tests] Update wallet_multiwallet.py functional test (random-zebra) 3955ee9 [Doc] Update release notes (random-zebra) 4fd5913 [wallet] [rpc] Add listwallets RPC (John Newbery) 1525281 [wallet] [rpc] print wallet name in getwalletinfo (John Newbery) fdf5da0 [wallet] fix comment for CWallet::Verify() (John Newbery) cf4a90b Remove factor of 3 from definition of dust. (random-zebra) a1c56fd [Policy] Introduce -dustrelayfee (random-zebra) 9fb29cc [Doc] Update multiwallet release notes (random-zebra) 379255e [Tests][Trivial] Add wallet_multiwallet.py to test_runner (random-zebra) 808fbc3 [Bugfix] consider boolean value of -zapwallettxes ParameterInteraction (random-zebra) f9141f8 [QA] Add wallet_multiwallet.py functional test (John Newbery) 2e02006 Rename -usewallet to -rpcwallet (Alex Morcos) a64440b Select wallet based on the given endpoint (Jonas Schnelli) 5683a9c Complete previous commit by moving mn stuff out of libbitcoin_wallet (random-zebra) b0fe92f Fix test_pivx circular dependency issue (random-zebra) 6cb2b92 Add wallet endpoint support to bitcoin-cli (-usewallet) (Jonas Schnelli) 7dd3916 Register wallet endpoint (Jonas Schnelli) 5bd1bd7 Properly forbid -salvagewallet and -zapwallettxes for multi wallet. (Alex Morcos) 41a7335 Remove unused variables (practicalswift) 5c3d73f Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings (Russell Yanofsky) e7cafab [Refactoring] Mimic ListCoins for sapling notes (random-zebra) 54fa122 [qt] Move some WalletModel functions into CWallet (random-zebra) 494ba64 [test] Add test for getmemoryinfo (random-zebra) 2394083 [Wallet] unset change position when there is no change on exact match (Gregory Sanders) 7d977ac Remove unused C++ code not covered by unit tests (random-zebra) 60bb4da ApproximateBestSubset should take inputs by reference, not value (Ryan Havar) 3633d75 Initialize nRelockTime (Patrick Strateman) 3a599d0 [Refactor] Return safeTx boolean in CheckTXAvailability (random-zebra) f219be9 Add safe flag to listunspent result (NicolasDorier) 0201065 Add COutput::fSafe member for safe handling of unconfirmed outputs (random-zebra) 75c8c6d Disallow copy of CReserveKeys (Gregory Sanders) 8378322 [Refactor] Replace optional reserveKey in PBF with unique pointer (random-zebra) Pull request description: I think these are all the remaining Bitcoin Core v0.15 PRs in the wallet area that we don't have yet, and are useful to us. I've grouped them here since they are all pretty small, simple, and narrow-focused (on the wallet/rpcwallet files). This changeset is based on top of - [x] #2337 as it keeps going with the multiwallet implementation, adding: - RPC endpoint support - `listwallets` RPC - wallet name in `getwalletinfo` RPC - `wallet_multiwallet.py` functional test As in some areas we are much closer to upstream, some of the commits needed adaptations (especially the functional tests). A couple of commits have been extended to include shield-related code. List of upstream PRs backported/adapted: - bitcoin#9906 : Disallow copy constructor CReserveKeys - bitcoin#9830 : Add safe flag to listunspent result - bitcoin#9993 : Initialize nRelockTime - bitcoin#10108 : ApproximateBestSubset should take inputs by reference, not value - bitcoin#10075 : Remove unused C++ code not covered by unit tests - bitcoin#10257 : Add test for getmemoryinfo - bitcoin#10295 : Move some WalletModel functions into CWallet - bitcoin#10500 : Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings - bitcoin#10522 : Remove unused variables - bitcoin#10816 : Properly forbid -salvagewallet and -zapwallettxes for multi wallet - bitcoin#10849 : Multiwallet: simplest endpoint support - bitcoin#9380 : Separate different uses of minimum fees (only dustRelayFee) - bitcoin#10817 : Redefine Dust and add a discard_rate (only first commit) - bitcoin#10883 : Rename -usewallet to -rpcwallet - bitcoin#10604 : Add listwallets RPC, include wallet name in getwalletinfo + tests - bitcoin#10893 : Avoid running multiwallet.py twice - bitcoin#10870 : Use wallet 0 in rpc console if running with multiple wallets - bitcoin#10931 : Fix misleading "method not found" multiwallet errors - bitcoin#10885 : Reject invalid wallets - bitcoin#11022 : Basic keypool topup - bitcoin#10976 : [MOVEONLY] Move some static functions out of wallet.h/cpp - bitcoin#11310 : Test listwallets RPC - bitcoin#10858 : "errors" to getblockchaininfo + unify "errors" field in get*info RPCs - bitcoin#11492 : Fix leak in CDB constructor - bitcoin#11476 : Avoid opening copied wallet databases simultaneously - bitcoin#11590 : always show help-line of wallet encryption calls - bitcoin#11289 : Add wallet backup text to import* and add* RPCs ACKs for top commit: furszy: re-re-utACK d5526bd. Fuzzbawls: ACK d5526bd Tree-SHA512: 115c4916ee212539b0a1b2cb25783ddf6753f5376de739a084191e874ed8717fff2c7cd9d744e397891f14eaa459ef2f48d675168621ef7316e81758fa6559f2

fanquake added the Wallet label Jul 14, 2017

morcos force-pushed the discardmore branch 2 times, most recently from 6974f28 to 78ad0a3 Compare July 15, 2017 03:50

gmaxwell approved these changes Jul 17, 2017

View reviewed changes

morcos force-pushed the discardmore branch from 78ad0a3 to 6849695 Compare July 17, 2017 09:59

morcos force-pushed the discardmore branch from 6849695 to 28da790 Compare July 17, 2017 11:11

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

ryanofsky reviewed Jul 17, 2017

View reviewed changes

gmaxwell approved these changes Jul 17, 2017

View reviewed changes

laanwj added this to the 0.15.0 milestone Jul 17, 2017

TheBlueMatt reviewed Jul 17, 2017

View reviewed changes

Add a discard_rate

f4d00e6

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

morcos force-pushed the discardmore branch from 28da790 to f4d00e6 Compare July 17, 2017 17:41

gmaxwell approved these changes Jul 18, 2017

View reviewed changes

ryanofsky reviewed Jul 18, 2017

View reviewed changes

laanwj merged commit f4d00e6 into bitcoin:master Jul 19, 2017

maflcko mentioned this pull request Sep 5, 2017

Never create uneconomical output #7677

Closed

fastpok mentioned this pull request Mar 17, 2021

Prevent dust outputs hexresearch/ergvein#888

Closed

random-zebra mentioned this pull request Apr 28, 2021

[Wallet] Laggard wallet-related backports from btc 0.15 PIVX-Project/PIVX#2351

Merged

1 task

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Redefine Dust and add a discard_rate #10817

Redefine Dust and add a discard_rate #10817

Uh oh!

Conversation

morcos commented Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

morcos commented Jul 17, 2017

Uh oh!

morcos commented Jul 17, 2017

Uh oh!

morcos commented Jul 17, 2017

Uh oh!

instagibbs commented Jul 17, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

morcos Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

instagibbs Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

morcos Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

morcos commented Jul 17, 2017

Uh oh!

TheBlueMatt commented Jul 17, 2017

Uh oh!

achow101 commented Jul 17, 2017

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Jul 18, 2017

Uh oh!

Uh oh!

morcos commented Jul 13, 2017 •

edited

Loading