From f82c8c7f8c7f11c22f3aa2dd8c41933de96212c6 Mon Sep 17 00:00:00 2001 From: Michael Schmoock Date: Wed, 29 May 2019 12:51:58 +0200 Subject: [PATCH 01/13] test: drains a channel to crash the daemon --- tests/test_gossip.py | 94 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 93 insertions(+), 1 deletion(-) diff --git a/tests/test_gossip.py b/tests/test_gossip.py index 867a446936a3..471395001cc6 100644 --- a/tests/test_gossip.py +++ b/tests/test_gossip.py @@ -1,5 +1,5 @@ from fixtures import * # noqa: F401,F403 -from lightning import RpcError +from lightning import RpcError, Millisatoshi from utils import wait_for, TIMEOUT, only_one import json @@ -1153,3 +1153,95 @@ def test_gossip_store_compact(node_factory, bitcoind): # Should restart ok. l2.restart() wait_for(lambda: l2.daemon.is_in_log('gossip_store: Read ')) + + +@pytest.mark.xfail(strict=True) +def test_channel_drainage(node_factory, bitcoind): + """Test channel drainage. + + Test to drains a channels as much as possible, + especially in regards to commitment fee: + + [l1] <=> [l2] + """ + sats = 10**6 + l1, l2 = node_factory.line_graph(2, fundamount=sats, wait_for_announce=True) + + # wait for everyone to see every channel as active + for n in [l1, l2]: + wait_for(lambda: [c['active'] for c in n.rpc.listchannels()['channels']] == [True] * 2 * 1) + + spendable_l1 = l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] + spendable_l2 = l2.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] + + # so spendable is total capacity minus reserves + amount = spendable_l1 + # the next substraction is up to the millisatoshi the _exact_ value + # we need to make getroute find a route without fuzz. why is that? + amount -= Millisatoshi("10009800msat") + # the next substraction is to get around "WIRE_TEMPORARY_CHANNEL_FAILURE: Capacity exceeded" + # caused by the HTLC commitment fees at l1 + amount -= Millisatoshi("3431sat") + + payment_hash = l2.rpc.invoice('any', 'inv', 'for testing')['payment_hash'] + route = l1.rpc.getroute(l2.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] + fees = route[0]['amount_msat'] - amount + print("spendable:%s amount:%s fees:%s" % (spendable_l1, amount, fees)) + result = l1.rpc.sendpay(route, payment_hash) + print("sendpay", result) + result = l1.rpc.waitsendpay(payment_hash, 10) + print("waitsendpay", result) + + # wait until spendable is updated for both nodes + spendable_l1_bak = spendable_l1 + spendable_l2_bak = spendable_l2 + while spendable_l1_bak == spendable_l1 or spendable_l2_bak == spendable_l2: + spendable_l1 = l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] + spendable_l2 = l2.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] + + # now we drain twice to try to get into invalid channel state + # NOTE: draining twice is possible because the required commitment + # fees are less when there little in the channel. dunno why. + print("spendable after first drain", spendable_l1) # 13440800msat + amount = spendable_l1 + # now we substract again as much as needed to get around "Capacity exceeded" + amount -= Millisatoshi("10860sat") + payment_hash = l2.rpc.invoice('any', 'inv2', 'for testing')['payment_hash'] + route = l1.rpc.getroute(l2.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] + fees = route[0]['amount_msat'] - amount + print("spendable:%s amount:%s fees:%s" % (spendable_l1, amount, fees)) + result = l1.rpc.sendpay(route, payment_hash) + print("sendpay", result) + result = l1.rpc.waitsendpay(payment_hash, 10) + print("waitsendpay", result) + # wait again until spendable is updated for both nodes + spendable_l1_bak = spendable_l1 + spendable_l2_bak = spendable_l2 + while spendable_l1_bak == spendable_l1 or spendable_l2_bak == spendable_l2: + spendable_l1 = l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] + spendable_l2 = l2.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] + + # in the broken state the next bigger payment from l2 to l1 will crash the daemon at l2. + # Note1: A smaller payment (i.e. 10000sat) unlocks this state and recovers. + # Note2: When the remote is LND the payment will cracefully fail until the + # broken state is unlocked and recovered with a small initial payment. + """ +lightning_channeld: channeld/channeld.c:1382: handle_peer_commit_sig: Assertion `can_funder_afford_feerate(peer->channel, peer->channel->view[LOCAL] .feerate_per_kw)' failed. +FATAL SIGNAL 6 (version v0.7.0-397-gd803275)' +backtrace: common/daemon.c:45 (send_backtrace) 0x562b824fa13f' +backtrace: common/daemon.c:53 (crashdump) 0x562b824fa18f' +backtrace: (null):0 ((null)) 0x7f1028c3c8af' +backtrace: (null):0 ((null)) 0x7f1028c3c82f' +backtrace: (null):0 ((null)) 0x7f1028c27671' +backtrace: (null):0 ((null)) 0x7f1028c27547' +backtrace: (null):0 ((null)) 0x7f1028c34db5' +backtrace: channeld/channeld.c:1380 (handle_peer_commit_sig) 0x562b824e984d' +backtrace: channeld/channeld.c:1819 (peer_in) 0x562b824eadcd' +backtrace: channeld/channeld.c:3102 (main) 0x562b824ee14b' +backtrace: (null):0 ((null)) 0x7f1028c28ce2' +backtrace: (null):0 ((null)) 0x562b824e57fd' +backtrace: (null):0 ((null)) 0xffffffffffffffff' +lightning_channeld: FATAL SIGNAL 6 (version v0.7.0-397-gd803275) + """ + bolt11 = l1.rpc.invoice('100000sat', 'inv', 'for testing')['bolt11'] + result = l2.rpc.pay(bolt11) From 0585f3427f5848b10cea2f46eebfbc9d34332af9 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 31 May 2019 16:58:06 +0930 Subject: [PATCH 02/13] listchannels: add `htlc_minimum_msat` and `htlc_maximum_msat` fields. Signed-off-by: Rusty Russell --- CHANGELOG.md | 3 ++- doc/lightning-listchannels.7 | 28 ++++++++++++++++++++++++++-- doc/lightning-listchannels.7.txt | 2 ++ gossipd/gossipd.c | 2 ++ lightningd/gossip_control.c | 2 ++ lightningd/gossip_msg.c | 4 ++++ lightningd/gossip_msg.h | 1 + 7 files changed, 39 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58ce3ef5894a..393128ae003a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - JSON API: `listpeers` status now shows how many confirmations until channel is open (#2405) - Config: Adds parameter `min-capacity-sat` to reject tiny channels. - JSON API: `listforwards` now includes the time an HTLC was received and when it was resolved. Both are expressed as UNIX timestamps to facilitate parsing (Issue [#2491](https://github.com/ElementsProject/lightning/issues/2491), PR [#2528](https://github.com/ElementsProject/lightning/pull/2528)) -+- JSON API: new plugin hooks `invoice_payment` for intercepting invoices before they're paid, `openchannel` for intercepting channel opens, and `htlc_accepted` to decide whether to resolve, reject or continue an incoming or forwarded payment.. +- JSON API: new plugin hooks `invoice_payment` for intercepting invoices before they're paid, `openchannel` for intercepting channel opens, and `htlc_accepted` to decide whether to resolve, reject or continue an incoming or forwarded payment.. - plugin: the `connected` hook can now send an `error_message` to the rejected peer. - Protocol: we now enforce `option_upfront_shutdown_script` if a peer negotiates it. - JSON API: `listforwards` now includes the local_failed forwards with failcode (Issue [#2435](https://github.com/ElementsProject/lightning/issues/2435), PR [#2524](https://github.com/ElementsProject/lightning/pull/2524)) @@ -22,6 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 (Issue [#2409](https://github.com/ElementsProject/lightning/issues/2409)) - JSON API: new withdraw methods `txprepare`, `txsend` and `txdiscard`. - plugin: the `warning` notification can now detect any `LOG_UNUSUAL`/`LOG_BROKEN` level event. +- JSON API: `listchannels` has new fields `htlc_minimum_msat` and `htlc_maximum_msat`. ### Changed diff --git a/doc/lightning-listchannels.7 b/doc/lightning-listchannels.7 index 05d631c63196..a39e24d1d780 100644 --- a/doc/lightning-listchannels.7 +++ b/doc/lightning-listchannels.7 @@ -2,12 +2,12 @@ .\" Title: lightning-listchannels .\" Author: [see the "AUTHOR" section] .\" Generator: DocBook XSL Stylesheets v1.79.1 -.\" Date: 02/18/2019 +.\" Date: 05/31/2019 .\" Manual: \ \& .\" Source: \ \& .\" Language: English .\" -.TH "LIGHTNING\-LISTCHANN" "7" "02/18/2019" "\ \&" "\ \&" +.TH "LIGHTNING\-LISTCHANN" "7" "05/31/2019" "\ \&" "\ \&" .\" ----------------------------------------------------------------- .\" * Define some portability stuff .\" ----------------------------------------------------------------- @@ -206,6 +206,30 @@ message (BOLT #7)\&. : The number of blocks delay required to wait for on\-chain settlement when unilaterally closing the channel (BOLT #2)\&. .RE .sp +.RS 4 +.ie n \{\ +\h'-04'\(bu\h'+03'\c +.\} +.el \{\ +.sp -1 +.IP \(bu 2.3 +.\} +\fIhtlc_minimum_msat\fR +: The minimum payment which can be send through this channel\&. +.RE +.sp +.RS 4 +.ie n \{\ +\h'-04'\(bu\h'+03'\c +.\} +.el \{\ +.sp -1 +.IP \(bu 2.3 +.\} +\fIhtlc_maximum_msat\fR +: The maximum payment which can be send through this channel\&. +.RE +.sp If \fIshort_channel_id\fR or \fIsource\fR is supplied and no matching channels are found, a "channels" object with an empty list is returned\&. .SH "ERRORS" .sp diff --git a/doc/lightning-listchannels.7.txt b/doc/lightning-listchannels.7.txt index 648edcb38c36..7ebac38d5d93 100644 --- a/doc/lightning-listchannels.7.txt +++ b/doc/lightning-listchannels.7.txt @@ -59,6 +59,8 @@ HTLC (BOLT #2). transferred satoshi (BOLT #2). - 'delay' : The number of blocks delay required to wait for on-chain settlement when unilaterally closing the channel (BOLT #2). +- 'htlc_minimum_msat' : The minimum payment which can be send through this channel. +- 'htlc_maximum_msat' : The maximum payment which can be send through this channel. If 'short_channel_id' or 'source' is supplied and no matching channels are found, a "channels" object with an empty list is returned. diff --git a/gossipd/gossipd.c b/gossipd/gossipd.c index 3d126f1d2f00..6503c8614071 100644 --- a/gossipd/gossipd.c +++ b/gossipd/gossipd.c @@ -1948,6 +1948,8 @@ static struct gossip_halfchannel_entry *hc_entry(const tal_t *ctx, e->base_fee_msat = c->base_fee; e->fee_per_millionth = c->proportional_fee; e->delay = c->delay; + e->min = c->htlc_minimum; + e->max = c->htlc_maximum; return e; } diff --git a/lightningd/gossip_control.c b/lightningd/gossip_control.c index 06ac60910e5c..d0380a2d0ea7 100644 --- a/lightningd/gossip_control.c +++ b/lightningd/gossip_control.c @@ -389,6 +389,8 @@ static void json_add_halfchan(struct json_stream *response, json_add_num(response, "base_fee_millisatoshi", he->base_fee_msat); json_add_num(response, "fee_per_millionth", he->fee_per_millionth); json_add_num(response, "delay", he->delay); + json_add_amount_msat_only(response, "htlc_minimum_msat", he->min); + json_add_amount_msat_only(response, "htlc_maximum_msat", he->max); json_object_end(response); } diff --git a/lightningd/gossip_msg.c b/lightningd/gossip_msg.c index 7150ef9dd1cd..afd0654167d7 100644 --- a/lightningd/gossip_msg.c +++ b/lightningd/gossip_msg.c @@ -105,6 +105,8 @@ static void fromwire_gossip_halfchannel_entry(const u8 **pptr, size_t *max, entry->delay = fromwire_u32(pptr, max); entry->base_fee_msat = fromwire_u32(pptr, max); entry->fee_per_millionth = fromwire_u32(pptr, max); + entry->min = fromwire_amount_msat(pptr, max); + entry->max = fromwire_amount_msat(pptr, max); } struct gossip_getchannels_entry * @@ -144,6 +146,8 @@ static void towire_gossip_halfchannel_entry(u8 **pptr, towire_u32(pptr, entry->delay); towire_u32(pptr, entry->base_fee_msat); towire_u32(pptr, entry->fee_per_millionth); + towire_amount_msat(pptr, entry->min); + towire_amount_msat(pptr, entry->max); } void towire_gossip_getchannels_entry(u8 **pptr, diff --git a/lightningd/gossip_msg.h b/lightningd/gossip_msg.h index 26ab7c898e27..de6b387825f5 100644 --- a/lightningd/gossip_msg.h +++ b/lightningd/gossip_msg.h @@ -27,6 +27,7 @@ struct gossip_halfchannel_entry { u32 delay; u32 base_fee_msat; u32 fee_per_millionth; + struct amount_msat min, max; }; struct gossip_getchannels_entry { From 92e0db23fdbd101af35a312893ed8edaf28eb1a5 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 31 May 2019 17:00:19 +0930 Subject: [PATCH 03/13] tests: add plugin for simple invoice delay. Signed-off-by: Rusty Russell --- tests/plugins/hold_invoice.py | 18 ++++++++++++++++++ tests/test_plugin.py | 12 +++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) create mode 100755 tests/plugins/hold_invoice.py diff --git a/tests/plugins/hold_invoice.py b/tests/plugins/hold_invoice.py new file mode 100755 index 000000000000..ec11d18bf464 --- /dev/null +++ b/tests/plugins/hold_invoice.py @@ -0,0 +1,18 @@ +#!/usr/bin/env python3 +"""Simple plugin to allow testing while closing of HTLC is delayed. +""" + +from lightning import Plugin +import time + +plugin = Plugin() + + +@plugin.hook('invoice_payment') +def on_payment(payment, plugin): + time.sleep(float(plugin.get_option('holdtime'))) + return {} + + +plugin.add_option('holdtime', '10', 'The time to hold invoice for.') +plugin.run() diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 4278097e797c..3fdcac2856f8 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -2,7 +2,7 @@ from fixtures import * # noqa: F401,F403 from flaky import flaky # noqa: F401 from lightning import RpcError, Millisatoshi -from utils import only_one, wait_for +from utils import only_one, wait_for, TIMEOUT import os import pytest @@ -293,6 +293,16 @@ def test_invoice_payment_hook(node_factory): l2.daemon.wait_for_log('preimage=' + '0' * 64) +def test_invoice_payment_hook_hold(node_factory): + """ l1 uses the hold_invoice plugin to delay invoice payment. + """ + opts = [{}, {'plugin': 'tests/plugins/hold_invoice.py', 'holdtime': TIMEOUT / 2}] + l1, l2 = node_factory.line_graph(2, opts=opts) + + inv1 = l2.rpc.invoice(123000, 'label', 'description', preimage='1' * 64) + l1.rpc.pay(inv1['bolt11']) + + def test_openchannel_hook(node_factory, bitcoind): """ l2 uses the reject_odd_funding_amounts plugin to reject some openings. """ From 910614abf8eeefd5cf690384eebb2d7cb1d22915 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 31 May 2019 17:00:32 +0930 Subject: [PATCH 04/13] pytest: move test_channel_drainage to test_pay.py This is where payment tests should go. Also mark it xfail for the moment, and remove developer-only tag (propagating gossip is only 60 seconds, which is OK). Signed-off-by: Rusty Russell --- tests/test_gossip.py | 94 +------------------------------------------- tests/test_pay.py | 92 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 93 deletions(-) diff --git a/tests/test_gossip.py b/tests/test_gossip.py index 471395001cc6..867a446936a3 100644 --- a/tests/test_gossip.py +++ b/tests/test_gossip.py @@ -1,5 +1,5 @@ from fixtures import * # noqa: F401,F403 -from lightning import RpcError, Millisatoshi +from lightning import RpcError from utils import wait_for, TIMEOUT, only_one import json @@ -1153,95 +1153,3 @@ def test_gossip_store_compact(node_factory, bitcoind): # Should restart ok. l2.restart() wait_for(lambda: l2.daemon.is_in_log('gossip_store: Read ')) - - -@pytest.mark.xfail(strict=True) -def test_channel_drainage(node_factory, bitcoind): - """Test channel drainage. - - Test to drains a channels as much as possible, - especially in regards to commitment fee: - - [l1] <=> [l2] - """ - sats = 10**6 - l1, l2 = node_factory.line_graph(2, fundamount=sats, wait_for_announce=True) - - # wait for everyone to see every channel as active - for n in [l1, l2]: - wait_for(lambda: [c['active'] for c in n.rpc.listchannels()['channels']] == [True] * 2 * 1) - - spendable_l1 = l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] - spendable_l2 = l2.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] - - # so spendable is total capacity minus reserves - amount = spendable_l1 - # the next substraction is up to the millisatoshi the _exact_ value - # we need to make getroute find a route without fuzz. why is that? - amount -= Millisatoshi("10009800msat") - # the next substraction is to get around "WIRE_TEMPORARY_CHANNEL_FAILURE: Capacity exceeded" - # caused by the HTLC commitment fees at l1 - amount -= Millisatoshi("3431sat") - - payment_hash = l2.rpc.invoice('any', 'inv', 'for testing')['payment_hash'] - route = l1.rpc.getroute(l2.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] - fees = route[0]['amount_msat'] - amount - print("spendable:%s amount:%s fees:%s" % (spendable_l1, amount, fees)) - result = l1.rpc.sendpay(route, payment_hash) - print("sendpay", result) - result = l1.rpc.waitsendpay(payment_hash, 10) - print("waitsendpay", result) - - # wait until spendable is updated for both nodes - spendable_l1_bak = spendable_l1 - spendable_l2_bak = spendable_l2 - while spendable_l1_bak == spendable_l1 or spendable_l2_bak == spendable_l2: - spendable_l1 = l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] - spendable_l2 = l2.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] - - # now we drain twice to try to get into invalid channel state - # NOTE: draining twice is possible because the required commitment - # fees are less when there little in the channel. dunno why. - print("spendable after first drain", spendable_l1) # 13440800msat - amount = spendable_l1 - # now we substract again as much as needed to get around "Capacity exceeded" - amount -= Millisatoshi("10860sat") - payment_hash = l2.rpc.invoice('any', 'inv2', 'for testing')['payment_hash'] - route = l1.rpc.getroute(l2.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] - fees = route[0]['amount_msat'] - amount - print("spendable:%s amount:%s fees:%s" % (spendable_l1, amount, fees)) - result = l1.rpc.sendpay(route, payment_hash) - print("sendpay", result) - result = l1.rpc.waitsendpay(payment_hash, 10) - print("waitsendpay", result) - # wait again until spendable is updated for both nodes - spendable_l1_bak = spendable_l1 - spendable_l2_bak = spendable_l2 - while spendable_l1_bak == spendable_l1 or spendable_l2_bak == spendable_l2: - spendable_l1 = l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] - spendable_l2 = l2.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] - - # in the broken state the next bigger payment from l2 to l1 will crash the daemon at l2. - # Note1: A smaller payment (i.e. 10000sat) unlocks this state and recovers. - # Note2: When the remote is LND the payment will cracefully fail until the - # broken state is unlocked and recovered with a small initial payment. - """ -lightning_channeld: channeld/channeld.c:1382: handle_peer_commit_sig: Assertion `can_funder_afford_feerate(peer->channel, peer->channel->view[LOCAL] .feerate_per_kw)' failed. -FATAL SIGNAL 6 (version v0.7.0-397-gd803275)' -backtrace: common/daemon.c:45 (send_backtrace) 0x562b824fa13f' -backtrace: common/daemon.c:53 (crashdump) 0x562b824fa18f' -backtrace: (null):0 ((null)) 0x7f1028c3c8af' -backtrace: (null):0 ((null)) 0x7f1028c3c82f' -backtrace: (null):0 ((null)) 0x7f1028c27671' -backtrace: (null):0 ((null)) 0x7f1028c27547' -backtrace: (null):0 ((null)) 0x7f1028c34db5' -backtrace: channeld/channeld.c:1380 (handle_peer_commit_sig) 0x562b824e984d' -backtrace: channeld/channeld.c:1819 (peer_in) 0x562b824eadcd' -backtrace: channeld/channeld.c:3102 (main) 0x562b824ee14b' -backtrace: (null):0 ((null)) 0x7f1028c28ce2' -backtrace: (null):0 ((null)) 0x562b824e57fd' -backtrace: (null):0 ((null)) 0xffffffffffffffff' -lightning_channeld: FATAL SIGNAL 6 (version v0.7.0-397-gd803275) - """ - bolt11 = l1.rpc.invoice('100000sat', 'inv', 'for testing')['bolt11'] - result = l2.rpc.pay(bolt11) diff --git a/tests/test_pay.py b/tests/test_pay.py index 368db720a6a2..1654db18375b 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -2045,3 +2045,95 @@ def test_setchannelfee_all(node_factory, bitcoind): assert result['channels'][0]['short_channel_id'] == scid2 assert result['channels'][1]['peer_id'] == l3.info['id'] assert result['channels'][1]['short_channel_id'] == scid3 + + +@pytest.mark.xfail(strict=True) +def test_channel_drainage(node_factory, bitcoind): + """Test channel drainage. + + Test to drains a channels as much as possible, + especially in regards to commitment fee: + + [l1] <=> [l2] + """ + sats = 10**6 + l1, l2 = node_factory.line_graph(2, fundamount=sats, wait_for_announce=True) + + # wait for everyone to see every channel as active + for n in [l1, l2]: + wait_for(lambda: [c['active'] for c in n.rpc.listchannels()['channels']] == [True] * 2 * 1) + + spendable_l1 = l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] + spendable_l2 = l2.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] + + # so spendable is total capacity minus reserves + amount = spendable_l1 + # the next substraction is up to the millisatoshi the _exact_ value + # we need to make getroute find a route without fuzz. why is that? + amount -= Millisatoshi("10009800msat") + # the next substraction is to get around "WIRE_TEMPORARY_CHANNEL_FAILURE: Capacity exceeded" + # caused by the HTLC commitment fees at l1 + amount -= Millisatoshi("3431sat") + + payment_hash = l2.rpc.invoice('any', 'inv', 'for testing')['payment_hash'] + route = l1.rpc.getroute(l2.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] + fees = route[0]['amount_msat'] - amount + print("spendable:%s amount:%s fees:%s" % (spendable_l1, amount, fees)) + result = l1.rpc.sendpay(route, payment_hash) + print("sendpay", result) + result = l1.rpc.waitsendpay(payment_hash, 10) + print("waitsendpay", result) + + # wait until spendable is updated for both nodes + spendable_l1_bak = spendable_l1 + spendable_l2_bak = spendable_l2 + while spendable_l1_bak == spendable_l1 or spendable_l2_bak == spendable_l2: + spendable_l1 = l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] + spendable_l2 = l2.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] + + # now we drain twice to try to get into invalid channel state + # NOTE: draining twice is possible because the required commitment + # fees are less when there little in the channel. dunno why. + print("spendable after first drain", spendable_l1) # 13440800msat + amount = spendable_l1 + # now we substract again as much as needed to get around "Capacity exceeded" + amount -= Millisatoshi("10860sat") + payment_hash = l2.rpc.invoice('any', 'inv2', 'for testing')['payment_hash'] + route = l1.rpc.getroute(l2.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] + fees = route[0]['amount_msat'] - amount + print("spendable:%s amount:%s fees:%s" % (spendable_l1, amount, fees)) + result = l1.rpc.sendpay(route, payment_hash) + print("sendpay", result) + result = l1.rpc.waitsendpay(payment_hash, 10) + print("waitsendpay", result) + # wait again until spendable is updated for both nodes + spendable_l1_bak = spendable_l1 + spendable_l2_bak = spendable_l2 + while spendable_l1_bak == spendable_l1 or spendable_l2_bak == spendable_l2: + spendable_l1 = l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] + spendable_l2 = l2.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] + + # in the broken state the next bigger payment from l2 to l1 will crash the daemon at l2. + # Note1: A smaller payment (i.e. 10000sat) unlocks this state and recovers. + # Note2: When the remote is LND the payment will cracefully fail until the + # broken state is unlocked and recovered with a small initial payment. + """ +lightning_channeld: channeld/channeld.c:1382: handle_peer_commit_sig: Assertion `can_funder_afford_feerate(peer->channel, peer->channel->view[LOCAL] .feerate_per_kw)' failed. +FATAL SIGNAL 6 (version v0.7.0-397-gd803275)' +backtrace: common/daemon.c:45 (send_backtrace) 0x562b824fa13f' +backtrace: common/daemon.c:53 (crashdump) 0x562b824fa18f' +backtrace: (null):0 ((null)) 0x7f1028c3c8af' +backtrace: (null):0 ((null)) 0x7f1028c3c82f' +backtrace: (null):0 ((null)) 0x7f1028c27671' +backtrace: (null):0 ((null)) 0x7f1028c27547' +backtrace: (null):0 ((null)) 0x7f1028c34db5' +backtrace: channeld/channeld.c:1380 (handle_peer_commit_sig) 0x562b824e984d' +backtrace: channeld/channeld.c:1819 (peer_in) 0x562b824eadcd' +backtrace: channeld/channeld.c:3102 (main) 0x562b824ee14b' +backtrace: (null):0 ((null)) 0x7f1028c28ce2' +backtrace: (null):0 ((null)) 0x562b824e57fd' +backtrace: (null):0 ((null)) 0xffffffffffffffff' +lightning_channeld: FATAL SIGNAL 6 (version v0.7.0-397-gd803275) + """ + bolt11 = l1.rpc.invoice('100000sat', 'inv', 'for testing')['bolt11'] + result = l2.rpc.pay(bolt11) From 1aea063804b4dbf253ba0d3176c5f2a0bb1afb1e Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 31 May 2019 17:00:32 +0930 Subject: [PATCH 05/13] pytest: extract separate tests that spendable_msat is accurate. Turns out we needed more comprehensive testing; we ended up with three separate tests. To avoid changing test_channel_drainage as we fix spendable_msat, I substituted raw numbers there. The first is a variation of the existing tests, testing we can't exceed spendable_msat, and we can pay it, both ways. The second is with a larger amount, which triggers a different problem. The final is with a giant channel, which tests our 2^32-1 msat cap. Signed-off-by: Rusty Russell --- tests/test_pay.py | 129 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 97 insertions(+), 32 deletions(-) diff --git a/tests/test_pay.py b/tests/test_pay.py index 1654db18375b..06841797a01e 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -1,7 +1,7 @@ from fixtures import * # noqa: F401,F403 from flaky import flaky # noqa: F401 from lightning import RpcError, Millisatoshi -from utils import DEVELOPER, wait_for, only_one, sync_blockheight, SLOW_MACHINE +from utils import DEVELOPER, wait_for, only_one, sync_blockheight, SLOW_MACHINE, TIMEOUT import copy @@ -2047,6 +2047,92 @@ def test_setchannelfee_all(node_factory, bitcoind): assert result['channels'][1]['short_channel_id'] == scid3 +@pytest.mark.xfail(strict=True) +def test_channel_spendable(node_factory, bitcoind): + """Test that spendable_msat is accurate""" + sats = 10**6 + l1, l2 = node_factory.line_graph(2, fundamount=sats, wait_for_announce=True, + opts={'plugin': 'tests/plugins/hold_invoice.py', 'holdtime': str(TIMEOUT / 2)}) + + payment_hash = l2.rpc.invoice('any', 'inv', 'for testing')['payment_hash'] + + # We should be able to spend this much, and not one msat more! + amount = l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] + route = l1.rpc.getroute(l2.info['id'], amount + 1, riskfactor=1, fuzzpercent=0)['route'] + l1.rpc.sendpay(route, payment_hash) + + # This should fail locally with "capacity exceeded" + with pytest.raises(RpcError, match=r"Capacity exceeded.*'erring_index': 0"): + l1.rpc.waitsendpay(payment_hash, TIMEOUT) + + # Exact amount should succeed. + route = l1.rpc.getroute(l2.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] + l1.rpc.sendpay(route, payment_hash) + + # Amount should drop to 0 once HTLC is sent; we have time, thanks to + # hold_invoice.py plugin. + wait_for(lambda: len(l1.rpc.listpeers()['peers'][0]['channels'][0]['htlcs']) == 1) + assert l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] == Millisatoshi(0) + l1.rpc.waitsendpay(payment_hash, TIMEOUT) + + # Make sure l2 thinks it's all over. + wait_for(lambda: len(l2.rpc.listpeers()['peers'][0]['channels'][0]['htlcs']) == 0) + # Now, reverse should work similarly. + payment_hash = l1.rpc.invoice('any', 'inv', 'for testing')['payment_hash'] + amount = l2.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] + + # Turns out we this won't route, as it's over max - reserve: + route = l2.rpc.getroute(l1.info['id'], amount + 1, riskfactor=1, fuzzpercent=0)['route'] + l2.rpc.sendpay(route, payment_hash) + + # This should fail locally with "capacity exceeded" + with pytest.raises(RpcError, match=r"Capacity exceeded.*'erring_index': 0"): + l2.rpc.waitsendpay(payment_hash, TIMEOUT) + + # Exact amount should succeed. + route = l2.rpc.getroute(l1.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] + l2.rpc.sendpay(route, payment_hash) + + # Amount should drop to 0 once HTLC is sent; we have time, thanks to + # hold_invoice.py plugin. + wait_for(lambda: len(l2.rpc.listpeers()['peers'][0]['channels'][0]['htlcs']) == 1) + assert l2.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] == Millisatoshi(0) + l2.rpc.waitsendpay(payment_hash, TIMEOUT) + + +@pytest.mark.xfail(strict=True) +def test_channel_spendable_large(node_factory, bitcoind): + """Test that spendable_msat is accurate for large channels""" + # This is almost the max allowable spend. + sats = 4294967 + l1, l2 = node_factory.line_graph(2, fundamount=sats, wait_for_announce=True, + opts={'plugin': 'tests/plugins/hold_invoice.py', 'holdtime': str(TIMEOUT / 2)}) + + payment_hash = l2.rpc.invoice('any', 'inv', 'for testing')['payment_hash'] + + # We should be able to spend this much, and not one msat more! + amount = l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] + + # route or waitsendpay fill fail. + with pytest.raises(RpcError): + route = l1.rpc.getroute(l2.info['id'], amount + 1, riskfactor=1, fuzzpercent=0)['route'] + l1.rpc.sendpay(route, payment_hash) + l1.rpc.waitsendpay(payment_hash, TIMEOUT) + + # Exact amount should succeed. + route = l1.rpc.getroute(l2.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] + l1.rpc.sendpay(route, payment_hash) + l1.rpc.waitsendpay(payment_hash, TIMEOUT) + + +@pytest.mark.xfail(strict=True) +def test_channel_spendable_capped(node_factory, bitcoind): + """Test that spendable_msat is capped at 2^32-1""" + sats = 16777215 + l1, l2 = node_factory.line_graph(2, fundamount=sats, wait_for_announce=False) + assert l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] == Millisatoshi(0xFFFFFFFF) + + @pytest.mark.xfail(strict=True) def test_channel_drainage(node_factory, bitcoind): """Test channel drainage. @@ -2063,55 +2149,34 @@ def test_channel_drainage(node_factory, bitcoind): for n in [l1, l2]: wait_for(lambda: [c['active'] for c in n.rpc.listchannels()['channels']] == [True] * 2 * 1) - spendable_l1 = l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] - spendable_l2 = l2.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] - - # so spendable is total capacity minus reserves - amount = spendable_l1 - # the next substraction is up to the millisatoshi the _exact_ value - # we need to make getroute find a route without fuzz. why is that? - amount -= Millisatoshi("10009800msat") - # the next substraction is to get around "WIRE_TEMPORARY_CHANNEL_FAILURE: Capacity exceeded" - # caused by the HTLC commitment fees at l1 - amount -= Millisatoshi("3431sat") - + amount = Millisatoshi("976559200msat") payment_hash = l2.rpc.invoice('any', 'inv', 'for testing')['payment_hash'] route = l1.rpc.getroute(l2.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] fees = route[0]['amount_msat'] - amount - print("spendable:%s amount:%s fees:%s" % (spendable_l1, amount, fees)) result = l1.rpc.sendpay(route, payment_hash) print("sendpay", result) result = l1.rpc.waitsendpay(payment_hash, 10) print("waitsendpay", result) - # wait until spendable is updated for both nodes - spendable_l1_bak = spendable_l1 - spendable_l2_bak = spendable_l2 - while spendable_l1_bak == spendable_l1 or spendable_l2_bak == spendable_l2: - spendable_l1 = l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] - spendable_l2 = l2.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] + # wait until totally settled + wait_for(lambda: len(l1.rpc.listpeers()['peers'][0]['channels'][0]['htlcs']) == 0) + wait_for(lambda: len(l2.rpc.listpeers()['peers'][0]['channels'][0]['htlcs']) == 0) # now we drain twice to try to get into invalid channel state # NOTE: draining twice is possible because the required commitment # fees are less when there little in the channel. dunno why. - print("spendable after first drain", spendable_l1) # 13440800msat - amount = spendable_l1 - # now we substract again as much as needed to get around "Capacity exceeded" - amount -= Millisatoshi("10860sat") + amount = Millisatoshi("2580800msat") payment_hash = l2.rpc.invoice('any', 'inv2', 'for testing')['payment_hash'] route = l1.rpc.getroute(l2.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] fees = route[0]['amount_msat'] - amount - print("spendable:%s amount:%s fees:%s" % (spendable_l1, amount, fees)) result = l1.rpc.sendpay(route, payment_hash) print("sendpay", result) - result = l1.rpc.waitsendpay(payment_hash, 10) + result = l1.rpc.waitsendpay(payment_hash, TIMEOUT) print("waitsendpay", result) - # wait again until spendable is updated for both nodes - spendable_l1_bak = spendable_l1 - spendable_l2_bak = spendable_l2 - while spendable_l1_bak == spendable_l1 or spendable_l2_bak == spendable_l2: - spendable_l1 = l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] - spendable_l2 = l2.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] + + # wait until totally settled + wait_for(lambda: len(l1.rpc.listpeers()['peers'][0]['channels'][0]['htlcs']) == 0) + wait_for(lambda: len(l2.rpc.listpeers()['peers'][0]['channels'][0]['htlcs']) == 0) # in the broken state the next bigger payment from l2 to l1 will crash the daemon at l2. # Note1: A smaller payment (i.e. 10000sat) unlocks this state and recovers. From b0918afc20f6098bdb9f8a69dee6282a825e3091 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 31 May 2019 17:00:32 +0930 Subject: [PATCH 06/13] channeld: extract HTLC trim logic into common. Signed-off-by: Rusty Russell --- channeld/Makefile | 1 + channeld/commit_tx.c | 33 +++------------------------------ channeld/test/Makefile | 1 + common/Makefile | 1 + common/htlc_trim.c | 41 +++++++++++++++++++++++++++++++++++++++++ common/htlc_trim.h | 14 ++++++++++++++ common/htlc_tx.h | 3 +++ 7 files changed, 64 insertions(+), 30 deletions(-) create mode 100644 common/htlc_trim.c create mode 100644 common/htlc_trim.h diff --git a/channeld/Makefile b/channeld/Makefile index 18224212f6f8..6e3583c90ff0 100644 --- a/channeld/Makefile +++ b/channeld/Makefile @@ -49,6 +49,7 @@ CHANNELD_COMMON_OBJS := \ common/gen_peer_status_wire.o \ common/gossip_store.o \ common/htlc_state.o \ + common/htlc_trim.o \ common/htlc_tx.o \ common/htlc_wire.o \ common/initial_channel.o \ diff --git a/channeld/commit_tx.c b/channeld/commit_tx.c index c0b917074a94..1ee72406f348 100644 --- a/channeld/commit_tx.c +++ b/channeld/commit_tx.c @@ -2,6 +2,7 @@ #include #include #include +#include #include #include #include @@ -16,36 +17,8 @@ static bool trim(const struct htlc *htlc, struct amount_sat dust_limit, enum side side) { - struct amount_sat htlc_fee, htlc_min; - - /* BOLT #3: - * - * - for every offered HTLC: - * - if the HTLC amount minus the HTLC-timeout fee would be less than - * `dust_limit_satoshis` set by the transaction owner: - * - MUST NOT contain that output. - * - otherwise: - * - MUST be generated as specified in - * [Offered HTLC Outputs](#offered-htlc-outputs). - */ - if (htlc_owner(htlc) == side) - htlc_fee = htlc_timeout_fee(feerate_per_kw); - /* BOLT #3: - * - * - for every received HTLC: - * - if the HTLC amount minus the HTLC-success fee would be less than - * `dust_limit_satoshis` set by the transaction owner: - * - MUST NOT contain that output. - * - otherwise: - * - MUST be generated as specified in - */ - else - htlc_fee = htlc_success_fee(feerate_per_kw); - - /* If these overflow, it implies htlc must be less. */ - if (!amount_sat_add(&htlc_min, dust_limit, htlc_fee)) - return true; - return amount_msat_less_sat(htlc->amount, htlc_min); + return htlc_is_trimmed(htlc_owner(htlc), htlc->amount, + feerate_per_kw, dust_limit, side); } size_t commit_tx_num_untrimmed(const struct htlc **htlcs, diff --git a/channeld/test/Makefile b/channeld/test/Makefile index 55403a1fc0db..f197c070d5f8 100644 --- a/channeld/test/Makefile +++ b/channeld/test/Makefile @@ -11,6 +11,7 @@ CHANNELD_TEST_COMMON_OBJS := \ common/amount.o \ common/daemon_conn.o \ common/htlc_state.o \ + common/htlc_trim.o \ common/htlc_tx.o \ common/initial_commit_tx.o \ common/key_derive.o \ diff --git a/common/Makefile b/common/Makefile index 951611bea108..f53a7710c206 100644 --- a/common/Makefile +++ b/common/Makefile @@ -21,6 +21,7 @@ COMMON_SRC_NOGEN := \ common/gossip_store.c \ common/hash_u5.c \ common/htlc_state.c \ + common/htlc_trim.c \ common/htlc_tx.c \ common/htlc_wire.c \ common/initial_channel.c \ diff --git a/common/htlc_trim.c b/common/htlc_trim.c new file mode 100644 index 000000000000..e2d01f8eb090 --- /dev/null +++ b/common/htlc_trim.c @@ -0,0 +1,41 @@ +#include +#include + +/* If this htlc too small to create an output on @side's commitment tx? */ +bool htlc_is_trimmed(enum side htlc_owner, + struct amount_msat htlc_amount, + u32 feerate_per_kw, + struct amount_sat dust_limit, + enum side side) +{ + struct amount_sat htlc_fee, htlc_min; + + /* BOLT #3: + * + * - for every offered HTLC: + * - if the HTLC amount minus the HTLC-timeout fee would be less than + * `dust_limit_satoshis` set by the transaction owner: + * - MUST NOT contain that output. + * - otherwise: + * - MUST be generated as specified in + * [Offered HTLC Outputs](#offered-htlc-outputs). + */ + if (htlc_owner == side) + htlc_fee = htlc_timeout_fee(feerate_per_kw); + /* BOLT #3: + * + * - for every received HTLC: + * - if the HTLC amount minus the HTLC-success fee would be less than + * `dust_limit_satoshis` set by the transaction owner: + * - MUST NOT contain that output. + * - otherwise: + * - MUST be generated as specified in + */ + else + htlc_fee = htlc_success_fee(feerate_per_kw); + + /* If these overflow, it implies htlc must be less. */ + if (!amount_sat_add(&htlc_min, dust_limit, htlc_fee)) + return true; + return amount_msat_less_sat(htlc_amount, htlc_min); +} diff --git a/common/htlc_trim.h b/common/htlc_trim.h new file mode 100644 index 000000000000..3a8a2300385e --- /dev/null +++ b/common/htlc_trim.h @@ -0,0 +1,14 @@ +#ifndef LIGHTNING_COMMON_HTLC_TRIM_H +#define LIGHTNING_COMMON_HTLC_TRIM_H +#include "config.h" +#include +#include + +/* If this htlc too small to create an output on @side's commitment tx? */ +bool htlc_is_trimmed(enum side htlc_owner, + struct amount_msat htlc_amount, + u32 feerate_per_kw, + struct amount_sat dust_limit, + enum side side); + +#endif /* LIGHTNING_COMMON_HTLC_TRIM_H */ diff --git a/common/htlc_tx.h b/common/htlc_tx.h index b0cf87101007..93e7b771c9d4 100644 --- a/common/htlc_tx.h +++ b/common/htlc_tx.h @@ -4,9 +4,12 @@ #include #include +struct bitcoin_signature; +struct bitcoin_txid; struct keyset; struct preimage; struct pubkey; +struct ripemd160; static inline struct amount_sat htlc_timeout_fee(u32 feerate_per_kw) { From 7b066d6687923d0aad9c8a35a4c533b3641c9478 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 31 May 2019 17:00:32 +0930 Subject: [PATCH 07/13] lightningd: make 'spendable_msatoshi' more realistic. Take into account the fee we'd have to pay if we're the funder, and also drop to 0 if the amount is less than the smallest HTLC the peer will accept. Signed-off-by: Rusty Russell --- lightningd/Makefile | 1 + lightningd/peer_control.c | 63 +++++++++++++++++++++ lightningd/test/run-invoice-select-inchan.c | 7 +++ wallet/test/run-wallet.c | 7 +++ 4 files changed, 78 insertions(+) diff --git a/lightningd/Makefile b/lightningd/Makefile index 462447fd1810..115f2eb90102 100644 --- a/lightningd/Makefile +++ b/lightningd/Makefile @@ -31,6 +31,7 @@ LIGHTNINGD_COMMON_OBJS := \ common/gen_status_wire.o \ common/hash_u5.o \ common/htlc_state.o \ + common/htlc_trim.o \ common/htlc_wire.o \ common/key_derive.o \ common/io_lock.o \ diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 5184534d5653..02b457844653 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -436,6 +437,7 @@ static void json_add_htlcs(struct lightningd *ld, struct htlc_in_map_iter ini; const struct htlc_out *hout; struct htlc_out_map_iter outi; + u32 local_feerate = channel->channel_info.feerate_per_kw[LOCAL]; /* FIXME: Add more fields. */ json_array_start(response, "htlcs"); @@ -455,6 +457,9 @@ static void json_add_htlcs(struct lightningd *ld, &hin->payment_hash, sizeof(hin->payment_hash)); json_add_string(response, "state", htlc_state_name(hin->hstate)); + if (htlc_is_trimmed(REMOTE, hin->msat, local_feerate, + channel->our_config.dust_limit, LOCAL)) + json_add_bool(response, "local_trimmed", true); json_object_end(response); } @@ -474,6 +479,9 @@ static void json_add_htlcs(struct lightningd *ld, &hout->payment_hash, sizeof(hout->payment_hash)); json_add_string(response, "state", htlc_state_name(hout->hstate)); + if (htlc_is_trimmed(LOCAL, hout->msat, local_feerate, + channel->our_config.dust_limit, LOCAL)) + json_add_bool(response, "local_trimmed", true); json_object_end(response); } json_array_end(response); @@ -491,6 +499,49 @@ static void json_add_sat_only(struct json_stream *result, type_to_string(tmpctx, struct amount_msat, &msat)); } +/* This is quite a lot of work to figure out what it would cost us! */ +static struct amount_sat commit_txfee(const struct channel *channel, + struct amount_msat spendable) +{ + /* FIXME: make per-channel htlc maps! */ + const struct htlc_in *hin; + struct htlc_in_map_iter ini; + const struct htlc_out *hout; + struct htlc_out_map_iter outi; + struct lightningd *ld = channel->peer->ld; + u32 local_feerate = channel->channel_info.feerate_per_kw[LOCAL]; + size_t num_untrimmed_htlcs = 0; + + /* Assume we tried to spend "spendable" */ + if (!htlc_is_trimmed(LOCAL, spendable, + local_feerate, channel->our_config.dust_limit, + LOCAL)) + num_untrimmed_htlcs++; + + for (hin = htlc_in_map_first(&ld->htlcs_in, &ini); + hin; + hin = htlc_in_map_next(&ld->htlcs_in, &ini)) { + if (hin->key.channel != channel) + continue; + if (!htlc_is_trimmed(REMOTE, hin->msat, local_feerate, + channel->our_config.dust_limit, + LOCAL)) + num_untrimmed_htlcs++; + } + for (hout = htlc_out_map_first(&ld->htlcs_out, &outi); + hout; + hout = htlc_out_map_next(&ld->htlcs_out, &outi)) { + if (hout->key.channel != channel) + continue; + if (!htlc_is_trimmed(LOCAL, hout->msat, local_feerate, + channel->our_config.dust_limit, + LOCAL)) + num_untrimmed_htlcs++; + } + + return commit_tx_base_fee(local_feerate, num_untrimmed_htlcs); +} + static void json_add_channel(struct lightningd *ld, struct json_stream *response, const char *key, const struct channel *channel) @@ -605,6 +656,18 @@ static void json_add_channel(struct lightningd *ld, channel->channel_info.their_config.channel_reserve)) spendable = AMOUNT_MSAT(0); + /* If we're funder, subtract txfees we'll need to spend this */ + if (channel->funder == LOCAL) { + if (!amount_msat_sub_sat(&spendable, spendable, + commit_txfee(channel, spendable))) + spendable = AMOUNT_MSAT(0); + } + + /* We can't offer an HTLC less than the other side will accept. */ + if (amount_msat_less(spendable, + channel->channel_info.their_config.htlc_minimum)) + spendable = AMOUNT_MSAT(0); + json_add_amount_msat_compat(response, spendable, "spendable_msatoshi", "spendable_msat"); json_add_amount_msat_compat(response, diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c index 2197f28db487..fb21f5bb68c1 100644 --- a/lightningd/test/run-invoice-select-inchan.c +++ b/lightningd/test/run-invoice-select-inchan.c @@ -112,6 +112,13 @@ const struct chainparams *get_chainparams(const struct lightningd *ld UNNEEDED) /* Generated stub for get_log_level */ enum log_level get_log_level(struct log_book *lr UNNEEDED) { fprintf(stderr, "get_log_level called!\n"); abort(); } +/* Generated stub for htlc_is_trimmed */ +bool htlc_is_trimmed(enum side htlc_owner UNNEEDED, + struct amount_msat htlc_amount UNNEEDED, + u32 feerate_per_kw UNNEEDED, + struct amount_sat dust_limit UNNEEDED, + enum side side UNNEEDED) +{ fprintf(stderr, "htlc_is_trimmed called!\n"); abort(); } /* Generated stub for htlcs_reconnect */ void htlcs_reconnect(struct lightningd *ld UNNEEDED, struct htlc_in_map *htlcs_in UNNEEDED, diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index 13fefd28aa87..c207b3fa32fa 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -112,6 +112,13 @@ u32 get_block_height(const struct chain_topology *topo UNNEEDED) /* Generated stub for get_chainparams */ const struct chainparams *get_chainparams(const struct lightningd *ld UNNEEDED) { fprintf(stderr, "get_chainparams called!\n"); abort(); } +/* Generated stub for htlc_is_trimmed */ +bool htlc_is_trimmed(enum side htlc_owner UNNEEDED, + struct amount_msat htlc_amount UNNEEDED, + u32 feerate_per_kw UNNEEDED, + struct amount_sat dust_limit UNNEEDED, + enum side side UNNEEDED) +{ fprintf(stderr, "htlc_is_trimmed called!\n"); abort(); } /* Generated stub for invoices_create */ bool invoices_create(struct invoices *invoices UNNEEDED, struct invoice *pinvoice UNNEEDED, From acce773b081029e8aa501eba3f416f85c45f1315 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 31 May 2019 17:00:33 +0930 Subject: [PATCH 08/13] gossipd: don't charge ourselves fees when calculating route. This means there's now a semantic difference between the default `fromid` and setting `fromid` explicitly to our own node_id. In the default case, it means we don't charge ourselves fees on the route. This means we can spend the full channel balance. We still want to consider the pricing of local channels, however: there's a *reason* to discount one over another, and that is to bias things. So we add the first-hop fee to the *risk* value instead. Signed-off-by: Rusty Russell --- gossipd/gossip_wire.csv | 3 +- gossipd/gossipd.c | 14 ++++++--- gossipd/routing.c | 62 +++++++++++++++++++++++++++---------- lightningd/gossip_control.c | 2 +- 4 files changed, 58 insertions(+), 23 deletions(-) diff --git a/gossipd/gossip_wire.csv b/gossipd/gossip_wire.csv index 575d3ae775b5..ac740ce097f5 100644 --- a/gossipd/gossip_wire.csv +++ b/gossipd/gossip_wire.csv @@ -26,7 +26,8 @@ gossip_getnodes_reply,,nodes,num_nodes*struct gossip_getnodes_entry # Pass JSON-RPC getroute call through gossip_getroute_request,3006 -gossip_getroute_request,,source,struct node_id +# Source defaults to "us", and means we don't consider first-hop channel fees +gossip_getroute_request,,source,?struct node_id gossip_getroute_request,,destination,struct node_id gossip_getroute_request,,msatoshi,struct amount_msat # We don't pass doubles, so pass riskfactor * 1000000. diff --git a/gossipd/gossipd.c b/gossipd/gossipd.c index 6503c8614071..1c30ea715ee6 100644 --- a/gossipd/gossipd.c +++ b/gossipd/gossipd.c @@ -1880,7 +1880,7 @@ static struct io_plan *gossip_init(struct io_conn *conn, static struct io_plan *getroute_req(struct io_conn *conn, struct daemon *daemon, const u8 *msg) { - struct node_id source, destination; + struct node_id *source, destination; struct amount_msat msat; u32 final_cltv; u64 riskfactor_by_million; @@ -1895,7 +1895,12 @@ static struct io_plan *getroute_req(struct io_conn *conn, struct daemon *daemon, * we'll pay), how to trade off more locktime vs. more fees, and how * much cltv we need a the final node to give exact values for each * intermediate hop, as well as how much random fuzz to inject to - * avoid being too predictable. */ + * avoid being too predictable. + * + * We also treat routing slightly differently if we're asking + * for a route from ourselves (the usual case): in that case, + * we don't have to consider fees on our own outgoing channels. + */ if (!fromwire_gossip_getroute_request(msg, msg, &source, &destination, &msat, &riskfactor_by_million, @@ -1905,12 +1910,13 @@ static struct io_plan *getroute_req(struct io_conn *conn, struct daemon *daemon, master_badmsg(WIRE_GOSSIP_GETROUTE_REQUEST, msg); status_trace("Trying to find a route from %s to %s for %s", - type_to_string(tmpctx, struct node_id, &source), + source + ? type_to_string(tmpctx, struct node_id, source) : "(me)", type_to_string(tmpctx, struct node_id, &destination), type_to_string(tmpctx, struct amount_msat, &msat)); /* routing.c does all the hard work; can return NULL. */ - hops = get_route(tmpctx, daemon->rstate, &source, &destination, + hops = get_route(tmpctx, daemon->rstate, source, &destination, msat, riskfactor_by_million / 1000000.0, final_cltv, fuzz, pseudorand_u64(), excluded, max_hops); diff --git a/gossipd/routing.c b/gossipd/routing.c index 7604316bbd6a..93a5ab137e7a 100644 --- a/gossipd/routing.c +++ b/gossipd/routing.c @@ -517,6 +517,7 @@ static bool fuzz_fee(u64 *fee, * sets newtotal and newrisk */ static bool can_reach(const struct half_chan *c, const struct short_channel_id *scid, + bool no_charge, struct amount_msat total, struct amount_msat risk, double riskfactor, @@ -530,18 +531,29 @@ static bool can_reach(const struct half_chan *c, if (!amount_msat_fee(&fee, total, c->base_fee, c->proportional_fee)) return false; - if (!fuzz_fee(&fee.millisatoshis, scid, fuzz, base_seed)) /* Raw: double manipulation */ + if (!fuzz_fee(&fee.millisatoshis, scid, fuzz, base_seed)) /* Raw: double manipulation */ return false; - if (!amount_msat_add(newtotal, total, fee)) - return false; + if (no_charge) { + *newtotal = total; + + /* We still want to consider the "charge", since it's indicative + * of a bias (we discounted one channel for a reason), but we + * don't pay it. So we count it as additional risk. */ + if (!amount_msat_add(newrisk, risk, fee)) + return false; + } else { + *newrisk = risk; + + if (!amount_msat_add(newtotal, total, fee)) + return false; + } /* Skip a channel if it indicated that it won't route the * requested amount. */ if (!hc_can_carry(c, *newtotal)) return false; - *newrisk = risk; if (!risk_add_fee(newrisk, *newtotal, c->delay, riskfactor, riskbias)) return false; @@ -736,6 +748,7 @@ static void remove_unvisited(struct node *node, struct unvisited *unvisited, static void update_unvisited_neighbors(struct routing_state *rstate, struct node *cur, + const struct node *me, double riskfactor, u64 riskbias, double fuzz, @@ -772,7 +785,9 @@ static void update_unvisited_neighbors(struct routing_state *rstate, continue; } - if (!can_reach(&chan->half[idx], &chan->scid, + /* We're looking at channels *backwards*, so peer == me + * is the right test here for whether we don't charge fees. */ + if (!can_reach(&chan->half[idx], &chan->scid, peer == me, cur->dijkstra.total, cur->dijkstra.risk, riskfactor, riskbias, fuzz, base_seed, &total, &risk)) { @@ -819,6 +834,7 @@ static struct node *first_unvisited(struct unvisited *unvisited) static void dijkstra(struct routing_state *rstate, const struct node *dst, + const struct node *me, double riskfactor, u64 riskbias, double fuzz, const struct siphash_seed *base_seed, @@ -828,7 +844,8 @@ static void dijkstra(struct routing_state *rstate, struct node *cur; while ((cur = first_unvisited(unvisited)) != NULL) { - update_unvisited_neighbors(rstate, cur, riskfactor, riskbias, + update_unvisited_neighbors(rstate, cur, me, + riskfactor, riskbias, fuzz, base_seed, unvisited, costfn); remove_unvisited(cur, unvisited, costfn); if (cur == dst) @@ -842,6 +859,7 @@ static struct chan **build_route(const tal_t *ctx, struct routing_state *rstate, const struct node *from, const struct node *to, + const struct node *me, double riskfactor, u64 riskbias, double fuzz, @@ -888,7 +906,7 @@ static struct chan **build_route(const tal_t *ctx, continue; } - if (!can_reach(hc, &chan->scid, + if (!can_reach(hc, &chan->scid, i == me, peer->dijkstra.total, peer->dijkstra.risk, riskfactor, riskbias, @@ -984,6 +1002,7 @@ static void dijkstra_cleanup(struct unvisited *unvisited) static struct chan ** find_shorter_route(const tal_t *ctx, struct routing_state *rstate, struct node *src, struct node *dst, + const struct node *me, struct amount_msat msat, size_t max_hops, double fuzz, const struct siphash_seed *base_seed, @@ -1017,12 +1036,12 @@ find_shorter_route(const tal_t *ctx, struct routing_state *rstate, SUPERVERBOSE("Running shortest path from %s -> %s", type_to_string(tmpctx, struct node_id, &dst->id), type_to_string(tmpctx, struct node_id, &src->id)); - dijkstra(rstate, dst, riskfactor, 1, fuzz, base_seed, + dijkstra(rstate, dst, NULL, riskfactor, 1, fuzz, base_seed, unvisited, shortest_cost_function); dijkstra_cleanup(unvisited); /* This must succeed, since we found a route before */ - short_route = build_route(ctx, rstate, dst, src, riskfactor, 1, + short_route = build_route(ctx, rstate, dst, src, me, riskfactor, 1, fuzz, base_seed, fee); assert(short_route); if (!amount_msat_sub(&short_cost, @@ -1064,11 +1083,12 @@ find_shorter_route(const tal_t *ctx, struct routing_state *rstate, unvisited = dijkstra_prepare(tmpctx, rstate, src, msat, normal_cost_function); - dijkstra(rstate, dst, riskfactor, riskbias, fuzz, base_seed, + dijkstra(rstate, dst, me, riskfactor, riskbias, fuzz, base_seed, unvisited, normal_cost_function); dijkstra_cleanup(unvisited); - route = build_route(ctx, rstate, dst, src, riskfactor, riskbias, + route = build_route(ctx, rstate, dst, src, me, + riskfactor, riskbias, fuzz, base_seed, &this_fee); SUPERVERBOSE("riskbias %"PRIu64" rlen %zu", @@ -1111,20 +1131,28 @@ find_route(const tal_t *ctx, struct routing_state *rstate, struct amount_msat *fee) { struct node *src, *dst; + const struct node *me; struct unvisited *unvisited; struct chan **route; /* Note: we map backwards, since we know the amount of satoshi we want * at the end, and need to derive how much we need to send. */ - dst = get_node(rstate, from); src = get_node(rstate, to); + /* If from is NULL, that's means it's us. */ + if (!from) + me = dst = get_node(rstate, &rstate->local_id); + else { + dst = get_node(rstate, from); + me = NULL; + } + if (!src) { status_info("find_route: cannot find %s", type_to_string(tmpctx, struct node_id, to)); return NULL; } else if (!dst) { - status_info("find_route: cannot find myself (%s)", + status_info("find_route: cannot find source (%s)", type_to_string(tmpctx, struct node_id, to)); return NULL; } else if (dst == src) { @@ -1135,17 +1163,17 @@ find_route(const tal_t *ctx, struct routing_state *rstate, unvisited = dijkstra_prepare(tmpctx, rstate, src, msat, normal_cost_function); - dijkstra(rstate, dst, riskfactor, 1, fuzz, base_seed, + dijkstra(rstate, dst, me, riskfactor, 1, fuzz, base_seed, unvisited, normal_cost_function); dijkstra_cleanup(unvisited); - route = build_route(ctx, rstate, dst, src, riskfactor, 1, + route = build_route(ctx, rstate, dst, src, me, riskfactor, 1, fuzz, base_seed, fee); if (tal_count(route) <= max_hops) return route; /* This is the far more unlikely case */ - return find_shorter_route(ctx, rstate, src, dst, msat, + return find_shorter_route(ctx, rstate, src, dst, me, msat, max_hops, fuzz, base_seed, route, fee); } @@ -2334,7 +2362,7 @@ struct route_hop *get_route(const tal_t *ctx, struct routing_state *rstate, total_delay += c->delay; n = other_node(n, route[i]); } - assert(node_id_eq(&n->id, source)); + assert(node_id_eq(&n->id, source ? source : &rstate->local_id)); return hops; } diff --git a/lightningd/gossip_control.c b/lightningd/gossip_control.c index d0380a2d0ea7..b8d1bdba66dd 100644 --- a/lightningd/gossip_control.c +++ b/lightningd/gossip_control.c @@ -307,7 +307,7 @@ static struct command_result *json_getroute(struct command *cmd, p_req("msatoshi", param_msat, &msat), p_req("riskfactor", param_double, &riskfactor), p_opt_def("cltv", param_number, &cltv, 9), - p_opt_def("fromid", param_node_id, &source, ld->id), + p_opt("fromid", param_node_id, &source), p_opt_def("fuzzpercent", param_percent, &fuzz, 5.0), p_opt("exclude", param_array, &excludetok), p_opt_def("maxhops", param_number, &max_hops, From 65f8b31a6400a26d9c9ac94c70cab1a5f073b47d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 31 May 2019 17:00:33 +0930 Subject: [PATCH 09/13] lightningd: take into account outstanding HTLCs for 'spendable_msat' The current calculation ignores them, which is unrealistic. Signed-off-by: Rusty Russell --- lightningd/peer_control.c | 20 ++++++++++++++++++++ tests/test_pay.py | 1 - 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 02b457844653..e73dbde1800f 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -542,6 +542,23 @@ static struct amount_sat commit_txfee(const struct channel *channel, return commit_tx_base_fee(local_feerate, num_untrimmed_htlcs); } +static void subtract_offered_htlcs(const struct channel *channel, + struct amount_msat *amount) +{ + const struct htlc_out *hout; + struct htlc_out_map_iter outi; + struct lightningd *ld = channel->peer->ld; + + for (hout = htlc_out_map_first(&ld->htlcs_out, &outi); + hout; + hout = htlc_out_map_next(&ld->htlcs_out, &outi)) { + if (hout->key.channel != channel) + continue; + if (!amount_msat_sub(amount, *amount, hout->msat)) + *amount = AMOUNT_MSAT(0); + } +} + static void json_add_channel(struct lightningd *ld, struct json_stream *response, const char *key, const struct channel *channel) @@ -656,6 +673,9 @@ static void json_add_channel(struct lightningd *ld, channel->channel_info.their_config.channel_reserve)) spendable = AMOUNT_MSAT(0); + /* Take away any currently-offered HTLCs. */ + subtract_offered_htlcs(channel, &spendable); + /* If we're funder, subtract txfees we'll need to spend this */ if (channel->funder == LOCAL) { if (!amount_msat_sub_sat(&spendable, spendable, diff --git a/tests/test_pay.py b/tests/test_pay.py index 06841797a01e..586fe6d57706 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -2047,7 +2047,6 @@ def test_setchannelfee_all(node_factory, bitcoind): assert result['channels'][1]['short_channel_id'] == scid3 -@pytest.mark.xfail(strict=True) def test_channel_spendable(node_factory, bitcoind): """Test that spendable_msat is accurate""" sats = 10**6 From 6707ad08488e7e9b5068099eb633b8860ea90016 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 31 May 2019 17:00:33 +0930 Subject: [PATCH 10/13] lightningd: don't let spendable_msat go wumbo. Signed-off-by: Rusty Russell --- lightningd/peer_control.c | 4 ++++ tests/test_pay.py | 1 - 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index e73dbde1800f..2c5004ca1c96 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -688,6 +688,10 @@ static void json_add_channel(struct lightningd *ld, channel->channel_info.their_config.htlc_minimum)) spendable = AMOUNT_MSAT(0); + /* We can't offer an HTLC over the max payment threshold either. */ + if (amount_msat_greater(spendable, get_chainparams(ld)->max_payment)) + spendable = get_chainparams(ld)->max_payment; + json_add_amount_msat_compat(response, spendable, "spendable_msatoshi", "spendable_msat"); json_add_amount_msat_compat(response, diff --git a/tests/test_pay.py b/tests/test_pay.py index 586fe6d57706..dbd01c66e254 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -2124,7 +2124,6 @@ def test_channel_spendable_large(node_factory, bitcoind): l1.rpc.waitsendpay(payment_hash, TIMEOUT) -@pytest.mark.xfail(strict=True) def test_channel_spendable_capped(node_factory, bitcoind): """Test that spendable_msat is capped at 2^32-1""" sats = 16777215 From f4212315d82eb6fb9c9f69eb0993464e502cb453 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 31 May 2019 17:00:33 +0930 Subject: [PATCH 11/13] channeld: don't subtract both reserves from advertized htlc_max. Subtracting both arbitrarily reduces our capacity, even for ourselves since the routing logic uses this maximum. I also changed 'advertise' to 'advertize', since we use american spelling. Signed-off-by: Rusty Russell --- channeld/channeld.c | 28 +++++++++------------------- tests/test_pay.py | 2 +- 2 files changed, 10 insertions(+), 20 deletions(-) diff --git a/channeld/channeld.c b/channeld/channeld.c index c49d93450d39..8f1333f37b27 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -237,36 +237,26 @@ static const u8 *hsm_req(const tal_t *ctx, const u8 *req TAKES) * The maximum msat that this node will accept for an htlc. * It's flagged as an optional field in `channel_update`. * - * We advertise the maximum value possible, defined as the smaller + * We advertize the maximum value possible, defined as the smaller * of the remote's maximum in-flight HTLC or the total channel - * capacity minus the cumulative reserve. + * capacity the reserve we have to keep. * FIXME: does this need fuzz? */ -static struct amount_msat advertised_htlc_max(const struct channel *channel) +static struct amount_msat advertized_htlc_max(const struct channel *channel) { - struct amount_sat cumulative_reserve, lower_bound; + struct amount_sat lower_bound; struct amount_msat lower_bound_msat; /* This shouldn't fail */ - if (!amount_sat_add(&cumulative_reserve, - channel->config[LOCAL].channel_reserve, + if (!amount_sat_sub(&lower_bound, channel->funding, channel->config[REMOTE].channel_reserve)) { status_failed(STATUS_FAIL_INTERNAL_ERROR, - "reserve overflow: local %s + remote %s", - type_to_string(tmpctx, struct amount_sat, - &channel->config[LOCAL].channel_reserve), - type_to_string(tmpctx, struct amount_sat, - &channel->config[REMOTE].channel_reserve)); - } - - /* This shouldn't fail either */ - if (!amount_sat_sub(&lower_bound, channel->funding, cumulative_reserve)) { - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "funding %s - cumulative_reserve %s?", + "funding %s - remote reserve %s?", type_to_string(tmpctx, struct amount_sat, &channel->funding), type_to_string(tmpctx, struct amount_sat, - &cumulative_reserve)); + &channel->config[REMOTE] + .channel_reserve)); } if (!amount_sat_to_msat(&lower_bound_msat, lower_bound)) { @@ -313,7 +303,7 @@ static void send_channel_update(struct peer *peer, int disable_flag) peer->channel->config[REMOTE].htlc_minimum, peer->fee_base, peer->fee_per_satoshi, - advertised_htlc_max(peer->channel)); + advertized_htlc_max(peer->channel)); wire_sync_write(peer->pps->gossip_fd, take(msg)); } diff --git a/tests/test_pay.py b/tests/test_pay.py index dbd01c66e254..ad652343d6e7 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -2099,7 +2099,6 @@ def test_channel_spendable(node_factory, bitcoind): l2.rpc.waitsendpay(payment_hash, TIMEOUT) -@pytest.mark.xfail(strict=True) def test_channel_spendable_large(node_factory, bitcoind): """Test that spendable_msat is accurate for large channels""" # This is almost the max allowable spend. @@ -2118,6 +2117,7 @@ def test_channel_spendable_large(node_factory, bitcoind): l1.rpc.sendpay(route, payment_hash) l1.rpc.waitsendpay(payment_hash, TIMEOUT) + print(l2.rpc.listchannels()) # Exact amount should succeed. route = l1.rpc.getroute(l2.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] l1.rpc.sendpay(route, payment_hash) From 726ad8f92fdb5a644b2816fa1848a8617b03b98a Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 31 May 2019 17:00:33 +0930 Subject: [PATCH 12/13] channeld: don't add HTLCs if that would drive us negative. We track whether each change is affordable as we go; test_channel_drainage got us so close that the difference mattered; we hit an assert when we tried to commit the tx and realized we couldn't afford it. We should not be trying to add an HTLC if it will result in the funder being unable to afford it on either the local *or remote* commitments. Note the test still "fails" because it refuses to send the final payment. Signed-off-by: Rusty Russell --- channeld/channeld.c | 6 +- channeld/full_channel.c | 199 ++++++++++++++++++++++++++++------------ 2 files changed, 146 insertions(+), 59 deletions(-) diff --git a/channeld/channeld.c b/channeld/channeld.c index 8f1333f37b27..ac8509dbd8c3 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -1381,10 +1381,14 @@ static void handle_peer_commit_sig(struct peer *peer, const u8 *msg) } /* We were supposed to check this was affordable as we go. */ - if (peer->channel->funder == REMOTE) + if (peer->channel->funder == REMOTE) { + status_trace("Feerates are %u/%u", + peer->channel->view[LOCAL].feerate_per_kw, + peer->channel->view[REMOTE].feerate_per_kw); assert(can_funder_afford_feerate(peer->channel, peer->channel->view[LOCAL] .feerate_per_kw)); + } if (!fromwire_commitment_signed(tmpctx, msg, &channel_id, &commit_sig.s, &htlc_sigs)) diff --git a/channeld/full_channel.c b/channeld/full_channel.c index 464ff1640fe3..7dce4521cde5 100644 --- a/channeld/full_channel.c +++ b/channeld/full_channel.c @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -291,6 +292,69 @@ struct bitcoin_tx **channel_txs(const tal_t *ctx, return txs; } +/* If @side is faced with these HTLCs, how much will it have left + * above reserve (eg. to pay fees). Returns false if would be < 0. */ +static bool get_room_above_reserve(const struct channel *channel, + const struct channel_view *view, + const struct htlc **adding, + const struct htlc **removing, + enum side side, + struct amount_msat *balance) +{ + bool ok; + /* Reserve is set by the *other* side */ + struct amount_sat reserve = channel->config[!side].channel_reserve; + + *balance = view->owed[side]; + + ok = true; + for (size_t i = 0; i < tal_count(removing); i++) + ok &= balance_remove_htlc(balance, removing[i], side); + + for (size_t i = 0; i < tal_count(adding); i++) + ok &= balance_add_htlc(balance, adding[i], side); + + /* Overflow shouldn't happen, but if it does, complain */ + if (!ok) { + status_broken("Failed to add %zu remove %zu htlcs", + tal_count(adding), tal_count(removing)); + return false; + } + + if (!amount_msat_sub_sat(balance, *balance, reserve)) { + status_trace("%s cannot afford htlc: would make balance %s" + " below reserve %s", + side_to_str(side), + type_to_string(tmpctx, struct amount_msat, + balance), + type_to_string(tmpctx, struct amount_sat, + &reserve)); + return false; + } + return true; +} + +static struct amount_sat fee_for_htlcs(const struct channel *channel, + const struct channel_view *view, + const struct htlc **committed, + const struct htlc **adding, + const struct htlc **removing, + enum side side) +{ + u32 feerate = view->feerate_per_kw; + struct amount_sat dust_limit = channel->config[side].dust_limit; + size_t untrimmed; + + untrimmed = commit_tx_num_untrimmed(committed, feerate, dust_limit, + side) + + commit_tx_num_untrimmed(adding, feerate, dust_limit, + side) + - commit_tx_num_untrimmed(removing, feerate, dust_limit, + side); + + return commit_tx_base_fee(feerate, untrimmed); +} + static enum channel_add_err add_htlc(struct channel *channel, enum htlc_state state, u64 id, @@ -304,12 +368,9 @@ static enum channel_add_err add_htlc(struct channel *channel, { struct htlc *htlc, *old; struct amount_msat msat_in_htlcs, committed_msat, adding_msat, removing_msat; - struct amount_sat fee; enum side sender = htlc_state_owner(state), recipient = !sender; const struct htlc **committed, **adding, **removing; const struct channel_view *view; - bool ok; - size_t i; htlc = tal(tmpctx, struct htlc); @@ -432,71 +493,86 @@ static enum channel_add_err add_htlc(struct channel *channel, * reserve): * - SHOULD fail the channel. */ - if (channel->funder == htlc_owner(htlc)) { - u32 feerate = view->feerate_per_kw; - struct amount_sat dust_limit = channel->config[recipient].dust_limit; - size_t untrimmed; - - untrimmed = commit_tx_num_untrimmed(committed, feerate, dust_limit, - recipient) - + commit_tx_num_untrimmed(adding, feerate, dust_limit, - recipient) - - commit_tx_num_untrimmed(removing, feerate, dust_limit, - recipient); - - fee = commit_tx_base_fee(feerate, untrimmed); - } else - fee = AMOUNT_SAT(0); - - assert((s64)fee.satoshis >= 0); /* Raw: explicit signedness test */ - if (htlc_fee) *htlc_fee = fee; /* set fee output pointer if given */ - if (enforce_aggregate_limits) { - /* Figure out what balance sender would have after applying all - * pending changes. */ - struct amount_msat balance = view->owed[sender]; + struct amount_msat balance; + struct amount_sat fee = fee_for_htlcs(channel, view, + committed, + adding, + removing, + recipient); + /* set fee output pointer if given */ + if (htlc_fee) + *htlc_fee = fee; + /* This is a little subtle: * * The change is being applied to the receiver but it will * come back to the sender after revoke_and_ack. So the check * here is that the balance to the sender doesn't go below the * sender's reserve. */ - const struct amount_sat reserve - = channel->config[!sender].channel_reserve; - - assert(amount_msat_greater_eq(balance, AMOUNT_MSAT(0))); - ok = true; - for (i = 0; i < tal_count(removing); i++) - ok &= balance_remove_htlc(&balance, removing[i], sender); - assert(amount_msat_greater_eq(balance, AMOUNT_MSAT(0))); - for (i = 0; i < tal_count(adding); i++) - ok &= balance_add_htlc(&balance, adding[i], sender); - - /* Overflow shouldn't happen, but if it does, complain */ - if (!ok) { - status_broken("Failed to add %zu remove %zu htlcs", - tal_count(adding), tal_count(removing)); + if (!get_room_above_reserve(channel, view, + adding, removing, sender, + &balance)) return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED; - } - if (!amount_msat_sub_sat(&balance, balance, fee)) { - status_trace("Cannot afford fee %s with balance %s", - type_to_string(tmpctx, struct amount_sat, - &fee), - type_to_string(tmpctx, struct amount_msat, - &balance)); - return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED; + if (channel->funder == sender) { + if (amount_msat_less_sat(balance, fee)) { + status_trace("Cannot afford fee %s with %s above reserve", + type_to_string(tmpctx, struct amount_sat, + &fee), + type_to_string(tmpctx, struct amount_msat, + &balance)); + return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED; + } } - if (!amount_msat_greater_eq_sat(balance, reserve)) { - status_trace("Cannot afford fee %s: would make balance %s" - " below reserve %s", - type_to_string(tmpctx, struct amount_sat, - &fee), - type_to_string(tmpctx, struct amount_msat, - &balance), - type_to_string(tmpctx, struct amount_sat, - &reserve)); - return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED; + + /* Try not to add a payment which will take funder into fees + * on either our side or theirs. */ + if (sender == LOCAL) { + if (!get_room_above_reserve(channel, view, + adding, removing, + channel->funder, + &balance)) + return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED; + /* Should be able to afford both their own commit tx + * fee, and other's commit tx fee, which are subtly + * different! */ + fee = fee_for_htlcs(channel, view, + committed, + adding, + removing, + channel->funder); + /* set fee output pointer if given */ + if (htlc_fee && amount_sat_greater(fee, *htlc_fee)) + *htlc_fee = fee; + if (amount_msat_less_sat(balance, fee)) { + status_trace("Funder could not afford own fee %s with %s above reserve", + type_to_string(tmpctx, + struct amount_sat, + &fee), + type_to_string(tmpctx, + struct amount_msat, + &balance)); + return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED; + } + fee = fee_for_htlcs(channel, view, + committed, + adding, + removing, + !channel->funder); + /* set fee output pointer if given */ + if (htlc_fee && amount_sat_greater(fee, *htlc_fee)) + *htlc_fee = fee; + if (amount_msat_less_sat(balance, fee)) { + status_trace("Funder could not afford peer's fee %s with %s above reserve", + type_to_string(tmpctx, + struct amount_sat, + &fee), + type_to_string(tmpctx, + struct amount_msat, + &balance)); + return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED; + } } } @@ -832,6 +908,13 @@ bool can_funder_afford_feerate(const struct channel *channel, u32 feerate_per_kw type_to_string(tmpctx, struct amount_sat, &channel->config[!channel->funder].channel_reserve)); + status_trace("We need %s at feerate %u for %zu untrimmed htlcs: we have %s/%s", + type_to_string(tmpctx, struct amount_sat, &needed), + feerate_per_kw, untrimmed, + type_to_string(tmpctx, struct amount_msat, + &channel->view[LOCAL].owed[channel->funder]), + type_to_string(tmpctx, struct amount_msat, + &channel->view[REMOTE].owed[channel->funder])); return amount_msat_greater_eq_sat(channel->view[!channel->funder].owed[channel->funder], needed); } From c69beb6c0a5357883edf30d977427ae428080e23 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 31 May 2019 17:00:33 +0930 Subject: [PATCH 13/13] pytest: simplify and annotate test_channel_drainage. Remove gratuitous prints, add explanations of what's going on, and demonstrate that we can add a final trimmed HTLC but not a non-trimmed one. Signed-off-by: Rusty Russell --- tests/test_pay.py | 62 ++++++++++++++++++----------------------------- 1 file changed, 24 insertions(+), 38 deletions(-) diff --git a/tests/test_pay.py b/tests/test_pay.py index ad652343d6e7..efb12116fb7a 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -2131,7 +2131,6 @@ def test_channel_spendable_capped(node_factory, bitcoind): assert l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] == Millisatoshi(0xFFFFFFFF) -@pytest.mark.xfail(strict=True) def test_channel_drainage(node_factory, bitcoind): """Test channel drainage. @@ -2147,56 +2146,43 @@ def test_channel_drainage(node_factory, bitcoind): for n in [l1, l2]: wait_for(lambda: [c['active'] for c in n.rpc.listchannels()['channels']] == [True] * 2 * 1) + # This first HTLC drains the channel. amount = Millisatoshi("976559200msat") payment_hash = l2.rpc.invoice('any', 'inv', 'for testing')['payment_hash'] route = l1.rpc.getroute(l2.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] - fees = route[0]['amount_msat'] - amount - result = l1.rpc.sendpay(route, payment_hash) - print("sendpay", result) - result = l1.rpc.waitsendpay(payment_hash, 10) - print("waitsendpay", result) + l1.rpc.sendpay(route, payment_hash) + l1.rpc.waitsendpay(payment_hash, 10) # wait until totally settled wait_for(lambda: len(l1.rpc.listpeers()['peers'][0]['channels'][0]['htlcs']) == 0) wait_for(lambda: len(l2.rpc.listpeers()['peers'][0]['channels'][0]['htlcs']) == 0) - # now we drain twice to try to get into invalid channel state - # NOTE: draining twice is possible because the required commitment - # fees are less when there little in the channel. dunno why. + # But we can get more! By using a trimmed htlc output; this doesn't cause + # an increase in tx fee, so it's allowed. amount = Millisatoshi("2580800msat") payment_hash = l2.rpc.invoice('any', 'inv2', 'for testing')['payment_hash'] route = l1.rpc.getroute(l2.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] - fees = route[0]['amount_msat'] - amount - result = l1.rpc.sendpay(route, payment_hash) - print("sendpay", result) - result = l1.rpc.waitsendpay(payment_hash, TIMEOUT) - print("waitsendpay", result) + l1.rpc.sendpay(route, payment_hash) + l1.rpc.waitsendpay(payment_hash, TIMEOUT) # wait until totally settled wait_for(lambda: len(l1.rpc.listpeers()['peers'][0]['channels'][0]['htlcs']) == 0) wait_for(lambda: len(l2.rpc.listpeers()['peers'][0]['channels'][0]['htlcs']) == 0) - # in the broken state the next bigger payment from l2 to l1 will crash the daemon at l2. - # Note1: A smaller payment (i.e. 10000sat) unlocks this state and recovers. - # Note2: When the remote is LND the payment will cracefully fail until the - # broken state is unlocked and recovered with a small initial payment. - """ -lightning_channeld: channeld/channeld.c:1382: handle_peer_commit_sig: Assertion `can_funder_afford_feerate(peer->channel, peer->channel->view[LOCAL] .feerate_per_kw)' failed. -FATAL SIGNAL 6 (version v0.7.0-397-gd803275)' -backtrace: common/daemon.c:45 (send_backtrace) 0x562b824fa13f' -backtrace: common/daemon.c:53 (crashdump) 0x562b824fa18f' -backtrace: (null):0 ((null)) 0x7f1028c3c8af' -backtrace: (null):0 ((null)) 0x7f1028c3c82f' -backtrace: (null):0 ((null)) 0x7f1028c27671' -backtrace: (null):0 ((null)) 0x7f1028c27547' -backtrace: (null):0 ((null)) 0x7f1028c34db5' -backtrace: channeld/channeld.c:1380 (handle_peer_commit_sig) 0x562b824e984d' -backtrace: channeld/channeld.c:1819 (peer_in) 0x562b824eadcd' -backtrace: channeld/channeld.c:3102 (main) 0x562b824ee14b' -backtrace: (null):0 ((null)) 0x7f1028c28ce2' -backtrace: (null):0 ((null)) 0x562b824e57fd' -backtrace: (null):0 ((null)) 0xffffffffffffffff' -lightning_channeld: FATAL SIGNAL 6 (version v0.7.0-397-gd803275) - """ - bolt11 = l1.rpc.invoice('100000sat', 'inv', 'for testing')['bolt11'] - result = l2.rpc.pay(bolt11) + # Now, l1 is paying fees, but it can't afford a larger tx, so any + # attempt to add an HTLC which is not trimmed will fail. + payment_hash = l1.rpc.invoice('any', 'inv', 'for testing')['payment_hash'] + + # feerate_per_kw = 15000, so htlc_timeout_fee = 663 * 15000 / 1000 = 9945. + # dust_limit is 546. So it's trimmed if < 9945 + 546. + amount = Millisatoshi("10491sat") + route = l2.rpc.getroute(l1.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] + l2.rpc.sendpay(route, payment_hash) + with pytest.raises(RpcError, match=r"Capacity exceeded.*'erring_index': 0"): + l2.rpc.waitsendpay(payment_hash, TIMEOUT) + + # But if it's trimmed, we're ok. + amount -= 1 + route = l2.rpc.getroute(l1.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] + l2.rpc.sendpay(route, payment_hash) + l2.rpc.waitsendpay(payment_hash, TIMEOUT)