Skip to content

Fix one-confirmation funding_locked generation #491

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
TheBlueMatt merged 3 commits into from
Feb 11, 2020
Merged

Fix one-confirmation funding_locked generation #491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b02ccbb
Fix sending funding_locked with 1 conf.
TheBlueMatt Feb 10, 2020
8c0ec5c
Add test for 1-conf channels
TheBlueMatt Feb 10, 2020
ff24530
Add some trace logging for funding_locked and announcement_sigs
TheBlueMatt Feb 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 45 additions & 41 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2987,49 +2987,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
pub fn block_connected(&mut self, header: &BlockHeader, height: u32, txn_matched: &[&Transaction], indexes_of_txn_matched: &[u32]) -> Result<Option<msgs::FundingLocked>, msgs::ErrorMessage> {
let non_shutdown_state = self.channel_state & (!MULTI_STATE_FLAGS);
if header.bitcoin_hash() != self.last_block_connected {
self.last_block_connected = header.bitcoin_hash();
self.channel_monitor.last_block_hash = self.last_block_connected;
if self.funding_tx_confirmations > 0 {
self.funding_tx_confirmations += 1;
if self.funding_tx_confirmations == self.minimum_depth as u64 {
let need_commitment_update = if non_shutdown_state == ChannelState::FundingSent as u32 {
self.channel_state |= ChannelState::OurFundingLocked as u32;
true
} else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::TheirFundingLocked as u32) {
self.channel_state = ChannelState::ChannelFunded as u32 | (self.channel_state & MULTI_STATE_FLAGS);
self.channel_update_count += 1;
true
} else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::OurFundingLocked as u32) {
// We got a reorg but not enough to trigger a force close, just update
// funding_tx_confirmed_in and return.
false
} else if self.channel_state < ChannelState::ChannelFunded as u32 {
panic!("Started confirming a channel in a state pre-FundingSent?: {}", self.channel_state);
} else {
// We got a reorg but not enough to trigger a force close, just update
// funding_tx_confirmed_in and return.
false
};
self.funding_tx_confirmed_in = Some(header.bitcoin_hash());

//TODO: Note that this must be a duplicate of the previous commitment point they sent us,
//as otherwise we will have a commitment transaction that they can't revoke (well, kinda,
//they can by sending two revoke_and_acks back-to-back, but not really). This appears to be
//a protocol oversight, but I assume I'm just missing something.
if need_commitment_update {
if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 {
let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
return Ok(Some(msgs::FundingLocked {
channel_id: self.channel_id,
next_per_commitment_point: next_per_commitment_point,
}));
} else {
self.monitor_pending_funding_locked = true;
return Ok(None);
}
}
}
}
}
if non_shutdown_state & !(ChannelState::TheirFundingLocked as u32) == ChannelState::FundingSent as u32 {
Expand Down Expand Up @@ -3072,6 +3031,51 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
}
}
}
if header.bitcoin_hash() != self.last_block_connected {
self.last_block_connected = header.bitcoin_hash();
self.channel_monitor.last_block_hash = self.last_block_connected;
if self.funding_tx_confirmations > 0 {
Copy link

@ariard ariard Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to support zero-conf channels ? If so we should remove this check I guess.

Copy link
Collaborator Author

@TheBlueMatt TheBlueMatt Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in theory I think we do, but it probably shouldnt be with this API - we should do some kind of risk API that lets you limit exposure to 0confs and such...I havent thought much about how to structure it, yet, though.

if self.funding_tx_confirmations == self.minimum_depth as u64 {
let need_commitment_update = if non_shutdown_state == ChannelState::FundingSent as u32 {
self.channel_state |= ChannelState::OurFundingLocked as u32;
true
} else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::TheirFundingLocked as u32) {
self.channel_state = ChannelState::ChannelFunded as u32 | (self.channel_state & MULTI_STATE_FLAGS);
self.channel_update_count += 1;
true
} else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::OurFundingLocked as u32) {
// We got a reorg but not enough to trigger a force close, just update
// funding_tx_confirmed_in and return.
false
} else if self.channel_state < ChannelState::ChannelFunded as u32 {
panic!("Started confirming a channel in a state pre-FundingSent?: {}", self.channel_state);
} else {
// We got a reorg but not enough to trigger a force close, just update
// funding_tx_confirmed_in and return.
false
};
self.funding_tx_confirmed_in = Some(header.bitcoin_hash());

//TODO: Note that this must be a duplicate of the previous commitment point they sent us,
//as otherwise we will have a commitment transaction that they can't revoke (well, kinda,
//they can by sending two revoke_and_acks back-to-back, but not really). This appears to be
//a protocol oversight, but I assume I'm just missing something.
if need_commitment_update {
if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 {
let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
return Ok(Some(msgs::FundingLocked {
channel_id: self.channel_id,
next_per_commitment_point: next_per_commitment_point,
}));
} else {
self.monitor_pending_funding_locked = true;
return Ok(None);
}
}
}
}
}
Ok(None)
}

Expand Down
9 changes: 8 additions & 1 deletion lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1268,7 +1268,10 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> where M::T
}

fn get_announcement_sigs(&self, chan: &Channel<ChanSigner>) -> Option<msgs::AnnouncementSignatures> {
if !chan.should_announce() { return None }
if !chan.should_announce() {
log_trace!(self, "Can't send announcement_signatures for private channel {}", log_bytes!(chan.channel_id()));
return None
}

let (announcement, our_bitcoin_sig) = match chan.get_channel_announcement(self.get_our_node_id(), self.genesis_hash.clone()) {
Ok(res) => res,
Expand Down Expand Up @@ -1984,6 +1987,7 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> where M::T
}
try_chan_entry!(self, chan.get_mut().funding_locked(&msg), channel_state, chan);
if let Some(announcement_sigs) = self.get_announcement_sigs(chan.get()) {
log_trace!(self, "Sending announcement_signatures for {} in response to funding_locked", log_bytes!(chan.get().channel_id()));
// If we see locking block before receiving remote funding_locked, we broadcast our
// announcement_sigs at remote funding_locked reception. If we receive remote
// funding_locked before seeing locking block, we broadcast our announcement_sigs at locking
Expand Down Expand Up @@ -2578,10 +2582,13 @@ impl<ChanSigner: ChannelKeys, M: Deref + Sync + Send> ChainListener for ChannelM
msg: funding_locked,
});
if let Some(announcement_sigs) = self.get_announcement_sigs(channel) {
log_trace!(self, "Sending funding_locked and announcement_signatures for {}", log_bytes!(channel.channel_id()));
pending_msg_events.push(events::MessageSendEvent::SendAnnouncementSignatures {
node_id: channel.get_their_node_id(),
msg: announcement_sigs,
});
} else {
log_trace!(self, "Sending funding_locked WITHOUT announcement_signatures for {}", log_bytes!(channel.channel_id()));
}
short_to_id.insert(channel.get_short_channel_id().unwrap(), channel.channel_id());
} else if let Err(e) = chan_res {
Expand Down
35 changes: 35 additions & 0 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,41 @@ fn test_multi_flight_update_fee() {
check_added_monitors!(nodes[1], 1);
}

#[test]
fn test_1_conf_open() {
// Previously, if the minium_depth config was set to 1, we'd never send a funding_locked. This
// tests that we properly send one in that case.
let mut alice_config = UserConfig::default();
alice_config.own_channel_config.minimum_depth = 1;
alice_config.channel_options.announced_channel = true;
alice_config.peer_channel_config_limits.force_announced_channel_preference = false;
let mut bob_config = UserConfig::default();
bob_config.own_channel_config.minimum_depth = 1;
bob_config.channel_options.announced_channel = true;
bob_config.peer_channel_config_limits.force_announced_channel_preference = false;
let node_cfgs = create_node_cfgs(2);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(alice_config), Some(bob_config)]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);

let tx = create_chan_between_nodes_with_value_init(&nodes[0], &nodes[1], 100000, 10001, InitFeatures::supported(), InitFeatures::supported());
assert!(nodes[0].chain_monitor.does_match_tx(&tx));
assert!(nodes[1].chain_monitor.does_match_tx(&tx));

let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
nodes[1].block_notifier.block_connected_checked(&header, 1, &[&tx; 1], &[tx.version; 1]);
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()));

nodes[0].block_notifier.block_connected_checked(&header, 1, &[&tx; 1], &[tx.version; 1]);
let (funding_locked, _) = create_chan_between_nodes_with_value_confirm_second(&nodes[1], &nodes[0]);
let (announcement, as_update, bs_update) = create_chan_between_nodes_with_value_b(&nodes[0], &nodes[1], &funding_locked);

for node in nodes {
assert!(node.router.handle_channel_announcement(&announcement).unwrap());
node.router.handle_channel_update(&as_update).unwrap();
node.router.handle_channel_update(&bs_update).unwrap();
}
}

#[test]
fn test_update_fee_vanilla() {
let node_cfgs = create_node_cfgs(2);
Expand Down