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

Never guess feerates #1862

Closed

Conversation

rustyrussell
Copy link
Contributor

This is part 1 of splitting #1846. No changes from that, however.

Six patches are test infrastructure: I know @cdecker wants to change that, but I'd prefer that happen afterwards rather than blocking these fixes.

Things this fixes:

  1. There was a window on startup where we'd use default feerate. Instead, we wait.
  2. We did not correctly update fees when restarting, or if they changed before funding tx locked in.
  3. If we couldn't get a fee estimate on startup, we put zeroes into the averager, which meant when bitcoind started giving estimates, we'd be low for quite a while.
  4. Since we hid the default feerate in the estimator, callers couldn't know when it was being used.
    In practice, they either have other sources to estimate feerate (existing tx), or should refuse
    (opening channels, withdrawing funds).

It introduces imprecision (took 1 satoshi off results in the coming
tests), and we have a helper for this already.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It may fail, but it's better than having a window where we're using
the default feerate.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In particular, this lets us intercept individual commands, such as
estimatesmartfee.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We're going to use it to override specific commands.  It's non-valgrinded
already since we use '--trace-children-skip=*bitcoin-cli*' so the overhead
should be minimal.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It (almost?) always fails for regtest; best to override it directly.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We don't update a channel's feerate on reestablishment: we insert a restart
in test_onchain_different_fees() (which we'll need soon anyway) to show it.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is a noop if we're opening a new channel (channel_fees_can_change(channel)
is false until funding locked in), but important if we're restarting.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We don't respond to fee changes until we're locked in: make sure we catch
up at that point.

Note that we use NORMAL fees during opening, but IMMEDIATE after, so
this often sends a fee update.  The tests which break, we set those
feerates to be equal.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…ntry.

Not just during startup: we could have bitcoind not give estimates until
later, but we don't want to smooth with zero.

The test changes in next patch trigger this, so I didn't write a test
with this patch.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Manipulate fees via fake-bitcoin-cli.  It's not quite the same, as
these are pre-smoothing, so we need a restart to override that where
we really need an exact change.  Or we can wait until it reaches a
certain value in cases we don't care about exact amounts.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Useful it we want to intercept bitcoin-cli first.

We move the getinfo() caching into start(), as that's when we can actually
use RPC.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We use feerate in several places, and each one really should react
differently when it's not available (such as when bitcoind is still
catching up):

1. For general fee-enforcement, we use the broadest possible limits.
2. For closingd, we use it as our opening negotiation point: just use half
   the last tx feerate.
3. For onchaind, we can use the last tx feerate as a guide for our own txs;
   it might be too high, but at least we know it was sufficient to be mined.
4. For withdraw and fund_channel, we can simply refuse.

Fixes: ElementsProject#1836
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell added this to the v0.6.1 milestone Aug 22, 2018
@rustyrussell rustyrussell deleted the never-guess-feerates branch August 22, 2018 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant