[wallet] feebumper: discard change outputs below discard rate #12749

1. 
   instagibbs commented at 7:35 pm on March 21, 2018: member

   The “discard rate” is the concept we use to ensure the wallet isnt creating not so useful just-above-relay dust.

   Outside of bumpfee previous to this PR, and manually creating such an output, the wallet will never make change outputs of that size, preferring to send them to fees instead.

   “Worst case” for the user is that users pay a slightly higher feerate than they were expecting, which is already a possibility with relay dust.

2. feebumper: discard change outputs below discard rate 5805d6fead
3. instagibbs renamed this:
   feebumper: discard change outputs below discard rate
   [wallet] feebumper: discard change outputs below discard rate
   on Mar 21, 2018
4. 
   morcos commented at 7:39 pm on March 21, 2018: member

   concept ACK
5. 
   achow101 commented at 7:40 pm on March 21, 2018: member

   utACK 5805d6fead8d54ec14bfc89991d692bd75940d56
6. meshcollider added the label Wallet on Mar 21, 2018
7. fanquake requested review from sdaftuar on Mar 21, 2018
8. 
   Xekyo commented at 4:32 pm on March 27, 2018: member

   As far as I understand, the discard rate is always as big as or bigger than the dustRelayFee. Since it appears that the discard rate is here being used for its eponymous purpose, this seems like a correct change to me.

   utACK 5805d6fead8d54ec14bfc89991d692bd75940d56

9. in src/wallet/feebumper.cpp:197 in 5805d6fead outdated

   193@@ -194,7 +194,7 @@ Result CreateTransaction(const CWallet* wallet, const uint256& txid, const CCoin
   194 
   195     // If the output would become dust, discard it (converting the dust to fee)
   196     poutput->nValue -= nDelta;
   197-    if (poutput->nValue <= GetDustThreshold(*poutput, ::dustRelayFee)) {
   198+    if (poutput->nValue <= GetDustThreshold(*poutput, GetDiscardRate(::feeEstimator))) {
   

   ---

   ---

   

   sdaftuar commented at 6:22 pm on March 28, 2018:

   micro-nit: it looks like we use IsDust() in most(?) places in the code, and use GetDustThreshold() directly in just a couple. Perhaps we should use IsDust() here instead (which apparently uses < rather than <=, so this would help us be consistent).
10. sdaftuar approved
11. 
    sdaftuar commented at 6:46 pm on March 28, 2018: member

    Would be nice to update the wallet_bumpfee test as well (there is already a test case that if the change output would be dust, that it would be discarded).

    I noticed in my review that there seems to be a bug in the dust calculation, now that our wallet supports segwit. GetDustThreshold() esssentially assumes that outputs are either native P2WPKH or P2PKH, which is not the case for our wallet, which might produce change outputs that are P2SH-P2WPKH. If I calculated correctly, I think this would cause us to potentially throw away an extra ~530 satoshis or so (at the 10sat/byte discard rate) above what the software intends.

    I think fixing this can be done in a separate PR, but figured this might be helpful to point out and note if you decide to update the bumpfee test in this PR.

    utACK

12. adapt bumpfee change discard test to be more strict and add note on p2sh discrep f526046ef5
13. 
    instagibbs commented at 7:39 pm on March 28, 2018: member

    After some discussion on how changing internals of GetDustThreshold changes relay policy, I’ve decided to adapt the test a bit to be tighter and give explanation, and do further work after more discussion.
14. 
    sdaftuar commented at 7:42 pm on March 28, 2018: member

    re-utACK, thanks for updating the test.
15. 
    laanwj commented at 5:31 pm on April 10, 2018: member

    utACK f526046ef5d8300eab6f112f56f59fd3b7e11d33
16. laanwj merged this on Apr 10, 2018
17. laanwj closed this on Apr 10, 2018

    ---

18. laanwj referenced this in commit a84b056d5f on Apr 10, 2018
19. MarcoFalke locked this on Sep 8, 2021

Contributors
instagibbs morcos achow101 Xekyo sdaftuar laanwj

Labels
Wallet