feat: Improved Fee Rate Computation #2972

gregorycoppola · 2021-12-14T01:51:24Z

#2954 (comment) discusses four proposals for improving fee estimation.

We address these here:

Address outlier block sensitivity by using a median of a window, rather than exponential windowing.
Address impact of STX transfers by using percentiles weighted by the proportion of the block limit consumed rather than 5th, 50th, and 95th percentiles. See weighted percentiles.
Address impact of non-full blocks by treating the non-full proportion of the block as a transaction at the relay fee minimum (a fee of 0 is too low for admission to the mempool).
We should address fee changes in the face of congestion using a strategy of "fuzzing" the estimated fee rate: when queried, nodes will add some random noise to the estimated fee rate. In periods of congestion, clients that used the higher fee would be included in the next block, pushing the fee estimate for the next block up.

Testing

There are new unit tests for the two FeeEstimator components, to test the computations.
There is an integration in neon_integrations.rs that tests that we can load this new estimator, and use it.

jcnelson

This mostly LGTM. Just had two comments about the fee math corner cases that must be addressed before merging. Thanks @gregorycoppola!

pavitthrap

just a few minor comments left!

CHANGELOG.md

src/cost_estimates/fee_medians.rs

pavitthrap · 2022-01-10T17:47:02Z

src/cost_estimates/fee_medians.rs

+            })
+            .expect("SQLite failure");
+
+        for result in results {


I think you should verify that you only iterate window_size times (or that results has length window_size)

What if it doesn't? panic?

seems drastic to panic? maybe only iterate window_size times and print a warn if results is longer than expected. thoughts @kantai ?

I think this would be an appropriate panic/assert, assert!(results.len() <= window_size), because the SQL statement has a LIMIT, there's nothing short of a bug in sqlite or a misunderstanding in our code that should cause that assertion to fail.

Let's just forget about this assert as:

we have a limit anyway, so we probably don't need to worry

this structure doesn't have a len:

error[E0599]: no method named `len` found for struct `AndThenRows` in the current scope --> src/cost_estimates/fee_medians.rs:130:25 | 130 | assert!(results.len() <= window_size); | ^^^ method not found in `AndThenRows<'_, [closure@src/cost_estimates/fee_medians.rs:122:69: 127:14]>`

That's fine with me.

src/cost_estimates/fee_rate_fuzzer.rs

testnet/stacks-node/src/tests/neon_integrations.rs

gregorycoppola

replying to @pavitthrap

testnet/stacks-node/src/tests/neon_integrations.rs

CHANGELOG.md

gregorycoppola · 2022-01-10T21:59:41Z

src/cost_estimates/fee_medians.rs

+            })
+            .expect("SQLite failure");
+
+        for result in results {


What if it doesn't? panic?

src/cost_estimates/fee_rate_fuzzer.rs

pavitthrap

responded to comments

src/cost_estimates/fee_rate_fuzzer.rs

pavitthrap · 2022-01-11T22:30:01Z

src/cost_estimates/fee_medians.rs

+            })
+            .expect("SQLite failure");
+
+        for result in results {


seems drastic to panic? maybe only iterate window_size times and print a warn if results is longer than expected. thoughts @kantai ?

testnet/stacks-node/src/tests/neon_integrations.rs

gregorycoppola

Two unresolved points.

testnet/stacks-node/src/tests/neon_integrations.rs

src/cost_estimates/fee_medians.rs

testnet/stacks-node/src/tests/neon_integrations.rs

testnet/stacks-node/src/config.rs

gregorycoppola · 2022-01-11T23:50:06Z

src/cost_estimates/fee_medians.rs

+        MINIMUM_TX_FEE_RATE
+    };
+    let fee_rate = fee as f64 / denominator;
+    if fee_rate >= MINIMUM_TX_FEE_RATE && fee_rate.is_finite() {


I chose to panic if the fee_rate is infite, because I think it should never be, unless the user had paid an infinte amount. does that sound ok @kantai

src/cost_estimates/fee_medians.rs

pavitthrap

lgtm - thanks for addressing my feedback!

gregorycoppola · 2022-01-12T18:45:13Z

Thanks for many rounds of reviews, everyone.

There was a broken that seemed flaky so re-running.

gregorycoppola · 2022-01-12T20:51:51Z

@kantai sorry about the no-op commits. Next time I'll do it the right way but was just tired of this PR and took a short cut.

blockstack-devops · 2024-11-19T00:22:08Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Greg Coppola added 30 commits December 6, 2021 21:09

separated out the update function

0d03b2a

factored out fee_rate_from_receipt

9f49d2b

renamed variables

a828e17

reorganize fee rates

dca3254

compiles

7faf443

weighte estimates in place but need to add the "minimum rate"

7c43fe8

factored into methods and added "maybe minimum fee rate"

c86e7d7

added rough code for the estimate updates

8d7082f

added in fee medians

05992aa

reinstate old fee_scalar

d45f441

branch tests

ba8a6aa

no such column: estimate_key

ddebc1e

diffed against aaron's version

095f488

two working tests

a2d38e1

test isn't working

dd475ba

interpolation is working

321fa8e

fixed test_single_contract_call

f9e6721

five calls is failing

03f87b0

test_one_block_mostly_filled added

a3f4677

test_ten_blocks_mostly_filled working

4958b7e

added comments

8f53b9b

revert src/cost_estimates/metrics.rs

785c2ed

removed warn's

b4df12c

fixed some test logic comments

a73d7b1

added basic structure to fuzzer

0c2dd1d

test compiles

b29221f

fee estimator compils but needs random seed

89d1255

fuzzer takes an optional seed input

a0d894f

creator function is working

e649463

test is almost ready to work

65eab2b

jcnelson approved these changes Jan 10, 2022

View reviewed changes

replace vec with array

3a24704

pavitthrap reviewed Jan 10, 2022

View reviewed changes

remove the fee_estimation

644c640

gregorycoppola mentioned this pull request Jan 10, 2022

Make a separate chainstate directory for fee_estimation constructs #2991

Closed

gregorycoppola linked an issue Jan 10, 2022 that may be closed by this pull request

Make a separate chainstate directory for fee_estimation constructs #2991

Closed

Merge branch 'develop' into feat/hook-up-estimator

e6d4756

gregorycoppola commented Jan 10, 2022

View reviewed changes

gregorycoppola requested a review from pavitthrap January 11, 2022 14:11

Greg Coppola added 2 commits January 11, 2022 14:12

pavi's comments

6305f48

fmt

0ee4c4f

pavitthrap reviewed Jan 11, 2022

View reviewed changes

add spender addr

25aaa1d

gregorycoppola commented Jan 11, 2022

View reviewed changes

Greg Coppola added 3 commits January 12, 2022 12:32

code clean ups

0e64d28

return none if infinite

14ca280

added warn

ff52409

gregorycoppola requested a review from pavitthrap January 12, 2022 12:49

fixed min/max bug

65f5d9f

pavitthrap approved these changes Jan 12, 2022

View reviewed changes

restart the tests

e599068

start tests again

e6cff5e

pavitthrap added the release 2.05.0.1.0 label Jan 12, 2022

gregorycoppola merged commit 7011f32 into develop Jan 12, 2022

kantai mentioned this pull request Feb 24, 2022

Fee estimate results leather-io/extension#2261

Closed

blockstack-devops added the locked label Nov 19, 2024

stacks-network locked as resolved and limited conversation to collaborators Nov 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Improved Fee Rate Computation #2972

feat: Improved Fee Rate Computation #2972

gregorycoppola commented Dec 14, 2021 •

edited

Loading

jcnelson left a comment

pavitthrap left a comment

pavitthrap Jan 10, 2022

gregorycoppola Jan 10, 2022

pavitthrap Jan 11, 2022

kantai Jan 12, 2022

gregorycoppola Jan 12, 2022

kantai Jan 12, 2022

gregorycoppola left a comment

gregorycoppola Jan 10, 2022

pavitthrap left a comment

pavitthrap Jan 11, 2022

gregorycoppola left a comment

gregorycoppola Jan 11, 2022

pavitthrap left a comment

gregorycoppola commented Jan 12, 2022

gregorycoppola commented Jan 12, 2022

blockstack-devops commented Nov 19, 2024

feat: Improved Fee Rate Computation #2972

feat: Improved Fee Rate Computation #2972

Conversation

gregorycoppola commented Dec 14, 2021 • edited Loading

Testing

jcnelson left a comment

Choose a reason for hiding this comment

pavitthrap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregorycoppola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavitthrap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregorycoppola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavitthrap left a comment

Choose a reason for hiding this comment

gregorycoppola commented Jan 12, 2022

gregorycoppola commented Jan 12, 2022

blockstack-devops commented Nov 19, 2024

gregorycoppola commented Dec 14, 2021 •

edited

Loading