Smart fees for wallet CreateTransaction by gavinandresen · Pull Request #4250 · bitcoin/bitcoin

gavinandresen · 2014-05-29T16:28:05Z

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 · 2014-06-06T15:05:33Z

ACK

gavinandresen · 2014-06-16T16:18:05Z

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

Diapolo · 2014-06-17T06:47:38Z

src/init.cpp

AFAIK we ordered the commands here alphabetically.

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

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

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

petertodd · 2014-06-21T15:23:15Z

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 · 2014-06-21T17:04:59Z

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 [email protected] 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 · 2014-06-21T18:20:35Z

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 · 2014-06-21T20:24:30Z

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.

laanwj · 2014-06-27T15:11:57Z

src/qt/coincontroldialog.cpp

This could be const static

laanwj · 2014-06-27T16:59:25Z

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

laanwj · 2014-07-01T11:00:09Z

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 · 2014-07-01T11:01:16Z

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

laanwj · 2014-07-01T11:07:10Z

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

sipa · 2014-07-01T11:08:00Z

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

Diapolo · 2014-07-01T12:11:41Z

src/wallet.cpp

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.

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.

Require at least 11 samples before giving fee/priority estimates. And have wallet-created transactions go throught the fee-sanity-check code path.

gavinandresen · 2014-07-03T18:29:11Z

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.

jgarzik · 2014-07-03T18:43:32Z

lightly tested ACK

BitcoinPullTester · 2014-07-03T19:40:00Z

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.

laanwj · 2014-07-04T03:36:48Z

Posthumous ACK.

Diapolo reviewed Jun 17, 2014
View reviewed changes

dexX7 mentioned this pull request Jun 17, 2014

Question: dependency to Bitcoin Core mastercoin-MSC/mastercore#12

Closed

laanwj reviewed Jun 27, 2014
View reviewed changes

src/qt/coincontroldialog.cpp Outdated

Copy link

Member

laanwj Jun 27, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be const static

Diapolo reviewed Jul 1, 2014
View reviewed changes

gavinandresen added 2 commits July 3, 2014 13:44

Sanity checks for estimates

4b7b1bb

Require at least 11 samples before giving fee/priority estimates. And have wallet-created transactions go throught the fee-sanity-check code path.

Move fee policy out of core

13fc83c

gavinandresen merged commit 13fc83c into bitcoin:master Jul 3, 2014

christophebiocca mentioned this pull request Aug 26, 2014

Durable transaction fee model #4146

Closed

morcos mentioned this pull request Nov 3, 2014

High fee warnings need to be reworked #5202

Closed

rnicoll mentioned this pull request Apr 25, 2015

Smart fees for wallet CreateTransaction dogecoin/dogecoin#1127

Merged

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

Conversation

gavinandresen commented May 29, 2014

Uh oh!

jgarzik commented Jun 6, 2014

Uh oh!

gavinandresen commented Jun 16, 2014

Uh oh!

Diapolo Jun 17, 2014

Choose a reason for hiding this comment

Uh oh!

laanwj Jun 17, 2014

Choose a reason for hiding this comment

Uh oh!

Diapolo Jun 17, 2014

Choose a reason for hiding this comment

Uh oh!

laanwj Jun 17, 2014

Choose a reason for hiding this comment

Uh oh!

Diapolo Jun 17, 2014

Choose a reason for hiding this comment

Uh oh!

petertodd commented Jun 21, 2014

Uh oh!

gavinandresen commented Jun 21, 2014

Uh oh!

petertodd commented Jun 21, 2014

Uh oh!

gmaxwell commented Jun 21, 2014

Uh oh!

laanwj Jun 27, 2014

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jun 27, 2014

Uh oh!

laanwj commented Jul 1, 2014

Uh oh!

sipa commented Jul 1, 2014

Uh oh!

laanwj commented Jul 1, 2014

Uh oh!

sipa commented Jul 1, 2014

Uh oh!

Diapolo Jul 1, 2014

Choose a reason for hiding this comment

Uh oh!

gavinandresen commented Jul 3, 2014

Uh oh!

jgarzik commented Jul 3, 2014

Uh oh!

BitcoinPullTester commented Jul 3, 2014

Uh oh!

laanwj commented Jul 4, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants