Smart fees for wallet CreateTransaction #4250

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:None 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:None 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:None 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