Panic if we're running with outdated state instead of force-closing #1564

TheBlueMatt · 2022-06-23T21:37:35Z

When we receive a channel_reestablish with a data_loss_protect
that proves we're running with a stale state, instead of
force-closing the channel, we immediately panic. This lines up with
our refusal to run if we find a ChannelMonitor which is stale
compared to our ChannelManager during ChannelManager
deserialization. Ultimately both are an indication of the same
thing - that the API requirements on chain::Watch were violated.

In the "running with outdated state but ChannelMonitor(s) and
ChannelManager lined up" case specifically its likely we're running
off of an old backup, in which case connecting to peers with
channels still live is explicitly dangerous. That said, because
this could be an operator error that is correctable, panicing
instead of force-closing may allow for normal operation again in
the future (cc #1207).

In any case, we provide instructions in the panic message for how
to force-close channels prior to peer connection, as well as a note
on how to broadcast the latest state if users are willing to take
the risk.

Note that this is still somewhat unsafe until we resolve #1563.

codecov-commenter · 2022-06-23T21:58:40Z

Codecov Report

Merging #1564 (162024b) into main (3676a05) will increase coverage by 0.06%.
The diff coverage is 85.93%.

❗ Current head 162024b differs from pull request most recent head caa2a9a. Consider uploading reports for the commit caa2a9a to get more accurate results

@@            Coverage Diff             @@
##             main    #1564      +/-   ##
==========================================
+ Coverage   91.04%   91.10%   +0.06%     
==========================================
  Files          80       80              
  Lines       44034    44412     +378     
  Branches    44034    44412     +378     
==========================================
+ Hits        40091    40462     +371     
- Misses       3943     3950       +7

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`88.72% <0.00%> (-0.01%)`	⬇️
lightning/src/util/events.rs	`41.66% <ø> (ø)`
lightning/src/ln/functional_tests.rs	`96.92% <79.48%> (-0.30%)`	⬇️
lightning-background-processor/src/lib.rs	`95.20% <100.00%> (ø)`
lightning-persister/src/lib.rs	`93.45% <100.00%> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.71% <100.00%> (ø)`
lightning/src/ln/channelmanager.rs	`84.74% <100.00%> (+0.35%)`	⬆️
lightning/src/ln/payment_tests.rs	`99.25% <100.00%> (ø)`
lightning/src/ln/priv_short_conf_tests.rs	`96.60% <100.00%> (ø)`
lightning/src/chain/onchaintx.rs	`93.98% <0.00%> (-0.93%)`	⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3676a05...caa2a9a. Read the comment docs.

wpaulino

LGTM, need to fix the build errors reported in https://github.com/lightningdevkit/rust-lightning/runs/7032273201.

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

jkczyz

Looks like fuzz tests need updating.

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

jkczyz · 2022-06-24T14:13:12Z

lightning/src/ln/channel.rs

-						return Err(ChannelError::CloseDelayBroadcast(
-							"We have fallen behind - we have received proof that if we broadcast remote is going to claim our funds - we can't do any automated broadcasting".to_owned()
-						));
+						macro_rules! log_and_panic {


Why is a macro needed? Could just use a variable for $err_msg.

Not directly - format strings have to be an explicit string, they can't be a &'static str or whatever, so I'm not sure how to do this without either repeating the whole string or a macro.

I believe you can format it first with let err_msg = format!("..", ..) and then use "{}", err_msg in each statement. Or maybe there is a more idiomatic way of using format_args!?

Hmm, no, format_args doesn't seem to do it either. Do you find the current code particularly unreadable? It seems fine to me, given its the one way to do it without building the full string first.

Nah, just figured we could do it without using a macro. Given it's gonna panic there's not a huge argument for building the string. Fine either way.

lightning/src/ln/channelmanager.rs

jkczyz · 2022-06-24T17:41:06Z

lightning/src/ln/channel.rs

-						return Err(ChannelError::CloseDelayBroadcast(
-							"We have fallen behind - we have received proof that if we broadcast remote is going to claim our funds - we can't do any automated broadcasting".to_owned()
-						));
+						macro_rules! log_and_panic {


I believe you can format it first with let err_msg = format!("..", ..) and then use "{}", err_msg in each statement. Or maybe there is a more idiomatic way of using format_args!?

lightning/src/ln/channel.rs

wpaulino · 2022-06-24T19:00:21Z

@TheBlueMatt feel free to squash.

If a user restores from a backup that they know is stale, they'd like to force-close all of their channels (or at least the ones they know are stale) *without* broadcasting the latest state, asking their peers to do so instead. This simply adds methods to do so, renaming the existing `force_close_channel` and `force_close_all_channels` methods to disambiguate further.

When we receive a `channel_reestablish` with a `data_loss_protect` that proves we're running with a stale state, instead of force-closing the channel, we immediately panic. This lines up with our refusal to run if we find a `ChannelMonitor` which is stale compared to our `ChannelManager` during `ChannelManager` deserialization. Ultimately both are an indication of the same thing - that the API requirements on `chain::Watch` were violated. In the "running with outdated state but ChannelMonitor(s) and ChannelManager lined up" case specifically its likely we're running off of an old backup, in which case connecting to peers with channels still live is explicitly dangerous. That said, because this could be an operator error that is correctable, panicing instead of force-closing may allow for normal operation again in the future (cc lightningdevkit#1207). In any case, we provide instructions in the panic message for how to force-close channels prior to peer connection, as well as a note on how to broadcast the latest state if users are willing to take the risk. Note that this is still somewhat unsafe until we resolve lightningdevkit#1563.

TheBlueMatt · 2022-06-25T02:25:56Z

$ git diff-tree -U1 162024b6 caa2a9a5
$

TheBlueMatt added this to the 0.0.110 milestone Jun 23, 2022

wpaulino reviewed Jun 23, 2022

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

jkczyz reviewed Jun 24, 2022

View reviewed changes

TheBlueMatt force-pushed the 2022-06-panic-on-behind branch from 7126128 to 5fc55c3 Compare June 24, 2022 15:24

jkczyz reviewed Jun 24, 2022

View reviewed changes

TheBlueMatt force-pushed the 2022-06-panic-on-behind branch from 5fc55c3 to 162024b Compare June 24, 2022 18:24

jkczyz previously approved these changes Jun 24, 2022

View reviewed changes

TheBlueMatt added 2 commits June 25, 2022 02:25

TheBlueMatt dismissed jkczyz’s stale review via caa2a9a June 25, 2022 02:25

TheBlueMatt force-pushed the 2022-06-panic-on-behind branch from 162024b to caa2a9a Compare June 25, 2022 02:25

jkczyz approved these changes Jun 27, 2022

View reviewed changes

TheBlueMatt assigned wpaulino Jun 27, 2022

wpaulino approved these changes Jun 27, 2022

View reviewed changes

TheBlueMatt merged commit a600eee into lightningdevkit:main Jun 27, 2022

TheBlueMatt mentioned this pull request Jul 11, 2022

Add flag to disable broadcasting when it's dangerous due to information loss #1593

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Panic if we're running with outdated state instead of force-closing #1564

Panic if we're running with outdated state instead of force-closing #1564

TheBlueMatt commented Jun 23, 2022

codecov-commenter commented Jun 23, 2022 •

edited

Loading

wpaulino left a comment

jkczyz left a comment

jkczyz Jun 24, 2022

TheBlueMatt Jun 24, 2022

jkczyz Jun 24, 2022

TheBlueMatt Jun 24, 2022

jkczyz Jun 24, 2022

jkczyz Jun 24, 2022

wpaulino commented Jun 24, 2022

TheBlueMatt commented Jun 25, 2022

Panic if we're running with outdated state instead of force-closing #1564

Panic if we're running with outdated state instead of force-closing #1564

Conversation

TheBlueMatt commented Jun 23, 2022

codecov-commenter commented Jun 23, 2022 • edited Loading

Codecov Report

wpaulino left a comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

jkczyz Jun 24, 2022

Choose a reason for hiding this comment

TheBlueMatt Jun 24, 2022

Choose a reason for hiding this comment

jkczyz Jun 24, 2022

Choose a reason for hiding this comment

TheBlueMatt Jun 24, 2022

Choose a reason for hiding this comment

jkczyz Jun 24, 2022

Choose a reason for hiding this comment

jkczyz Jun 24, 2022

Choose a reason for hiding this comment

wpaulino commented Jun 24, 2022

TheBlueMatt commented Jun 25, 2022

codecov-commenter commented Jun 23, 2022 •

edited

Loading