Smart fees for wallet CreateTransaction

gavinandresen commented at 4:28 pm on May 29, 2014: contributor

The wallet now uses the mempool fee estimator with a new command-line option: -txconfirmtarget (default: 1) instead of using hard-coded fees or priorities.

A new bitcoind that hasn’t seen enough transactions to estimate will fall back to the old hard-coded minimum priority or transaction fee.

-paytxfee option overrides -txconfirmtarget.

Relaying and mining code isn’t changed.

For Qt, the coin control dialog now uses priority estimates to label transaction priority (instead of hard-coded constants); unspent outputs were consistently labeled with a much higher priority than is justified by the free transactions actually being accepted into blocks.

I did not implement any GUI for setting -txconfirmtarget; I would suggest getting rid of the “Pay transaction fee” GUI and replace it with either “target number of confirmations” or maybe a “faster confirmation <–> lower fee” slider or select box.

Built on #3959 ; only the last commit is new.

jgarzik commented at 3:05 pm on June 6, 2014: contributor

ACK

gavinandresen commented at 4:18 pm on June 16, 2014: contributor

Rebased just to get pull-tester to re-test.

in src/init.cpp: in b609e5a2fd outdated

252@@ -253,6 +253,7 @@ std::string HelpMessage(HelpMessageMode hmm)
253     strUsage += "\n" + _("Wallet options:") + "\n";
254     strUsage += "  -disablewallet         " + _("Do not load the wallet and disable wallet RPC calls") + "\n";
255     strUsage += "  -paytxfee=<amt>        " + _("Fee per kB to add to transactions you send") + "\n";
256+    strUsage += "  -txconfirmtarget=<n>   " + _("If paytxfee is not set, include enough fee so transactions are confirmed on average within n blocks (default: 1)") + "\n";

Diapolo commented at 6:47 am on June 17, 2014:

AFAIK we ordered the commands here alphabetically.

laanwj commented at 6:55 am on June 17, 2014:

@Diapolo If that’s the goal at least add a comment at the top of HelpMessage() that they’re meant to be sorted alphabetically per category. Otherwise, it will not stay sorted very long.

Diapolo commented at 6:58 am on June 17, 2014:

If we can get to that consensus I’m going to add a short hint yes.

laanwj commented at 6:59 am on June 17, 2014:

No need for “consensus”, this is way too unimportant to vote about, just do it

Diapolo commented at 7:18 am on June 17, 2014:

#4357

petertodd commented at 3:23 pm on June 21, 2014: contributor

This would be extremely dangerous to implement right now as there is no ceiling on the estimated fee. A sybil attacker could easily skew your idea of what fees to pay upwards arbitrarily high, or for that matter, a fluke high estimate at startup. fRejectInsaneFee isn’t even set when AcceptToMemoryPool() is called in the CWallet::CommitTransaction() codepath so the fee could be essentially anything.

Needs a -maxfee option with a sane default.

gavinandresen commented at 5:04 pm on June 21, 2014: contributor

Not vulnerable to Sybil attacks– attacker would have to produce bogus blocks with valid POW to affect estimates.

On Jun 21, 2014, at 11:23 AM, Peter Todd notifications@github.com wrote:

This would be extremely dangerous to implement right now as there is no ceiling on the estimated fee. A sybil attacker could easily skew your idea of what fees to pay upwards arbitrarily high, or for that matter, a fluke high estimate at startup. fRejectInsaneFee isn’t even set when AcceptToMemoryPool() is called in the CWallet::CommitTransaction() codepath so the fee could be essentially anything.

Needs a -maxfee option with a sane default.

— Reply to this email directly or view it on GitHub.

petertodd commented at 6:20 pm on June 21, 2014: contributor

Steps to reproduce:

Delete fee_estimates.dat
./bitcoind -minrelaytxfee=0.1 -limitfreerelay=0 (and hack out priority code I think too)
Create a transaction with 1BTC fee.
Wait for confirmation.
Observe that ’estimatefee 1’ returns 5.208 BTC/KB
Send some funds.
Observe that Bitcoin happily sent a transaction with a 1BTC fee without even blinking based on a single transaction confirming that happened to have a high fee.

You can do the above with a second node -connect’ed to the first if you really want to see what a sybil attack would look like.

gmaxwell commented at 8:24 pm on June 21, 2014: contributor

In my experience talking to people the uncertainty they suffer around fees causes them a lot more problems than the fees themselves… people basically clawing their eyes out in panic when fees are non-zero because they just don’t know what they are. We’d probably save a lot of stress for users by supporting a setting of a hard maximum fee per transaction and a setting for a hard maximum fee per KB, … and avoid the kind of trouble petertodd is pointing to above by setting them to some prudent values like the sendrawtransaction sanity check.

in src/qt/coincontroldialog.cpp: in 427842c2d2 outdated

406+QString CoinControlDialog::getPriorityLabel(const CTxMemPool& pool, double dPriority)
407 {
408-    if (AllowFree(dPriority)) // at least medium
409+    // confirmations -> textual description
410+    typedef std::map<unsigned int, QString> PriorityDescription;
411+    static PriorityDescription priorityDescriptions = boost::assign::map_list_of

laanwj commented at 3:11 pm on June 27, 2014:

This could be const static

laanwj commented at 4:59 pm on June 27, 2014: member

Code changes look good to me. Haven’t tested.

laanwj commented at 11:00 am on July 1, 2014: member

BTW:

After this, CFeeRate CTransaction::minTxFee can be moved to wallet.cpp, as it is no longer used in main.cpp.
Similarly, -mintxfee option could be moved to the Wallet options category in HelpMessage.

sipa commented at 11:01 am on July 1, 2014: member

I think all fee related code (CFeeRate, CTransaction::minTxFee, …) can move out of core.

laanwj commented at 11:07 am on July 1, 2014: member

Moving the minrelayfee would involve more work though, as it’s used in many functions in main still.

sipa commented at 11:08 am on July 1, 2014: member

Yes, to main. There’s no need for it to be in core.

Use fee/priority estimates in wallet CreateTransaction

The wallet now uses the mempool fee estimator with a new
command-line option: -txconfirmtarget (default: 1) instead
of using hard-coded fees or priorities.

A new bitcoind that hasn't seen enough transactions to estimate
will fall back to the old hard-coded minimum priority or
transaction fee.

-paytxfee option overrides -txconfirmtarget.

Relaying and mining code isn't changed.

For Qt, the coin control dialog now uses priority estimates to
label transaction priority (instead of hard-coded constants);
unspent outputs were consistently labeled with a much higher
priority than is justified by the free transactions actually
being accepted into blocks.

I did not implement any GUI for setting -txconfirmtarget; I would
suggest getting rid of the "Pay transaction fee" GUI and replace
it with either "target number of confirmations" or maybe
a "faster confirmation <--> lower fee" slider or select box.

b33d1f5ee5

Sanity checks for estimates

Require at least 11 samples before giving fee/priority estimates.

And have wallet-created transactions go throught the fee-sanity-check
code path.

4b7b1bb1ac

in src/wallet.cpp: in 427842c2d2 outdated

17@@ -18,6 +18,7 @@ using namespace std;
18 
19 // Settings
20 CFeeRate payTxFee(DEFAULT_TRANSACTION_FEE);
21+unsigned int nTxConfirmTarget = 1;

Diapolo commented at 12:11 pm on July 1, 2014:

May I suggest to add a DEFAULT_TX_CONFIRMTARGET and use that where currently 1 is used. It’s also nice to use this in the help string, because when changing a default we don’t need to change the help string.

See e.g. -blockmaxsize=<n> in init.cpp.

gavinandresen commented at 6:29 pm on July 3, 2014: contributor

Rebased, and nits picked (fee variables moved from core to wallet/main).

If pull-tester is happy I’m going to pull. @Diapolo: I didn’t bother with DEFAULT_TX_CONFIRMTARGET because one is the obvious choice for a default, and I doubt we’ll ever decide that two or three or eleven makes more sense than one.

Move fee policy out of core 13fc83c77b

jgarzik commented at 6:43 pm on July 3, 2014: contributor

lightly tested ACK

BitcoinPullTester commented at 7:40 pm on July 3, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4250_13fc83c77bb9108c00dd7709ce17719edb763273/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

gavinandresen merged this on Jul 3, 2014

gavinandresen closed this on Jul 3, 2014

laanwj commented at 3:36 am on July 4, 2014: member

Posthumous ACK.

DrahtBot locked this on Sep 8, 2021

Smart fees for wallet CreateTransaction #4250