Merge pull request #319 from TheBlueMatt/2019-03-htlc-sorting
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Mon, 25 Mar 2019 17:26:31 +0000 (13:26 -0400)
committerGitHub <noreply@github.com>
Mon, 25 Mar 2019 17:26:31 +0000 (13:26 -0400)
Fix HTLC-output-in-commitment sorting for duplicate-HTLCs

src/ln/channel.rs
src/ln/functional_test_utils.rs
src/ln/functional_tests.rs

index 3d883cd76260b899a9d43ecc71e663829f68def6..b67468b3859ecd5e1a50c951ce02d6670f39d05d 100644 (file)
@@ -107,19 +107,19 @@ enum OutboundHTLCState {
        Committed,
        /// Remote removed this (outbound) HTLC. We're waiting on their commitment_signed to finalize
        /// the change (though they'll need to revoke before we fail the payment).
-       RemoteRemoved,
+       RemoteRemoved(Option<HTLCFailReason>),
        /// Remote removed this and sent a commitment_signed (implying we've revoke_and_ack'ed it), but
        /// the remote side hasn't yet revoked their previous state, which we need them to do before we
        /// can do any backwards failing. Implies AwaitingRemoteRevoke.
        /// We also have not yet removed this HTLC in a commitment_signed message, and are waiting on a
        /// remote revoke_and_ack on a previous state before we can do so.
-       AwaitingRemoteRevokeToRemove,
+       AwaitingRemoteRevokeToRemove(Option<HTLCFailReason>),
        /// Remote removed this and sent a commitment_signed (implying we've revoke_and_ack'ed it), but
        /// the remote side hasn't yet revoked their previous state, which we need them to do before we
        /// can do any backwards failing. Implies AwaitingRemoteRevoke.
        /// We have removed this HTLC in our latest commitment_signed and are now just waiting on a
        /// revoke_and_ack to drop completely.
-       AwaitingRemovedRemoteRevoke,
+       AwaitingRemovedRemoteRevoke(Option<HTLCFailReason>),
 }
 
 struct OutboundHTLCOutput {
@@ -129,8 +129,6 @@ struct OutboundHTLCOutput {
        payment_hash: PaymentHash,
        state: OutboundHTLCState,
        source: HTLCSource,
-       /// If we're in a removed state, set if they failed, otherwise None
-       fail_reason: Option<HTLCFailReason>,
 }
 
 /// See AwaitingRemoteRevoke ChannelState for more info
@@ -859,9 +857,9 @@ impl Channel {
                        let (include, state_name) = match htlc.state {
                                OutboundHTLCState::LocalAnnounced(_) => (generated_by_local, "LocalAnnounced"),
                                OutboundHTLCState::Committed => (true, "Committed"),
-                               OutboundHTLCState::RemoteRemoved => (generated_by_local, "RemoteRemoved"),
-                               OutboundHTLCState::AwaitingRemoteRevokeToRemove => (generated_by_local, "AwaitingRemoteRevokeToRemove"),
-                               OutboundHTLCState::AwaitingRemovedRemoteRevoke => (false, "AwaitingRemovedRemoteRevoke"),
+                               OutboundHTLCState::RemoteRemoved(_) => (generated_by_local, "RemoteRemoved"),
+                               OutboundHTLCState::AwaitingRemoteRevokeToRemove(_) => (generated_by_local, "AwaitingRemoteRevokeToRemove"),
+                               OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) => (false, "AwaitingRemovedRemoteRevoke"),
                        };
 
                        if include {
@@ -870,13 +868,11 @@ impl Channel {
                        } else {
                                log_trace!(self, "   ...not including outbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, log_bytes!(htlc.payment_hash.0), htlc.amount_msat, state_name);
                                match htlc.state {
-                                       OutboundHTLCState::AwaitingRemoteRevokeToRemove|OutboundHTLCState::AwaitingRemovedRemoteRevoke => {
-                                               if htlc.fail_reason.is_none() {
-                                                       value_to_self_msat_offset -= htlc.amount_msat as i64;
-                                               }
+                                       OutboundHTLCState::AwaitingRemoteRevokeToRemove(None)|OutboundHTLCState::AwaitingRemovedRemoteRevoke(None) => {
+                                               value_to_self_msat_offset -= htlc.amount_msat as i64;
                                        },
-                                       OutboundHTLCState::RemoteRemoved => {
-                                               if !generated_by_local && htlc.fail_reason.is_none() {
+                                       OutboundHTLCState::RemoteRemoved(None) => {
+                                               if !generated_by_local {
                                                        value_to_self_msat_offset -= htlc.amount_msat as i64;
                                                }
                                        },
@@ -885,9 +881,14 @@ impl Channel {
                        }
                }
 
-
                let value_to_self_msat: i64 = (self.value_to_self_msat - local_htlc_total_msat) as i64 + value_to_self_msat_offset;
-               let value_to_remote_msat: i64 = (self.channel_value_satoshis * 1000 - self.value_to_self_msat - remote_htlc_total_msat) as i64 - value_to_self_msat_offset;
+               assert!(value_to_self_msat >= 0);
+               // Note that in case they have several just-awaiting-last-RAA fulfills in-progress (ie
+               // AwaitingRemoteRevokeToRemove or AwaitingRemovedRemoteRevoke) we may have allowed them to
+               // "violate" their reserve value by couting those against it. Thus, we have to convert
+               // everything to i64 before subtracting as otherwise we can overflow.
+               let value_to_remote_msat: i64 = (self.channel_value_satoshis * 1000) as i64 - (self.value_to_self_msat as i64) - (remote_htlc_total_msat as i64) - value_to_self_msat_offset;
+               assert!(value_to_remote_msat >= 0);
 
                #[cfg(debug_assertions)]
                {
@@ -1611,7 +1612,24 @@ impl Channel {
                // Check our_channel_reserve_satoshis (we're getting paid, so they have to at least meet
                // the reserve_satoshis we told them to always have as direct payment so that they lose
                // something if we punish them for broadcasting an old state).
-               if htlc_inbound_value_msat + msg.amount_msat + self.value_to_self_msat > (self.channel_value_satoshis - Channel::get_our_channel_reserve_satoshis(self.channel_value_satoshis)) * 1000 {
+               // Note that we don't really care about having a small/no to_remote output in our local
+               // commitment transactions, as the purpose of the channel reserve is to ensure we can
+               // punish *them* if they misbehave, so we discount any outbound HTLCs which will not be
+               // present in the next commitment transaction we send them (at least for fulfilled ones,
+               // failed ones won't modify value_to_self).
+               // Note that we will send HTLCs which another instance of rust-lightning would think
+               // violate the reserve value if we do not do this (as we forget inbound HTLCs from the
+               // Channel state once they will not be present in the next received commitment
+               // transaction).
+               let mut removed_outbound_total_msat = 0;
+               for ref htlc in self.pending_outbound_htlcs.iter() {
+                       if let OutboundHTLCState::AwaitingRemoteRevokeToRemove(None) = htlc.state {
+                               removed_outbound_total_msat += htlc.amount_msat;
+                       } else if let OutboundHTLCState::AwaitingRemovedRemoteRevoke(None) = htlc.state {
+                               removed_outbound_total_msat += htlc.amount_msat;
+                       }
+               }
+               if htlc_inbound_value_msat + msg.amount_msat + self.value_to_self_msat > (self.channel_value_satoshis - Channel::get_our_channel_reserve_satoshis(self.channel_value_satoshis)) * 1000 + removed_outbound_total_msat {
                        return Err(ChannelError::Close("Remote HTLC add would put them over their reserve value"));
                }
                if self.next_remote_htlc_id != msg.htlc_id {
@@ -1657,10 +1675,9 @@ impl Channel {
                                        OutboundHTLCState::LocalAnnounced(_) =>
                                                return Err(ChannelError::Close("Remote tried to fulfill/fail HTLC before it had been committed")),
                                        OutboundHTLCState::Committed => {
-                                               htlc.state = OutboundHTLCState::RemoteRemoved;
-                                               htlc.fail_reason = fail_reason;
+                                               htlc.state = OutboundHTLCState::RemoteRemoved(fail_reason);
                                        },
-                                       OutboundHTLCState::AwaitingRemoteRevokeToRemove | OutboundHTLCState::AwaitingRemovedRemoteRevoke | OutboundHTLCState::RemoteRemoved =>
+                                       OutboundHTLCState::AwaitingRemoteRevokeToRemove(_) | OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) | OutboundHTLCState::RemoteRemoved(_) =>
                                                return Err(ChannelError::Close("Remote tried to fulfill/fail HTLC that they'd already fulfilled/failed")),
                                }
                                return Ok(&htlc.source);
@@ -1813,8 +1830,10 @@ impl Channel {
                        }
                }
                for htlc in self.pending_outbound_htlcs.iter_mut() {
-                       if let OutboundHTLCState::RemoteRemoved = htlc.state {
-                               htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove;
+                       if let Some(fail_reason) = if let &mut OutboundHTLCState::RemoteRemoved(ref mut fail_reason) = &mut htlc.state {
+                               Some(fail_reason.take())
+                       } else { None } {
+                               htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove(fail_reason);
                                need_our_commitment = true;
                        }
                }
@@ -1867,6 +1886,8 @@ impl Channel {
        fn free_holding_cell_htlcs(&mut self) -> Result<Option<(msgs::CommitmentUpdate, ChannelMonitor)>, ChannelError> {
                assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, 0);
                if self.holding_cell_htlc_updates.len() != 0 || self.holding_cell_update_fee.is_some() {
+                       log_trace!(self, "Freeing holding cell with {} HTLC updates{}", self.holding_cell_htlc_updates.len(), if self.holding_cell_update_fee.is_some() { " and a fee update" } else { "" });
+
                        let mut htlc_updates = Vec::new();
                        mem::swap(&mut htlc_updates, &mut self.holding_cell_htlc_updates);
                        let mut update_add_htlcs = Vec::with_capacity(htlc_updates.len());
@@ -2032,9 +2053,9 @@ impl Channel {
                                } else { true }
                        });
                        pending_outbound_htlcs.retain(|htlc| {
-                               if let OutboundHTLCState::AwaitingRemovedRemoteRevoke = htlc.state {
+                               if let &OutboundHTLCState::AwaitingRemovedRemoteRevoke(ref fail_reason) = &htlc.state {
                                        log_trace!(logger, " ...removing outbound AwaitingRemovedRemoteRevoke {}", log_bytes!(htlc.payment_hash.0));
-                                       if let Some(reason) = htlc.fail_reason.clone() { // We really want take() here, but, again, non-mut ref :(
+                                       if let Some(reason) = fail_reason.clone() { // We really want take() here, but, again, non-mut ref :(
                                                revoked_htlcs.push((htlc.source.clone(), htlc.payment_hash, reason));
                                        } else {
                                                // They fulfilled, so we sent them money
@@ -2085,9 +2106,12 @@ impl Channel {
                                if let OutboundHTLCState::LocalAnnounced(_) = htlc.state {
                                        log_trace!(logger, " ...promoting outbound LocalAnnounced {} to Committed", log_bytes!(htlc.payment_hash.0));
                                        htlc.state = OutboundHTLCState::Committed;
-                               } else if let OutboundHTLCState::AwaitingRemoteRevokeToRemove = htlc.state {
+                               }
+                               if let Some(fail_reason) = if let &mut OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref mut fail_reason) = &mut htlc.state {
+                                       Some(fail_reason.take())
+                               } else { None } {
                                        log_trace!(logger, " ...promoting outbound AwaitingRemoteRevokeToRemove {} to AwaitingRemovedRemoteRevoke", log_bytes!(htlc.payment_hash.0));
-                                       htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke;
+                                       htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(fail_reason);
                                        require_commitment = true;
                                }
                        }
@@ -2243,7 +2267,7 @@ impl Channel {
                self.next_remote_htlc_id -= inbound_drop_count;
 
                for htlc in self.pending_outbound_htlcs.iter_mut() {
-                       if let OutboundHTLCState::RemoteRemoved = htlc.state {
+                       if let OutboundHTLCState::RemoteRemoved(_) = htlc.state {
                                // They sent us an update to remove this but haven't yet sent the corresponding
                                // commitment_signed, we need to move it back to Committed and they can re-send
                                // the update upon reconnection.
@@ -3261,7 +3285,6 @@ impl Channel {
                        cltv_expiry: cltv_expiry,
                        state: OutboundHTLCState::LocalAnnounced(Box::new(onion_routing_packet.clone())),
                        source,
-                       fail_reason: None,
                });
 
                let res = msgs::UpdateAddHTLC {
@@ -3326,8 +3349,10 @@ impl Channel {
                        }
                }
                for htlc in self.pending_outbound_htlcs.iter_mut() {
-                       if let OutboundHTLCState::AwaitingRemoteRevokeToRemove = htlc.state {
-                               htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke;
+                       if let Some(fail_reason) = if let &mut OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref mut fail_reason) = &mut htlc.state {
+                               Some(fail_reason.take())
+                       } else { None } {
+                               htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(fail_reason);
                        }
                }
 
@@ -3593,7 +3618,6 @@ impl Writeable for Channel {
                        htlc.cltv_expiry.write(writer)?;
                        htlc.payment_hash.write(writer)?;
                        htlc.source.write(writer)?;
-                       write_option!(htlc.fail_reason);
                        match &htlc.state {
                                &OutboundHTLCState::LocalAnnounced(ref onion_packet) => {
                                        0u8.write(writer)?;
@@ -3602,14 +3626,17 @@ impl Writeable for Channel {
                                &OutboundHTLCState::Committed => {
                                        1u8.write(writer)?;
                                },
-                               &OutboundHTLCState::RemoteRemoved => {
+                               &OutboundHTLCState::RemoteRemoved(ref fail_reason) => {
                                        2u8.write(writer)?;
+                                       write_option!(*fail_reason);
                                },
-                               &OutboundHTLCState::AwaitingRemoteRevokeToRemove => {
+                               &OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref fail_reason) => {
                                        3u8.write(writer)?;
+                                       write_option!(*fail_reason);
                                },
-                               &OutboundHTLCState::AwaitingRemovedRemoteRevoke => {
+                               &OutboundHTLCState::AwaitingRemovedRemoteRevoke(ref fail_reason) => {
                                        4u8.write(writer)?;
+                                       write_option!(*fail_reason);
                                },
                        }
                }
@@ -3772,13 +3799,12 @@ impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {
                                cltv_expiry: Readable::read(reader)?,
                                payment_hash: Readable::read(reader)?,
                                source: Readable::read(reader)?,
-                               fail_reason: Readable::read(reader)?,
                                state: match <u8 as Readable<R>>::read(reader)? {
                                        0 => OutboundHTLCState::LocalAnnounced(Box::new(Readable::read(reader)?)),
                                        1 => OutboundHTLCState::Committed,
-                                       2 => OutboundHTLCState::RemoteRemoved,
-                                       3 => OutboundHTLCState::AwaitingRemoteRevokeToRemove,
-                                       4 => OutboundHTLCState::AwaitingRemovedRemoteRevoke,
+                                       2 => OutboundHTLCState::RemoteRemoved(Readable::read(reader)?),
+                                       3 => OutboundHTLCState::AwaitingRemoteRevokeToRemove(Readable::read(reader)?),
+                                       4 => OutboundHTLCState::AwaitingRemovedRemoteRevoke(Readable::read(reader)?),
                                        _ => return Err(DecodeError::InvalidValue),
                                },
                        });
@@ -4173,7 +4199,6 @@ mod tests {
                                payment_hash: PaymentHash([0; 32]),
                                state: OutboundHTLCState::Committed,
                                source: HTLCSource::dummy(),
-                               fail_reason: None,
                        };
                        out.payment_hash.0 = Sha256::hash(&hex::decode("0202020202020202020202020202020202020202020202020202020202020202").unwrap()).into_inner();
                        out
@@ -4186,7 +4211,6 @@ mod tests {
                                payment_hash: PaymentHash([0; 32]),
                                state: OutboundHTLCState::Committed,
                                source: HTLCSource::dummy(),
-                               fail_reason: None,
                        };
                        out.payment_hash.0 = Sha256::hash(&hex::decode("0303030303030303030303030303030303030303030303030303030303030303").unwrap()).into_inner();
                        out
index 1f11fe74bef235e92fe491cca95436c9551690e2..387c83761a69cb1ee063b90299174c873d6cda77 100644 (file)
@@ -542,6 +542,33 @@ macro_rules! expect_pending_htlcs_forwardable {
        }}
 }
 
+macro_rules! expect_payment_received {
+       ($node: expr, $expected_payment_hash: expr, $expected_recv_value: expr) => {
+               let events = $node.node.get_and_clear_pending_events();
+               assert_eq!(events.len(), 1);
+               match events[0] {
+                       Event::PaymentReceived { ref payment_hash, amt } => {
+                               assert_eq!($expected_payment_hash, *payment_hash);
+                               assert_eq!($expected_recv_value, amt);
+                       },
+                       _ => panic!("Unexpected event"),
+               }
+       }
+}
+
+macro_rules! expect_payment_sent {
+       ($node: expr, $expected_payment_preimage: expr) => {
+               let events = $node.node.get_and_clear_pending_events();
+               assert_eq!(events.len(), 1);
+               match events[0] {
+                       Event::PaymentSent { ref payment_preimage } => {
+                               assert_eq!($expected_payment_preimage, *payment_preimage);
+                       },
+                       _ => panic!("Unexpected event"),
+               }
+       }
+}
+
 pub fn send_along_route_with_hash(origin_node: &Node, route: Route, expected_route: &[&Node], recv_value: u64, our_payment_hash: PaymentHash) {
        let mut payment_event = {
                origin_node.node.send_payment(route, our_payment_hash).unwrap();
@@ -664,14 +691,7 @@ pub fn claim_payment_along_route(origin_node: &Node, expected_route: &[&Node], s
 
        if !skip_last {
                last_update_fulfill_dance!(origin_node, expected_route.first().unwrap());
-               let events = origin_node.node.get_and_clear_pending_events();
-               assert_eq!(events.len(), 1);
-               match events[0] {
-                       Event::PaymentSent { payment_preimage } => {
-                               assert_eq!(payment_preimage, our_payment_preimage);
-                       },
-                       _ => panic!("Unexpected event"),
-               }
+               expect_payment_sent!(origin_node, our_payment_preimage);
        }
 }
 
@@ -935,20 +955,6 @@ pub fn get_announce_close_broadcast_events(nodes: &Vec<Node>, a: usize, b: usize
        }
 }
 
-macro_rules! expect_payment_received {
-       ($node: expr, $expected_payment_hash: expr, $expected_recv_value: expr) => {
-               let events = $node.node.get_and_clear_pending_events();
-               assert_eq!(events.len(), 1);
-               match events[0] {
-                       Event::PaymentReceived { ref payment_hash, amt } => {
-                               assert_eq!($expected_payment_hash, *payment_hash);
-                               assert_eq!($expected_recv_value, amt);
-                       },
-                       _ => panic!("Unexpected event"),
-               }
-       }
-}
-
 macro_rules! get_channel_value_stat {
        ($node: expr, $channel_id: expr) => {{
                let chan_lock = $node.node.channel_state.lock().unwrap();
index 049777dad46bd76c41a4dbab13b0225e79007a51..0e82b4e696b91084068b156c9dc0cac9e9cae55f 100644 (file)
@@ -1450,6 +1450,153 @@ fn channel_reserve_test() {
        do_channel_reserve_test(true);
 }
 
+#[test]
+fn channel_reserve_in_flight_removes() {
+       // In cases where one side claims an HTLC, it thinks it has additional available funds that it
+       // can send to its counterparty, but due to update ordering, the other side may not yet have
+       // considered those HTLCs fully removed.
+       // This tests that we don't count HTLCs which will not be included in the next remote
+       // commitment transaction towards the reserve value (as it implies no commitment transaction
+       // will be generated which violates the remote reserve value).
+       // This was broken previously, and discovered by the chanmon_fail_consistency fuzz test.
+       // To test this we:
+       //  * route two HTLCs from A to B (note that, at a high level, this test is checking that, when
+       //    you consider the values of both of these HTLCs, B may not send an HTLC back to A, but if
+       //    you only consider the value of the first HTLC, it may not),
+       //  * start routing a third HTLC from A to B,
+       //  * claim the first two HTLCs (though B will generate an update_fulfill for one, and put
+       //    the other claim in its holding cell, as it immediately goes into AwaitingRAA),
+       //  * deliver the first fulfill from B
+       //  * deliver the update_add and an RAA from A, resulting in B freeing the second holding cell
+       //    claim,
+       //  * deliver A's response CS and RAA.
+       //    This results in A having the second HTLC in AwaitingRemovedRemoteRevoke, but B having
+       //    removed it fully. B now has the push_msat plus the first two HTLCs in value.
+       //  * Now B happily sends another HTLC, potentially violating its reserve value from A's point
+       //    of view (if A counts the AwaitingRemovedRemoteRevoke HTLC).
+       let mut nodes = create_network(2);
+       let chan_1 = create_announced_chan_between_nodes(&nodes, 0, 1);
+
+       let b_chan_values = get_channel_value_stat!(nodes[1], chan_1.2);
+       // Route the first two HTLCs.
+       let (payment_preimage_1, _) = route_payment(&nodes[0], &[&nodes[1]], b_chan_values.channel_reserve_msat - b_chan_values.value_to_self_msat - 10000);
+       let (payment_preimage_2, _) = route_payment(&nodes[0], &[&nodes[1]], 20000);
+
+       // Start routing the third HTLC (this is just used to get everyone in the right state).
+       let (payment_preimage_3, payment_hash_3) = get_payment_preimage_hash!(nodes[0]);
+       let send_1 = {
+               let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &[], 100000, TEST_FINAL_CLTV).unwrap();
+               nodes[0].node.send_payment(route, payment_hash_3).unwrap();
+               check_added_monitors!(nodes[0], 1);
+               let mut events = nodes[0].node.get_and_clear_pending_msg_events();
+               assert_eq!(events.len(), 1);
+               SendEvent::from_event(events.remove(0))
+       };
+
+       // Now claim both of the first two HTLCs on B's end, putting B in AwaitingRAA and generating an
+       // initial fulfill/CS.
+       assert!(nodes[1].node.claim_funds(payment_preimage_1));
+       check_added_monitors!(nodes[1], 1);
+       let bs_removes = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
+
+       // This claim goes in B's holding cell, allowing us to have a pending B->A RAA which does not
+       // remove the second HTLC when we send the HTLC back from B to A.
+       assert!(nodes[1].node.claim_funds(payment_preimage_2));
+       check_added_monitors!(nodes[1], 1);
+       assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+
+       nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_removes.update_fulfill_htlcs[0]).unwrap();
+       nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_removes.commitment_signed).unwrap();
+       check_added_monitors!(nodes[0], 1);
+       let as_raa = get_event_msg!(nodes[0], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());
+       expect_payment_sent!(nodes[0], payment_preimage_1);
+
+       nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &send_1.msgs[0]).unwrap();
+       nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &send_1.commitment_msg).unwrap();
+       check_added_monitors!(nodes[1], 1);
+       // B is already AwaitingRAA, so cant generate a CS here
+       let bs_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[0].node.get_our_node_id());
+
+       nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_raa).unwrap();
+       check_added_monitors!(nodes[1], 1);
+       let bs_cs = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
+
+       nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_raa).unwrap();
+       check_added_monitors!(nodes[0], 1);
+       let as_cs = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
+
+       nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_cs.commitment_signed).unwrap();
+       check_added_monitors!(nodes[1], 1);
+       let bs_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[0].node.get_our_node_id());
+
+       // The second HTLCis removed, but as A is in AwaitingRAA it can't generate a CS here, so the
+       // RAA that B generated above doesn't fully resolve the second HTLC from A's point of view.
+       // However, the RAA A generates here *does* fully resolve the HTLC from B's point of view (as A
+       // can no longer broadcast a commitment transaction with it and B has the preimage so can go
+       // on-chain as necessary).
+       nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_cs.update_fulfill_htlcs[0]).unwrap();
+       nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_cs.commitment_signed).unwrap();
+       check_added_monitors!(nodes[0], 1);
+       let as_raa = get_event_msg!(nodes[0], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());
+       expect_payment_sent!(nodes[0], payment_preimage_2);
+
+       nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_raa).unwrap();
+       check_added_monitors!(nodes[1], 1);
+       assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+
+       expect_pending_htlcs_forwardable!(nodes[1]);
+       expect_payment_received!(nodes[1], payment_hash_3, 100000);
+
+       // Note that as this RAA was generated before the delivery of the update_fulfill it shouldn't
+       // resolve the second HTLC from A's point of view.
+       nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_raa).unwrap();
+       check_added_monitors!(nodes[0], 1);
+       let as_cs = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
+
+       // Now that B doesn't have the second RAA anymore, but A still does, send a payment from B back
+       // to A to ensure that A doesn't count the almost-removed HTLC in update_add processing.
+       let (payment_preimage_4, payment_hash_4) = get_payment_preimage_hash!(nodes[1]);
+       let send_2 = {
+               let route = nodes[1].router.get_route(&nodes[0].node.get_our_node_id(), None, &[], 10000, TEST_FINAL_CLTV).unwrap();
+               nodes[1].node.send_payment(route, payment_hash_4).unwrap();
+               check_added_monitors!(nodes[1], 1);
+               let mut events = nodes[1].node.get_and_clear_pending_msg_events();
+               assert_eq!(events.len(), 1);
+               SendEvent::from_event(events.remove(0))
+       };
+
+       nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &send_2.msgs[0]).unwrap();
+       nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &send_2.commitment_msg).unwrap();
+       check_added_monitors!(nodes[0], 1);
+       let as_raa = get_event_msg!(nodes[0], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());
+
+       // Now just resolve all the outstanding messages/HTLCs for completeness...
+
+       nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_cs.commitment_signed).unwrap();
+       check_added_monitors!(nodes[1], 1);
+       let bs_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[0].node.get_our_node_id());
+
+       nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_raa).unwrap();
+       check_added_monitors!(nodes[1], 1);
+
+       nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_raa).unwrap();
+       check_added_monitors!(nodes[0], 1);
+       let as_cs = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
+
+       nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_cs.commitment_signed).unwrap();
+       check_added_monitors!(nodes[1], 1);
+       let bs_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[0].node.get_our_node_id());
+
+       nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_raa).unwrap();
+       check_added_monitors!(nodes[0], 1);
+
+       expect_pending_htlcs_forwardable!(nodes[0]);
+       expect_payment_received!(nodes[0], payment_hash_4, 10000);
+
+       claim_payment(&nodes[1], &[&nodes[0]], payment_preimage_4);
+       claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_3);
+}
+
 #[test]
 fn channel_monitor_network_test() {
        // Simple test which builds a network of ChannelManagers, connects them to each other, and