All the feerate you can eat... #1846

rustyrussell · 2018-08-15T06:52:29Z

Lots of things in here, mainly weaning us off manual fee manipulation where possible.

SimonVrouwe · 2018-08-15T09:34:41Z

lightningd/chaintopology.c

@@ -444,6 +459,20 @@ static void json_feerates(struct command *cmd,
 	if (missing)
 		json_add_string(response, "warning",
 				"Some fee estimates unavailable: bitcoind startup?");
+	else {
+		json_object_start(response, "onchain_estimates");


change onchain_estimates to onchain_fee_estimates to make it more descriptive

rustyrussell · 2018-08-16T00:18:36Z

Rebased and folded @SimonVrouwe 's suggested fix...

SimonVrouwe · 2018-08-16T10:02:37Z

Like the idea of feehistory, but it should take into account the time constant poll_second, which can change using --dev-bitcoind-poll.

It looks feehistory is currently only used to determine the min and max feerate estimates of the recent past.

lightning/lightningd/chaintopology.c

Lines 744 to 748 in f5e023d

    
           min = feerate; 
        
           /* If one of last three was an outlier, use that. */ 
        
           for (size_t i = 0; i < FEE_HISTORY_NUM; i++) { 
        
           	if (feehistory[i] < min) 
        
           		min = feehistory[i];

lightning/lightningd/chaintopology.c

Lines 785 to 788 in f5e023d

    
           /* If one of last three was an outlier, use that. */ 
        
           for (size_t i = 0; i < FEE_HISTORY_NUM; i++) { 
        
           	if (feehistory[i] > feerate) 
        
           		feerate = feehistory[i];

Just some thoughts:

Perhaps feehistory should consist of just the min/max outliers of past (lets say) 90 seconds.
Not sure how to code that, maybe by adding timestamp or a (poll_counter) to these outliers to replace them when too old.

SimonVrouwe · 2018-08-16T14:31:24Z

Or forget about timestamping, set a floor to poll_seconds of (lets say) 5 seconds? Currently it can be set to any u32 using --dev-bitcoind-poll.

Then give fee_history array 18 elements so it can store 90 seconds of history. And limit the search for the outliers to the first floor(90 / poll_seconds) elements of that array.

rustyrussell · 2018-08-18T23:35:26Z

It's tempting to get more sophisticated, but remember that dev options are only for testing: here having only 3 is a feature.

The long term averaging is The Right Thing, and it's already in place. But older nodes don't do that, plus there may be other reasons why they're using what we consider outliers, eg. just starting up.

For min-max range checks, we really want to know "is this reasonable": I think if it's a value that estimatefee returns, the answer has to be "yes".

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>

And no more filtering out messages, as we should no longer spam the logs with them (the 'Connected json input' one was removed some time ago). Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

This is only used for logging now, but it gets more important as it enters the RPC API. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

This is useful mainly in the case where bitcoind is not giving estimates, but can also be used to bias results if you want. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

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

We don't know what our peer is doing, but if we see those values, maybe they did too, and for longer. And add the min/max acceptable values into our JSON API. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@SimonVrouwe

We could refine this later (based on existing wallet, for example), but this gives some estimate. [ Rename onchain_estimates -> onchain_fee_estimates Suggested-by: @SimonVrouwe ] Suggested-by: @molxyz Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2018-08-20T01:40:49Z

Rebased to fix conflicts with new param sweep...

rustyrussell · 2018-08-21T00:26:04Z

Travis uncovered a real issue, will address tomorrow, meanwhile this is now a WIP

SimonVrouwe · 2018-08-21T10:18:02Z

Testing this branch and the feerates command showed some weird results concerning unit sipa<->bitcoind conversions.

I did some digging and found, which looks like an old bug:

lightning/lightningd/bitcoind.c

Line 335 in 4e0e149

efee->satoshi_per_kw[efee->i] = feerate / 4;

should be multiply by 4 because feerate [sat/kilobyte] is feerate [sat /( kw/4)] is 4*feerate [sat/kw]

EDIT: No you you cannot do this, bytes and weight are separate quantities/dimensions!

same here:

lightning/lightningd/chaintopology.c

Lines 414 to 416 in 4e0e149

    
           	feerates[i] = (feerates[i] + 3) / 4; 
        
           bitcoind_style = true; 
        
           mulfactor = 4;

/ 4 should be * 4 and mulfactor should 1/4 but maybe better change mulfactor to u32 divfactor ...etc

lightning/lightningd/chaintopology.c

Line 458 in 4e0e149

opening_feerate(cmd->ld->topology) * 702);

needs / 1000 factor

cdecker · 2018-08-21T11:22:52Z

Travis uncovered a real issue, will address tomorrow, meanwhile this is now a WIP

Can we split this PR somehow, cherry-picking the commits that are necessary for other PRs that build on it? Otherwise this becomes a head of line blocker.

(smaller PRs would be easier to review and reduce dependencies as well)

cdecker · 2018-08-21T12:00:14Z

lightningd/chaintopology.c

+		return;
+	if (!topo->root)
+		return;
+	io_break(topo);


Looks a bit like we're trying to reinvent the countdown latch here :-)

cdecker · 2018-08-21T12:01:47Z

lightningd/chaintopology.c

+
+	if (topo->feerate_uninitialized) {
+		/* Moving this forward in time is ok, but feerate smoothing is effectively
+		 * disabled until topo->startup is set to false */


topo->startup was removed, comment needs adjusting.

cdecker · 2018-08-21T12:03:36Z

tests/utils.py

        # Create and rename, for atomicity.
        f = os.path.join(self.daemon.lightning_dir, "bitcoin-cli-fail.tmp")
        with open(f, "w") as text_file:
-            text_file.write("%d" % exitcode)
-        os.rename(f, os.path.join(self.daemon.lightning_dir, "bitcoin-cli-fail"))
+            text_file.write(failscript)


We'll definitely need to change this, a python script writing a ~~python~~bash script that is then run by a subprocess, that's an antipattern if I ever saw one 😄

But it's faster (at least, if it's dash, not bash!).

I just extrapolated from the existing fake, which was a shell script. Happy to take more patches which neaten it up, but this is test infrastructure which is always a second-class citizen...

rustyrussell · 2018-08-22T00:27:00Z

This PR is hard to split. About half the changes are test changes required to test the code changes.

The initial motivation was #1836 but when I started digging I found more cases of problemsl hence the branch name :(

I've split it up, but the cost is that some are now based on a merge of other PRs; let's see how well github deals with that!

rustyrussell · 2018-08-22T01:02:11Z

@SimonVrouwe I don't think that's right (we had it that way in the past though, so you're not alone!)

If the rate is 1 satoshi per byte, and we have a 250 byte tx, it costs 1 * 250 = 250 satoshi.

That tx is 1000 sipa, so the rate per kw is 0.25: 0.25 * 1000 = 250 satoshi. So to convert bitcoind-style feerates to ours, we divide by 4.

Your point about the gross feerate estimate is correct however. I've invited you to be a r/o collaborator, which means I can ping you for reviews in future :)

SimonVrouwe · 2018-08-22T06:01:18Z

@rustyrussell Thanks for clearing that out! I found myself a bit lost in the woods there 😰
What looks like a unit conversion of a certain quantity (size of serialized tx), but it is not. Weight and size are really separate quantities/dimensions which happen to have a (numerical) relation between them.

rustyrussell requested a review from cdecker August 15, 2018 06:52

SimonVrouwe reviewed Aug 15, 2018

View reviewed changes

rustyrussell mentioned this pull request Aug 15, 2018

Support log rotation, don't append crash.log files but use per-peer. #1847

Merged

rustyrussell force-pushed the guilt/i-dream-of-feerate branch from 85dd296 to bb2fa39 Compare August 15, 2018 11:16

rustyrussell changed the title ~~See branch name...~~ All the feerate you can eat... Aug 16, 2018

rustyrussell force-pushed the guilt/i-dream-of-feerate branch from bb2fa39 to f5e023d Compare August 16, 2018 00:24

rustyrussell added 19 commits August 20, 2018 10:43

bitcoind: don't use double in extracting feerate.

0f23126

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>

chaintopology: try to get a feerate estimate before we complete startup.

ce0ea18

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>

pytest: allow more elaborate bitcoin-cli override.

f4a902d

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

pytest: always override estimatesmartfee.

3d0c5b9

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

pytest: demonstrate feerate on restart bug.

ea35bce

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>

channeld: update fees if we're restarting.

e7f9a91

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>

moveonly: move feerate routines from peer_htlcs.c to channel_control.c

a33255b

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

pytest: allow NodeFactory to create non-started peers.

8bbf8f5

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>

options: remove default-fee-rate now we don't use it.

8aa286e

And no more filtering out messages, as we should no longer spam the logs with them (the 'Connected json input' one was removed some time ago). Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

cleanup: lowercase name of feerates, immediate -> urgent.

c75f5ec

This is only used for logging now, but it gets more important as it enters the RPC API. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

feerates: new command to inject/query fee estimates.

ae33d7b

This is useful mainly in the case where bitcoind is not giving estimates, but can also be used to bias results if you want. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

moveonly: feerate_min and feerate_max belong in chaintopology.c

12a7239

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

feerates: consider last three raw values for min/max.

cbc5c0a

We don't know what our peer is doing, but if we see those values, maybe they did too, and for longer. And add the min/max acceptable values into our JSON API. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the guilt/i-dream-of-feerate branch from f5e023d to 4e0e149 Compare August 20, 2018 01:40

rustyrussell added the Work in Progress label Aug 21, 2018

cdecker reviewed Aug 21, 2018

View reviewed changes

rustyrussell mentioned this pull request Aug 21, 2018

Reconnect after feerate bad sig fix #1861

Merged

This was referenced Aug 22, 2018

Never guess feerates #1862

Closed

Never guess feerate #1864

Merged

Pytest tweaks #1863

Merged

rustyrussell closed this Aug 22, 2018

cdecker mentioned this pull request Aug 22, 2018

Replace the fake-bitcoin-cli with a proxy that can be instrumented from tests #1860

Merged

rustyrussell deleted the guilt/i-dream-of-feerate branch August 23, 2018 12:57

rustyrussell mentioned this pull request Aug 24, 2018

Feerate enhancements: cleanups, feerate command and wider min-max tolerances. #1870

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

All the feerate you can eat... #1846

All the feerate you can eat... #1846

rustyrussell commented Aug 15, 2018

SimonVrouwe Aug 15, 2018

rustyrussell Aug 15, 2018

rustyrussell commented Aug 16, 2018

SimonVrouwe commented Aug 16, 2018

SimonVrouwe commented Aug 16, 2018

rustyrussell commented Aug 18, 2018

rustyrussell commented Aug 20, 2018

rustyrussell commented Aug 21, 2018

SimonVrouwe commented Aug 21, 2018 •

edited

Loading

cdecker commented Aug 21, 2018

cdecker Aug 21, 2018

cdecker Aug 21, 2018

cdecker Aug 21, 2018 •

edited

Loading

rustyrussell Aug 22, 2018

rustyrussell commented Aug 22, 2018 •

edited

Loading

rustyrussell commented Aug 22, 2018

SimonVrouwe commented Aug 22, 2018 •

edited

Loading

All the feerate you can eat... #1846

All the feerate you can eat... #1846

Conversation

rustyrussell commented Aug 15, 2018

SimonVrouwe Aug 15, 2018

Choose a reason for hiding this comment

rustyrussell Aug 15, 2018

Choose a reason for hiding this comment

rustyrussell commented Aug 16, 2018

SimonVrouwe commented Aug 16, 2018

SimonVrouwe commented Aug 16, 2018

rustyrussell commented Aug 18, 2018

rustyrussell commented Aug 20, 2018

rustyrussell commented Aug 21, 2018

SimonVrouwe commented Aug 21, 2018 • edited Loading

cdecker commented Aug 21, 2018

cdecker Aug 21, 2018

Choose a reason for hiding this comment

cdecker Aug 21, 2018

Choose a reason for hiding this comment

cdecker Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

rustyrussell Aug 22, 2018

Choose a reason for hiding this comment

rustyrussell commented Aug 22, 2018 • edited Loading

rustyrussell commented Aug 22, 2018

SimonVrouwe commented Aug 22, 2018 • edited Loading

SimonVrouwe commented Aug 21, 2018 •

edited

Loading

cdecker Aug 21, 2018 •

edited

Loading

rustyrussell commented Aug 22, 2018 •

edited

Loading

SimonVrouwe commented Aug 22, 2018 •

edited

Loading