Handle peer disconnect tracking/channel_reestablish #178
Conversation
TheBlueMatt
force-pushed
the
2018-09-channel_reestablish
branch
2 times, most recently
from
f530de5 to
b79886b
Compare
TheBlueMatt
force-pushed
the
2018-09-channel_reestablish
branch
from
b79886b to
d50101f
Compare
|
Now based on #179. |
src/ln/channel.rs
Outdated
| let non_shutdown_state = self.channel_state & (!BOTH_SIDES_SHUTDOWN_MASK); | ||
| if non_shutdown_state == ChannelState::FundingSent as u32 { | ||
| self.channel_state |= ChannelState::TheirFundingLocked as u32; | ||
| } else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::OurFundingLocked as u32) { | ||
| self.channel_state = ChannelState::ChannelFunded as u32 | (self.channel_state & BOTH_SIDES_SHUTDOWN_MASK); | ||
| self.channel_update_count += 1; | ||
| } else if self.channel_state & (ChannelState::ChannelFunded as u32) != 0 && | ||
| self.cur_local_commitment_transaction_number == (1 << 48) - 2 && | ||
| self.cur_remote_commitment_transaction_number == (1 << 48) - 2 { |
There was a problem hiding this comment.
Sorry didn't get the rationale for testing against these specific values, why decrease max commitment transaction number by 2 ?
There was a problem hiding this comment.
Added a comment.
| err = Some(e); | ||
| if let Some(msgs::ErrorAction::IgnoreError) = e.action {} | ||
| else { | ||
| panic!("Got a non-IgnoreError action trying to fulfill holding cell HTLC"); |
There was a problem hiding this comment.
Wouldn't be "trying to fail holding cell HTLC" here ?
There was a problem hiding this comment.
Fixed, though note the comment was on the wrong line, this one is fulfilil, the fail one is the next hunk down.
| } | ||
| } | ||
|
|
||
| self.holding_cell_htlc_updates.retain(|htlc_update| { |
There was a problem hiding this comment.
should be a good pattern to use drain_filter and avoid clone, sad it's still nightly-only..
There was a problem hiding this comment.
Yea, sadly gonna be a while before we can upgrade to that :(
src/ln/channel.rs
Outdated
| return Err(HandleError{err: "Peer sent a loose channel_reestablish not after reconnect", action: Some(msgs::ErrorAction::SendErrorMessage{msg: msgs::ErrorMessage{data: "Peer sent a loose channel_reestablish not after reconnect".to_string(), channel_id: msg.channel_id}})}); | ||
| } | ||
|
|
||
| if msg.next_local_commitment_number == 0 || msg.next_local_commitment_number >= 0xffffffffffff || |
There was a problem hiding this comment.
Is a const MAX_COMMITMENT_NUMBER could be more meaningful? (mostly to avoid confusion with 0xfffffffffffe on a quick skim)
There was a problem hiding this comment.
Added a constant (note that its INITIAL_COMMITMENT_NUMBER not MAX, see the comment in Channel above the commitment number fields for why).
src/ln/channel.rs
Outdated
| self.channel_state &= !(ChannelState::PeerDisconnected as u32); | ||
|
|
||
| let mut required_revoke = None; | ||
| if msg.next_remote_commitment_number == 0xffffffffffff - self.cur_local_commitment_transaction_number { |
There was a problem hiding this comment.
maybe add a comment there? "Remote and local nodes are up-to-date" if understand it well, void if clause is still confusing at first.
There was a problem hiding this comment.
Added more comment.
There was a problem hiding this comment.
Thanks ! Easier to understand it with this and constify of 2**48 -1 !
src/ln/channel.rs
Outdated
| if (self.channel_state & (ChannelState::PeerDisconnected as u32)) == (ChannelState::PeerDisconnected as u32) { | ||
| // Note that this should never really happen, if we're !is_live() on receipt of an | ||
| // incoming HTLC for relay will result in us rejecting the HTLC and we won't allow | ||
| // the user to send directly into a !is_live() channel. However, if we we |
TheBlueMatt
force-pushed
the
2018-09-channel_reestablish
branch
3 times, most recently
from
fd9673f to
4949470
Compare
src/ln/channel.rs
Outdated
| for (idx, htlc) in self.pending_inbound_htlcs.iter().enumerate() { | ||
| if htlc.htlc_id == htlc_id_arg { | ||
| if htlc.state != InboundHTLCState::Committed { | ||
| debug_assert!(false, "Have an inbound HTLC we tried to claim before it was fully committed to"); |
TheBlueMatt
force-pushed
the
2018-09-channel_reestablish
branch
2 times, most recently
from
109f18c to
57c070c
Compare
|
ACK, have reviewed the remaining part of ChannelManager and tests. Seems to me that channel_reestablish is compliant with Message Retransmission of BOLT 2, except of option_data_loss_protect optional part. |
Specifically, there really should be no Errs, but in case there is some case where duplicate HTLC removes are possible, return IgnoreError and debug_assert to see if fuzzing can find them.
Setting/removing it comes next
TheBlueMatt
force-pushed
the
2018-09-channel_reestablish
branch
from
57c070c to
e606f13
Compare
This is based on #177 just cause thats easier with my workflow, but mostly tracks peer/channel connectedness/disconnectedness status and handles generating/processing channel_reestablish messages. It still needs some tests before its mergeable but I figured I'd open it up in case anyone wanted to review.
This is most of #138!