[wallet] Default fPayAtLeastCustomFee to false #6708

maflcko · 2015-09-21T11:14:24Z

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 Never sends fees with less than paytxfee #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.

jonasschnelli · 2015-09-21T11:40:06Z

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

jonasschnelli · 2015-09-21T11:40:28Z

src/wallet/rpcwallet.cpp

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


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

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

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

jonasschnelli · 2015-09-21T11:42:35Z

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

laanwj · 2015-09-23T17:47:23Z

src/wallet/wallet.cpp

@@ -41,7 +41,7 @@ CAmount maxTxFee = DEFAULT_TRANSACTION_MAXFEE;
 unsigned int nTxConfirmTarget = DEFAULT_TX_CONFIRM_TARGET;
 bool bSpendZeroConfChange = true;
 bool fSendFreeTransactions = false;
-bool fPayAtLeastCustomFee = true;
+bool fPayAtLeastCustomFee = false;


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

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

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?

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]

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?

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

dexX7 · 2015-09-24T11:52:32Z

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

maflcko · 2015-09-28T12:39:15Z

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 · 2015-09-28T12:54:02Z

@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 · 2015-10-12T05:11:49Z

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

maflcko · 2015-10-20T10:30:45Z

@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 · 2015-10-21T22:37:43Z

concept ACK

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

maflcko · 2015-11-19T12:49:18Z

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

maflcko · 2015-11-19T13:31:19Z

qa/rpc-tests/wallet.py

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


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.

jonasschnelli · 2015-11-25T13:30:50Z

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.

maflcko force-pushed the MarcoFalke-2015-walletFixPayTxFee branch from d911ab9 to a607781 Compare September 21, 2015 11:25

jonasschnelli reviewed Sep 21, 2015
View reviewed changes

maflcko force-pushed the MarcoFalke-2015-walletFixPayTxFee branch from a607781 to e24df5f Compare September 21, 2015 14:07

laanwj added the Tests label Sep 23, 2015

laanwj reviewed Sep 23, 2015
View reviewed changes

maflcko changed the title ~~[wallet] Add rpc tests to verify fee calculation~~ [wallet] Default fPayAtLeastCustomFee to false Sep 26, 2015

RHavar mentioned this pull request Oct 12, 2015

Default fPayAtLeastCustomFee to false #6649

Closed

Ryan Havar and others added 4 commits November 18, 2015 13:31

[trivial] rpcwallet: Clarify what settxfee does

4db5df7

[wallet] Add rpc tests to verify fee calculations

85c9d90

[wallet] Introduce -payatleastcustomfee

66b670d

maflcko force-pushed the MarcoFalke-2015-walletFixPayTxFee branch from b7d549e to 66b670d Compare November 18, 2015 12:34

jonasschnelli added the Wallet label Nov 19, 2015

fixup! [trivial] Bike shed variable names

a567e15

maflcko reviewed Nov 19, 2015
View reviewed changes

jonasschnelli mentioned this pull request Nov 25, 2015

[Wallet] Improve minimum absolute fee GUI options #7096

Merged

jonasschnelli closed this Nov 25, 2015

maflcko deleted the MarcoFalke-2015-walletFixPayTxFee branch November 30, 2015 17:30

maflcko mentioned this pull request Dec 21, 2015

Sweep up dust inputs when there is room #1675

Closed

maflcko mentioned this pull request Mar 8, 2016

paytxfee behavior changed without warning #7633

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wallet] Default fPayAtLeastCustomFee to false #6708

[wallet] Default fPayAtLeastCustomFee to false #6708

maflcko commented Sep 21, 2015

jonasschnelli commented Sep 21, 2015

jonasschnelli Sep 21, 2015

maflcko Sep 21, 2015

jonasschnelli Nov 18, 2015

jonasschnelli commented Sep 21, 2015

laanwj Sep 23, 2015

maflcko Sep 24, 2015

laanwj Sep 24, 2015

maflcko Sep 24, 2015

laanwj Sep 24, 2015

maflcko Nov 18, 2015

dexX7 commented Sep 24, 2015

maflcko commented Sep 28, 2015

dexX7 commented Sep 28, 2015

RHavar commented Oct 12, 2015

maflcko commented Oct 20, 2015

dcousens commented Oct 21, 2015

maflcko commented Nov 19, 2015

maflcko Nov 19, 2015

jonasschnelli commented Nov 25, 2015

[wallet] Default fPayAtLeastCustomFee to false #6708

[wallet] Default fPayAtLeastCustomFee to false #6708

Conversation

maflcko commented Sep 21, 2015

jonasschnelli commented Sep 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasschnelli commented Sep 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dexX7 commented Sep 24, 2015

maflcko commented Sep 28, 2015

dexX7 commented Sep 28, 2015

RHavar commented Oct 12, 2015

maflcko commented Oct 20, 2015

dcousens commented Oct 21, 2015

maflcko commented Nov 19, 2015

Choose a reason for hiding this comment

jonasschnelli commented Nov 25, 2015