Enforce minRelayTxFee on wallet created tx and add a maxtxfee option. #5485

gmaxwell commented at 9:47 AM on December 16, 2014: contributor

Previously the minRelayTxFee was only enforced on user specified values.

It was possible for smartfee to produce a fee below minRelayTxFee which would just result in the transaction getting stuck because it can't be relayed.

This also introduces a maxtxfee option which sets an absolute maximum for any fee created by the wallet, with an intention of increasing user confidence that the automatic fees won't burn them. This was frequently a concern even before smartfees.

If the configured fee policy won't even allow the wallet to meet the relay fee the transaction creation may be aborted.

laanwj added the label Wallet on Dec 16, 2014

laanwj added this to the milestone 0.10.0 on Dec 16, 2014

gmaxwell commented at 9:49 AM on December 16, 2014: contributor

Because of the non-enforcement of minRelayTxFee this patch is somewhat bigger than I wanted.

I've not tested this yet and I will do so, but wanted to get it up for discussion unless there is something I've missed where we don't need to enforce the relay fee minimum anymore.

Presumably if smartfee is working it'll always be higher than that in any case, but I wouldn't want to be in a case where all bitcoin core wallets suddenly start producing transactions that won't relay after some unusual blocks.

morcos commented at 4:41 PM on December 16, 2014: member

I reviewed this code and it looks good to me. I was wondering if you saw #5202 as there is already some incarnation of maximum fees, I think this is better than the mess that already exists and maybe we can get rid of some of the old ones now.

One slight negative of enforcing the minRelayTxFee is this precludes the option to take your chances on relaying, but boost your chances of being included in a block by offering a very small fee. In practice that probably works right now, but I don't see it as much of a loss.

gmaxwell commented at 7:10 PM on December 16, 2014: contributor

@morcos Thanks for the review. I'll go through 5202, I saw that it was there... I'd been not following the fees logic, so I hadn't reviewed it. On this PR I wasn't trying for general improvements just bugfix grade foot-gun prevention.

Re "take your chance", well-- I don't believe you'll relay it yourself under this condition, so I think at a minimum something else would need to change. And if we'd relay our own transaction when we wouldn't a third party transaction thats perhaps deanonymizing (which might be fine for some users, but probably shouldn't be a default). If you do want that, you can just lower your relay fee... so you still do have that option.

The major consideration here is that we handle stuck transactions very poorly; you can end up with a large amount of coins tied up and unspendable. E.g. spend 0.01 btc, your wallet picks a 10 BTC coin, makes an unrelayable transaction... And now those 10 BTC are unavailable to you days later. There is no user friendly way to abort, you can basically only drop to a commandline, blow away all your wallet metadata, and rescan your wallet. You might not realize you can do this, and you might abandon the funds. So all this favors being conservative by default when it comes to creating transactions that may not relay at all.

IIRC we do the 'can we send this for free' logic basically looking one block ahead-- since one block isn't an undue burden-- and maybe that should now change to follow the txconfirm target, e.g. if your target is zero we shouldn't be looking ahead.

morcos commented at 8:34 PM on December 16, 2014: member

Maybe I'm missing something but I thought that by default free or very low fee transactions were relayed but they were just rate limited, so I don't see why you wouldn't often relay your own or others transactions. The flag fSendFreeTransactions which lets you put 0 fee is different than the fAllowFree flag to GetMinRelayFee which is always true and I don't think your decision to relay could distinguish between an intentionally free transaction and one which you just put a below minimum fee on. Anyway no point to debate it, because I agree with your argument to be conservative.

But now that I think about it, if its a design goal to allow high priority free transactions to be confirmed (as the default mining code does), then maybe the rate-limiting code should be taking into account priority? But thats a separate issue...

gmaxwell commented at 4:45 AM on December 17, 2014: contributor

@morcos There is a floor under which transactions without priority will not be relayed at all, this prevents varrious dos attacks (e.g. if it's merely rate limited I can fill your memory at that rate, and block all other free transactions). (If somehow that slipped out, that needs to be fixed. :) )

in src/init.cpp:None in c3b41c136c outdated

 287 | @@ -288,6 +288,7 @@ std::string HelpMessage(HelpMessageMode mode)
 288 |      strUsage += "  -sendfreetransactions  " + strprintf(_("Send transactions as zero-fee transactions if possible (default: %u)"), 0) + "\n";
 289 |      strUsage += "  -spendzeroconfchange   " + strprintf(_("Spend unconfirmed change when sending transactions (default: %u)"), 1) + "\n";
 290 |      strUsage += "  -txconfirmtarget=<n>   " + strprintf(_("If paytxfee is not set, include enough fee so transactions are confirmed on average within n blocks (default: %u)"), 1) + "\n";
 291 | +    strUsage += "  -maxtxfee=<amt>        " + strprintf(_("Absolute maximum value fees to use in a single wallet transaction, setting low may abort large transactions (default: %s)"), FormatMoney(maxTxFee)) + "\n";

laanwj commented at 3:28 PM on December 17, 2014:

In contrast to the other fee options (which are per KB) this one is total, I think that requires extra mention.

laanwj commented at 3:48 PM on December 17, 2014: member

utACK

jonasschnelli commented at 7:11 PM on December 17, 2014: contributor

ACK. Tested with -maxtxfee=0.00001 and Qt (big multiout tx). LLDB stepped the code. Looks good. nit: as mentioned by @laanwj, ./src/bitcoind --help | grep fee shows:

  -paytxfee=<amt>        Fee (in BTC/kB) to add to transactions you send (default: 0.00)
  -sendfreetransactions  Send transactions as zero-fee transactions if possible (default: 0)
  -txconfirmtarget=<n>   If paytxfee is not set, include enough fee so transactions are confirmed on average within n blocks (default: 1)
  -maxtxfee=<amt>        Absolute maximum value fees to use in a single wallet transaction, setting low may abort large transactions (default: 0.10)
  -minrelaytxfee=<amt>   Fees (in BTC/Kb) smaller than this are considered zero fee for relaying (default: 0.00001)
  -blockprioritysize=<n> Set maximum size of high-priority/low-fee transactions in bytes (default: 50000)

The -maxtxfee arg-help should explicit tell the user that it's not in BTC/kB

Enforce minRelayTxFee on wallet created tx and add a maxtxfee option.

Previously the minRelayTxFee was only enforced on user specified values.

It was possible for smartfee to produce a fee below minRelayTxFee which
 would just result in the transaction getting stuck because it can't be
 relayed.

This also introduces a maxtxfee option which sets an absolute maximum
 for any fee created by the wallet, with an intention of increasing
 user confidence that the automatic fees won't burn them. This was
 frequently a concern even before smartfees.

If the configured fee policy won't even allow the wallet to meet the relay
 fee the transaction creation may be aborted.

aa279d6131

in src/wallet.cpp:None in 85a5b4bced outdated

1519 | -                    break; // Done, enough fee included.
1520 | +                CAmount nFeeNeeded = GetMinimumFee(nBytes, nTxConfirmTarget, mempool);
1521 |  
1522 | -                // Too big to send for free? Include more fee and try again:
1523 | -                if (!fSendFreeTransactions || nBytes > MAX_FREE_TRANSACTION_CREATE_SIZE)
1524 | +                // If we made it here and we're aren't even able to meet the relay fee on the next pass, give up

fanquake commented at 11:30 AM on December 19, 2014:

s/we're/we

gmaxwell commented at 8:05 PM on December 19, 2014:

fixed

laanwj merged this on Dec 23, 2014

laanwj closed this on Dec 23, 2014

laanwj referenced this in commit d01bcc446e on Dec 23, 2014

gmaxwell referenced this in commit 11855c1f99 on Dec 23, 2014

reddink referenced this in commit 8b955f8f58 on May 27, 2020

MarcoFalke locked this on Sep 8, 2021