Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lower default relay fees #13922

Closed
wants to merge 2 commits into from

Conversation

@ajtowns
Copy link
Contributor

commented Aug 9, 2018

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:

  • #13987 -- report minfee in getpeerinfo. useful to observe if your peers have lowered their min relay fees
  • #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
  • #13990 -- fix fee estimation to work with lower fees; currently fee estimation won't give estimates lower than 1000 satoshi per kilobyte.
@ken2812221

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

Concept ACK

@ajtowns

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

Reviewers, this pull request conflicts with the following ones:
  • #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.

src/policy/fees.cpp Outdated
confAvg.resize(maxPeriods);
for (unsigned int i = 0; i < maxPeriods; i++) {
confAvg[i].resize(buckets.size());
}

This comment has been minimized.

Copy link
@domob1812

domob1812 Aug 9, 2018

Contributor

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

This comment has been minimized.

Copy link
@ajtowns

ajtowns Aug 16, 2018

Author Contributor

Nit now applies to #13990

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Aug 9, 2018

@MarcoFalke MarcoFalke added the Feature label Aug 9, 2018

BOOST_CHECK_EQUAL(pool.GetMinFee(1).GetFeePerK(), 0);
// ... unless it has gone all the way to 0 (after getting past 1000/2)
// ... unless it has gone all the way to 0 (after getting past minrelayfee/2)

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 9, 2018

Member

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?

This comment has been minimized.

Copy link
@ajtowns

ajtowns Aug 10, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@luke-jr

luke-jr Aug 27, 2018

Member

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

This comment has been minimized.

Copy link
@promag

promag Aug 27, 2018

Member

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

src/wallet/rpcwallet.cpp Outdated
@@ -2982,6 +2982,14 @@ static UniValue settxfee(const JSONRPCRequest& request)
LOCK2(cs_main, pwallet->cs_wallet);

CAmount nAmount = AmountFromValue(request.params[0]);
CFeeRate nFeeRate(nAmount, 1000);

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 9, 2018

Member

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.

This comment has been minimized.

Copy link
@ajtowns

ajtowns Aug 16, 2018

Author Contributor

Fixed in independent PR #13988

src/rpc/net.cpp Outdated
@@ -169,6 +170,7 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
obj.pushKV("inflight", heights);
}
obj.pushKV("whitelisted", stats.fWhitelisted);
obj.pushKV("minfee", ValueFromAmount(stats.minFeeFilter));

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 9, 2018

Member

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

This comment has been minimized.

Copy link
@ajtowns

ajtowns Aug 16, 2018

Author Contributor

Test added in separate #13987

@Xekyo

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

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

Show resolved Hide resolved src/policy/fees.cpp Outdated
src/policy/fees.cpp Outdated
LogPrintf("%s: loaded a bunch of fee estimation data, version %d, buckets %d (target: %d\n", __func__, nVersionThatWrote, numBuckets, buckets.size());

LogPrintf("%s: buckets.size %d, buckets[0] %f, buckets[1] %f\n", __func__, buckets.size(), buckets[0], buckets[1]);
LogPrintf("%s: fileBuckets.size %d, fileBuckets[0] %f, fileBuckets[1] %f\n", __func__, fileBuckets.size(), fileBuckets[0], fileBuckets[1]);

This comment has been minimized.

Copy link
@Empact

Empact Aug 10, 2018

Member

nit: whitespace

This comment has been minimized.

Copy link
@ajtowns

ajtowns Aug 16, 2018

Author Contributor

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

@ajtowns ajtowns force-pushed the ajtowns:minfeefilter branch Aug 16, 2018

@promag

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

Concept ACK.

Mind adding a release note?

@ajtowns ajtowns force-pushed the ajtowns:minfeefilter branch Aug 31, 2018

@DrahtBot DrahtBot removed the Needs rebase label Aug 31, 2018

@ajtowns

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2018

Rebased and release note added

doc/release-notes-13922.md Outdated
blockmintxfee and incrementalrelayfee options.

Note that until these lower defaults are widely adopted across the
network, transactions created with lower fee rates may not propogate

This comment has been minimized.

Copy link
@practicalswift

practicalswift Sep 1, 2018

Member

Typo found by codespell: propogate

test/functional/mempool_limit.py Outdated
@@ -13,7 +13,7 @@ class MempoolLimitTest(BitcoinTestFramework):
def set_test_params(self):
self.setup_clean_chain = True
self.num_nodes = 1
self.extra_args = [["-maxmempool=5", "-spendzeroconfchange=0"]]
self.extra_args = [["-maxmempool=5", "-spendzeroconfchange=0","-minrelaytxfee=0.00001"]]

This comment has been minimized.

Copy link
@practicalswift

practicalswift Sep 11, 2018

Member
./test/functional/mempool_limit.py:16:70: E231 missing whitespace after ','
Set minrelaytxfee=0.00001 in tests
Some tests assume a minrelaytxfee of 1000 satoshi/kB, so explicitly set
that in preparation for lowering the default.
Lower default fees
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 ajtowns force-pushed the ajtowns:minfeefilter branch to a04a014 Sep 18, 2018

@ajtowns

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2018

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

@DrahtBot DrahtBot removed the Needs rebase label Sep 18, 2018

@ajtowns

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2018

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

@ajtowns ajtowns closed this Oct 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.