Skip to content

Commit 283d40f

Browse files
TheBlueMattTheBlueMatt
authored
Merge pull request #364 from TheBlueMatt/2019-07-no-unimpl
Implement the last three (relevant) unimplemented()s in ChannelManager
2 parents 20bd2b1 + 8ba3529 commit 283d40f

File tree

4 files changed

+299
-61
lines changed

4 files changed

+299
-61
lines changed

‎src/ln/chanmon_update_fail_tests.rs

Lines changed: 130 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
use ln::channelmanager::{RAACommitmentOrder, PaymentPreimage, PaymentHash};
77
use ln::channelmonitor::ChannelMonitorUpdateErr;
88
use ln::msgs;
9-
use ln::msgs::{ChannelMessageHandler, LocalFeatures};
9+
use ln::msgs::{ChannelMessageHandler, LocalFeatures, RoutingMessageHandler};
1010
use util::events::{Event, EventsProvider, MessageSendEvent, MessageSendEventsProvider};
1111
use util::errors::APIError;
1212

@@ -1553,3 +1553,132 @@ fn monitor_update_claim_fail_no_response() {
15531553

15541554
claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
15551555
}
1556+
1557+
// Note that restore_between_fails with !fail_on_generate is useless
1558+
// Also note that !fail_on_generate && !fail_on_signed is useless
1559+
// Finally, note that !fail_on_signed is not possible with fail_on_generate && !restore_between_fails
1560+
// confirm_a_first and restore_b_before_conf are wholly unrelated to earlier bools and
1561+
// restore_b_before_conf has no meaning if !confirm_a_first
1562+
fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails: bool, fail_on_signed: bool, confirm_a_first: bool, restore_b_before_conf: bool) {
1563+
// Test that if the monitor update generated by funding_transaction_generated fails we continue
1564+
// the channel setup happily after the update is restored.
1565+
let mut nodes = create_network(2, &[None, None]);
1566+
1567+
nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 43).unwrap();
1568+
nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), LocalFeatures::new(), &get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id())).unwrap();
1569+
nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), LocalFeatures::new(), &get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id())).unwrap();
1570+
1571+
let (temporary_channel_id, funding_tx, funding_output) = create_funding_transaction(&nodes[0], 100000, 43);
1572+
1573+
if fail_on_generate {
1574+
*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
1575+
}
1576+
nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_output);
1577+
check_added_monitors!(nodes[0], 1);
1578+
1579+
*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
1580+
nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id())).unwrap();
1581+
check_added_monitors!(nodes[1], 1);
1582+
1583+
if restore_between_fails {
1584+
assert!(fail_on_generate);
1585+
*nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
1586+
nodes[0].node.test_restore_channel_monitor();
1587+
check_added_monitors!(nodes[0], 1);
1588+
assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
1589+
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
1590+
}
1591+
1592+
if fail_on_signed {
1593+
*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
1594+
} else {
1595+
assert!(restore_between_fails || !fail_on_generate); // We can't switch to good now (there's no monitor update)
1596+
assert!(fail_on_generate); // Somebody has to fail
1597+
}
1598+
let funding_signed_res = nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()));
1599+
if fail_on_signed || !restore_between_fails {
1600+
if let msgs::HandleError { err, action: Some(msgs::ErrorAction::IgnoreError) } = funding_signed_res.unwrap_err() {
1601+
if fail_on_generate && !restore_between_fails {
1602+
assert_eq!(err, "Previous monitor update failure prevented funding_signed from allowing funding broadcast");
1603+
check_added_monitors!(nodes[0], 0);
1604+
} else {
1605+
assert_eq!(err, "Failed to update ChannelMonitor");
1606+
check_added_monitors!(nodes[0], 1);
1607+
}
1608+
} else { panic!(); }
1609+
1610+
assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
1611+
*nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
1612+
nodes[0].node.test_restore_channel_monitor();
1613+
} else {
1614+
funding_signed_res.unwrap();
1615+
}
1616+
1617+
check_added_monitors!(nodes[0], 1);
1618+
1619+
let events = nodes[0].node.get_and_clear_pending_events();
1620+
assert_eq!(events.len(), 1);
1621+
match events[0] {
1622+
Event::FundingBroadcastSafe { ref funding_txo, user_channel_id } => {
1623+
assert_eq!(user_channel_id, 43);
1624+
assert_eq!(*funding_txo, funding_output);
1625+
},
1626+
_ => panic!("Unexpected event"),
1627+
};
1628+
1629+
if confirm_a_first {
1630+
confirm_transaction(&nodes[0].chain_monitor, &funding_tx, funding_tx.version);
1631+
nodes[1].node.handle_funding_locked(&nodes[0].node.get_our_node_id(), &get_event_msg!(nodes[0], MessageSendEvent::SendFundingLocked, nodes[1].node.get_our_node_id())).unwrap();
1632+
} else {
1633+
assert!(!restore_b_before_conf);
1634+
confirm_transaction(&nodes[1].chain_monitor, &funding_tx, funding_tx.version);
1635+
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
1636+
}
1637+
1638+
// Make sure nodes[1] isn't stupid enough to re-send the FundingLocked on reconnect
1639+
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
1640+
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
1641+
reconnect_nodes(&nodes[0], &nodes[1], (false, confirm_a_first), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
1642+
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
1643+
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
1644+
1645+
if !restore_b_before_conf {
1646+
confirm_transaction(&nodes[1].chain_monitor, &funding_tx, funding_tx.version);
1647+
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
1648+
assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
1649+
}
1650+
1651+
*nodes[1].chan_monitor.update_ret.lock().unwrap() = Ok(());
1652+
nodes[1].node.test_restore_channel_monitor();
1653+
check_added_monitors!(nodes[1], 1);
1654+
1655+
let (channel_id, (announcement, as_update, bs_update)) = if !confirm_a_first {
1656+
nodes[0].node.handle_funding_locked(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingLocked, nodes[0].node.get_our_node_id())).unwrap();
1657+
1658+
confirm_transaction(&nodes[0].chain_monitor, &funding_tx, funding_tx.version);
1659+
let (funding_locked, channel_id) = create_chan_between_nodes_with_value_confirm_second(&nodes[1], &nodes[0]);
1660+
(channel_id, create_chan_between_nodes_with_value_b(&nodes[0], &nodes[1], &funding_locked))
1661+
} else {
1662+
if restore_b_before_conf {
1663+
confirm_transaction(&nodes[1].chain_monitor, &funding_tx, funding_tx.version);
1664+
}
1665+
let (funding_locked, channel_id) = create_chan_between_nodes_with_value_confirm_second(&nodes[0], &nodes[1]);
1666+
(channel_id, create_chan_between_nodes_with_value_b(&nodes[1], &nodes[0], &funding_locked))
1667+
};
1668+
for node in nodes.iter() {
1669+
assert!(node.router.handle_channel_announcement(&announcement).unwrap());
1670+
node.router.handle_channel_update(&as_update).unwrap();
1671+
node.router.handle_channel_update(&bs_update).unwrap();
1672+
}
1673+
1674+
send_payment(&nodes[0], &[&nodes[1]], 8000000);
1675+
close_channel(&nodes[0], &nodes[1], &channel_id, funding_tx, true);
1676+
}
1677+
1678+
#[test]
1679+
fn during_funding_monitor_fail() {
1680+
do_during_funding_monitor_fail(false, false, true, true, true);
1681+
do_during_funding_monitor_fail(true, false, true, false, false);
1682+
do_during_funding_monitor_fail(true, true, true, true, false);
1683+
do_during_funding_monitor_fail(true, true, false, false, false);
1684+
}

‎src/ln/channel.rs

Lines changed: 61 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -181,9 +181,9 @@ enum ChannelState {
181181
/// "disconnected" and no updates are allowed until after we've done a channel_reestablish
182182
/// dance.
183183
PeerDisconnected = (1 << 7),
184-
/// Flag which is set on ChannelFunded and FundingSent indicating the user has told us they
185-
/// failed to update our ChannelMonitor somewhere and we should pause sending any outbound
186-
/// messages until they've managed to do so.
184+
/// Flag which is set on ChannelFunded, FundingCreated, and FundingSent indicating the user has
185+
/// told us they failed to update our ChannelMonitor somewhere and we should pause sending any
186+
/// outbound messages until they've managed to do so.
187187
MonitorUpdateFailed = (1 << 8),
188188
/// Flag which implies that we have sent a commitment_signed but are awaiting the responding
189189
/// revoke_and_ack message. During this time period, we can't generate new commitment_signed
@@ -248,6 +248,7 @@ pub(super) struct Channel {
248248
/// send it first.
249249
resend_order: RAACommitmentOrder,
250250

251+
monitor_pending_funding_locked: bool,
251252
monitor_pending_revoke_and_ack: bool,
252253
monitor_pending_commitment_signed: bool,
253254
monitor_pending_forwards: Vec<(PendingForwardHTLCInfo, u64)>,
@@ -457,6 +458,7 @@ impl Channel {
457458

458459
resend_order: RAACommitmentOrder::CommitmentFirst,
459460

461+
monitor_pending_funding_locked: false,
460462
monitor_pending_revoke_and_ack: false,
461463
monitor_pending_commitment_signed: false,
462464
monitor_pending_forwards: Vec::new(),
@@ -672,6 +674,7 @@ impl Channel {
672674

673675
resend_order: RAACommitmentOrder::CommitmentFirst,
674676

677+
monitor_pending_funding_locked: false,
675678
monitor_pending_revoke_and_ack: false,
676679
monitor_pending_commitment_signed: false,
677680
monitor_pending_forwards: Vec::new(),
@@ -1538,7 +1541,7 @@ impl Channel {
15381541
if !self.channel_outbound {
15391542
return Err(ChannelError::Close("Received funding_signed for an inbound channel?"));
15401543
}
1541-
if self.channel_state != ChannelState::FundingCreated as u32 {
1544+
if self.channel_state & !(ChannelState::MonitorUpdateFailed as u32) != ChannelState::FundingCreated as u32 {
15421545
return Err(ChannelError::Close("Received funding_signed in strange state!"));
15431546
}
15441547
if self.channel_monitor.get_min_seen_secret() != (1 << 48) ||
@@ -1559,10 +1562,14 @@ impl Channel {
15591562
self.sign_commitment_transaction(&mut local_initial_commitment_tx, &msg.signature);
15601563
self.channel_monitor.provide_latest_local_commitment_tx_info(local_initial_commitment_tx.clone(), local_keys, self.feerate_per_kw, Vec::new());
15611564
self.last_local_commitment_txn = vec![local_initial_commitment_tx];
1562-
self.channel_state = ChannelState::FundingSent as u32;
1565+
self.channel_state = ChannelState::FundingSent as u32 | (self.channel_state & (ChannelState::MonitorUpdateFailed as u32));
15631566
self.cur_local_commitment_transaction_number -= 1;
15641567

1565-
Ok(self.channel_monitor.clone())
1568+
if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 {
1569+
Ok(self.channel_monitor.clone())
1570+
} else {
1571+
Err(ChannelError::Ignore("Previous monitor update failure prevented funding_signed from allowing funding broadcast"))
1572+
}
15661573
}
15671574

15681575
pub fn funding_locked(&mut self, msg: &msgs::FundingLocked) -> Result<(), ChannelError> {
@@ -1577,10 +1584,13 @@ impl Channel {
15771584
} else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::OurFundingLocked as u32) {
15781585
self.channel_state = ChannelState::ChannelFunded as u32 | (self.channel_state & MULTI_STATE_FLAGS);
15791586
self.channel_update_count += 1;
1580-
} else if self.channel_state & (ChannelState::ChannelFunded as u32) != 0 &&
1581-
// Note that funding_signed/funding_created will have decremented both by 1!
1582-
self.cur_local_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 1 &&
1583-
self.cur_remote_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 1 {
1587+
} else if (self.channel_state & (ChannelState::ChannelFunded as u32) != 0 &&
1588+
// Note that funding_signed/funding_created will have decremented both by 1!
1589+
self.cur_local_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 1 &&
1590+
self.cur_remote_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 1) ||
1591+
// If we reconnected before sending our funding locked they may still resend theirs:
1592+
(self.channel_state & (ChannelState::FundingSent as u32 | ChannelState::TheirFundingLocked as u32) ==
1593+
(ChannelState::FundingSent as u32 | ChannelState::TheirFundingLocked as u32)) {
15841594
if self.their_cur_commitment_point != Some(msg.next_per_commitment_point) {
15851595
return Err(ChannelError::Close("Peer sent a reconnect funding_locked with a different point"));
15861596
}
@@ -2343,10 +2353,29 @@ impl Channel {
23432353
/// Indicates that the latest ChannelMonitor update has been committed by the client
23442354
/// successfully and we should restore normal operation. Returns messages which should be sent
23452355
/// to the remote side.
2346-
pub fn monitor_updating_restored(&mut self) -> (Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, RAACommitmentOrder, Vec<(PendingForwardHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>) {
2356+
pub fn monitor_updating_restored(&mut self) -> (Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, RAACommitmentOrder, Vec<(PendingForwardHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, bool, Option<msgs::FundingLocked>) {
23472357
assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, ChannelState::MonitorUpdateFailed as u32);
23482358
self.channel_state &= !(ChannelState::MonitorUpdateFailed as u32);
23492359

2360+
let needs_broadcast_safe = self.channel_state & (ChannelState::FundingSent as u32) != 0 && self.channel_outbound;
2361+
2362+
// Because we will never generate a FundingBroadcastSafe event when we're in
2363+
// MonitorUpdateFailed, if we assume the user only broadcast the funding transaction when
2364+
// they received the FundingBroadcastSafe event, we can only ever hit
2365+
// monitor_pending_funding_locked when we're an inbound channel which failed to persist the
2366+
// monitor on funding_created, and we even got the funding transaction confirmed before the
2367+
// monitor was persisted.
2368+
let funding_locked = if self.monitor_pending_funding_locked {
2369+
assert!(!self.channel_outbound, "Funding transaction broadcast without FundingBroadcastSafe!");
2370+
self.monitor_pending_funding_locked = false;
2371+
let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
2372+
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
2373+
Some(msgs::FundingLocked {
2374+
channel_id: self.channel_id(),
2375+
next_per_commitment_point: next_per_commitment_point,
2376+
})
2377+
} else { None };
2378+
23502379
let mut forwards = Vec::new();
23512380
mem::swap(&mut forwards, &mut self.monitor_pending_forwards);
23522381
let mut failures = Vec::new();
@@ -2355,7 +2384,7 @@ impl Channel {
23552384
if self.channel_state & (ChannelState::PeerDisconnected as u32) != 0 {
23562385
self.monitor_pending_revoke_and_ack = false;
23572386
self.monitor_pending_commitment_signed = false;
2358-
return (None, None, RAACommitmentOrder::RevokeAndACKFirst, forwards, failures);
2387+
return (None, None, RAACommitmentOrder::RevokeAndACKFirst, forwards, failures, needs_broadcast_safe, funding_locked);
23592388
}
23602389

23612390
let raa = if self.monitor_pending_revoke_and_ack {
@@ -2368,11 +2397,12 @@ impl Channel {
23682397
self.monitor_pending_revoke_and_ack = false;
23692398
self.monitor_pending_commitment_signed = false;
23702399
let order = self.resend_order.clone();
2371-
log_trace!(self, "Restored monitor updating resulting in {} commitment update and {} RAA, with {} first",
2400+
log_trace!(self, "Restored monitor updating resulting in {}{} commitment update and {} RAA, with {} first",
2401+
if needs_broadcast_safe { "a funding broadcast safe, " } else { "" },
23722402
if commitment_update.is_some() { "a" } else { "no" },
23732403
if raa.is_some() { "an" } else { "no" },
23742404
match order { RAACommitmentOrder::CommitmentFirst => "commitment", RAACommitmentOrder::RevokeAndACKFirst => "RAA"});
2375-
(raa, commitment_update, order, forwards, failures)
2405+
(raa, commitment_update, order, forwards, failures, needs_broadcast_safe, funding_locked)
23762406
}
23772407

23782408
pub fn update_fee(&mut self, fee_estimator: &FeeEstimator, msg: &msgs::UpdateFee) -> Result<(), ChannelError> {
@@ -2482,7 +2512,9 @@ impl Channel {
24822512
} else { None };
24832513

24842514
if self.channel_state & (ChannelState::FundingSent as u32) == ChannelState::FundingSent as u32 {
2485-
if self.channel_state & ChannelState::OurFundingLocked as u32 == 0 {
2515+
// If we're waiting on a monitor update, we shouldn't re-send any funding_locked's.
2516+
if self.channel_state & (ChannelState::OurFundingLocked as u32) == 0 ||
2517+
self.channel_state & (ChannelState::MonitorUpdateFailed as u32) != 0 {
24862518
if msg.next_remote_commitment_number != 0 {
24872519
return Err(ChannelError::Close("Peer claimed they saw a revoke_and_ack but we haven't sent funding_locked yet"));
24882520
}
@@ -2972,12 +3004,17 @@ impl Channel {
29723004
//they can by sending two revoke_and_acks back-to-back, but not really). This appears to be
29733005
//a protocol oversight, but I assume I'm just missing something.
29743006
if need_commitment_update {
2975-
let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
2976-
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
2977-
return Ok(Some(msgs::FundingLocked {
2978-
channel_id: self.channel_id,
2979-
next_per_commitment_point: next_per_commitment_point,
2980-
}));
3007+
if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 {
3008+
let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
3009+
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
3010+
return Ok(Some(msgs::FundingLocked {
3011+
channel_id: self.channel_id,
3012+
next_per_commitment_point: next_per_commitment_point,
3013+
}));
3014+
} else {
3015+
self.monitor_pending_funding_locked = true;
3016+
return Ok(None);
3017+
}
29813018
}
29823019
}
29833020
}
@@ -3696,6 +3733,7 @@ impl Writeable for Channel {
36963733
RAACommitmentOrder::RevokeAndACKFirst => 1u8.write(writer)?,
36973734
}
36983735

3736+
self.monitor_pending_funding_locked.write(writer)?;
36993737
self.monitor_pending_revoke_and_ack.write(writer)?;
37003738
self.monitor_pending_commitment_signed.write(writer)?;
37013739

@@ -3863,6 +3901,7 @@ impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {
38633901
_ => return Err(DecodeError::InvalidValue),
38643902
};
38653903

3904+
let monitor_pending_funding_locked = Readable::read(reader)?;
38663905
let monitor_pending_revoke_and_ack = Readable::read(reader)?;
38673906
let monitor_pending_commitment_signed = Readable::read(reader)?;
38683907

@@ -3959,6 +3998,7 @@ impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {
39593998

39603999
resend_order,
39614000

4001+
monitor_pending_funding_locked,
39624002
monitor_pending_revoke_and_ack,
39634003
monitor_pending_commitment_signed,
39644004
monitor_pending_forwards,

0 commit comments

Comments
 (0)