Req+check payment secrets for inbound payments pre-PaymentReceived
authorMatt Corallo <git@bluematt.me>
Fri, 23 Apr 2021 04:04:55 +0000 (04:04 +0000)
committerMatt Corallo <git@bluematt.me>
Wed, 28 Apr 2021 19:30:25 +0000 (15:30 -0400)
Our current PaymentReceived API is incredibly easy to mis-use -
the "obvious" way to implement a client is to always call
`ChannelManager::claim_funds` in response to a `PaymentReceived`
event. However, users are *required* to check the payment secret
and value against the expected values before claiming in order to
avoid a number of potentially funds-losing attacks.

Instead, if we rely on payment secrets being pre-registered with
the ChannelManager before we receive HTLCs for a payment we can
simply check the payment secrets and never generate
`PaymentReceived` events if they do not match. Further, when the
user knows the value to expect in advance, we can have them
register it as well, allowing us to check it for them.

Other implementations already require payment secrets for inbound
payments, so this shouldn't materially lose compatibility.

lightning/src/ln/channelmanager.rs
lightning/src/ln/functional_tests.rs
lightning/src/ln/onion_route_tests.rs

index bfd41d11a8c1bc3f815d676db9336b39794e34a0..853c50142bba41e78205870b2023bc9015cca1f4 100644 (file)
@@ -96,7 +96,7 @@ enum PendingHTLCRouting {
                short_channel_id: u64, // This should be NonZero<u64> eventually when we bump MSRV
        },
        Receive {
-               payment_data: Option<msgs::FinalOnionHopData>,
+               payment_data: msgs::FinalOnionHopData,
                incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
        },
 }
@@ -156,11 +156,10 @@ pub(crate) struct HTLCPreviousHopData {
 struct ClaimableHTLC {
        prev_hop: HTLCPreviousHopData,
        value: u64,
-       /// Filled in when the HTLC was received with a payment_secret packet, which contains a
-       /// total_msat (which may differ from value if this is a Multi-Path Payment) and a
+       /// Contains a total_msat (which may differ from value if this is a Multi-Path Payment) and a
        /// payment_secret which prevents path-probing attacks and can associate different HTLCs which
        /// are part of the same payment.
-       payment_data: Option<msgs::FinalOnionHopData>,
+       payment_data: msgs::FinalOnionHopData,
        cltv_expiry: u32,
 }
 
@@ -327,12 +326,11 @@ pub(super) struct ChannelHolder<Signer: Sign> {
        /// guarantees are made about the existence of a channel with the short id here, nor the short
        /// ids in the PendingHTLCInfo!
        pub(super) forward_htlcs: HashMap<u64, Vec<HTLCForwardInfo>>,
-       /// (payment_hash, payment_secret) -> Vec<HTLCs> for tracking HTLCs that
-       /// were to us and can be failed/claimed by the user
+       /// Map from payment hash to any HTLCs which are to us and can be failed/claimed by the user.
        /// Note that while this is held in the same mutex as the channels themselves, no consistency
        /// guarantees are made about the channels given here actually existing anymore by the time you
        /// go to read them!
-       claimable_htlcs: HashMap<(PaymentHash, Option<PaymentSecret>), Vec<ClaimableHTLC>>,
+       claimable_htlcs: HashMap<PaymentHash, Vec<ClaimableHTLC>>,
        /// Messages to send to peers - pushed to in the same lock that they are generated in (except
        /// for broadcast messages, where ordering isn't as strict).
        pub(super) pending_msg_events: Vec<MessageSendEvent>,
@@ -1247,6 +1245,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                        msgs::OnionHopDataFormat::FinalNode { payment_data } => payment_data,
                                };
 
+                               if payment_data.is_none() {
+                                       return_err!("We require payment_secrets", 0x4000|0x2000|3, &[0;0]);
+                               }
+
                                // Note that we could obviously respond immediately with an update_fulfill_htlc
                                // message, however that would leak that we are the recipient of this payment, so
                                // instead we stay symmetric with the forwarding case, only responding (after a
@@ -1254,7 +1256,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 
                                PendingHTLCStatus::Forward(PendingHTLCInfo {
                                        routing: PendingHTLCRouting::Receive {
-                                               payment_data,
+                                               payment_data: payment_data.unwrap(),
                                                incoming_cltv_expiry: msg.cltv_expiry,
                                        },
                                        payment_hash: msg.payment_hash.clone(),
@@ -1948,61 +1950,93 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                                        routing: PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry },
                                                                        incoming_shared_secret, payment_hash, amt_to_forward, .. },
                                                                        prev_funding_outpoint } => {
-                                                               let prev_hop = HTLCPreviousHopData {
-                                                                       short_channel_id: prev_short_channel_id,
-                                                                       outpoint: prev_funding_outpoint,
-                                                                       htlc_id: prev_htlc_id,
-                                                                       incoming_packet_shared_secret: incoming_shared_secret,
-                                                               };
-
-                                                               let mut total_value = 0;
-                                                               let payment_secret_opt =
-                                                                       if let &Some(ref data) = &payment_data { Some(data.payment_secret.clone()) } else { None };
-                                                               let htlcs = channel_state.claimable_htlcs.entry((payment_hash, payment_secret_opt))
-                                                                       .or_insert(Vec::new());
-                                                               htlcs.push(ClaimableHTLC {
-                                                                       prev_hop,
+                                                               let claimable_htlc = ClaimableHTLC {
+                                                                       prev_hop: HTLCPreviousHopData {
+                                                                               short_channel_id: prev_short_channel_id,
+                                                                               outpoint: prev_funding_outpoint,
+                                                                               htlc_id: prev_htlc_id,
+                                                                               incoming_packet_shared_secret: incoming_shared_secret,
+                                                                       },
                                                                        value: amt_to_forward,
                                                                        payment_data: payment_data.clone(),
                                                                        cltv_expiry: incoming_cltv_expiry,
-                                                               });
-                                                               if let &Some(ref data) = &payment_data {
-                                                                       for htlc in htlcs.iter() {
-                                                                               total_value += htlc.value;
-                                                                               if htlc.payment_data.as_ref().unwrap().total_msat != data.total_msat {
-                                                                                       total_value = msgs::MAX_VALUE_MSAT;
-                                                                               }
-                                                                               if total_value >= msgs::MAX_VALUE_MSAT { break; }
-                                                                       }
-                                                                       if total_value >= msgs::MAX_VALUE_MSAT || total_value > data.total_msat  {
-                                                                               for htlc in htlcs.iter() {
-                                                                                       let mut htlc_msat_height_data = byte_utils::be64_to_array(htlc.value).to_vec();
-                                                                                       htlc_msat_height_data.extend_from_slice(
-                                                                                               &byte_utils::be32_to_array(self.best_block.read().unwrap().height()),
-                                                                                       );
-                                                                                       failed_forwards.push((HTLCSource::PreviousHopData(HTLCPreviousHopData {
-                                                                                                       short_channel_id: htlc.prev_hop.short_channel_id,
-                                                                                                       outpoint: prev_funding_outpoint,
-                                                                                                       htlc_id: htlc.prev_hop.htlc_id,
-                                                                                                       incoming_packet_shared_secret: htlc.prev_hop.incoming_packet_shared_secret,
-                                                                                               }), payment_hash,
-                                                                                               HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: htlc_msat_height_data }
-                                                                                       ));
-                                                                               }
-                                                                       } else if total_value == data.total_msat {
-                                                                               new_events.push(events::Event::PaymentReceived {
-                                                                                       payment_hash,
-                                                                                       payment_secret: Some(data.payment_secret),
-                                                                                       amt: total_value,
-                                                                               });
+                                                               };
+
+                                                               macro_rules! fail_htlc {
+                                                                       ($htlc: expr) => {
+                                                                               let mut htlc_msat_height_data = byte_utils::be64_to_array($htlc.value).to_vec();
+                                                                               htlc_msat_height_data.extend_from_slice(
+                                                                                       &byte_utils::be32_to_array(self.best_block.read().unwrap().height()),
+                                                                               );
+                                                                               failed_forwards.push((HTLCSource::PreviousHopData(HTLCPreviousHopData {
+                                                                                               short_channel_id: $htlc.prev_hop.short_channel_id,
+                                                                                               outpoint: prev_funding_outpoint,
+                                                                                               htlc_id: $htlc.prev_hop.htlc_id,
+                                                                                               incoming_packet_shared_secret: $htlc.prev_hop.incoming_packet_shared_secret,
+                                                                                       }), payment_hash,
+                                                                                       HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: htlc_msat_height_data }
+                                                                               ));
                                                                        }
-                                                               } else {
-                                                                       new_events.push(events::Event::PaymentReceived {
-                                                                               payment_hash,
-                                                                               payment_secret: None,
-                                                                               amt: amt_to_forward,
-                                                                       });
                                                                }
+
+                                                               // Check that the payment hash and secret are known. Note that we
+                                                               // MUST take care to handle the "unknown payment hash" and
+                                                               // "incorrect payment secret" cases here identically or we'd expose
+                                                               // that we are the ultimate recipient of the given payment hash.
+                                                               // Further, we must not expose whether we have any other HTLCs
+                                                               // associated with the same payment_hash pending or not.
+                                                               let mut payment_secrets = self.pending_inbound_payments.lock().unwrap();
+                                                               match payment_secrets.entry(payment_hash) {
+                                                                       hash_map::Entry::Vacant(_) => {
+                                                                               log_trace!(self.logger, "Failing new HTLC with payment_hash {} as we didn't have a corresponding inbound payment.", log_bytes!(payment_hash.0));
+                                                                               fail_htlc!(claimable_htlc);
+                                                                       },
+                                                                       hash_map::Entry::Occupied(inbound_payment) => {
+                                                                               if inbound_payment.get().payment_secret != payment_data.payment_secret {
+                                                                                       log_trace!(self.logger, "Failing new HTLC with payment_hash {} as it didn't match our expected payment secret.", log_bytes!(payment_hash.0));
+                                                                                       fail_htlc!(claimable_htlc);
+                                                                               } else if inbound_payment.get().min_value_msat.is_some() && payment_data.total_msat < inbound_payment.get().min_value_msat.unwrap() {
+                                                                                       log_trace!(self.logger, "Failing new HTLC with payment_hash {} as it didn't match our minimum value (had {}, needed {}).",
+                                                                                               log_bytes!(payment_hash.0), payment_data.total_msat, inbound_payment.get().min_value_msat.unwrap());
+                                                                                       fail_htlc!(claimable_htlc);
+                                                                               } else {
+                                                                                       let mut total_value = 0;
+                                                                                       let htlcs = channel_state.claimable_htlcs.entry(payment_hash)
+                                                                                               .or_insert(Vec::new());
+                                                                                       htlcs.push(claimable_htlc);
+                                                                                       for htlc in htlcs.iter() {
+                                                                                               total_value += htlc.value;
+                                                                                               if htlc.payment_data.total_msat != payment_data.total_msat {
+                                                                                                       log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the HTLCs had inconsistent total values (eg {} and {})",
+                                                                                                               log_bytes!(payment_hash.0), payment_data.total_msat, htlc.payment_data.total_msat);
+                                                                                                       total_value = msgs::MAX_VALUE_MSAT;
+                                                                                               }
+                                                                                               if total_value >= msgs::MAX_VALUE_MSAT { break; }
+                                                                                       }
+                                                                                       if total_value >= msgs::MAX_VALUE_MSAT || total_value > payment_data.total_msat {
+                                                                                               log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the total value {} ran over expected value {} (or HTLCs were inconsistent)",
+                                                                                                       log_bytes!(payment_hash.0), total_value, payment_data.total_msat);
+                                                                                               for htlc in htlcs.iter() {
+                                                                                                       fail_htlc!(htlc);
+                                                                                               }
+                                                                                       } else if total_value == payment_data.total_msat {
+                                                                                               new_events.push(events::Event::PaymentReceived {
+                                                                                                       payment_hash,
+                                                                                                       payment_secret: Some(payment_data.payment_secret),
+                                                                                                       amt: total_value,
+                                                                                               });
+                                                                                               // Only ever generate at most one PaymentReceived
+                                                                                               // per registered payment_hash, even if it isn't
+                                                                                               // claimed.
+                                                                                               inbound_payment.remove_entry();
+                                                                                       } else {
+                                                                                               // Nothing to do - we haven't reached the total
+                                                                                               // payment value yet, wait until we receive more
+                                                                                               // MPP parts.
+                                                                                       }
+                                                                               }
+                                                                       },
+                                                               };
                                                        },
                                                        HTLCForwardInfo::AddHTLC { .. } => {
                                                                panic!("short_channel_id == 0 should imply any pending_forward entries are of type Receive");
@@ -2092,11 +2126,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
 
                let mut channel_state = Some(self.channel_state.lock().unwrap());
-               let payment_secrets = self.pending_inbound_payments.lock().unwrap();
-               let payment_secret = if let Some(secret) = payment_secrets.get(&payment_hash) {
-                       Some(secret.payment_secret)
-               } else { return false; };
-               let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&(*payment_hash, payment_secret));
+               let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(payment_hash);
                if let Some(mut sources) = removed_source {
                        for htlc in sources.drain(..) {
                                if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); }
@@ -2278,11 +2308,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
 
                let mut channel_state = Some(self.channel_state.lock().unwrap());
-               let payment_secrets = self.pending_inbound_payments.lock().unwrap();
-               let payment_secret = if let Some(secret) = payment_secrets.get(&payment_hash) {
-                       Some(secret.payment_secret)
-               } else { return false; };
-               let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&(payment_hash, payment_secret));
+               let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&payment_hash);
                if let Some(mut sources) = removed_source {
                        assert!(!sources.is_empty());
 
@@ -2298,13 +2324,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                        // provide the preimage, so worrying too much about the optimal handling isn't worth
                        // it.
 
-                       let (is_mpp, mut valid_mpp) = if let &Some(ref data) = &sources[0].payment_data {
-                               assert!(payment_secret.is_some());
-                               (true, data.total_msat >= expected_amount)
-                       } else {
-                               assert!(payment_secret.is_none());
-                               (false, false)
-                       };
+                       let is_mpp = true;
+                       let mut valid_mpp = sources[0].payment_data.total_msat >= expected_amount;
 
                        for htlc in sources.iter() {
                                if !is_mpp || !valid_mpp { break; }
@@ -3667,7 +3688,7 @@ where
                        });
 
                        if let Some(height) = height_opt {
-                               channel_state.claimable_htlcs.retain(|&(ref payment_hash, _), htlcs| {
+                               channel_state.claimable_htlcs.retain(|payment_hash, htlcs| {
                                        htlcs.retain(|htlc| {
                                                // If height is approaching the number of blocks we think it takes us to get
                                                // our commitment transaction confirmed before the HTLC expires, plus the
@@ -4041,7 +4062,8 @@ impl Writeable for PendingHTLCInfo {
                        },
                        &PendingHTLCRouting::Receive { ref payment_data, ref incoming_cltv_expiry } => {
                                1u8.write(writer)?;
-                               payment_data.write(writer)?;
+                               payment_data.payment_secret.write(writer)?;
+                               payment_data.total_msat.write(writer)?;
                                incoming_cltv_expiry.write(writer)?;
                        },
                }
@@ -4062,7 +4084,10 @@ impl Readable for PendingHTLCInfo {
                                        short_channel_id: Readable::read(reader)?,
                                },
                                1u8 => PendingHTLCRouting::Receive {
-                                       payment_data: Readable::read(reader)?,
+                                       payment_data: msgs::FinalOnionHopData {
+                                               payment_secret: Readable::read(reader)?,
+                                               total_msat: Readable::read(reader)?,
+                                       },
                                        incoming_cltv_expiry: Readable::read(reader)?,
                                },
                                _ => return Err(DecodeError::InvalidValue),
@@ -4134,12 +4159,29 @@ impl_writeable!(HTLCPreviousHopData, 0, {
        incoming_packet_shared_secret
 });
 
-impl_writeable!(ClaimableHTLC, 0, {
-       prev_hop,
-       value,
-       payment_data,
-       cltv_expiry
-});
+impl Writeable for ClaimableHTLC {
+       fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
+               self.prev_hop.write(writer)?;
+               self.value.write(writer)?;
+               self.payment_data.payment_secret.write(writer)?;
+               self.payment_data.total_msat.write(writer)?;
+               self.cltv_expiry.write(writer)
+       }
+}
+
+impl Readable for ClaimableHTLC {
+       fn read<R: ::std::io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
+               Ok(ClaimableHTLC {
+                       prev_hop: Readable::read(reader)?,
+                       value: Readable::read(reader)?,
+                       payment_data: msgs::FinalOnionHopData {
+                               payment_secret: Readable::read(reader)?,
+                               total_msat: Readable::read(reader)?,
+                       },
+                       cltv_expiry: Readable::read(reader)?,
+               })
+       }
+}
 
 impl Writeable for HTLCSource {
        fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
index 0ca1c66cf13ad3ce4a854726abe5623436621aa7..0e3e3f01eee84c0278017ed6f2b3efda261982a7 100644 (file)
@@ -5098,22 +5098,25 @@ fn test_onchain_to_onchain_claim() {
 
 #[test]
 fn test_duplicate_payment_hash_one_failure_one_success() {
-       // Topology : A --> B --> C
+       // Topology : A --> B --> C --> D
        // We route 2 payments with same hash between B and C, one will be timeout, the other successfully claim
-       let chanmon_cfgs = create_chanmon_cfgs(3);
-       let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
-       let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
-       let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+       // Note that because C will refuse to generate two payment secrets for the same payment hash,
+       // we forward one of the payments onwards to D.
+       let chanmon_cfgs = create_chanmon_cfgs(4);
+       let node_cfgs = create_node_cfgs(4, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]);
+       let mut nodes = create_network(4, &node_cfgs, &node_chanmgrs);
 
        create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
        let chan_2 = create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known());
+       create_announced_chan_between_nodes(&nodes, 2, 3, InitFeatures::known(), InitFeatures::known());
 
        let (our_payment_preimage, duplicate_payment_hash, _) = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 900000);
 
-       let (_, _, payment_secret) = get_payment_preimage_hash!(nodes[1]);
+       let payment_secret = nodes[3].node.create_inbound_payment_for_hash(duplicate_payment_hash, None, 7200).unwrap();
        let route = get_route(&nodes[0].node.get_our_node_id(), &nodes[0].net_graph_msg_handler.network_graph.read().unwrap(),
-               &nodes[2].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 900000, TEST_FINAL_CLTV, nodes[0].logger).unwrap();
-       send_along_route_with_secret(&nodes[0], route, &[&[&nodes[1], &nodes[2]]], 900000, duplicate_payment_hash, payment_secret);
+               &nodes[3].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 900000, TEST_FINAL_CLTV, nodes[0].logger).unwrap();
+       send_along_route_with_secret(&nodes[0], route, &[&[&nodes[1], &nodes[2], &nodes[3]]], 900000, duplicate_payment_hash, payment_secret);
 
        let commitment_txn = get_local_commitment_txn!(nodes[2], chan_2.2);
        assert_eq!(commitment_txn[0].input.len(), 1);
index 7c3e13cf1e51ef7dfc9f156bc19d2f07cbb2c171..1de45d6891a2a833cb2a7a32f15705c2cfb85752 100644 (file)
@@ -268,9 +268,8 @@ fn test_onion_failure() {
        }
        let channels = [create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()), create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known())];
        let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]);
-       let net_graph_msg_handler = &nodes[0].net_graph_msg_handler;
        let logger = test_utils::TestLogger::new();
-       let route = get_route(&nodes[0].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[2].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 40000, TEST_FINAL_CLTV, &logger).unwrap();
+       let route = get_route(&nodes[0].node.get_our_node_id(), &nodes[0].net_graph_msg_handler.network_graph.read().unwrap(), &nodes[2].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 40000, TEST_FINAL_CLTV, &logger).unwrap();
        // positve case
        send_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 40000, 40_000);
 
@@ -328,6 +327,7 @@ fn test_onion_failure() {
        }, ||{
                nodes[2].node.fail_htlc_backwards(&payment_hash, &None);
        }, true, Some(NODE|2), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.paths[0][1].pubkey, is_permanent: false}));
+       let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]);
 
        // intermediate node failure
        run_onion_failure_test_with_fail_intercept("permanent_node_failure", 100, &nodes, &route, &payment_hash, &payment_secret, |msg| {
@@ -346,6 +346,7 @@ fn test_onion_failure() {
        }, ||{
                nodes[2].node.fail_htlc_backwards(&payment_hash, &None);
        }, false, Some(PERM|NODE|2), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.paths[0][1].pubkey, is_permanent: true}));
+       let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]);
 
        // intermediate node failure
        run_onion_failure_test_with_fail_intercept("required_node_feature_missing", 100, &nodes, &route, &payment_hash, &payment_secret, |msg| {
@@ -366,6 +367,7 @@ fn test_onion_failure() {
        }, ||{
                nodes[2].node.fail_htlc_backwards(&payment_hash, &None);
        }, false, Some(PERM|NODE|3), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.paths[0][1].pubkey, is_permanent: true}));
+       let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]);
 
        run_onion_failure_test("invalid_onion_version", 0, &nodes, &route, &payment_hash, &payment_secret, |msg| { msg.onion_routing_packet.version = 1; }, ||{}, true,
                Some(BADONION|PERM|4), None);
@@ -439,6 +441,7 @@ fn test_onion_failure() {
        run_onion_failure_test("unknown_payment_hash", 2, &nodes, &route, &payment_hash, &payment_secret, |_| {}, || {
                nodes[2].node.fail_htlc_backwards(&payment_hash, &None);
        }, false, Some(PERM|15), None);
+       let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]);
 
        run_onion_failure_test("final_expiry_too_soon", 1, &nodes, &route, &payment_hash, &payment_secret, |msg| {
                let height = msg.cltv_expiry - CLTV_CLAIM_BUFFER - LATENCY_GRACE_PERIOD_BLOCKS + 1;