From: Matt Corallo <649246+TheBlueMatt@users.noreply.github.com> Date: Mon, 25 Mar 2019 21:12:00 +0000 (-0400) Subject: Merge pull request #325 from TheBlueMatt/2019-03-322-cleanup X-Git-Tag: v0.0.12~219 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=91dc91f0532b45dc0874a4003cc945aff600d991;hp=3cc7666c5ae1e3ac2e73870acf67eadac2a532be;p=rust-lightning Merge pull request #325 from TheBlueMatt/2019-03-322-cleanup Extract preimage from revoked HTLC-Success to claim backward --- diff --git a/src/ln/channel.rs b/src/ln/channel.rs index 64da86e58..b67468b38 100644 --- a/src/ln/channel.rs +++ b/src/ln/channel.rs @@ -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), /// 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), /// 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), } 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, } /// 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)] { @@ -934,7 +935,19 @@ impl Channel { }, None)); } - transaction_utils::sort_outputs(&mut txouts); + transaction_utils::sort_outputs(&mut txouts, |a, b| { + if let &Some(ref a_htlc) = a { + if let &Some(ref b_htlc) = b { + a_htlc.0.cltv_expiry.cmp(&b_htlc.0.cltv_expiry) + // Note that due to hash collisions, we have to have a fallback comparison + // here for fuzztarget mode (otherwise at least chanmon_fail_consistency + // may fail)! + .then(a_htlc.0.payment_hash.0.cmp(&b_htlc.0.payment_hash.0)) + // For non-HTLC outputs, if they're copying our SPK we don't really care if we + // close the channel due to mismatches - they're doing something dumb: + } else { cmp::Ordering::Equal } + } else { cmp::Ordering::Equal } + }); let mut outputs: Vec = Vec::with_capacity(txouts.len()); let mut htlcs_included: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(txouts.len() + included_dust_htlcs.len()); @@ -1010,7 +1023,7 @@ impl Channel { }, ())); } - transaction_utils::sort_outputs(&mut txouts); + transaction_utils::sort_outputs(&mut txouts, |_, _| { cmp::Ordering::Equal }); // Ordering doesnt matter if they used our pubkey... let mut outputs: Vec = Vec::new(); for out in txouts.drain(..) { @@ -1599,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 { @@ -1645,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); @@ -1801,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; } } @@ -1855,6 +1886,8 @@ impl Channel { fn free_holding_cell_htlcs(&mut self) -> Result, 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()); @@ -2020,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 @@ -2073,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; } } @@ -2231,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. @@ -3249,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 { @@ -3314,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); } } @@ -3581,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)?; @@ -3590,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); }, } } @@ -3760,13 +3799,12 @@ impl ReadableArgs> for Channel { cltv_expiry: Readable::read(reader)?, payment_hash: Readable::read(reader)?, source: Readable::read(reader)?, - fail_reason: Readable::read(reader)?, state: match >::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), }, }); @@ -4161,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 @@ -4174,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 diff --git a/src/ln/functional_test_utils.rs b/src/ln/functional_test_utils.rs index 1f11fe74b..387c83761 100644 --- a/src/ln/functional_test_utils.rs +++ b/src/ln/functional_test_utils.rs @@ -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, 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(); diff --git a/src/ln/functional_tests.rs b/src/ln/functional_tests.rs index 049777dad..0e82b4e69 100644 --- a/src/ln/functional_tests.rs +++ b/src/ln/functional_tests.rs @@ -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 diff --git a/src/util/transaction_utils.rs b/src/util/transaction_utils.rs index 74ea81adb..f9ee1bd82 100644 --- a/src/util/transaction_utils.rs +++ b/src/util/transaction_utils.rs @@ -1,34 +1,14 @@ -use bitcoin::blockdata::transaction::{TxIn, TxOut}; -use bitcoin_hashes::sha256d::Hash as Sha256dHash; +use bitcoin::blockdata::transaction::TxOut; use std::cmp::Ordering; -pub fn sort_outputs(outputs: &mut Vec<(TxOut, T)>) { +pub fn sort_outputs Ordering>(outputs: &mut Vec<(TxOut, T)>, tie_breaker: C) { outputs.sort_unstable_by(|a, b| { - a.0.value.cmp(&b.0.value).then( - a.0.script_pubkey[..].cmp(&b.0.script_pubkey[..]) - ) - }); -} - -fn cmp(a: &Sha256dHash, b: &Sha256dHash) -> Ordering { - use bitcoin_hashes::Hash; - - let av = a.into_inner(); - let bv = b.into_inner(); - for i in (0..32).rev() { - let cmp = av[i].cmp(&bv[i]); - if cmp != Ordering::Equal { - return cmp; - } - } - Ordering::Equal -} - -pub fn sort_inputs(inputs: &mut Vec<(TxIn, T)>) { - inputs.sort_unstable_by(|a, b| { - cmp( &a.0.previous_output.txid, &b.0.previous_output.txid).then( - a.0.previous_output.vout.cmp(&b.0.previous_output.vout)) + a.0.value.cmp(&b.0.value).then_with(|| { + a.0.script_pubkey[..].cmp(&b.0.script_pubkey[..]).then_with(|| { + tie_breaker(&a.1, &b.1) + }) + }) }); } @@ -37,9 +17,7 @@ mod tests { use super::*; use bitcoin::blockdata::script::{Script, Builder}; - use bitcoin::blockdata::transaction::{TxOut, OutPoint}; - use bitcoin_hashes::sha256d::Hash as Sha256dHash; - use bitcoin_hashes::hex::FromHex; + use bitcoin::blockdata::transaction::TxOut; use hex::decode; @@ -58,7 +36,7 @@ mod tests { let txout2_ = txout2.clone(); let mut outputs = vec![(txout1, "ignore"), (txout2, "ignore")]; - sort_outputs(&mut outputs); + sort_outputs(&mut outputs, |_, _| { unreachable!(); }); assert_eq!( &outputs, @@ -81,7 +59,7 @@ mod tests { let txout2_ = txout2.clone(); let mut outputs = vec![(txout1, "ignore"), (txout2, "ignore")]; - sort_outputs(&mut outputs); + sort_outputs(&mut outputs, |_, _| { unreachable!(); }); assert_eq!( &outputs, @@ -105,7 +83,7 @@ mod tests { let txout2_ = txout2.clone(); let mut outputs = vec![(txout1, "ignore"), (txout2, "ignore")]; - sort_outputs(&mut outputs); + sort_outputs(&mut outputs, |_, _| { unreachable!(); }); assert_eq!(&outputs, &vec![(txout1_, "ignore"), (txout2_, "ignore")]); } @@ -131,7 +109,7 @@ mod tests { outputs.reverse(); // prep it // actually do the work! - sort_outputs(&mut outputs); + sort_outputs(&mut outputs, |_, _| { unreachable!(); }); assert_eq!(outputs, expected); } @@ -151,63 +129,4 @@ mod tests { bip69_txout_test_1: TXOUT1.to_vec(), bip69_txout_test_2: TXOUT2.to_vec(), } - - macro_rules! bip_txin_tests { - ($($name:ident: $value:expr,)*) => { - $( - #[test] - fn $name() { - let expected_raw: Vec<(&str, u32)> = $value; - let expected: Vec<(TxIn, &str)> = expected_raw.iter().map( - |txin_raw| TxIn { - previous_output: OutPoint { - txid: Sha256dHash::from_hex(txin_raw.0).unwrap(), - vout: txin_raw.1, - }, - script_sig: Script::new(), - sequence: 0, - witness: vec![] - } - ).map(|txin| (txin, "ignore")).collect(); - - let mut inputs = expected.clone(); - inputs.reverse(); - - sort_inputs(&mut inputs); - - assert_eq!(expected, inputs); - } - )* - } - } - - const TXIN1_BIP69: [(&str, u32); 17] = [ - ("0e53ec5dfb2cb8a71fec32dc9a634a35b7e24799295ddd5278217822e0b31f57", 0), - ("26aa6e6d8b9e49bb0630aac301db6757c02e3619feb4ee0eea81eb1672947024", 1), - ("28e0fdd185542f2c6ea19030b0796051e7772b6026dd5ddccd7a2f93b73e6fc2", 0), - ("381de9b9ae1a94d9c17f6a08ef9d341a5ce29e2e60c36a52d333ff6203e58d5d", 1), - ("3b8b2f8efceb60ba78ca8bba206a137f14cb5ea4035e761ee204302d46b98de2", 0), - ("402b2c02411720bf409eff60d05adad684f135838962823f3614cc657dd7bc0a", 1), - ("54ffff182965ed0957dba1239c27164ace5a73c9b62a660c74b7b7f15ff61e7a", 1), - ("643e5f4e66373a57251fb173151e838ccd27d279aca882997e005016bb53d5aa", 0), - ("6c1d56f31b2de4bfc6aaea28396b333102b1f600da9c6d6149e96ca43f1102b1", 1), - ("7a1de137cbafb5c70405455c49c5104ca3057a1f1243e6563bb9245c9c88c191", 0), - ("7d037ceb2ee0dc03e82f17be7935d238b35d1deabf953a892a4507bfbeeb3ba4", 1), - ("a5e899dddb28776ea9ddac0a502316d53a4a3fca607c72f66c470e0412e34086", 0), - ("b4112b8f900a7ca0c8b0e7c4dfad35c6be5f6be46b3458974988e1cdb2fa61b8", 0), - ("bafd65e3c7f3f9fdfdc1ddb026131b278c3be1af90a4a6ffa78c4658f9ec0c85", 0), - ("de0411a1e97484a2804ff1dbde260ac19de841bebad1880c782941aca883b4e9", 1), - ("f0a130a84912d03c1d284974f563c5949ac13f8342b8112edff52971599e6a45", 0), - ("f320832a9d2e2452af63154bc687493484a0e7745ebd3aaf9ca19eb80834ad60", 0), - ]; - - - const TXIN2_BIP69: [(&str, u32); 2] = [ - ("35288d269cee1941eaebb2ea85e32b42cdb2b04284a56d8b14dcc3f5c65d6055", 0), - ("35288d269cee1941eaebb2ea85e32b42cdb2b04284a56d8b14dcc3f5c65d6055", 1), - ]; - bip_txin_tests! { - bip69_txin_test_1: TXIN1_BIP69.to_vec(), - bip69_txin_test_2: TXIN2_BIP69.to_vec(), - } }