Allow fee estimation to work with lower fees

ajtowns commented at 8:16 am on August 16, 2018: contributor

Changes the minimum bucket for fee estimation from 1000 satoshi per kilobyte to 4 satoshi per kilobyte, and upgrades any saved fee estimation data.

fanquake added the label TX fees and policy on Aug 16, 2018

in src/policy/fees.cpp:209 in f9dd933a75 outdated

201@@ -195,6 +202,45 @@ TxConfirmStats::TxConfirmStats(const std::vector<double>& defaultBuckets,
202     resizeInMemoryCounters(buckets.size());
203 }
204 
205+TxConfirmStats::TxConfirmStats(const std::vector<double>& defaultBuckets, const std::map<double, unsigned int>& defaultBucketMap, const TxConfirmStats& txstatsSource)
206+    : buckets(defaultBuckets), bucketMap(defaultBucketMap)
207+{
208+    decay = txstatsSource.decay;
209+    scale = txstatsSource.scale;

Empact commented at 7:58 pm on September 1, 2018:

nit: these could go in the initializer list

in src/policy/fees.cpp:227 in f9dd933a75 outdated

222+
223+    txCtAvg.resize(buckets.size());
224+    avg.resize(buckets.size());
225+
226+    unsigned int src = 0;
227+    for (unsigned int dst = 0; dst < buckets.size(); dst++) {

Empact commented at 7:59 pm on September 1, 2018:

nit: ++dst here and elsewhere

in src/policy/fees.cpp:238 in f9dd933a75 outdated

233+            confAvg[i][dst] = txstatsSource.confAvg[i][src];
234+            failAvg[i][dst] = txstatsSource.failAvg[i][src];
235+        }
236+
237+        src++;
238+        if (src >= txstatsSource.buckets.size()) break;

Empact commented at 8:00 pm on September 1, 2018:

nit: can move these conditions into the for, so that it manages both src and dst.

ajtowns force-pushed on Sep 3, 2018

ajtowns commented at 8:47 am on September 3, 2018: contributor

The really high fee rates were due to not handling the buckets/bucketMap objects correctly, believe that’s fixed now.

ajtowns force-pushed on Sep 5, 2018

ajtowns force-pushed on Sep 7, 2018

ajtowns renamed this:
~~WIP: allow fee estimation to work with lower fees~~
Allow fee estimation to work with lower fees
on Sep 7, 2018

ajtowns commented at 4:39 am on September 7, 2018: contributor

Dropped the WIP tag, this should be good to review/nitpick now.

I couldn’t come up with a reasonable way to add functional tests for this change; if someone has a good idea, suggestions/patches welcome :) Though… I guess just generating an estimates file directly in python, pointing bitcoind at it, and seeing what estimates bitcoind comes up with after upgrading it without ever actually putting txes in the blockchain could work okay…

ajtowns force-pushed on Jan 3, 2019

ajtowns commented at 3:33 pm on January 3, 2019: contributor

Reworked based on a suggestion from @Sjors; could still use some functional tests to check that loading a particular set of fee rates produces an expected set of fee estimates.

DrahtBot commented at 5:38 pm on January 5, 2019: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	tryphe, ghost
Concept ACK	Empact

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26376 (test: Use type-safe NodeSeconds for TestMemPoolEntryHelper 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.

in src/policy/fees.h:191 in f28cf3c0ab outdated

196     static constexpr double FEE_SPACING = 1.05;
197 
198 public:
199     /** Create new BlockPolicyEstimator and initialize stats tracking classes with default values */
200-    CBlockPolicyEstimator();
201+    CBlockPolicyEstimator(double min_bucket_feerate = MIN_BUCKET_FEERATE);

practicalswift commented at 2:35 pm on January 7, 2019:

Should be explicit?

ajtowns commented at 4:25 am on March 23, 2020:

Yep.

DrahtBot closed this on Mar 9, 2020

DrahtBot reopened this on Mar 9, 2020

stevenroose commented at 4:19 pm on March 17, 2020: contributor

Is this work abandoned?

Empact commented at 5:27 pm on March 18, 2020: member

Concept ACK

ajtowns commented at 11:50 pm on March 19, 2020: contributor

Had stalled this on writing some tests. Having now written a test, it’s found a bug, so now trying to fix that… :)

ajtowns force-pushed on Mar 20, 2020

ajtowns force-pushed on Mar 23, 2020

ajtowns commented at 4:35 am on March 23, 2020: contributor

Rebased, test case added as first commit, should be reviewable.

Note that this only makes estimations work for fee rates below 1sat/byte if you ever actually accept fee rates that low into your mempool. To make your mempool accept/relay transactions that cheap requires setting -minrelaytxfee or changing DEFAULT_MIN_RELAY_TX_FEE; and to have your wallet use fees that low means setting-mintxfee or changing DEFAULT_TRANSACTION_MINFEE and potentially also WALLET_INCREMENTAL_RELAY_FEE. Doing those things now for mainnet would probably just be a spam vector, rather than particularly useful IMHO.

maflcko commented at 12:31 pm on March 23, 2020: member

Doing those things now for mainnet would probably just be a spam vector

Also open a huge surface for privacy leaks

in src/policy/fees.cpp:176 in 5f76ecbb09 outdated

132@@ -133,8 +133,83 @@ class TxConfirmStats
133      * variables with this state.
134      */
135     void Read(CAutoFile& filein, int nFileVersion, size_t numBuckets);
136+
137+    /**
138+     * Helper function to check that TxConfirmStats behavior is consistent
139+     */
140+    int CheckConsistent(size_t expected_bucket_count) const;
141+    int CheckConsistent(const std::vector<double>& exp_buckets, const std::map<double, unsigned int>& exp_bucket_map) const;

stevenroose commented at 6:19 pm on April 11, 2020:

It’s not very clear to me what these methods are supposed to return. Just from reading the definition, not the implementation.

in src/policy/fees.h:282 in 5f76ecbb09 outdated

256@@ -254,6 +257,9 @@ class CBlockPolicyEstimator
257     /** Initialise bucketMap */
258     void InitBucketMap() EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator);
259 
260+    /** Check stats are consistent (for unit tests) */
261+    int CheckConsistent() const EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator);

stevenroose commented at 6:20 pm on April 11, 2020:

For this method it’s also not clear what it returns. I suppose either a method’s name or documentation should make that clear.

in src/policy/fees.h:170 in c68f796c41 outdated

165@@ -166,6 +166,7 @@ class CBlockPolicyEstimator
166      * The MIN_BUCKET_FEERATE and MAX_BUCKET_FEERATE should just be set to
167      * the lowest and highest reasonable feerate we might ever want to track.
168      */
169+    static constexpr double MIN_BUCKET_FEERATE = 4;

stevenroose commented at 10:00 pm on April 11, 2020:

The PR OP says “8 satoshi”. I don’t know if this conflicts with the concept ACKs on the PR.

ajtowns commented at 9:30 pm on December 23, 2021:

Right, if I remember correctly, the original patchset didn’t preserve the estimate data, but did have a lower bound of exactly 8sat/kvb. The patchset now does preserve the old estimates, but as a result only approximates the lower bound – choose 4 gives an error of 0.8% whereas 8 gave an error of 4.7%. Updated the PR description.

in src/policy/fees.cpp:602 in c68f796c41 outdated

598@@ -599,7 +599,8 @@ CBlockPolicyEstimator::CBlockPolicyEstimator(double min_bucket_feerate)
599     : nBestSeenHeight(0), firstRecordedHeight(0), historicalFirst(0), historicalBest(0), trackedTxs(0), untrackedTxs(0)
600 {
601     assert(0 < min_bucket_feerate);
602-    static_assert(BASE_BUCKET_FEERATE > 0, "Base feerate must be nonzero");

stevenroose commented at 10:00 pm on April 11, 2020:

Will BASE_BUCKET_FEERATE still be used?

ajtowns commented at 9:31 pm on December 23, 2021:

BASE_BUCKET_FEERATE defines the bucket alignment, MIN and MAX then define the extremities.

in src/policy/fees.h:286 in b8c7ec4be4 outdated

260@@ -260,6 +261,9 @@ class CBlockPolicyEstimator
261     /** Check stats are consistent (for unit tests) */
262     int CheckConsistent() const EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator);
263 
264+    /** Initialise buckets up to base */
265+    size_t ExtendBucketsUpTo(double base, double min) EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator);

stevenroose commented at 10:08 pm on April 11, 2020:

I would appreciate the documentation on this method to be a little bit more descriptive. IIUC it adds buckets on the lower end from min up to base?

ajtowns commented at 9:36 pm on December 23, 2021:

Update the comment

in src/policy/fees.h:173 in b8c7ec4be4 outdated

169-     * invalidates old estimates files. So leave it at 1000 unless it becomes
170-     * necessary to lower it, and then lower it substantially.
171+     * The MIN_BUCKET_FEERATE and MAX_BUCKET_FEERATE should just be set to
172+     * the lowest and highest reasonable feerate we might ever want to track.
173      */
174-    static constexpr double MIN_BUCKET_FEERATE = 1000;

stevenroose commented at 10:24 pm on April 11, 2020:

Hmm, I think something weird happened in the separation of the commits. Here you seem to remove the MIN_BUCKET_FEERATE constant, while in https://github.com/bitcoin/bitcoin/pull/13990/commits/c68f796c41b9b145c9c1b2c24aea505362ad1b63 you’re adding it again.

ajtowns commented at 9:39 pm on December 23, 2021:

Prior to this PR, MIN_BUCKET_FEERATE defines both the lower bound of the buckets, and how they’re aligned (that is, all buckets are at FEE_SPACING**n * MIN_BUCKET_FEERATE). This commit renames MIN_BUCKET_FEERATE to BASE_BUCKET_FEERATE because the alignment will continue to be based on it, even as the minimum gets reduced in the later commit that reintroduce MIN_BUCKET_FEERATE at a new, lower value.

stevenroose commented at 10:27 pm on April 11, 2020: contributor

I felt obliged to take a look at the entire PR as it seemed that I prompted renewed interest. I’m not fully familiar with the bucket system, but all seemed reasonable.

maflcko added the label Waiting for author on Aug 13, 2020

ghost commented at 3:12 am on September 29, 2020: none

Doing those things now for mainnet would probably just be a spam vector, rather than particularly useful IMHO. @ajtowns what if those things only work if mempool has transactions less than 10 MB and switch to default if greater than 10 MB

Also open a huge surface for privacy leaks @MarcoFalke what kind of privacy leaks are possible with such changes?

DrahtBot added the label Needs rebase on Dec 7, 2020

ghost commented at 1:35 am on December 25, 2020: none

Got no response for the last comment. Still trying to understand what can go wrong if fee rate below 1 sat/vByte is allowed

https://bitcoin.stackexchange.com/questions/99975/what-bitcoin-core-options-can-be-changed-without-a-restart

Related PR in which it was reduced in past: #3305

If miners revenue is a concern or incentives in Bitcoin, maybe we will have to look at lot of charts, consider everything and discuss with more people especially who use bitcoin like coinjoin etc. I don’t see spam and privacy as reasons or maybe need more details.

maflcko removed the label Waiting for author on Feb 12, 2021

rebroad commented at 11:59 am on April 15, 2021: contributor

@ajtowns sorry for the noob-like question, but why is so much code needed to simply reduce the minfee…? I’d have thought it ought to be a one-line change for this. What am I missing?

in src/policy/fees.cpp:201 in c68f796c41 outdated

161+    return res;
162+}
163+
164+int TxConfirmStats::CheckConsistent(size_t expected_bucket_count) const
165+{
166+    if (decay <= 0 || 1 <= decay) return 1;

GeneFerneau commented at 8:07 pm on July 29, 2021:

Might be helpful to wrap these return values in an struct/class enum with variants that describe the consistency conditions. You could then downcast back to an int when doing arithmetic like in CBlockPlicyEstimator::CheckConsistent.

Then, in the test cases, check the calls to CheckConsistent match the expected variant, instead of just being non-zero.

If you decide to use an enum, maybe add docs about where the values come from. If not, maybe add some docs above the function about return value meaning/origin. Where do you get these values?

ajtowns commented at 9:23 pm on December 23, 2021:

They don’t have any meaning, they’re just distinct to allow you to refer back to the line that found that inconsistency.

tryphe commented at 10:01 pm on July 29, 2021: contributor

Concept ACK involving estimation/stats/tests code changes NACK on changing the default rate unless others really want it and there is good reasoning for it light code review ACK c68f796c41b9b145c9c1b2c24aea505362ad1b63

I think it might be helpful to split this up into two PRs: one changing the minimum feerate variable(which doesn’t necessarily do what it seems unless there is no demand for blockspace), and one involving estimation/stats/tests. There seems to be multiple interpretations and some confusion on what the default minimum should be (https://github.com/bitcoin/bitcoin/issues/14012#issuecomment-820363879), but I think the other changes are good changes.

ghost commented at 10:38 pm on July 29, 2021: none

I had asked a related question in December 2020. Pieter Wuille answered it with following things:

Why can’t we change default minimum fee rate to something below 1 sat/vByte?

The primary reason for the minimum feerate (and related things like the discard feerate) is preventing the use of the P2P network as a cheap global broadcast system. If it’s easy to produce transaction that will never confirm yet will relay across the network, it’d open the network to abuse. I don’t know what the right level is to make sure that doesn’t happen, but 1 sat/vbyte seems pretty low

It was done in past: #3305 why not again?

That was before the network and mempool logic had floating fees. Now the feerate is primarily set by market pressure, and lowering the minimum would at most have an impact during times when there is ~no demand for block space. I personally think the ecosystem should expect to function fine when there is continuous back pressure, so I’m not sure what is gained by reducing it.

Do you think it will result in spamming? I don’t think so and few other people as well: https://www.reddit.com/r/Bitcoin/comments/kjrbny/should_default_minimum_fee_rate_be_01_satvbyte_now/ggyym67

I don’t think spam is the problem. Lately the feerate has been significantly higher than 1 sat/vbyte anyway, and long term I hope that whatever the minimum is set to, it’s effectively never hit. What matters is the discard feerate, which is not set by the market (it’s the “additional” feerate you need to pay to replace a transaction in the mempool through RBF or mempool limiting, in order to make sure there is some cost to relaying the transaction that is now expected to not be mined anymore). That discard feerate is by default set to 1 sat/vbyte as well, which seems low.

After reading this answer and looking at few stats I am not sure if this is required. Also there is no way to test if lower fee rates will increase transactions and fee revenue for miners or decrease. Or if someone can find better charts and ways to test this before being used then we can re-evaluate. If we don’t have lower default min fee rates, fee estimation for lower fee rates doesn’t make sense so I have no opinion on this or weak NACK for now:

ajtowns force-pushed on Dec 23, 2021

ajtowns commented at 9:22 pm on December 23, 2021: contributor

@prayank23

Still trying to understand what can go wrong if fee rate below 1 sat/vByte is allowed

This PR isn’t changing that, so it’s not entirely on-topic, suggest discussing elsewhere (eg on stackoverflow or in a PR that does change the fee rate or perhaps creating an issue on the topic if there isn’t already one). There’s two sorts of spam made more accessible by lowering the fee rate: spam that makes it into what would otherwise have been non-full blocks, which increases the resources needed for IBD; and spam that only ever goes through the p2p network and mempool, increasing the bandwidth cost of running a full node – if you change the min fee to 0.1 sat/vbyte but keep the min incremental relay fee were left at 1 sat/vbyte, this might be pretty minimal though. @rebroad

sorry for the noob-like question, but why is so much code needed to simply reduce the minfee…?

It’s not changing the minfee, it’s changing the range of values the fee estimation code supports. That needs a bunch of code mostly in order to support updating the old data set with a smaller range to a new data set with a larger range.

If you did the “one-line change” to update the minfee without updating the fee estimates, you’d always be told you need to pay at least 1sat/vbyte for your txs anyway; if you did it without the update code, fee estimates would only go below 1sat/vbyte for new installs or if you delete the fee_estimates.dat file. @tryphe

I think it might be helpful to split this up into two PRs: one changing the minimum feerate variable

This PR doesn’t change the minimum feerate variable – that’s DEFAULT_MIN_RELAY_TX_FEE (validation.h) and DEFAULT_INCREMENTAL_RELAY_FEE (policy/policy.h) for mempool, and also WALLET_INCREMENTAL_RELAY_FEE (wallet/wallet.h) for the wallet.

ajtowns force-pushed on Dec 23, 2021

ajtowns commented at 9:45 pm on December 23, 2021: contributor

@MarcoFalke thanks for the ping; somehow I’d forgotten that it needed a rebase and followups

Rebased, responded to comments. If someone wants to take this over, they’re welcome to – I don’t see this as a huge priority while a full 1sat/vb block would leave fees as less than 1% of the block subsidy. But better to have it open and potentially mergeable than end up forgotten and completely out of date.

ghost commented at 10:00 pm on December 23, 2021: none

This PR isn’t changing that, so it’s not entirely on-topic, suggest discussing elsewhere (eg on stackoverflow or in a PR that does change the fee rate or perhaps creating an issue on the topic if there isn’t already one). There’s two sorts of spam made more accessible by lowering the fee rate: spam that makes it into what would otherwise have been non-full blocks, which increases the resources needed for IBD; and spam that only ever goes through the p2p network and mempool, increasing the bandwidth cost of running a full node – if you change the min fee to 0.1 sat/vbyte but keep the min incremental relay fee were left at 1 sat/vbyte, this might be pretty minimal though.

This PR is not needed if we don’t have fee rates below 1 sat/vbyte. And I already asked on stackexchange which was answered by sipa: https://bitcoin.stackexchange.com/questions/100861/why-cant-default-minimum-fee-rate-be-changed-to-0-1-0-2-sat-vbyte

unknown changes_requested

unknown commented at 10:09 pm on December 23, 2021: none

Concept NACK

Fee rates below 1 sat/vbyte do not work right now and there are no plans or benefits for changing default minimum anytime soon. So fee estimation is not required for things that do not exist or relayed.

ajtowns commented at 10:22 pm on December 23, 2021: contributor

Fee rates below 1 sat/vbyte do not work right now and there are no plans or benefits for changing default minimum anytime soon. So fee estimation is not required for things that do not exist or relayed.

Fee rate below 1 sat/vbyte do work right now; they just require changing the default. My nodes where I have change the default have received transactions offering such low fee rates, and occassionally they are mined – that is, while rare, they do exist and are relayed on current mainnet.

Having fee estimation work correctly with lower fee rates should be done prior to changing the defaults, not after, so that software attempting to work with the new defaults does not misbehave in unexpected ways.

DrahtBot removed the label Needs rebase on Dec 23, 2021

ghost commented at 11:28 pm on December 23, 2021: none

Such experiments do not require changes proposed in this PR which is a slippery slope to make things worse for fee market which already has issues.

I have tried experimenting with fee rate below 1 sat/vbyte: https://bitcoin.stackexchange.com/questions/99089/can-we-use-fee-rate-less-than-1-sat-vbyte/

bitcoin deleted a comment on Dec 24, 2021

DrahtBot added the label Needs rebase on Jan 6, 2022

ajtowns force-pushed on Jan 12, 2022

DrahtBot removed the label Needs rebase on Jan 12, 2022

murchandamus commented at 8:59 pm on February 7, 2022: contributor

Seems reasonable, definitely agree that making everything else ready before changing the DEFAULT_MIN_RELAY_TX_FEE is the way to go. But if we’re looking at a full block at minRelayTxFee bringing more than 1% of the fees, that’s about a decade away, so I agree that it doesn’t seem very urgent.

ajtowns force-pushed on Apr 22, 2022

DrahtBot added the label Needs rebase on Jun 29, 2022

glozow commented at 5:55 pm on October 12, 2022: member

@ajtowns are you still working on this?

[tests] Check estimations when loaded from file

Makes feature_fee_estimates load a fixed fee_estimates.dat and
checks that the estimates given afterwards are also fixed.

2ebb1a46ba

Add comment to CBlockPolicyEstimator::Read 8050f44f5d

[refactor] Add CBlockPolicyEstimator::InitBucketMap helper a20e5d3c23

Add CBlockPolicyEstimator::CheckConsistent helper 19c84e6f4b

Allow extending fee estimation buckets 48218ddd9b

Make fee buckets go much lower d7f903c333

ajtowns force-pushed on Oct 17, 2022

ajtowns marked this as a draft on Oct 17, 2022

ajtowns commented at 6:19 am on October 17, 2022: contributor

Rebased, but is failing fuzz testing for reasons that aren’t immediately obvious, so converted to draft

DrahtBot removed the label Needs rebase on Oct 17, 2022

DrahtBot added the label Needs rebase on Nov 22, 2022

DrahtBot commented at 11:18 am on November 22, 2022: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot commented at 1:22 am on February 20, 2023: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

DrahtBot commented at 1:18 am on May 21, 2023: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

stevenroose commented at 8:53 pm on June 3, 2023: contributor

Lately definitely looks like it is less relevant. Nothing says we can’t go back to the situation from a year or two ago where txs with 1 sat/vB would easily confirm in a few hours.

DrahtBot commented at 2:00 am on September 1, 2023: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

achow101 added the label Up for grabs on Sep 23, 2023

achow101 commented at 4:37 pm on September 23, 2023: member

Closing as up for grabs due to lack of activity.

achow101 closed this on Sep 23, 2023

bitcoin locked this on Sep 22, 2024

Allow fee estimation to work with lower fees #13990

Reviews

Conflicts