Channel lockup corner case workaround by rustyrussell · Pull Request #3500 · ElementsProject/lightning

rustyrussell · 2020-02-11T02:54:23Z

This is a previously-discussed problem (as shown in lightning/bolts#728 ) but @m-schmoock wrote a proof-of-concept in #3498 so I am pushing a mitigation for the imminent release.

The funder pays the onchain fees: this keeps it simple. If the funder has spent all their funds, they obv. can't use the channel, but the fundee also cannot: most implementations will not add an HTLC if the funder could not afford the resulting fee (c-lightning included). If we were to loosen that, I'm not sure how other implementations would respond :(

The simplest workaround is to keep a little extra around if we're the funder. It's still possible to get into a state (particularly with fee changes) where we're stuck, but this makes it less likely.

ZmnSCPxj

ACK 46bcf8a

ZmnSCPxj · 2020-02-11T02:58:24Z

+ * additional constraint when we're funder trying to add an HTLC: make
+ * sure we can afford one more HTLC, even if fees increase 50%.
+ *
+ * We could do this for the peer, as well, by rejecting their HTLC


In this case the peer would have to be funder and us the fundee?

ZmnSCPxj · 2020-02-11T03:02:50Z

Should we add the remote, no-onchain-feerate-change-needed test case made by @m-schmoock in #3498 as well? We would have to modify it to expect failure at the second drain attempt.

rustyrussell · 2020-02-11T03:05:39Z

Should we add the remote, no-onchain-feerate-change-needed test case made by @m-schmoock in #3498 as well? We would have to modify it to expect failure at the second drain attempt.

I originally did that, but we have to change the numbers now it doesn't allow the second HTLC. Seemed cleaner to leave it as a POC unmerged, and have this fast generic one in-tree.

rustyrussell · 2020-02-11T06:28:36Z

Trivial rebase, Changelog line added.

This is inspired by @m-schmook's ElementsProject#3498 except this is simply a two-channel version which probes for the amount. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Extract out num_untrimmed_htlcs() from inside fee_for_htlcs(), and remove unused view arg. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2020-02-11T07:27:15Z

Another trivial rebase, fix flake8 warnings.

m-schmoock · 2020-02-11T08:54:23Z

About the mitigation, adding a soft reserve related to fee makes sense. Some questions:

About the fundee rejecting it's peer running into this situation, @rustyrussell can you elaborate how/why the founder 'gets upset' with us?
How do we mitigate channels that are already in that state right now? Our software could check this and make a circular payment to unlock them on its own.
We need to check that the meaning of 'spendable_msat' in listpeers will not be broken.

I will test try out this PR this evening...

rustyrussell · 2020-02-11T09:25:05Z

Note we're overdue for the 0.8.1 release.

If this code rejects the remote attempt to add an HTLC (i.e. incoming), channeld will close the channel for violating the protocol since the peer should not have tried to do that. If it rejects a local attempt (i.e. outgoing), channeld returns an error to lightningd; this is common (e.g. no capacity). We would need to add a new failure mechanism for remote, to say "they added it, but we need to immediately fail it" which is complex.
Fixing channels already in this state is out of scope for this PR (and I don't think necessary, since it's a not been seen much in the wild?).
Yes, I will need to fix that, good catch.

Much of the testsuite also broke. (Actually, testsuite caught #3 as well, thanks!)

Add new check if we're funder trying to add HTLC, keeping us with enough extra funds to pay for another HTLC the peer might add. We also need to adjust the spendable_msat calculation, and update various tests which try to unbalance channels. We eliminate the now-redundant test_channel_drainage entirely. Changelog-Fixed: Corner case where channel could become unusable (lightning/bolts#728) Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

cdecker · 2020-02-11T14:34:00Z

ACK dc3a718

cdecker · 2020-02-11T14:32:21Z

+
+	/* Now, how much would it cost us if feerate increases 50% and we added
+ * another HTLC? */
+	fee = commit_tx_base_fee(feerate + feerate/2, untrimmed + 1);


Cleanup: we should split out the feerate + feerate/2 into a separate variable.

m-schmoock · 2020-02-11T14:56:48Z

hm... that was quick :D
I will still do my tests this evening

rustyrussell · 2020-02-11T02:54:23Z

This is a previously-discussed problem (as shown in lightning/bolts#728 ) but @m-schmoock wrote a proof-of-concept in #3498 so I am pushing a mitigation for the imminent release.

The funder pays the onchain fees: this keeps it simple. If the funder has spent all their funds, they obv. can't use the channel, but the fundee also cannot: most implementations will not add an HTLC if the funder could not afford the resulting fee (c-lightning included). If we were to loosen that, I'm not sure how other implementations would respond :(

The simplest workaround is to keep a little extra around if we're the funder. It's still possible to get into a state (particularly with fee changes) where we're stuck, but this makes it less likely.

ZmnSCPxj

ACK 46bcf8a

ZmnSCPxj · 2020-02-11T02:58:24Z

+ * additional constraint when we're funder trying to add an HTLC: make
+ * sure we can afford one more HTLC, even if fees increase 50%.
+ *
+ * We could do this for the peer, as well, by rejecting their HTLC


In this case the peer would have to be funder and us the fundee?

ZmnSCPxj · 2020-02-11T03:02:50Z

Should we add the remote, no-onchain-feerate-change-needed test case made by @m-schmoock in #3498 as well? We would have to modify it to expect failure at the second drain attempt.

rustyrussell · 2020-02-11T03:05:39Z

Should we add the remote, no-onchain-feerate-change-needed test case made by @m-schmoock in #3498 as well? We would have to modify it to expect failure at the second drain attempt.

I originally did that, but we have to change the numbers now it doesn't allow the second HTLC. Seemed cleaner to leave it as a POC unmerged, and have this fast generic one in-tree.

rustyrussell · 2020-02-11T06:28:36Z

Trivial rebase, Changelog line added.

This is inspired by @m-schmook's ElementsProject#3498 except this is simply a two-channel version which probes for the amount. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Extract out num_untrimmed_htlcs() from inside fee_for_htlcs(), and remove unused view arg. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2020-02-11T07:27:15Z

Another trivial rebase, fix flake8 warnings.

m-schmoock · 2020-02-11T08:54:23Z

About the mitigation, adding a soft reserve related to fee makes sense. Some questions:

About the fundee rejecting it's peer running into this situation, @rustyrussell can you elaborate how/why the founder 'gets upset' with us?
How do we mitigate channels that are already in that state right now? Our software could check this and make a circular payment to unlock them on its own.
We need to check that the meaning of 'spendable_msat' in listpeers will not be broken.

I will test try out this PR this evening...

rustyrussell · 2020-02-11T09:25:05Z

Note we're overdue for the 0.8.1 release.

If this code rejects the remote attempt to add an HTLC (i.e. incoming), channeld will close the channel for violating the protocol since the peer should not have tried to do that. If it rejects a local attempt (i.e. outgoing), channeld returns an error to lightningd; this is common (e.g. no capacity). We would need to add a new failure mechanism for remote, to say "they added it, but we need to immediately fail it" which is complex.
Fixing channels already in this state is out of scope for this PR (and I don't think necessary, since it's a not been seen much in the wild?).
Yes, I will need to fix that, good catch.

Much of the testsuite also broke. (Actually, testsuite caught #3 as well, thanks!)

Add new check if we're funder trying to add HTLC, keeping us with enough extra funds to pay for another HTLC the peer might add. We also need to adjust the spendable_msat calculation, and update various tests which try to unbalance channels. We eliminate the now-redundant test_channel_drainage entirely. Changelog-Fixed: Corner case where channel could become unusable (lightning/bolts#728) Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

cdecker · 2020-02-11T14:34:00Z

ACK dc3a718

cdecker · 2020-02-11T14:32:21Z

+
+	/* Now, how much would it cost us if feerate increases 50% and we added
+ * another HTLC? */
+	fee = commit_tx_base_fee(feerate + feerate/2, untrimmed + 1);


Cleanup: we should split out the feerate + feerate/2 into a separate variable.

m-schmoock · 2020-02-11T14:56:48Z

hm... that was quick :D
I will still do my tests this evening

rustyrussell · 2020-02-11T02:54:23Z

This is a previously-discussed problem (as shown in lightning/bolts#728 ) but @m-schmoock wrote a proof-of-concept in #3498 so I am pushing a mitigation for the imminent release.

The funder pays the onchain fees: this keeps it simple. If the funder has spent all their funds, they obv. can't use the channel, but the fundee also cannot: most implementations will not add an HTLC if the funder could not afford the resulting fee (c-lightning included). If we were to loosen that, I'm not sure how other implementations would respond :(

The simplest workaround is to keep a little extra around if we're the funder. It's still possible to get into a state (particularly with fee changes) where we're stuck, but this makes it less likely.

ZmnSCPxj

ACK 46bcf8a

ZmnSCPxj · 2020-02-11T02:58:24Z

+ * additional constraint when we're funder trying to add an HTLC: make
+ * sure we can afford one more HTLC, even if fees increase 50%.
+ *
+ * We could do this for the peer, as well, by rejecting their HTLC


In this case the peer would have to be funder and us the fundee?

ZmnSCPxj · 2020-02-11T03:02:50Z

Should we add the remote, no-onchain-feerate-change-needed test case made by @m-schmoock in #3498 as well? We would have to modify it to expect failure at the second drain attempt.

rustyrussell · 2020-02-11T03:05:39Z

Should we add the remote, no-onchain-feerate-change-needed test case made by @m-schmoock in #3498 as well? We would have to modify it to expect failure at the second drain attempt.

I originally did that, but we have to change the numbers now it doesn't allow the second HTLC. Seemed cleaner to leave it as a POC unmerged, and have this fast generic one in-tree.

rustyrussell · 2020-02-11T06:28:36Z

Trivial rebase, Changelog line added.

This is inspired by @m-schmook's ElementsProject#3498 except this is simply a two-channel version which probes for the amount. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Extract out num_untrimmed_htlcs() from inside fee_for_htlcs(), and remove unused view arg. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2020-02-11T07:27:15Z

Another trivial rebase, fix flake8 warnings.

m-schmoock · 2020-02-11T08:54:23Z

About the mitigation, adding a soft reserve related to fee makes sense. Some questions:

About the fundee rejecting it's peer running into this situation, @rustyrussell can you elaborate how/why the founder 'gets upset' with us?
How do we mitigate channels that are already in that state right now? Our software could check this and make a circular payment to unlock them on its own.
We need to check that the meaning of 'spendable_msat' in listpeers will not be broken.

I will test try out this PR this evening...

rustyrussell · 2020-02-11T09:25:05Z

Note we're overdue for the 0.8.1 release.

If this code rejects the remote attempt to add an HTLC (i.e. incoming), channeld will close the channel for violating the protocol since the peer should not have tried to do that. If it rejects a local attempt (i.e. outgoing), channeld returns an error to lightningd; this is common (e.g. no capacity). We would need to add a new failure mechanism for remote, to say "they added it, but we need to immediately fail it" which is complex.
Fixing channels already in this state is out of scope for this PR (and I don't think necessary, since it's a not been seen much in the wild?).
Yes, I will need to fix that, good catch.

Much of the testsuite also broke. (Actually, testsuite caught #3 as well, thanks!)

Add new check if we're funder trying to add HTLC, keeping us with enough extra funds to pay for another HTLC the peer might add. We also need to adjust the spendable_msat calculation, and update various tests which try to unbalance channels. We eliminate the now-redundant test_channel_drainage entirely. Changelog-Fixed: Corner case where channel could become unusable (lightning/bolts#728) Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

cdecker · 2020-02-11T14:34:00Z

ACK dc3a718

cdecker · 2020-02-11T14:32:21Z

+
+	/* Now, how much would it cost us if feerate increases 50% and we added
+ * another HTLC? */
+	fee = commit_tx_base_fee(feerate + feerate/2, untrimmed + 1);


Cleanup: we should split out the feerate + feerate/2 into a separate variable.

m-schmoock · 2020-02-11T14:56:48Z

hm... that was quick :D
I will still do my tests this evening

rustyrussell · 2020-02-11T02:54:23Z

This is a previously-discussed problem (as shown in lightning/bolts#728 ) but @m-schmoock wrote a proof-of-concept in #3498 so I am pushing a mitigation for the imminent release.

The funder pays the onchain fees: this keeps it simple. If the funder has spent all their funds, they obv. can't use the channel, but the fundee also cannot: most implementations will not add an HTLC if the funder could not afford the resulting fee (c-lightning included). If we were to loosen that, I'm not sure how other implementations would respond :(

The simplest workaround is to keep a little extra around if we're the funder. It's still possible to get into a state (particularly with fee changes) where we're stuck, but this makes it less likely.

ZmnSCPxj

ACK 46bcf8a

ZmnSCPxj · 2020-02-11T02:58:24Z

+ * additional constraint when we're funder trying to add an HTLC: make
+ * sure we can afford one more HTLC, even if fees increase 50%.
+ *
+ * We could do this for the peer, as well, by rejecting their HTLC


In this case the peer would have to be funder and us the fundee?

ZmnSCPxj · 2020-02-11T03:02:50Z

Should we add the remote, no-onchain-feerate-change-needed test case made by @m-schmoock in #3498 as well? We would have to modify it to expect failure at the second drain attempt.

rustyrussell · 2020-02-11T03:05:39Z

Should we add the remote, no-onchain-feerate-change-needed test case made by @m-schmoock in #3498 as well? We would have to modify it to expect failure at the second drain attempt.

I originally did that, but we have to change the numbers now it doesn't allow the second HTLC. Seemed cleaner to leave it as a POC unmerged, and have this fast generic one in-tree.

rustyrussell · 2020-02-11T06:28:36Z

Trivial rebase, Changelog line added.

This is inspired by @m-schmook's ElementsProject#3498 except this is simply a two-channel version which probes for the amount. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Extract out num_untrimmed_htlcs() from inside fee_for_htlcs(), and remove unused view arg. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2020-02-11T07:27:15Z

Another trivial rebase, fix flake8 warnings.

m-schmoock · 2020-02-11T08:54:23Z

About the mitigation, adding a soft reserve related to fee makes sense. Some questions:

About the fundee rejecting it's peer running into this situation, @rustyrussell can you elaborate how/why the founder 'gets upset' with us?
How do we mitigate channels that are already in that state right now? Our software could check this and make a circular payment to unlock them on its own.
We need to check that the meaning of 'spendable_msat' in listpeers will not be broken.

I will test try out this PR this evening...

rustyrussell · 2020-02-11T09:25:05Z

Note we're overdue for the 0.8.1 release.

If this code rejects the remote attempt to add an HTLC (i.e. incoming), channeld will close the channel for violating the protocol since the peer should not have tried to do that. If it rejects a local attempt (i.e. outgoing), channeld returns an error to lightningd; this is common (e.g. no capacity). We would need to add a new failure mechanism for remote, to say "they added it, but we need to immediately fail it" which is complex.
Fixing channels already in this state is out of scope for this PR (and I don't think necessary, since it's a not been seen much in the wild?).
Yes, I will need to fix that, good catch.

Much of the testsuite also broke. (Actually, testsuite caught #3 as well, thanks!)

Add new check if we're funder trying to add HTLC, keeping us with enough extra funds to pay for another HTLC the peer might add. We also need to adjust the spendable_msat calculation, and update various tests which try to unbalance channels. We eliminate the now-redundant test_channel_drainage entirely. Changelog-Fixed: Corner case where channel could become unusable (lightning/bolts#728) Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

cdecker · 2020-02-11T14:34:00Z

ACK dc3a718

cdecker · 2020-02-11T14:32:21Z

+
+	/* Now, how much would it cost us if feerate increases 50% and we added
+ * another HTLC? */
+	fee = commit_tx_base_fee(feerate + feerate/2, untrimmed + 1);


Cleanup: we should split out the feerate + feerate/2 into a separate variable.

m-schmoock · 2020-02-11T14:56:48Z

hm... that was quick :D
I will still do my tests this evening

rustyrussell · 2020-02-11T02:54:23Z

This is a previously-discussed problem (as shown in lightning/bolts#728 ) but @m-schmoock wrote a proof-of-concept in #3498 so I am pushing a mitigation for the imminent release.

The funder pays the onchain fees: this keeps it simple. If the funder has spent all their funds, they obv. can't use the channel, but the fundee also cannot: most implementations will not add an HTLC if the funder could not afford the resulting fee (c-lightning included). If we were to loosen that, I'm not sure how other implementations would respond :(

The simplest workaround is to keep a little extra around if we're the funder. It's still possible to get into a state (particularly with fee changes) where we're stuck, but this makes it less likely.

ZmnSCPxj

ACK 46bcf8a

ZmnSCPxj · 2020-02-11T02:58:24Z

+ * additional constraint when we're funder trying to add an HTLC: make
+ * sure we can afford one more HTLC, even if fees increase 50%.
+ *
+ * We could do this for the peer, as well, by rejecting their HTLC


In this case the peer would have to be funder and us the fundee?

ZmnSCPxj · 2020-02-11T03:02:50Z

Should we add the remote, no-onchain-feerate-change-needed test case made by @m-schmoock in #3498 as well? We would have to modify it to expect failure at the second drain attempt.

rustyrussell · 2020-02-11T03:05:39Z

Should we add the remote, no-onchain-feerate-change-needed test case made by @m-schmoock in #3498 as well? We would have to modify it to expect failure at the second drain attempt.

I originally did that, but we have to change the numbers now it doesn't allow the second HTLC. Seemed cleaner to leave it as a POC unmerged, and have this fast generic one in-tree.

rustyrussell · 2020-02-11T06:28:36Z

Trivial rebase, Changelog line added.

This is inspired by @m-schmook's ElementsProject#3498 except this is simply a two-channel version which probes for the amount. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Extract out num_untrimmed_htlcs() from inside fee_for_htlcs(), and remove unused view arg. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2020-02-11T07:27:15Z

Another trivial rebase, fix flake8 warnings.

m-schmoock · 2020-02-11T08:54:23Z

About the mitigation, adding a soft reserve related to fee makes sense. Some questions:

About the fundee rejecting it's peer running into this situation, @rustyrussell can you elaborate how/why the founder 'gets upset' with us?
How do we mitigate channels that are already in that state right now? Our software could check this and make a circular payment to unlock them on its own.
We need to check that the meaning of 'spendable_msat' in listpeers will not be broken.

I will test try out this PR this evening...

rustyrussell · 2020-02-11T09:25:05Z

Note we're overdue for the 0.8.1 release.

If this code rejects the remote attempt to add an HTLC (i.e. incoming), channeld will close the channel for violating the protocol since the peer should not have tried to do that. If it rejects a local attempt (i.e. outgoing), channeld returns an error to lightningd; this is common (e.g. no capacity). We would need to add a new failure mechanism for remote, to say "they added it, but we need to immediately fail it" which is complex.
Fixing channels already in this state is out of scope for this PR (and I don't think necessary, since it's a not been seen much in the wild?).
Yes, I will need to fix that, good catch.

Much of the testsuite also broke. (Actually, testsuite caught #3 as well, thanks!)

Add new check if we're funder trying to add HTLC, keeping us with enough extra funds to pay for another HTLC the peer might add. We also need to adjust the spendable_msat calculation, and update various tests which try to unbalance channels. We eliminate the now-redundant test_channel_drainage entirely. Changelog-Fixed: Corner case where channel could become unusable (lightning/bolts#728) Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

cdecker · 2020-02-11T14:34:00Z

ACK dc3a718

cdecker · 2020-02-11T14:32:21Z

+
+	/* Now, how much would it cost us if feerate increases 50% and we added
+ * another HTLC? */
+	fee = commit_tx_base_fee(feerate + feerate/2, untrimmed + 1);


Cleanup: we should split out the feerate + feerate/2 into a separate variable.

m-schmoock · 2020-02-11T14:56:48Z

hm... that was quick :D
I will still do my tests this evening

rustyrussell · 2020-02-11T02:54:23Z

This is a previously-discussed problem (as shown in lightning/bolts#728 ) but @m-schmoock wrote a proof-of-concept in #3498 so I am pushing a mitigation for the imminent release.

The funder pays the onchain fees: this keeps it simple. If the funder has spent all their funds, they obv. can't use the channel, but the fundee also cannot: most implementations will not add an HTLC if the funder could not afford the resulting fee (c-lightning included). If we were to loosen that, I'm not sure how other implementations would respond :(

The simplest workaround is to keep a little extra around if we're the funder. It's still possible to get into a state (particularly with fee changes) where we're stuck, but this makes it less likely.

ZmnSCPxj

ACK 46bcf8a

ZmnSCPxj · 2020-02-11T02:58:24Z

+ * additional constraint when we're funder trying to add an HTLC: make
+ * sure we can afford one more HTLC, even if fees increase 50%.
+ *
+ * We could do this for the peer, as well, by rejecting their HTLC


In this case the peer would have to be funder and us the fundee?

ZmnSCPxj · 2020-02-11T03:02:50Z

Should we add the remote, no-onchain-feerate-change-needed test case made by @m-schmoock in #3498 as well? We would have to modify it to expect failure at the second drain attempt.

rustyrussell · 2020-02-11T03:05:39Z

Should we add the remote, no-onchain-feerate-change-needed test case made by @m-schmoock in #3498 as well? We would have to modify it to expect failure at the second drain attempt.

I originally did that, but we have to change the numbers now it doesn't allow the second HTLC. Seemed cleaner to leave it as a POC unmerged, and have this fast generic one in-tree.

rustyrussell · 2020-02-11T06:28:36Z

Trivial rebase, Changelog line added.

This is inspired by @m-schmook's ElementsProject#3498 except this is simply a two-channel version which probes for the amount. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Extract out num_untrimmed_htlcs() from inside fee_for_htlcs(), and remove unused view arg. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2020-02-11T07:27:15Z

Another trivial rebase, fix flake8 warnings.

m-schmoock · 2020-02-11T08:54:23Z

About the mitigation, adding a soft reserve related to fee makes sense. Some questions:

About the fundee rejecting it's peer running into this situation, @rustyrussell can you elaborate how/why the founder 'gets upset' with us?
How do we mitigate channels that are already in that state right now? Our software could check this and make a circular payment to unlock them on its own.
We need to check that the meaning of 'spendable_msat' in listpeers will not be broken.

I will test try out this PR this evening...

rustyrussell · 2020-02-11T09:25:05Z

Note we're overdue for the 0.8.1 release.

If this code rejects the remote attempt to add an HTLC (i.e. incoming), channeld will close the channel for violating the protocol since the peer should not have tried to do that. If it rejects a local attempt (i.e. outgoing), channeld returns an error to lightningd; this is common (e.g. no capacity). We would need to add a new failure mechanism for remote, to say "they added it, but we need to immediately fail it" which is complex.
Fixing channels already in this state is out of scope for this PR (and I don't think necessary, since it's a not been seen much in the wild?).
Yes, I will need to fix that, good catch.

Much of the testsuite also broke. (Actually, testsuite caught #3 as well, thanks!)

Add new check if we're funder trying to add HTLC, keeping us with enough extra funds to pay for another HTLC the peer might add. We also need to adjust the spendable_msat calculation, and update various tests which try to unbalance channels. We eliminate the now-redundant test_channel_drainage entirely. Changelog-Fixed: Corner case where channel could become unusable (lightning/bolts#728) Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

cdecker · 2020-02-11T14:34:00Z

ACK dc3a718

cdecker · 2020-02-11T14:32:21Z

+
+	/* Now, how much would it cost us if feerate increases 50% and we added
+ * another HTLC? */
+	fee = commit_tx_base_fee(feerate + feerate/2, untrimmed + 1);


Cleanup: we should split out the feerate + feerate/2 into a separate variable.

m-schmoock · 2020-02-11T14:56:48Z

hm... that was quick :D
I will still do my tests this evening

rustyrussell · 2020-02-11T02:54:23Z

This is a previously-discussed problem (as shown in lightning/bolts#728 ) but @m-schmoock wrote a proof-of-concept in #3498 so I am pushing a mitigation for the imminent release.

The funder pays the onchain fees: this keeps it simple. If the funder has spent all their funds, they obv. can't use the channel, but the fundee also cannot: most implementations will not add an HTLC if the funder could not afford the resulting fee (c-lightning included). If we were to loosen that, I'm not sure how other implementations would respond :(

The simplest workaround is to keep a little extra around if we're the funder. It's still possible to get into a state (particularly with fee changes) where we're stuck, but this makes it less likely.

ZmnSCPxj

ACK 46bcf8a

ZmnSCPxj · 2020-02-11T02:58:24Z

+ * additional constraint when we're funder trying to add an HTLC: make
+ * sure we can afford one more HTLC, even if fees increase 50%.
+ *
+ * We could do this for the peer, as well, by rejecting their HTLC


In this case the peer would have to be funder and us the fundee?

ZmnSCPxj · 2020-02-11T03:02:50Z

Should we add the remote, no-onchain-feerate-change-needed test case made by @m-schmoock in #3498 as well? We would have to modify it to expect failure at the second drain attempt.

rustyrussell · 2020-02-11T03:05:39Z

Should we add the remote, no-onchain-feerate-change-needed test case made by @m-schmoock in #3498 as well? We would have to modify it to expect failure at the second drain attempt.

I originally did that, but we have to change the numbers now it doesn't allow the second HTLC. Seemed cleaner to leave it as a POC unmerged, and have this fast generic one in-tree.

rustyrussell · 2020-02-11T06:28:36Z

Trivial rebase, Changelog line added.

This is inspired by @m-schmook's ElementsProject#3498 except this is simply a two-channel version which probes for the amount. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Extract out num_untrimmed_htlcs() from inside fee_for_htlcs(), and remove unused view arg. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2020-02-11T07:27:15Z

Another trivial rebase, fix flake8 warnings.

m-schmoock · 2020-02-11T08:54:23Z

About the mitigation, adding a soft reserve related to fee makes sense. Some questions:

About the fundee rejecting it's peer running into this situation, @rustyrussell can you elaborate how/why the founder 'gets upset' with us?
How do we mitigate channels that are already in that state right now? Our software could check this and make a circular payment to unlock them on its own.
We need to check that the meaning of 'spendable_msat' in listpeers will not be broken.

I will test try out this PR this evening...

rustyrussell · 2020-02-11T09:25:05Z

Note we're overdue for the 0.8.1 release.

If this code rejects the remote attempt to add an HTLC (i.e. incoming), channeld will close the channel for violating the protocol since the peer should not have tried to do that. If it rejects a local attempt (i.e. outgoing), channeld returns an error to lightningd; this is common (e.g. no capacity). We would need to add a new failure mechanism for remote, to say "they added it, but we need to immediately fail it" which is complex.
Fixing channels already in this state is out of scope for this PR (and I don't think necessary, since it's a not been seen much in the wild?).
Yes, I will need to fix that, good catch.

Much of the testsuite also broke. (Actually, testsuite caught #3 as well, thanks!)

Add new check if we're funder trying to add HTLC, keeping us with enough extra funds to pay for another HTLC the peer might add. We also need to adjust the spendable_msat calculation, and update various tests which try to unbalance channels. We eliminate the now-redundant test_channel_drainage entirely. Changelog-Fixed: Corner case where channel could become unusable (lightning/bolts#728) Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

cdecker · 2020-02-11T14:34:00Z

ACK dc3a718

cdecker · 2020-02-11T14:32:21Z

+
+	/* Now, how much would it cost us if feerate increases 50% and we added
+ * another HTLC? */
+	fee = commit_tx_base_fee(feerate + feerate/2, untrimmed + 1);


Cleanup: we should split out the feerate + feerate/2 into a separate variable.

m-schmoock · 2020-02-11T14:56:48Z

hm... that was quick :D
I will still do my tests this evening

rustyrussell · 2020-02-11T02:54:23Z

This is a previously-discussed problem (as shown in lightning/bolts#728 ) but @m-schmoock wrote a proof-of-concept in #3498 so I am pushing a mitigation for the imminent release.

The funder pays the onchain fees: this keeps it simple. If the funder has spent all their funds, they obv. can't use the channel, but the fundee also cannot: most implementations will not add an HTLC if the funder could not afford the resulting fee (c-lightning included). If we were to loosen that, I'm not sure how other implementations would respond :(

The simplest workaround is to keep a little extra around if we're the funder. It's still possible to get into a state (particularly with fee changes) where we're stuck, but this makes it less likely.

ZmnSCPxj

ACK 46bcf8a

ZmnSCPxj · 2020-02-11T02:58:24Z

+ * additional constraint when we're funder trying to add an HTLC: make
+ * sure we can afford one more HTLC, even if fees increase 50%.
+ *
+ * We could do this for the peer, as well, by rejecting their HTLC


In this case the peer would have to be funder and us the fundee?

ZmnSCPxj · 2020-02-11T03:02:50Z

Should we add the remote, no-onchain-feerate-change-needed test case made by @m-schmoock in #3498 as well? We would have to modify it to expect failure at the second drain attempt.

rustyrussell · 2020-02-11T03:05:39Z

Should we add the remote, no-onchain-feerate-change-needed test case made by @m-schmoock in #3498 as well? We would have to modify it to expect failure at the second drain attempt.

I originally did that, but we have to change the numbers now it doesn't allow the second HTLC. Seemed cleaner to leave it as a POC unmerged, and have this fast generic one in-tree.

rustyrussell · 2020-02-11T06:28:36Z

Trivial rebase, Changelog line added.

This is inspired by @m-schmook's ElementsProject#3498 except this is simply a two-channel version which probes for the amount. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Extract out num_untrimmed_htlcs() from inside fee_for_htlcs(), and remove unused view arg. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2020-02-11T07:27:15Z

Another trivial rebase, fix flake8 warnings.

m-schmoock · 2020-02-11T08:54:23Z

About the mitigation, adding a soft reserve related to fee makes sense. Some questions:

About the fundee rejecting it's peer running into this situation, @rustyrussell can you elaborate how/why the founder 'gets upset' with us?
How do we mitigate channels that are already in that state right now? Our software could check this and make a circular payment to unlock them on its own.
We need to check that the meaning of 'spendable_msat' in listpeers will not be broken.

I will test try out this PR this evening...

rustyrussell · 2020-02-11T09:25:05Z

Note we're overdue for the 0.8.1 release.

If this code rejects the remote attempt to add an HTLC (i.e. incoming), channeld will close the channel for violating the protocol since the peer should not have tried to do that. If it rejects a local attempt (i.e. outgoing), channeld returns an error to lightningd; this is common (e.g. no capacity). We would need to add a new failure mechanism for remote, to say "they added it, but we need to immediately fail it" which is complex.
Fixing channels already in this state is out of scope for this PR (and I don't think necessary, since it's a not been seen much in the wild?).
Yes, I will need to fix that, good catch.

Much of the testsuite also broke. (Actually, testsuite caught #3 as well, thanks!)

Add new check if we're funder trying to add HTLC, keeping us with enough extra funds to pay for another HTLC the peer might add. We also need to adjust the spendable_msat calculation, and update various tests which try to unbalance channels. We eliminate the now-redundant test_channel_drainage entirely. Changelog-Fixed: Corner case where channel could become unusable (lightning/bolts#728) Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

cdecker · 2020-02-11T14:34:00Z

ACK dc3a718

cdecker · 2020-02-11T14:32:21Z

+
+	/* Now, how much would it cost us if feerate increases 50% and we added
+ * another HTLC? */
+	fee = commit_tx_base_fee(feerate + feerate/2, untrimmed + 1);


Cleanup: we should split out the feerate + feerate/2 into a separate variable.

m-schmoock · 2020-02-11T14:56:48Z

hm... that was quick :D
I will still do my tests this evening

rustyrussell added channeld labels Feb 11, 2020

rustyrussell added this to the 0.8.1 milestone Feb 11, 2020

rustyrussell requested a review from cdecker as a code owner February 11, 2020 02:54

ZmnSCPxj approved these changes Feb 11, 2020
View reviewed changes

rustyrussell force-pushed the guilt/pr-3498 branch from 46bcf8a to 5f94c04Compare February 11, 2020 03:03

rustyrussell mentioned this pull request Feb 11, 2020
[DRAFT] channeld: allow fundee dip into funder reserves, once. #3501
Closed

rustyrussell force-pushed the guilt/pr-3498 branch from 5f94c04 to c56b700Compare February 11, 2020 06:28

rustyrussell added 2 commits February 11, 2020 17:55

pytest: simplified drain test (xfail)
9fe6ac8
This is inspired by @m-schmook's ElementsProject#3498 except this is simply a two-channel version which probes for the amount. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

channeld: trivial refactor of fee_for_htlcs().
46d19dc
Extract out num_untrimmed_htlcs() from inside fee_for_htlcs(), and remove unused view arg. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the guilt/pr-3498 branch from c56b700 to 2eecba0Compare February 11, 2020 07:26

rustyrussell force-pushed the guilt/pr-3498 branch from 2eecba0 to dc3a718Compare February 11, 2020 10:14

cdecker merged commit 86c28b2 into ElementsProject:masterFeb 11, 2020

cdecker reviewed Feb 11, 2020
View reviewed changes

rustyrussell added channeld labels Feb 11, 2020

rustyrussell added this to the 0.8.1 milestone Feb 11, 2020

rustyrussell requested a review from cdecker as a code owner February 11, 2020 02:54

ZmnSCPxj approved these changes Feb 11, 2020
View reviewed changes

rustyrussell force-pushed the guilt/pr-3498 branch from 46bcf8a to 5f94c04Compare February 11, 2020 03:03

rustyrussell mentioned this pull request Feb 11, 2020
[DRAFT] channeld: allow fundee dip into funder reserves, once. #3501
Closed

rustyrussell force-pushed the guilt/pr-3498 branch from 5f94c04 to c56b700Compare February 11, 2020 06:28

rustyrussell added 2 commits February 11, 2020 17:55

pytest: simplified drain test (xfail)
9fe6ac8
This is inspired by @m-schmook's ElementsProject#3498 except this is simply a two-channel version which probes for the amount. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

channeld: trivial refactor of fee_for_htlcs().
46d19dc
Extract out num_untrimmed_htlcs() from inside fee_for_htlcs(), and remove unused view arg. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the guilt/pr-3498 branch from c56b700 to 2eecba0Compare February 11, 2020 07:26

rustyrussell force-pushed the guilt/pr-3498 branch from 2eecba0 to dc3a718Compare February 11, 2020 10:14

cdecker merged commit 86c28b2 into ElementsProject:masterFeb 11, 2020

cdecker reviewed Feb 11, 2020
View reviewed changes

rustyrussell added channeld labels Feb 11, 2020

rustyrussell added this to the 0.8.1 milestone Feb 11, 2020

rustyrussell requested a review from cdecker as a code owner February 11, 2020 02:54

ZmnSCPxj approved these changes Feb 11, 2020
View reviewed changes

rustyrussell force-pushed the guilt/pr-3498 branch from 46bcf8a to 5f94c04Compare February 11, 2020 03:03

rustyrussell mentioned this pull request Feb 11, 2020
[DRAFT] channeld: allow fundee dip into funder reserves, once. #3501
Closed

rustyrussell force-pushed the guilt/pr-3498 branch from 5f94c04 to c56b700Compare February 11, 2020 06:28

rustyrussell added 2 commits February 11, 2020 17:55

pytest: simplified drain test (xfail)
9fe6ac8
This is inspired by @m-schmook's ElementsProject#3498 except this is simply a two-channel version which probes for the amount. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

channeld: trivial refactor of fee_for_htlcs().
46d19dc
Extract out num_untrimmed_htlcs() from inside fee_for_htlcs(), and remove unused view arg. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the guilt/pr-3498 branch from c56b700 to 2eecba0Compare February 11, 2020 07:26

rustyrussell force-pushed the guilt/pr-3498 branch from 2eecba0 to dc3a718Compare February 11, 2020 10:14

cdecker merged commit 86c28b2 into ElementsProject:masterFeb 11, 2020

cdecker reviewed Feb 11, 2020
View reviewed changes

rustyrussell added channeld labels Feb 11, 2020

rustyrussell added this to the 0.8.1 milestone Feb 11, 2020

rustyrussell requested a review from cdecker as a code owner February 11, 2020 02:54

ZmnSCPxj approved these changes Feb 11, 2020
View reviewed changes

rustyrussell force-pushed the guilt/pr-3498 branch from 46bcf8a to 5f94c04Compare February 11, 2020 03:03

rustyrussell mentioned this pull request Feb 11, 2020
[DRAFT] channeld: allow fundee dip into funder reserves, once. #3501
Closed

rustyrussell force-pushed the guilt/pr-3498 branch from 5f94c04 to c56b700Compare February 11, 2020 06:28

rustyrussell added 2 commits February 11, 2020 17:55

pytest: simplified drain test (xfail)
9fe6ac8
This is inspired by @m-schmook's ElementsProject#3498 except this is simply a two-channel version which probes for the amount. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

channeld: trivial refactor of fee_for_htlcs().
46d19dc
Extract out num_untrimmed_htlcs() from inside fee_for_htlcs(), and remove unused view arg. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the guilt/pr-3498 branch from c56b700 to 2eecba0Compare February 11, 2020 07:26

rustyrussell force-pushed the guilt/pr-3498 branch from 2eecba0 to dc3a718Compare February 11, 2020 10:14

cdecker merged commit 86c28b2 into ElementsProject:masterFeb 11, 2020

cdecker reviewed Feb 11, 2020
View reviewed changes

rustyrussell added channeld labels Feb 11, 2020

rustyrussell added this to the 0.8.1 milestone Feb 11, 2020

rustyrussell requested a review from cdecker as a code owner February 11, 2020 02:54

ZmnSCPxj approved these changes Feb 11, 2020
View reviewed changes

rustyrussell force-pushed the guilt/pr-3498 branch from 46bcf8a to 5f94c04Compare February 11, 2020 03:03

rustyrussell mentioned this pull request Feb 11, 2020
[DRAFT] channeld: allow fundee dip into funder reserves, once. #3501
Closed

rustyrussell force-pushed the guilt/pr-3498 branch from 5f94c04 to c56b700Compare February 11, 2020 06:28

rustyrussell added 2 commits February 11, 2020 17:55

pytest: simplified drain test (xfail)
9fe6ac8
This is inspired by @m-schmook's ElementsProject#3498 except this is simply a two-channel version which probes for the amount. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

channeld: trivial refactor of fee_for_htlcs().
46d19dc
Extract out num_untrimmed_htlcs() from inside fee_for_htlcs(), and remove unused view arg. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the guilt/pr-3498 branch from c56b700 to 2eecba0Compare February 11, 2020 07:26

rustyrussell force-pushed the guilt/pr-3498 branch from 2eecba0 to dc3a718Compare February 11, 2020 10:14

cdecker merged commit 86c28b2 into ElementsProject:masterFeb 11, 2020

cdecker reviewed Feb 11, 2020
View reviewed changes

rustyrussell added channeld labels Feb 11, 2020

rustyrussell added this to the 0.8.1 milestone Feb 11, 2020

rustyrussell requested a review from cdecker as a code owner February 11, 2020 02:54

ZmnSCPxj approved these changes Feb 11, 2020
View reviewed changes

rustyrussell force-pushed the guilt/pr-3498 branch from 46bcf8a to 5f94c04Compare February 11, 2020 03:03

rustyrussell mentioned this pull request Feb 11, 2020
[DRAFT] channeld: allow fundee dip into funder reserves, once. #3501
Closed

rustyrussell force-pushed the guilt/pr-3498 branch from 5f94c04 to c56b700Compare February 11, 2020 06:28

rustyrussell added 2 commits February 11, 2020 17:55

pytest: simplified drain test (xfail)
9fe6ac8
This is inspired by @m-schmook's ElementsProject#3498 except this is simply a two-channel version which probes for the amount. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

channeld: trivial refactor of fee_for_htlcs().
46d19dc
Extract out num_untrimmed_htlcs() from inside fee_for_htlcs(), and remove unused view arg. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the guilt/pr-3498 branch from c56b700 to 2eecba0Compare February 11, 2020 07:26

rustyrussell force-pushed the guilt/pr-3498 branch from 2eecba0 to dc3a718Compare February 11, 2020 10:14

cdecker merged commit 86c28b2 into ElementsProject:masterFeb 11, 2020

cdecker reviewed Feb 11, 2020
View reviewed changes

rustyrussell added channeld labels Feb 11, 2020

rustyrussell added this to the 0.8.1 milestone Feb 11, 2020

rustyrussell requested a review from cdecker as a code owner February 11, 2020 02:54

ZmnSCPxj approved these changes Feb 11, 2020
View reviewed changes

rustyrussell force-pushed the guilt/pr-3498 branch from 46bcf8a to 5f94c04Compare February 11, 2020 03:03

rustyrussell mentioned this pull request Feb 11, 2020
[DRAFT] channeld: allow fundee dip into funder reserves, once. #3501
Closed

rustyrussell force-pushed the guilt/pr-3498 branch from 5f94c04 to c56b700Compare February 11, 2020 06:28

rustyrussell added 2 commits February 11, 2020 17:55

pytest: simplified drain test (xfail)
9fe6ac8
This is inspired by @m-schmook's ElementsProject#3498 except this is simply a two-channel version which probes for the amount. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

channeld: trivial refactor of fee_for_htlcs().
46d19dc
Extract out num_untrimmed_htlcs() from inside fee_for_htlcs(), and remove unused view arg. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the guilt/pr-3498 branch from c56b700 to 2eecba0Compare February 11, 2020 07:26

rustyrussell force-pushed the guilt/pr-3498 branch from 2eecba0 to dc3a718Compare February 11, 2020 10:14

cdecker merged commit 86c28b2 into ElementsProject:masterFeb 11, 2020

cdecker reviewed Feb 11, 2020
View reviewed changes

rustyrussell added channeld labels Feb 11, 2020

rustyrussell added this to the 0.8.1 milestone Feb 11, 2020

rustyrussell requested a review from cdecker as a code owner February 11, 2020 02:54

ZmnSCPxj approved these changes Feb 11, 2020
View reviewed changes

rustyrussell force-pushed the guilt/pr-3498 branch from 46bcf8a to 5f94c04Compare February 11, 2020 03:03

rustyrussell mentioned this pull request Feb 11, 2020
[DRAFT] channeld: allow fundee dip into funder reserves, once. #3501
Closed

rustyrussell force-pushed the guilt/pr-3498 branch from 5f94c04 to c56b700Compare February 11, 2020 06:28

rustyrussell added 2 commits February 11, 2020 17:55

pytest: simplified drain test (xfail)
9fe6ac8
This is inspired by @m-schmook's ElementsProject#3498 except this is simply a two-channel version which probes for the amount. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

channeld: trivial refactor of fee_for_htlcs().
46d19dc
Extract out num_untrimmed_htlcs() from inside fee_for_htlcs(), and remove unused view arg. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the guilt/pr-3498 branch from c56b700 to 2eecba0Compare February 11, 2020 07:26

rustyrussell force-pushed the guilt/pr-3498 branch from 2eecba0 to dc3a718Compare February 11, 2020 10:14

cdecker merged commit 86c28b2 into ElementsProject:masterFeb 11, 2020

cdecker reviewed Feb 11, 2020
View reviewed changes

Uh oh!

Conversation

rustyrussell commented Feb 11, 2020

Uh oh!

ZmnSCPxj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZmnSCPxj commented Feb 11, 2020

Uh oh!

rustyrussell commented Feb 11, 2020

Uh oh!

rustyrussell commented Feb 11, 2020

Uh oh!

rustyrussell commented Feb 11, 2020

Uh oh!

m-schmoock commented Feb 11, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustyrussell commented Feb 11, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdecker commented Feb 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m-schmoock commented Feb 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

rustyrussell commented Feb 11, 2020

Uh oh!

ZmnSCPxj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZmnSCPxj commented Feb 11, 2020

Uh oh!

rustyrussell commented Feb 11, 2020

Uh oh!

rustyrussell commented Feb 11, 2020

Uh oh!

rustyrussell commented Feb 11, 2020

Uh oh!

m-schmoock commented Feb 11, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustyrussell commented Feb 11, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdecker commented Feb 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m-schmoock commented Feb 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

m-schmoock commented Feb 11, 2020•
edited
Loading

rustyrussell commented Feb 11, 2020•
edited
Loading

m-schmoock commented Feb 11, 2020•
edited
Loading

rustyrussell commented Feb 11, 2020•
edited
Loading

m-schmoock commented Feb 11, 2020•
edited
Loading

rustyrussell commented Feb 11, 2020•
edited
Loading