Refactor blockchaininterface: fee estimation code #1504
Conversation
There was a problem hiding this comment.
This PR refactors the estimate_fee_per_kb method. Mostly splitting it into multiple methods, adding the relevant abstract methods, and adding type hint.
Tests pass.
I think it should be possible to clean up the estimate_fee_per_kb method further. In particular, the difference between the two cases (block target and sat/vb) seems very small and maybe we could de-duplicate it further.
| # cannot be estimated in that case the 2nd highest priority | ||
| # should be used instead of falling back to hardcoded values |
There was a problem hiding this comment.
nit in 68352d0:
Since this is now a standalone method, we could remove the instead of falling back to hardcoded values, since it's not something this method should care/know about.
kristapsk
force-pushed
the
refactor-fee-estimation
branch
from
20c3c5f to
936695a
Compare
|
@PulpCattel I think I have addressed all the review comments, if this looks good, would like to squash and merge soon.
This could go as a follow up PR in future. |
|
@PulpCattel Looks ok now? |
|
Yes, tested 36929c0. Tests pass, and I've done some basic manual testing on signet. |
kristapsk
force-pushed
the
refactor-fee-estimation
branch
from
36929c0 to
18a844b
Compare
| mempoolminfee_in_sat, mempoolminfee_in_sat * float(1 + tx_fees_factor)) | ||
|
|
||
| if self._fee_per_kb_has_been_manually_set(tx_fees): | ||
| N_res = random.uniform(tx_fees, tx_fees * float(1 - tx_fees_factor)) |
There was a problem hiding this comment.
Here's typo bug (found by manual testing), should be + instead of -, will update soon.
There was a problem hiding this comment.
Ah right, we randomize only upwards, so a lower value is a bug. Good catch.
This is one of the reasons we should eventually de-duplicate more out of this function.
There was a problem hiding this comment.
Agree about de-duplication and also fee estimation code currently lacks any unit tests, that could have catched this.
Co-authored-by: Pulp <[email protected]>
kristapsk
force-pushed
the
refactor-fee-estimation
branch
from
18a844b to
d5c240b
Compare
|
CI test failure is known unrelated issue that happens from time to time, merging. |
Part of splitting #1462 into smaller PRs for easier reviewing and testing.