Lower default relay fees by ajtowns · Pull Request #13922 · bitcoin/bitcoin

ajtowns · 2018-08-09T05:02:10Z

In the meeting of 2018-07-05, we discussed dropping the minimum fee rate below 1000 satoshi/kB. This patch set does only that, leaving related features for other PRs.

it changes the min tx fee to 200 s/kvB, and the incremental relay fee to 100 s/B
it leaves wallet min fee and incremental fee at 1000 s/kvB and 5000 s/kvB for compatibility while the network upgrades
it changes the functional tests that are written assuming 1000s/kvB as the min fee levels to explicitly set that via command line parameter

There's a bit more of an explanation for some of the choices in the individual commit messages.

Related PRs:

Report minfeefilter value in getpeerinfo rpc #13987 -- report minfee in getpeerinfo. useful to observe if your peers have lowered their min relay fees
Add checks for settxfee reasonableness #13988 -- settxfee checks. this is useful since currently settxfee to the minrelaytxfee works fine, but with a lower minrelaytxfee, settxfee will be ignored for the range of values between minrelaytxfee and mintxfee which can be confusing
Allow fee estimation to work with lower fees #13990 -- fix fee estimation to work with lower fees; currently fee estimation won't give estimates lower than 1000 satoshi per kilobyte.

ken2812221 · 2018-08-09T05:43:50Z

Concept ACK

ajtowns · 2018-08-09T06:20:30Z

This was more complicated than I expected, so careful review would probably be a good idea. Possible things worth focusing on:

Maybe the default block min fee should be left as is, so miners have deliberately/voluntarily lower minimum fees?
I've not looked much at the estimation code, any maybe I'm misunderstanding how it works. And my code could have bugs.
There's no tests for upgrading the fee estimates; "it works for me", but that's probably not really good enough...
Setting the default incremental relay fee higher than the minrelaytxfee causes init.cpp to automatically raise the minrelaytxfee to match, so I've set it as lower rather than the same, or changing that logic. I think lower makes sense, because an update to a tx probably imposes less cost on the network than an entire new tx. Not sure though.
Not a fan of all the hardcoding in the tests wrt min fee rates, but didn't see a reasonable way of generalising them either.

DrahtBot · 2018-08-09T07:19:30Z

Reviewers, this pull request conflicts with the following ones:

WIP: Transaction Pool Layer #13804 (Transaction Pool Layer by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

domob1812 · 2018-08-09T09:49:18Z

src/policy/fees.cpp

Nit (here and below): You could use a C++11 for-each loop.

Nit now applies to #13990

maflcko · 2018-08-09T10:54:43Z

src/test/mempool_tests.cpp

Instead of changing all the constants in this test, couldn't you set the global min relay fee at the start of the test to 1000 and then back to what it was initially at the end of the test?

I thought it made more sense to check the default behaviour works right, though that would be plausible too

If tests don't already run in parallel, it would be nice if they did someday...

@luke-jr just curious, why? Running in parallel makes it harder to debug and reason about.

maflcko · 2018-08-09T10:58:04Z

src/wallet/rpcwallet.cpp

nit: Should be snake_case: tx_fee_rate.

Also, could submit this feature independent of this pull request, i.e. create a separate pull request.

Fixed in independent PR #13988

maflcko · 2018-08-09T10:58:41Z

src/rpc/net.cpp

Missing test. Also, this feature should probably be submitted in separate pull request.

Test added in separate #13987

murchandamus · 2018-08-09T19:23:54Z

"it changes the min tx fee to 200 s/B, and the incremental relay fee to 100 s/B"

I think you mean kvB instead of B, here.

src/policy/fees.cpp

Empact · 2018-08-10T15:59:32Z

src/policy/fees.cpp

nit: whitespace

Ugh, those weren't meant to be committed in the first place...

promag · 2018-08-27T23:11:22Z

Concept ACK.

Mind adding a release note?

ajtowns · 2018-08-31T05:46:03Z

Rebased and release note added

practicalswift · 2018-09-01T21:09:50Z

doc/release-notes-13922.md

Typo found by codespell: propogate

practicalswift · 2018-09-11T12:14:07Z

test/functional/mempool_limit.py

./test/functional/mempool_limit.py:16:70: E231 missing whitespace after ','

Some tests assume a minrelaytxfee of 1000 satoshi/kB, so explicitly set that in preparation for lowering the default.

This changes the fee defaults to: BLOCK_MIN_TX_FEE = 200 DEFAULT_MIN_RELAY_TX_FEE = 200 DEFAULT_INCREMENTAL_RELAY_FEE = 100 DEFAULT_TRANSACTION_MINFEE = 1000 WALLET_INCREMENTAL_RELAY_FEE = 5000 These reduce default minimum network fees by a factor of 5 (from 1000s/kB to 200s/kB), which matches previous decreases in lowering the price of block data in USD to about 1c/kB: 2013-05: 50,000 to 10,000 at $100 USD/BTC: 5c/kB to 1c/kB 2014-11: 10,000 to 1,000 at $700 USD/BTC: 7c/kB to 0.7c/kB 2015-10: 1,000 to 5,000 and back to 1,000 at $250 USD/BTC: 0.25c/kB to 1.25c/kB to 0.25c/kB 2018-08: 1,000 to 200 at $6000 USD/BTC: 6c/kB to 1.2c/kB (Note that on a per-transaction basis, the witness discount generally decreases fees by about a further 50%, so for individual's a better comparison might be 3c/kB to 0.6c/kB) The incremental relay fee is lowered further, to allow cheaper updates of transactions, which makes better use of blockspace. Because it will take time for the network to broadly support these lower mining and relay fees, the wallet defaults are left unchanged at 1000s/kB and 5000s/kB.

ajtowns · 2018-09-18T13:59:15Z

Rebased due to conflict in python tests; fixed formatting nit; fixed amount typo in changes to mempool test (7000/5 is 1400 not 1500).

ajtowns · 2018-10-10T01:38:59Z

Repeating some in-person comments:

reducing the min incremental relay fee allows for cheap spamming, might be better to change the code so that the min incremental relay fee can be higher than the min relay fee, and leaving it higher (perhaps 200s/kB for relay, but 1000s/kB or 500s/kB for incremental relay/fee-bumping). Note that wallet defaults already have a higher incremental fee, so this would improve consistency.
(long term, not for this PR) it might be nice if min incremental relay fees were dynamically determined based on how much traffic there is -- if there's no traffic, updating a tx doesn't need to cost anything significant; if there's lots of traffic, or a spam attack, updating a tx should become as expensive as issuing a new tx (ie, incremental relay fee should be as high as mempool minfee, which may be well above the min relay fee)

ajtowns · 2018-10-10T01:50:05Z

Closing for now pending further progress on #13990, intending to reopen or refile later

brianddk · 2020-08-21T02:49:26Z

@ajtowns It's been two years, and minrelaytxfee is still a lot higher than it probably needs to be. Would you be willing to refile? Someone needs to get this change through.

The mempool has been emptying pretty much every week, and often every day. With LN interest catching on there is a larger need for "next-day" ultra-low fee on-chain TXNs.

fanquake added the TX fees and policy label Aug 9, 2018

This was referenced Aug 9, 2018

WIP: Transaction Pool Layer #13804

Closed

tx pool: Avoid passing redundant hash into addUnchecked (scripted-diff) #13792

Merged

domob1812 reviewed Aug 9, 2018

View reviewed changes

maflcko added this to the 0.18.0 milestone Aug 9, 2018

maflcko added the Feature label Aug 9, 2018

maflcko reviewed Aug 9, 2018

View reviewed changes

Empact reviewed Aug 10, 2018

View reviewed changes

src/policy/fees.cpp Outdated Show resolved Hide resolved

Empact reviewed Aug 10, 2018

View reviewed changes

DrahtBot mentioned this pull request Aug 16, 2018

Add checks for settxfee reasonableness #13988

Merged

ajtowns force-pushed the minfeefilter branch from 9718bf4 to 0d22375 Compare August 16, 2018 08:16

DrahtBot added the Needs rebase label Aug 29, 2018

ajtowns force-pushed the minfeefilter branch from 0d22375 to 42e2af5 Compare August 31, 2018 05:15

DrahtBot removed the Needs rebase label Aug 31, 2018

DrahtBot mentioned this pull request Aug 31, 2018

Minor style enhancement in documentation #14091

Closed

practicalswift reviewed Sep 1, 2018

View reviewed changes

doc/release-notes-13922.md Outdated

Copy link

Contributor

practicalswift Sep 1, 2018

Choose a reason for hiding this comment

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

Typo found by codespell: propogate

DrahtBot mentioned this pull request Sep 6, 2018

refactor: Make explicit CMutableTransaction -> CTransaction conversion. #14156

Closed

ajtowns force-pushed the minfeefilter branch from 42e2af5 to 56251ce Compare September 7, 2018 04:39

DrahtBot mentioned this pull request Sep 9, 2018

qa: Run all tests even if wallet is not compiled #14180

Merged

practicalswift reviewed Sep 11, 2018

View reviewed changes

test/functional/mempool_limit.py Outdated

Copy link

Contributor

practicalswift Sep 11, 2018

Choose a reason for hiding this comment

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

./test/functional/mempool_limit.py:16:70: E231 missing whitespace after ','

DrahtBot added the Needs rebase label Sep 13, 2018

Set minrelaytxfee=0.00001 in tests

036bac8

Some tests assume a minrelaytxfee of 1000 satoshi/kB, so explicitly set that in preparation for lowering the default.

ajtowns force-pushed the minfeefilter branch from 56251ce to a04a014 Compare September 18, 2018 13:57

DrahtBot removed the Needs rebase label Sep 18, 2018

ajtowns closed this Oct 10, 2018

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

ajtowns commented Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ken2812221 commented Aug 9, 2018

Uh oh!

ajtowns commented Aug 9, 2018

Uh oh!

DrahtBot commented Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murchandamus commented Aug 9, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Aug 27, 2018

Uh oh!

ajtowns commented Aug 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajtowns commented Sep 18, 2018

Uh oh!

ajtowns commented Oct 10, 2018

Uh oh!

ajtowns commented Oct 10, 2018

Uh oh!

brianddk commented Aug 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

ajtowns commented Aug 9, 2018 •

edited

Loading

DrahtBot commented Aug 9, 2018 •

edited

Loading