[wallet] Default fPayAtLeastCustomFee to false #6708

MarcoFalke commented at 11:14 AM on September 21, 2015: member

All fees in bitcoin core are specified in BTC/kB but -paytxfee silently ([init.cpp]) ignores this rule due to fPayAtLeastCustomFee = true.

This PR will:

Default fPayAtLeastCustomFee to false
Improve the rpc wallet tests: Always assert the fee per kB via check_amount() (Previously only fee == fee per 1000 bytes was asserted)
Introduce -payatleastcustomfee
Fixes #6479

For completeness: Copy of #6649 description:

This allows for much finer control of the transaction fees per kilobyte as it prevent small transactions using a fee that is more appropriate for one that is of a kilobyte.

This also allows controlling the fee per kilobyte over rpc such that:

bitcoin-cli settxfee `bitcoin-cli estimatefee 2`

would make sense, while currently it grossly fails often by a factor of x3

This fixes issue #6479, and has minimal impact as dynamic fees are currently the default. This default can be overriden in the qt wallet, although I have trouble imagining where that is useful. Possibly the entire concept of fPayAtLeastCustomFee can be removed in a later version.

MarcoFalke force-pushed on Sep 21, 2015

jonasschnelli commented at 11:40 AM on September 21, 2015: contributor

Concept ACK on better/more flexible fee handling for RPC test.

in src/wallet/rpcwallet.cpp:None in a607781ed7 outdated

2163 | @@ -2164,7 +2164,7 @@ UniValue settxfee(const UniValue& params, bool fHelp)
2164 |      if (fHelp || params.size() < 1 || params.size() > 1)
2165 |          throw runtime_error(
2166 |              "settxfee amount\n"
2167 | -            "\nSet the transaction fee per kB.\n"
2168 | +            "\nSet the transaction fee per kB. (Overwrites the paytxfee parameter)\n"

jonasschnelli commented at 11:40 AM on September 21, 2015:

~~What's the reason of removing the help string here?~~

MarcoFalke commented at 2:19 PM on September 21, 2015:

Can you elaborate? The trivial commit shouldn't remove anything. It's a cherry-pick from my cleanup-txfee-params branch.

jonasschnelli commented at 1:09 PM on November 18, 2015:

Right. Did recognized it the wrong way around. This looks good.

jonasschnelli commented at 11:42 AM on September 21, 2015: contributor

Would supersede #6649 (@MarcoFalke maybe include title and description from #6649 in this PR).

MarcoFalke force-pushed on Sep 21, 2015

laanwj added the label Tests on Sep 23, 2015

in src/wallet/wallet.cpp:None in e24df5f067 outdated

  40 | @@ -41,7 +41,7 @@ CAmount maxTxFee = DEFAULT_TRANSACTION_MAXFEE;
  41 |  unsigned int nTxConfirmTarget = DEFAULT_TX_CONFIRM_TARGET;
  42 |  bool bSpendZeroConfChange = true;
  43 |  bool fSendFreeTransactions = false;
  44 | -bool fPayAtLeastCustomFee = true;
  45 | +bool fPayAtLeastCustomFee = false;

laanwj commented at 5:47 PM on September 23, 2015:

You shouldn't change this default in a pull whose goal is to improve the tests.

MarcoFalke commented at 9:35 AM on September 24, 2015:

The tests need this to be false but you are right: This PR is not merge ready as of now. This needs at least one follow up commit but I am not sure which path to take:

Option 1: Make this PR change the default and do the required things (i.e. adjust release notes et al.)
Option 2: Introduce a command line parameter (or rpc?) to overwrite the default. (I am not sure if we want yet another param to keep track of... )

laanwj commented at 10:10 AM on September 24, 2015:

How this is usually done is to add a hidden command line option to override this setting. This is how most of the tests work that need special setup, for special tests.

On the other hand it is not good to have the entire wallet test depend on a non-standard setting, so then it'd have to be a new test.

Isn't there already a PR to change the default?

MarcoFalke commented at 10:30 AM on September 24, 2015:

Yes, this PR depends on and replaces #6649, thus fixes #6479.

I'd propose to rename this PR to "Default fPayAtLeastCustomFee to false" and go with Option 1? It seems better to get rid of this fixed-fee-regardless-of-tx-size approach completely. To cite @sipa :

[fixed fee amount per transaction] wouldn't make sense. You're paying for space inside blocks, and miners prioritize transactions based on fee per byte. As you don't know the size of a transaction before creating it, specifying its exact fee would likely lead to unexpected behaviour. – Pieter Wuille Aug 5 at 12:40 [1]

laanwj commented at 11:31 AM on September 24, 2015:

Ok, agreed. So it probably makes sense to make these tests changes part of #6649, as I assume it would solve the Travis issue there too?

MarcoFalke commented at 1:16 PM on November 18, 2015:

@laanwj Other PR is closed now. Everything is included in this one.

dexX7 commented at 11:52 AM on September 24, 2015: contributor

I've coded quite a few regtests for a related project, which is based on Core. If fPayAtLeastCustomFee is turned off by default, then I'd fear this messes with assumptions such as "the fee of this or that transaction will be x".

Instead of just changing the default, I would really like if there were options to set all fee related values via RPC or CLI:

nTxConfirmTarget (via "-txconfirmtarget")
payTxFee (via "-paytxfee")
fSendFreeTransactions (via "-sendfreetransactions")
fPayAtLeastCustomFee

Since it's already fully configurable via the GUI, adding an options such as "-payatleastcustomfee" would be a reasonable extension in my opinon.

Ideally the settings are visible somewhere (e.g. via "getinfo", similar to the "paytxfee").

MarcoFalke renamed this:
~~[wallet] Add rpc tests to verify fee calculation~~
[wallet] Default fPayAtLeastCustomFee to false
on Sep 26, 2015

MarcoFalke commented at 12:39 PM on September 28, 2015: member

If there is no valid reason to keep it (or set the default to true) bitcoin-core should not encourage bad practice (Not to mention the documentation inconsistencies: Both, paytxfee and settxfee are in BTC/kB). The earlier we get rid of that, the better.

Introducing a CLI/RPC parameter is trivial but serves no purpose, if there is no compelling reason to do so. We could still leave the flag in the source code for some grace period to let people modify it by hand if they really need it?

dexX7 commented at 12:54 PM on September 28, 2015: contributor

@MarcoFalke: from a practical perspective, say I'd wanted to create transactions with 0.005 BTC fixed fees, how could I do this? Unless I'm missing something here, I'd need to 1. estimate the size of the transaction, 2. then set an estimated fee/kB, 3. select enough coins, 4. create the transaction, and repeat until the estimated size equals the actual size. The issue here is that a different fee/size impacts the coin selection, thus also impacts the size.

RHavar commented at 5:11 AM on October 12, 2015: contributor

@dexX7 The current code does not support that (without the fragile assumption that coin selection won't pick enough dust to drive the transaction size over a kB), but if it's a use-case that should be supported, I think it should be treated as an entirely separate issue, with a "payExactFee".

On the other hand, this PR is extremely useful. I process hundreds of bitcoin transactions per day with bitcoin core over rpc, and it has strongly been requested that some of the transactions I send with higher and lower fees, depending on user requirements. Thanks to the estimatefee logic, I know exactly the fees per kilobyte I want to be using, but the current fPayAtLeastCusomFee setting makes it impossible to use.

Strongly in support of this change.

MarcoFalke commented at 10:30 AM on October 20, 2015: member

@dexX7 createrawtransaction is your friend creating fixed fee transactions. Just select enough coins and adjust the change.

But you could also use -payatleastcustomfee, now and hope your tx is less than 1001 bytes.

dcousens commented at 10:37 PM on October 21, 2015: contributor

concept ACK

Default fPayAtLeastCustomFee to false

This allows for much finer control of the transaction fees per kilobyte
as it prevent small transactions using a fee that is more appropriate
for one that is of a kilobyte.

This also allows controlling the fee per kilobyte over rpc such that:

bitcoin-cli settxfee `bitcoin-cli estimatefee 2`

would make sense, while currently it grossly fails often by a factor of x3

3239b85be2

[trivial] rpcwallet: Clarify what settxfee does 4db5df774a

[wallet] Add rpc tests to verify fee calculations 85c9d90374

[wallet] Introduce -payatleastcustomfee 66b670d049

MarcoFalke force-pushed on Nov 18, 2015

MarcoFalke commented at 12:49 PM on November 19, 2015: member

@RHavar @dexX7 @dcousens Mind to re-ACK?

jonasschnelli added the label Wallet on Nov 19, 2015

fixup! [trivial] Bike shed variable names a567e15106

in qa/rpc-tests/wallet.py:None in a567e15106

  31 | +        if fee < target_fee:
  32 | +            raise AssertionError("Fee of %s BTC too low! (Should be %s BTC)"%(str(fee), str(target_fee)))
  33 | +        # allow the node's estimation to be at most 2 bytes off
  34 | +        if fee > fee_per_byte * (tx_size + 2):
  35 | +            raise AssertionError("Fee of %s BTC too high! (Should be %s BTC)"%(str(fee), str(target_fee)))
  36 | +        return curr_balance

MarcoFalke commented at 1:31 PM on November 19, 2015:

This will produce the smallest diff but if it is preferred to make check_fee_amount() independent of curr_balance. I.e. def check_fee_amount(self, actual_fee, fee_per_byte, tx_size):, it could later also be used by other classes in the framework.

in src/init.cpp:None in 66b670d049 outdated

 390 | @@ -391,6 +391,8 @@ std::string HelpMessage(HelpMessageMode mode)
 391 |              CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MINFEE)));
 392 |      strUsage += HelpMessageOpt("-paytxfee=<amt>", strprintf(_("Fee (in %s/kB) to add to transactions you send (default: %s)"),
 393 |          CURRENCY_UNIT, FormatMoney(payTxFee.GetFeePerK())));
 394 | +    strUsage += HelpMessageOpt("-payatleastcustomfee", strprintf(_("Pay the fee set by paytxfee or settxfee considering at least 1000 bytes or the actual transaction size, whichever is higher (default: %u)"),
 395 | +        fPayAtLeastCustomFee));

MarcoFalke commented at 1:38 PM on November 19, 2015:

Nit: @jonasschnelli didn't like this. I think

this should hide behind if (showDebug)
this should be slowly deprecated. (i.e. GUI does not overwrite command line args at runtime and this option should be removed entirely from the GUI, and rpc after a grace period)

jonasschnelli commented at 1:30 PM on November 25, 2015: contributor

Closing this because its superseded by #7096 which includes some of this PRs work. @dexX7: There is no way to set an absolute fee over RPC. IMO this is the right thing. Absolute fees are not something we should encourage for sendto* commands because there you can't control the size of your transaction because the amount of inputs can vary. I agree, deterministic fees for transactions in regtest is now a bit harder or even impossible. Maybe RPC tests should deal with the volatility of the fees by not checking absolute values.

jonasschnelli closed this on Nov 25, 2015

MarcoFalke deleted the branch on Nov 30, 2015

MarcoFalke locked this on Sep 8, 2021