Wallet: Minimum output value depends on fee instead of minTxRelayFee #8356

NicolasDorier commented at 7:00 AM on July 18, 2016: contributor

When #6793 (bump of txMinRelayFee) was released, lots of transactions made by older wallet stopped being relayed (output occasionally below dust), causing trouble to users who suddenly thought that their transactions were not confirmed because of the 1MB limit.

This PR make sure that the wallet minimum value output is a function of fee instead of a function of txMinRelayFee, so any bump in the future would not have bad consequences.

It will also save some UTXO space by preventing the creation of output which would likely be uneconomical to redeem.

laanwj added the label Wallet on Jul 18, 2016

in src/wallet/wallet.cpp:None in 2467bd421c outdated

2096 | @@ -2097,6 +2097,9 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool ov
2097 |  bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, CAmount& nFeeRet,
2098 |                                  int& nChangePosInOut, std::string& strFailReason, const CCoinControl* coinControl, bool sign)
2099 |  {
2100 | +    CFeeRate minValueRate = mempool.estimateFee(6);

MarcoFalke commented at 7:24 AM on July 18, 2016:

Why 6?

sipa commented at 7:27 AM on July 18, 2016:

It's better to use the mempool relay fee here, I think. Estimatefee is about getting into a block and depends on the speed. The relay fee is just about being relayed.

NicolasDorier commented at 7:46 AM on July 18, 2016:

As explained, the problem with relay fee (txMinRelayFee right ?) is that when we bump it as we did on #6793, then old wallet starts to send unrelayable transaction which cause perturbations to users.

6 is arbitrary, the goal is to not create output which would cost more than their value to redeem in 6 block. I'm open to any other number, I just wanted it to be a bit higher than txMinRelayFee so we have space to increase this default later without impacting users.

MarcoFalke commented at 7:50 AM on July 18, 2016:

@NicolasDorier Please note that 6793 has been reverted in the meantime while adding the size limit to the mempool. What @sipa meant, is to use the minimum relay fee of your current mempool and not the minimum relay fee of the node (hardcoded) nor the expected transaction fee to confirm within 6 blocks.

NicolasDorier commented at 2:35 PM on July 18, 2016:

ohh I did not know that the mempool also had its own minimum relay fee.

MarcoFalke commented at 7:27 AM on July 18, 2016: member

I could imagine this will confuse users "Why can't I send this transaction? It has the same sized outputs as the transaction I send just 2 hours earlier..."

But thanks for working on this. @Xekyo is working on improving the coin selection so make sure to discuss any conflicts with his work.

NicolasDorier commented at 7:50 AM on July 18, 2016: contributor

@MarcoFalke oh that's cool, the coin selection code is very convoluted, I'm happy someone working on it.

I could imagine this will confuse users "Why can't I send this transaction? It has the same sized outputs as the transaction I send just 2 hours earlier..."

The code impact only the wallet, not the relay policy. Thus, for such thing to happen, you would need a sudden big fee variation AND not having enough satoshi for paying the small surplus. So this should be rather rare.

The bump of minTxRelayFee however, had long lasting impact until people upgraded their wallet (breadwallet was impacted, I reported the issue there long time ago, they also now use estimated fee for the min size of outputs)

morcos commented at 1:22 PM on July 18, 2016: member

I agree with @NicolasDorier that we need to do something to prevent what happened last time we raised the min relay fee.

I would suggest at least two things.

The actual isDust check for policy should be separated from the minRelayFee. We want to change this number as infrequently as possible. It doesn't need to change from it's current value right now I don't think, but we don't want it to accidentally change if people adjust minRelayFee.
The minimum output created by Bitcoin Core should be higher than the current isDust check. I would suggest using some maximum of say 2x or 4x the isDust check (at it's current level) and a check using a fee rate from estimatefee, as you do here, but I'd use estimateFee(25) at least, and consider a higher target when we have the ability to do so.

NicolasDorier commented at 2:39 PM on July 18, 2016: contributor

The actual isDust check for policy should be separated from the minRelayFee.

You mean changing the relay policy ? I think it should be eventually done, but in a separate PR. I remember trying to do it long time ago, and for some reason it was harder than I expected. (I don't remember why though)

I would suggest using some maximum of say 2x or 4x the isDust check @morcos the last bump was x5, which is why I think using estimateFee is a better idea. I am fine for estimateFee(25).

What about taking the max between estimateFee(25) and 4x txMinRelayFee ? Or maybe as sipa suggested, using the mempool relay fee instead of estimateFee. (I just learned such thing existed)

NicolasDorier commented at 2:44 PM on July 18, 2016: contributor

Ah and just remembered https://github.com/bitcoin/bitcoin/blob/d612837814020ae832499d18e6ee5eb919a87907/src/qt/coincontroldialog.cpp#L454 need to be modified as well...

Wallet: Minimum output value depends on fee instead of minTxRelayFee c5ec9ce334

NicolasDorier force-pushed on Jul 21, 2016

NicolasDorier commented at 9:51 AM on July 21, 2016: contributor

Updated:

Introduce CWallet::GetDustRate
Use of mempool.GetMinFee(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000) instead of estimateFee
CoinControlDialog::updateLabels also using CWallet::GetDustRate

morcos commented at 3:04 PM on July 21, 2016: member

@NicolasDorier What I meant by separating the min relay fee from isDust, is that I think the next time we adjust minRelayFee or if anyone adjusts it locally, it should not automatically change isDust. They should be separate limits. So if we do bump minRelayFee by 5x, isDust doesn't actually change at all, unless we intentionally change it. And we can intentionally make sure it stays below whatever the min output value we have used in the past couple releases is.

Using minimum mempool fee or fee estimation is a mistake. Both those things are completely volatile on a newly started node. And even estimatefee(25) can shoot up high temporarily under network load, but shouldn't cause you to adjust minimum output values unless its a more permanent shift. I think longer term more stable fee estimates will eventually be possible, but we don't have them now.

sipa commented at 3:09 PM on July 21, 2016: member

@morcos I disagree here. Even without any IsDust rule implemented on the network, wallets should (purely for their own sake) avoid creating change outputs whose value is so low that it is anywhere near close to uneconomical to spend. Sure, we don't have an accurate long-term prediction of fee rates on the network, but our best guess is close enough - even if it easily fluctuates by some constant factor.

NicolasDorier commented at 3:28 PM on July 21, 2016: contributor

@morcos I agree that minRelayFee should be completely separate from IsDust definition to prevent this problem. However, the best we have now is estimatefee and the mempool.minRelayFee.

Knowing whether an output is economical depends on 1. knowing when it will be spent, 2. how much the fees will be at this time. Both are unknown and can't be predicted in any way.

What I want to do in this PR though is mainly to prevent future ::minRelayFee default change of impacting too much relaying. Two nice side effect is smaller UTXO and wallets likely wasting less money.

I don't know how volatile is estimatefee if there is a smoother metric I'm all open. I don't know what is best between mempool's minRelayFee or estimatefee, what I know is that either of them are strictly better than only ::minRelayTxFee.

Would have liked to talk about that in dev's meeting, sadly it starts at 4am for my place :(

morcos commented at 3:41 PM on July 21, 2016: member

@NicolasDorier Sure or we could talk at a different time. All I'm suggesting is that I think a fixed value is better (at least for now). Use 5 satoshi per byte or something for GetDustRate (5 times the current minRelayFee but fixed)

NicolasDorier commented at 12:46 PM on July 26, 2016: contributor

I am closing it for now. It seems that coin selection prevent change below 0.01 BTC. To be sure about that I would need to test current behavior in more details, but I don't really have time for now. I may reopen after I tested the current behavior better.

NicolasDorier closed this on Jul 26, 2016

MarcoFalke locked this on Sep 8, 2021