Simplify tx checks in functional tests to make later commits simpler
[rust-lightning] / lightning / src / ln / functional_tests.rs
index 56c795d8edccee3dfe05cf16f0cd188d1e41b38f..f6150a53645744d8c680077d448e1fe9455a1e42 100644 (file)
@@ -18,8 +18,9 @@ use chain::channelmonitor;
 use chain::channelmonitor::{ChannelMonitor, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY};
 use chain::transaction::OutPoint;
 use chain::keysinterface::{KeysInterface, BaseSign};
+use ln::{PaymentPreimage, PaymentSecret, PaymentHash};
 use ln::channel::{COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC};
-use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentPreimage, PaymentHash, PaymentSendFailure, BREAKDOWN_TIMEOUT};
+use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentSendFailure, BREAKDOWN_TIMEOUT};
 use ln::channel::{Channel, ChannelError};
 use ln::{chan_utils, onion_utils};
 use routing::router::{Route, RouteHop, get_route};
@@ -28,7 +29,7 @@ use ln::msgs;
 use ln::msgs::{ChannelMessageHandler,RoutingMessageHandler,HTLCFailChannelUpdate, ErrorAction};
 use util::enforcing_trait_impls::EnforcingSigner;
 use util::{byte_utils, test_utils};
-use util::events::{Event, EventsProvider, MessageSendEvent, MessageSendEventsProvider};
+use util::events::{Event, MessageSendEvent, MessageSendEventsProvider};
 use util::errors::APIError;
 use util::ser::{Writeable, ReadableArgs};
 use util::config::UserConfig;
@@ -49,8 +50,10 @@ use bitcoin::secp256k1::key::{PublicKey,SecretKey};
 
 use regex;
 
-use std::collections::{BTreeSet, HashMap, HashSet};
-use std::default::Default;
+use prelude::*;
+use alloc::collections::BTreeSet;
+use std::collections::{HashMap, HashSet};
+use core::default::Default;
 use std::sync::Mutex;
 
 use ln::functional_test_utils::*;
@@ -1496,16 +1499,17 @@ fn test_duplicate_htlc_different_direction_onchain() {
 
        // Check we only broadcast 1 timeout tx
        let claim_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clone();
-       let htlc_pair = if claim_txn[0].output[0].value == 800_000 / 1000 { (claim_txn[0].clone(), claim_txn[1].clone()) } else { (claim_txn[1].clone(), claim_txn[0].clone()) };
        assert_eq!(claim_txn.len(), 5);
        check_spends!(claim_txn[2], chan_1.3);
        check_spends!(claim_txn[3], claim_txn[2]);
-       assert_eq!(htlc_pair.0.input.len(), 1);
-       assert_eq!(htlc_pair.0.input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); // HTLC 1 <--> 0, preimage tx
-       check_spends!(htlc_pair.0, remote_txn[0]);
-       assert_eq!(htlc_pair.1.input.len(), 1);
-       assert_eq!(htlc_pair.1.input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); // HTLC 0 <--> 1, timeout tx
-       check_spends!(htlc_pair.1, remote_txn[0]);
+       assert_eq!(claim_txn[1].input.len(), 1);
+       assert_eq!(claim_txn[1].input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); // HTLC 1 <--> 0, preimage tx
+       check_spends!(claim_txn[1], remote_txn[0]);
+       assert_eq!(remote_txn[0].output[claim_txn[1].input[0].previous_output.vout as usize].value, 800);
+       assert_eq!(claim_txn[0].input.len(), 1);
+       assert_eq!(claim_txn[0].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); // HTLC 0 <--> 1, timeout tx
+       check_spends!(claim_txn[0], remote_txn[0]);
+       assert_eq!(remote_txn[0].output[claim_txn[0].input[0].previous_output.vout as usize].value, 900);
 
        let events = nodes[0].node.get_and_clear_pending_msg_events();
        assert_eq!(events.len(), 3);
@@ -1763,7 +1767,7 @@ fn test_chan_reserve_dust_inbound_htlcs_outbound_chan() {
        // transaction fee with 0 HTLCs (183 sats)).
        create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 98817000, InitFeatures::known(), InitFeatures::known());
 
-       let dust_amt = 546000; // Dust amount
+       let dust_amt = 329000; // Dust amount
        // In the previous code, routing this dust payment would cause nodes[0] to perceive a channel
        // reserve violation even though it's a dust HTLC and therefore shouldn't count towards the
        // commitment transaction fee.
@@ -2070,16 +2074,18 @@ fn test_channel_reserve_holding_cell_htlcs() {
        let events = nodes[2].node.get_and_clear_pending_events();
        assert_eq!(events.len(), 2);
        match events[0] {
-               Event::PaymentReceived { ref payment_hash, ref payment_secret, amt, user_payment_id: _ } => {
+               Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt, user_payment_id: _ } => {
                        assert_eq!(our_payment_hash_21, *payment_hash);
+                       assert!(payment_preimage.is_none());
                        assert_eq!(our_payment_secret_21, *payment_secret);
                        assert_eq!(recv_value_21, amt);
                },
                _ => panic!("Unexpected event"),
        }
        match events[1] {
-               Event::PaymentReceived { ref payment_hash, ref payment_secret, amt, user_payment_id: _ } => {
+               Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt, user_payment_id: _ } => {
                        assert_eq!(our_payment_hash_22, *payment_hash);
+                       assert!(payment_preimage.is_none());
                        assert_eq!(our_payment_secret_22, *payment_secret);
                        assert_eq!(recv_value_22, amt);
                },
@@ -3493,6 +3499,34 @@ fn test_force_close_fail_back() {
        check_spends!(node_txn[0], tx);
 }
 
+#[test]
+fn test_dup_events_on_peer_disconnect() {
+       // Test that if we receive a duplicative update_fulfill_htlc message after a reconnect we do
+       // not generate a corresponding duplicative PaymentSent event. This did not use to be the case
+       // as we used to generate the event immediately upon receipt of the payment preimage in the
+       // update_fulfill_htlc message.
+
+       let chanmon_cfgs = create_chanmon_cfgs(2);
+       let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+       let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+       create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
+
+       let payment_preimage = route_payment(&nodes[0], &[&nodes[1]], 1000000).0;
+
+       assert!(nodes[1].node.claim_funds(payment_preimage));
+       check_added_monitors!(nodes[1], 1);
+       let claim_msgs = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
+       nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &claim_msgs.update_fulfill_htlcs[0]);
+       expect_payment_sent!(nodes[0], payment_preimage);
+
+       nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
+       nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
+
+       reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (1, 0), (0, 0), (0, 0), (false, false));
+       assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
+}
+
 #[test]
 fn test_simple_peer_disconnect() {
        // Test that we can reconnect when there are no lost messages
@@ -3646,8 +3680,9 @@ fn do_test_drop_messages_peer_disconnect(messages_delivered: u8) {
        let events_2 = nodes[1].node.get_and_clear_pending_events();
        assert_eq!(events_2.len(), 1);
        match events_2[0] {
-               Event::PaymentReceived { ref payment_hash, ref payment_secret, amt, user_payment_id: _ } => {
+               Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt, user_payment_id: _ } => {
                        assert_eq!(payment_hash_1, *payment_hash);
+                       assert!(payment_preimage.is_none());
                        assert_eq!(payment_secret_1, *payment_secret);
                        assert_eq!(amt, 1000000);
                },
@@ -3714,8 +3749,7 @@ fn do_test_drop_messages_peer_disconnect(messages_delivered: u8) {
        nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
        if messages_delivered < 2 {
                reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (1, 0), (0, 0), (0, 0), (false, false));
-               //TODO: Deduplicate PaymentSent events, then enable this if:
-               //if messages_delivered < 1 {
+               if messages_delivered < 1 {
                        let events_4 = nodes[0].node.get_and_clear_pending_events();
                        assert_eq!(events_4.len(), 1);
                        match events_4[0] {
@@ -3724,7 +3758,9 @@ fn do_test_drop_messages_peer_disconnect(messages_delivered: u8) {
                                },
                                _ => panic!("Unexpected event"),
                        }
-               //}
+               } else {
+                       assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
+               }
        } else if messages_delivered == 2 {
                // nodes[0] still wants its RAA + commitment_signed
                reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, -1), (0, 0), (0, 0), (0, 0), (false, true));
@@ -3773,7 +3809,10 @@ fn test_funding_peer_disconnect() {
        let chanmon_cfgs = create_chanmon_cfgs(2);
        let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
        let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
-       let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+       let persister: test_utils::TestPersister;
+       let new_chain_monitor: test_utils::TestChainMonitor;
+       let nodes_0_deserialized: ChannelManager<EnforcingSigner, &test_utils::TestChainMonitor, &test_utils::TestBroadcaster, &test_utils::TestKeysInterface, &test_utils::TestFeeEstimator, &test_utils::TestLogger>;
+       let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
        let tx = create_chan_between_nodes_with_value_init(&nodes[0], &nodes[1], 100000, 10001, InitFeatures::known(), InitFeatures::known());
 
        nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
@@ -3851,6 +3890,61 @@ fn test_funding_peer_disconnect() {
        let route = get_route(&nodes[0].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[1].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 1000000, TEST_FINAL_CLTV, &logger).unwrap();
        let (payment_preimage, _, _) = send_along_route(&nodes[0], route, &[&nodes[1]], 1000000);
        claim_payment(&nodes[0], &[&nodes[1]], payment_preimage);
+
+       // Check that after deserialization and reconnection we can still generate an identical
+       // channel_announcement from the cached signatures.
+       nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
+
+       let nodes_0_serialized = nodes[0].node.encode();
+       let mut chan_0_monitor_serialized = test_utils::TestVecWriter(Vec::new());
+       nodes[0].chain_monitor.chain_monitor.monitors.read().unwrap().iter().next().unwrap().1.write(&mut chan_0_monitor_serialized).unwrap();
+
+       persister = test_utils::TestPersister::new();
+       let keys_manager = &chanmon_cfgs[0].keys_manager;
+       new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), nodes[0].logger, node_cfgs[0].fee_estimator, &persister, keys_manager);
+       nodes[0].chain_monitor = &new_chain_monitor;
+       let mut chan_0_monitor_read = &chan_0_monitor_serialized.0[..];
+       let (_, mut chan_0_monitor) = <(BlockHash, ChannelMonitor<EnforcingSigner>)>::read(
+               &mut chan_0_monitor_read, keys_manager).unwrap();
+       assert!(chan_0_monitor_read.is_empty());
+
+       let mut nodes_0_read = &nodes_0_serialized[..];
+       let (_, nodes_0_deserialized_tmp) = {
+               let mut channel_monitors = HashMap::new();
+               channel_monitors.insert(chan_0_monitor.get_funding_txo().0, &mut chan_0_monitor);
+               <(BlockHash, ChannelManager<EnforcingSigner, &test_utils::TestChainMonitor, &test_utils::TestBroadcaster, &test_utils::TestKeysInterface, &test_utils::TestFeeEstimator, &test_utils::TestLogger>)>::read(&mut nodes_0_read, ChannelManagerReadArgs {
+                       default_config: UserConfig::default(),
+                       keys_manager,
+                       fee_estimator: node_cfgs[0].fee_estimator,
+                       chain_monitor: nodes[0].chain_monitor,
+                       tx_broadcaster: nodes[0].tx_broadcaster.clone(),
+                       logger: nodes[0].logger,
+                       channel_monitors,
+               }).unwrap()
+       };
+       nodes_0_deserialized = nodes_0_deserialized_tmp;
+       assert!(nodes_0_read.is_empty());
+
+       assert!(nodes[0].chain_monitor.watch_channel(chan_0_monitor.get_funding_txo().0, chan_0_monitor).is_ok());
+       nodes[0].node = &nodes_0_deserialized;
+       check_added_monitors!(nodes[0], 1);
+
+       reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
+
+       // as_announcement should be re-generated exactly by broadcast_node_announcement.
+       nodes[0].node.broadcast_node_announcement([0, 0, 0], [0; 32], Vec::new());
+       let msgs = nodes[0].node.get_and_clear_pending_msg_events();
+       let mut found_announcement = false;
+       for event in msgs.iter() {
+               match event {
+                       MessageSendEvent::BroadcastChannelAnnouncement { ref msg, .. } => {
+                               if *msg == as_announcement { found_announcement = true; }
+                       },
+                       MessageSendEvent::BroadcastNodeAnnouncement { .. } => {},
+                       _ => panic!("Unexpected event"),
+               }
+       }
+       assert!(found_announcement);
 }
 
 #[test]
@@ -3983,8 +4077,9 @@ fn test_drop_messages_peer_disconnect_dual_htlc() {
        let events_5 = nodes[1].node.get_and_clear_pending_events();
        assert_eq!(events_5.len(), 1);
        match events_5[0] {
-               Event::PaymentReceived { ref payment_hash, ref payment_secret, amt: _, user_payment_id: _ } => {
+               Event::PaymentReceived { ref payment_hash, ref payment_preimage, ref payment_secret, amt: _, user_payment_id: _ } => {
                        assert_eq!(payment_hash_2, *payment_hash);
+                       assert!(payment_preimage.is_none());
                        assert_eq!(payment_secret_2, *payment_secret);
                },
                _ => panic!("Unexpected event"),
@@ -4033,7 +4128,8 @@ fn do_test_htlc_timeout(send_partial_mpp: bool) {
        };
        connect_block(&nodes[0], &block);
        connect_block(&nodes[1], &block);
-       for _ in CHAN_CONFIRM_DEPTH + 2 ..TEST_FINAL_CLTV + CHAN_CONFIRM_DEPTH + 2 - CLTV_CLAIM_BUFFER - LATENCY_GRACE_PERIOD_BLOCKS {
+       let block_count = TEST_FINAL_CLTV + CHAN_CONFIRM_DEPTH + 2 - CLTV_CLAIM_BUFFER - LATENCY_GRACE_PERIOD_BLOCKS;
+       for _ in CHAN_CONFIRM_DEPTH + 2..block_count {
                block.header.prev_blockhash = block.block_hash();
                connect_block(&nodes[0], &block);
                connect_block(&nodes[1], &block);
@@ -4050,9 +4146,9 @@ fn do_test_htlc_timeout(send_partial_mpp: bool) {
 
        nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &htlc_timeout_updates.update_fail_htlcs[0]);
        commitment_signed_dance!(nodes[0], nodes[1], htlc_timeout_updates.commitment_signed, false);
-       // 100_000 msat as u64, followed by a height of TEST_FINAL_CLTV + 2 as u32
+       // 100_000 msat as u64, followed by the height at which we failed back above
        let mut expected_failure_data = byte_utils::be64_to_array(100_000).to_vec();
-       expected_failure_data.extend_from_slice(&byte_utils::be32_to_array(TEST_FINAL_CLTV + 2));
+       expected_failure_data.extend_from_slice(&byte_utils::be32_to_array(block_count - 1));
        expect_payment_failed!(nodes[0], our_payment_hash, true, 0x4000 | 15, &expected_failure_data[..]);
 }
 
@@ -4296,6 +4392,108 @@ fn test_no_txn_manager_serialize_deserialize() {
        send_payment(&nodes[0], &[&nodes[1]], 1000000);
 }
 
+#[test]
+fn test_dup_htlc_onchain_fails_on_reload() {
+       // When a Channel is closed, any outbound HTLCs which were relayed through it are simply
+       // dropped when the Channel is. From there, the ChannelManager relies on the ChannelMonitor
+       // having a copy of the relevant fail-/claim-back data and processes the HTLC fail/claim when
+       // the ChannelMonitor tells it to.
+       //
+       // If, due to an on-chain event, an HTLC is failed/claimed, and then we serialize the
+       // ChannelManager, we generally expect there not to be a duplicate HTLC fail/claim (eg via a
+       // PaymentFailed event appearing). However, because we may not serialize the relevant
+       // ChannelMonitor at the same time, this isn't strictly guaranteed. In order to provide this
+       // consistency, the ChannelManager explicitly tracks pending-onchain-resolution outbound HTLCs
+       // and de-duplicates ChannelMonitor events.
+       //
+       // This tests that explicit tracking behavior.
+       let chanmon_cfgs = create_chanmon_cfgs(2);
+       let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+       let persister: test_utils::TestPersister;
+       let new_chain_monitor: test_utils::TestChainMonitor;
+       let nodes_0_deserialized: ChannelManager<EnforcingSigner, &test_utils::TestChainMonitor, &test_utils::TestBroadcaster, &test_utils::TestKeysInterface, &test_utils::TestFeeEstimator, &test_utils::TestLogger>;
+       let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+       create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
+
+       // Route a payment, but force-close the channel before the HTLC fulfill message arrives at
+       // nodes[0].
+       let (payment_preimage, _, _) = route_payment(&nodes[0], &[&nodes[1]], 10000000);
+       nodes[0].node.force_close_channel(&nodes[0].node.list_channels()[0].channel_id).unwrap();
+       check_closed_broadcast!(nodes[0], true);
+       check_added_monitors!(nodes[0], 1);
+
+       nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
+       nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
+
+       let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
+       assert_eq!(node_txn.len(), 2);
+
+       assert!(nodes[1].node.claim_funds(payment_preimage));
+       check_added_monitors!(nodes[1], 1);
+
+       let mut header = BlockHeader { version: 0x20000000, prev_blockhash: nodes[1].best_block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
+       connect_block(&nodes[1], &Block { header, txdata: vec![node_txn[0].clone(), node_txn[1].clone()]});
+       check_closed_broadcast!(nodes[1], true);
+       check_added_monitors!(nodes[1], 1);
+       let claim_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
+
+       connect_block(&nodes[0], &Block { header, txdata: node_txn});
+
+       // Serialize out the ChannelMonitor before connecting the on-chain claim transactions. This is
+       // fairly normal behavior as ChannelMonitor(s) are often not re-serialized when on-chain events
+       // happen, unlike ChannelManager which tends to be re-serialized after any relevant event(s).
+       let mut chan_0_monitor_serialized = test_utils::TestVecWriter(Vec::new());
+       nodes[0].chain_monitor.chain_monitor.monitors.read().unwrap().iter().next().unwrap().1.write(&mut chan_0_monitor_serialized).unwrap();
+
+       header.prev_blockhash = header.block_hash();
+       let claim_block = Block { header, txdata: claim_txn};
+       connect_block(&nodes[0], &claim_block);
+       expect_payment_sent!(nodes[0], payment_preimage);
+
+       // ChannelManagers generally get re-serialized after any relevant event(s). Since we just
+       // connected a highly-relevant block, it likely gets serialized out now.
+       let mut chan_manager_serialized = test_utils::TestVecWriter(Vec::new());
+       nodes[0].node.write(&mut chan_manager_serialized).unwrap();
+
+       // Now reload nodes[0]...
+       persister = test_utils::TestPersister::new();
+       let keys_manager = &chanmon_cfgs[0].keys_manager;
+       new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), nodes[0].logger, node_cfgs[0].fee_estimator, &persister, keys_manager);
+       nodes[0].chain_monitor = &new_chain_monitor;
+       let mut chan_0_monitor_read = &chan_0_monitor_serialized.0[..];
+       let (_, mut chan_0_monitor) = <(BlockHash, ChannelMonitor<EnforcingSigner>)>::read(
+               &mut chan_0_monitor_read, keys_manager).unwrap();
+       assert!(chan_0_monitor_read.is_empty());
+
+       let (_, nodes_0_deserialized_tmp) = {
+               let mut channel_monitors = HashMap::new();
+               channel_monitors.insert(chan_0_monitor.get_funding_txo().0, &mut chan_0_monitor);
+               <(BlockHash, ChannelManager<EnforcingSigner, &test_utils::TestChainMonitor, &test_utils::TestBroadcaster, &test_utils::TestKeysInterface, &test_utils::TestFeeEstimator, &test_utils::TestLogger>)>
+                       ::read(&mut std::io::Cursor::new(&chan_manager_serialized.0[..]), ChannelManagerReadArgs {
+                               default_config: Default::default(),
+                               keys_manager,
+                               fee_estimator: node_cfgs[0].fee_estimator,
+                               chain_monitor: nodes[0].chain_monitor,
+                               tx_broadcaster: nodes[0].tx_broadcaster.clone(),
+                               logger: nodes[0].logger,
+                               channel_monitors,
+                       }).unwrap()
+       };
+       nodes_0_deserialized = nodes_0_deserialized_tmp;
+
+       assert!(nodes[0].chain_monitor.watch_channel(chan_0_monitor.get_funding_txo().0, chan_0_monitor).is_ok());
+       check_added_monitors!(nodes[0], 1);
+       nodes[0].node = &nodes_0_deserialized;
+
+       // Note that if we re-connect the block which exposed nodes[0] to the payment preimage (but
+       // which the current ChannelMonitor has not seen), the ChannelManager's de-duplication of
+       // payment events should kick in, leaving us with no pending events here.
+       nodes[0].chain_monitor.chain_monitor.block_connected(&claim_block, nodes[0].blocks.borrow().len() as u32 - 1);
+       assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
+}
+
 #[test]
 fn test_manager_serialize_deserialize_events() {
        // This test makes sure the events field in ChannelManager survives de/serialization
@@ -5111,11 +5309,20 @@ fn test_duplicate_payment_hash_one_failure_one_success() {
        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 node_max_height = std::cmp::max(std::cmp::max(nodes[0].blocks.borrow().len(), nodes[1].blocks.borrow().len()), std::cmp::max(nodes[2].blocks.borrow().len(), nodes[3].blocks.borrow().len())) as u32;
+       connect_blocks(&nodes[0], node_max_height - nodes[0].best_block_info().1);
+       connect_blocks(&nodes[1], node_max_height - nodes[1].best_block_info().1);
+       connect_blocks(&nodes[2], node_max_height - nodes[2].best_block_info().1);
+       connect_blocks(&nodes[3], node_max_height - nodes[3].best_block_info().1);
+
        let (our_payment_preimage, duplicate_payment_hash, _) = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 900000);
 
        let payment_secret = nodes[3].node.create_inbound_payment_for_hash(duplicate_payment_hash, None, 7200, 0).unwrap();
+       // We reduce the final CLTV here by a somewhat arbitrary constant to keep it under the one-byte
+       // script push size limit so that the below script length checks match
+       // ACCEPTED_HTLC_SCRIPT_WEIGHT.
        let route = get_route(&nodes[0].node.get_our_node_id(), &nodes[0].net_graph_msg_handler.network_graph.read().unwrap(),
-               &nodes[3].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 900000, TEST_FINAL_CLTV, nodes[0].logger).unwrap();
+               &nodes[3].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 900000, TEST_FINAL_CLTV - 40, 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);
@@ -5158,18 +5365,17 @@ fn test_duplicate_payment_hash_one_failure_one_success() {
        }
        let htlc_success_txn: Vec<_> = nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap().clone();
        assert_eq!(htlc_success_txn.len(), 5); // ChannelMonitor: HTLC-Success txn (*2 due to 2-HTLC outputs), ChannelManager: local commitment tx + HTLC-Success txn (*2 due to 2-HTLC outputs)
-       check_spends!(htlc_success_txn[2], chan_2.3);
-       check_spends!(htlc_success_txn[3], htlc_success_txn[2]);
-       check_spends!(htlc_success_txn[4], htlc_success_txn[2]);
-       assert_eq!(htlc_success_txn[0], htlc_success_txn[3]);
+       check_spends!(htlc_success_txn[0], commitment_txn[0]);
+       check_spends!(htlc_success_txn[1], commitment_txn[0]);
        assert_eq!(htlc_success_txn[0].input.len(), 1);
        assert_eq!(htlc_success_txn[0].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
-       assert_eq!(htlc_success_txn[1], htlc_success_txn[4]);
        assert_eq!(htlc_success_txn[1].input.len(), 1);
        assert_eq!(htlc_success_txn[1].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
        assert_ne!(htlc_success_txn[0].input[0], htlc_success_txn[1].input[0]);
-       check_spends!(htlc_success_txn[0], commitment_txn[0]);
-       check_spends!(htlc_success_txn[1], commitment_txn[0]);
+       assert_eq!(htlc_success_txn[2], commitment_txn[0]);
+       assert_eq!(htlc_success_txn[3], htlc_success_txn[0]);
+       assert_eq!(htlc_success_txn[4], htlc_success_txn[1]);
+
 
        mine_transaction(&nodes[1], &htlc_timeout_tx);
        connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
@@ -5177,7 +5383,7 @@ fn test_duplicate_payment_hash_one_failure_one_success() {
        let htlc_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
        assert!(htlc_updates.update_add_htlcs.is_empty());
        assert_eq!(htlc_updates.update_fail_htlcs.len(), 1);
-       assert_eq!(htlc_updates.update_fail_htlcs[0].htlc_id, 1);
+       let first_htlc_id = htlc_updates.update_fail_htlcs[0].htlc_id;
        assert!(htlc_updates.update_fulfill_htlcs.is_empty());
        assert!(htlc_updates.update_fail_malformed_htlcs.is_empty());
        check_added_monitors!(nodes[1], 1);
@@ -5202,7 +5408,7 @@ fn test_duplicate_payment_hash_one_failure_one_success() {
        assert!(updates.update_add_htlcs.is_empty());
        assert!(updates.update_fail_htlcs.is_empty());
        assert_eq!(updates.update_fulfill_htlcs.len(), 1);
-       assert_eq!(updates.update_fulfill_htlcs[0].htlc_id, 0);
+       assert_ne!(updates.update_fulfill_htlcs[0].htlc_id, first_htlc_id);
        assert!(updates.update_fail_malformed_htlcs.is_empty());
        check_added_monitors!(nodes[1], 1);
 
@@ -5883,6 +6089,31 @@ fn bolt2_open_channel_sending_node_checks_part2() {
        assert!(PublicKey::from_slice(&node0_to_1_send_open_channel.delayed_payment_basepoint.serialize()).is_ok());
 }
 
+#[test]
+fn bolt2_open_channel_sane_dust_limit() {
+       let chanmon_cfgs = create_chanmon_cfgs(2);
+       let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+       let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+       let channel_value_satoshis=1000000;
+       let push_msat=10001;
+       nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), channel_value_satoshis, push_msat, 42, None).unwrap();
+       let mut node0_to_1_send_open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
+       node0_to_1_send_open_channel.dust_limit_satoshis = 661;
+       node0_to_1_send_open_channel.channel_reserve_satoshis = 100001;
+
+       nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &node0_to_1_send_open_channel);
+       let events = nodes[1].node.get_and_clear_pending_msg_events();
+       let err_msg = match events[0] {
+               MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { ref msg }, node_id: _ } => {
+                       msg.clone()
+               },
+               _ => panic!("Unexpected event"),
+       };
+       assert_eq!(err_msg.data, "dust_limit_satoshis (661) is greater than the implementation limit (660)");
+}
+
 // Test that if we fail to send an HTLC that is being freed from the holding cell, and the HTLC
 // originated from our node, its failure is surfaced to the user. We trigger this failure to
 // free the HTLC by increasing our fee while the HTLC is in the holding cell such that the HTLC
@@ -7551,16 +7782,17 @@ fn test_announce_disable_channels() {
        nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
        nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
 
-       nodes[0].node.timer_tick_occurred(); // dirty -> stagged
-       nodes[0].node.timer_tick_occurred(); // staged -> fresh
+       nodes[0].node.timer_tick_occurred(); // Enabled -> DisabledStaged
+       nodes[0].node.timer_tick_occurred(); // DisabledStaged -> Disabled
        let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
        assert_eq!(msg_events.len(), 3);
+       let mut chans_disabled: HashSet<u64> = [short_id_1, short_id_2, short_id_3].iter().map(|a| *a).collect();
        for e in msg_events {
                match e {
                        MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
-                               let short_id = msg.contents.short_channel_id;
-                               // Check generated channel_update match list in PendingChannelUpdate
-                               if short_id != short_id_1 && short_id != short_id_2 && short_id != short_id_3 {
+                               assert_eq!(msg.contents.flags & (1<<1), 1<<1); // The "channel disabled" bit should be set
+                               // Check that each channel gets updated exactly once
+                               if !chans_disabled.remove(&msg.contents.short_channel_id) {
                                        panic!("Generated ChannelUpdate for wrong chan!");
                                }
                        },
@@ -7593,6 +7825,22 @@ fn test_announce_disable_channels() {
 
        nodes[0].node.timer_tick_occurred();
        assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
+       nodes[0].node.timer_tick_occurred();
+       let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
+       assert_eq!(msg_events.len(), 3);
+       chans_disabled = [short_id_1, short_id_2, short_id_3].iter().map(|a| *a).collect();
+       for e in msg_events {
+               match e {
+                       MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
+                               assert_eq!(msg.contents.flags & (1<<1), 0); // The "channel disabled" bit should be off
+                               // Check that each channel gets updated exactly once
+                               if !chans_disabled.remove(&msg.contents.short_channel_id) {
+                                       panic!("Generated ChannelUpdate for wrong chan!");
+                               }
+                       },
+                       _ => panic!("Unexpected event"),
+               }
+       }
 }
 
 #[test]
@@ -7713,9 +7961,13 @@ fn test_bump_penalty_txn_on_revoked_htlcs() {
        let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 
        let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1000000, 59000000, InitFeatures::known(), InitFeatures::known());
-       // Lock HTLC in both directions
-       let payment_preimage = route_payment(&nodes[0], &vec!(&nodes[1])[..], 3_000_000).0;
-       route_payment(&nodes[1], &vec!(&nodes[0])[..], 3_000_000).0;
+       // Lock HTLC in both directions (using a slightly lower CLTV delay to provide timely RBF bumps)
+       let route = get_route(&nodes[0].node.get_our_node_id(), &nodes[0].net_graph_msg_handler.network_graph.read().unwrap(),
+               &nodes[1].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 3_000_000, 50, nodes[0].logger).unwrap();
+       let payment_preimage = send_along_route(&nodes[0], route, &[&nodes[1]], 3_000_000).0;
+       let route = get_route(&nodes[1].node.get_our_node_id(), &nodes[1].net_graph_msg_handler.network_graph.read().unwrap(),
+               &nodes[0].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 3_000_000, 50, nodes[0].logger).unwrap();
+       send_along_route(&nodes[1], route, &[&nodes[0]], 3_000_000);
 
        let revoked_local_txn = get_local_commitment_txn!(nodes[1], chan.2);
        assert_eq!(revoked_local_txn[0].input.len(), 1);
@@ -8028,7 +8280,7 @@ fn test_bump_txn_sanitize_tracking_maps() {
        claim_payment(&nodes[0], &vec!(&nodes[1])[..], payment_preimage);
 
        // Broadcast set of revoked txn on A
-       connect_blocks(&nodes[0], 52 - CHAN_CONFIRM_DEPTH);
+       connect_blocks(&nodes[0], TEST_FINAL_CLTV + 2 - CHAN_CONFIRM_DEPTH);
        expect_pending_htlcs_forwardable_ignore!(nodes[0]);
        assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 0);
 
@@ -8123,6 +8375,174 @@ fn test_simple_mpp() {
        claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, payment_preimage);
 }
 
+#[test]
+fn test_preimage_storage() {
+       // Simple test of payment preimage storage allowing no client-side storage to claim payments
+       let chanmon_cfgs = create_chanmon_cfgs(2);
+       let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+       let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+       create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id;
+
+       {
+               let (payment_hash, payment_secret) = nodes[1].node.create_inbound_payment(Some(100_000), 7200, 42);
+
+               let logger = test_utils::TestLogger::new();
+               let net_graph_msg_handler = &nodes[0].net_graph_msg_handler;
+               let route = get_route(&nodes[0].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[1].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &[], 100_000, TEST_FINAL_CLTV, &logger).unwrap();
+               nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap();
+               check_added_monitors!(nodes[0], 1);
+               let mut events = nodes[0].node.get_and_clear_pending_msg_events();
+               let mut payment_event = SendEvent::from_event(events.pop().unwrap());
+               nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
+               commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false);
+       }
+       // Note that after leaving the above scope we have no knowledge of any arguments or return
+       // values from previous calls.
+       expect_pending_htlcs_forwardable!(nodes[1]);
+       let events = nodes[1].node.get_and_clear_pending_events();
+       assert_eq!(events.len(), 1);
+       match events[0] {
+               Event::PaymentReceived { payment_preimage, user_payment_id, .. } => {
+                       assert_eq!(user_payment_id, 42);
+                       claim_payment(&nodes[0], &[&nodes[1]], payment_preimage.unwrap());
+               },
+               _ => panic!("Unexpected event"),
+       }
+}
+
+#[test]
+fn test_secret_timeout() {
+       // Simple test of payment secret storage time outs
+       let chanmon_cfgs = create_chanmon_cfgs(2);
+       let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+       let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+       create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id;
+
+       let (payment_hash, payment_secret_1) = nodes[1].node.create_inbound_payment(Some(100_000), 2, 0);
+
+       // We should fail to register the same payment hash twice, at least until we've connected a
+       // block with time 7200 + CHAN_CONFIRM_DEPTH + 1.
+       if let Err(APIError::APIMisuseError { err }) = nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(100_000), 2, 0) {
+               assert_eq!(err, "Duplicate payment hash");
+       } else { panic!(); }
+       let mut block = Block {
+               header: BlockHeader {
+                       version: 0x2000000,
+                       prev_blockhash: nodes[1].blocks.borrow().last().unwrap().0.block_hash(),
+                       merkle_root: Default::default(),
+                       time: nodes[1].blocks.borrow().len() as u32 + 7200, bits: 42, nonce: 42 },
+               txdata: vec![],
+       };
+       connect_block(&nodes[1], &block);
+       if let Err(APIError::APIMisuseError { err }) = nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(100_000), 2, 0) {
+               assert_eq!(err, "Duplicate payment hash");
+       } else { panic!(); }
+
+       // If we then connect the second block, we should be able to register the same payment hash
+       // again with a different user_payment_id (this time getting a new payment secret).
+       block.header.prev_blockhash = block.header.block_hash();
+       block.header.time += 1;
+       connect_block(&nodes[1], &block);
+       let our_payment_secret = nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(100_000), 2, 42).unwrap();
+       assert_ne!(payment_secret_1, our_payment_secret);
+
+       {
+               let logger = test_utils::TestLogger::new();
+               let net_graph_msg_handler = &nodes[0].net_graph_msg_handler;
+               let route = get_route(&nodes[0].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[1].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &[], 100_000, TEST_FINAL_CLTV, &logger).unwrap();
+               nodes[0].node.send_payment(&route, payment_hash, &Some(our_payment_secret)).unwrap();
+               check_added_monitors!(nodes[0], 1);
+               let mut events = nodes[0].node.get_and_clear_pending_msg_events();
+               let mut payment_event = SendEvent::from_event(events.pop().unwrap());
+               nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
+               commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false);
+       }
+       // Note that after leaving the above scope we have no knowledge of any arguments or return
+       // values from previous calls.
+       expect_pending_htlcs_forwardable!(nodes[1]);
+       let events = nodes[1].node.get_and_clear_pending_events();
+       assert_eq!(events.len(), 1);
+       match events[0] {
+               Event::PaymentReceived { payment_preimage, payment_secret, user_payment_id, .. } => {
+                       assert!(payment_preimage.is_none());
+                       assert_eq!(user_payment_id, 42);
+                       assert_eq!(payment_secret, our_payment_secret);
+                       // We don't actually have the payment preimage with which to claim this payment!
+               },
+               _ => panic!("Unexpected event"),
+       }
+}
+
+#[test]
+fn test_bad_secret_hash() {
+       // Simple test of unregistered payment hash/invalid payment secret handling
+       let chanmon_cfgs = create_chanmon_cfgs(2);
+       let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+       let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+       create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id;
+
+       let random_payment_hash = PaymentHash([42; 32]);
+       let random_payment_secret = PaymentSecret([43; 32]);
+       let (our_payment_hash, our_payment_secret) = nodes[1].node.create_inbound_payment(Some(100_000), 2, 0);
+
+       let logger = test_utils::TestLogger::new();
+       let net_graph_msg_handler = &nodes[0].net_graph_msg_handler;
+       let route = get_route(&nodes[0].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[1].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &[], 100_000, TEST_FINAL_CLTV, &logger).unwrap();
+
+       // All the below cases should end up being handled exactly identically, so we macro the
+       // resulting events.
+       macro_rules! handle_unknown_invalid_payment_data {
+               () => {
+                       check_added_monitors!(nodes[0], 1);
+                       let mut events = nodes[0].node.get_and_clear_pending_msg_events();
+                       let payment_event = SendEvent::from_event(events.pop().unwrap());
+                       nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
+                       commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false);
+
+                       // We have to forward pending HTLCs once to process the receipt of the HTLC and then
+                       // again to process the pending backwards-failure of the HTLC
+                       expect_pending_htlcs_forwardable!(nodes[1]);
+                       expect_pending_htlcs_forwardable!(nodes[1]);
+                       check_added_monitors!(nodes[1], 1);
+
+                       // We should fail the payment back
+                       let mut events = nodes[1].node.get_and_clear_pending_msg_events();
+                       match events.pop().unwrap() {
+                               MessageSendEvent::UpdateHTLCs { node_id: _, updates: msgs::CommitmentUpdate { update_fail_htlcs, commitment_signed, .. } } => {
+                                       nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &update_fail_htlcs[0]);
+                                       commitment_signed_dance!(nodes[0], nodes[1], commitment_signed, false);
+                               },
+                               _ => panic!("Unexpected event"),
+                       }
+               }
+       }
+
+       let expected_error_code = 0x4000|15; // incorrect_or_unknown_payment_details
+       // Error data is the HTLC value (100,000) and current block height
+       let expected_error_data = [0, 0, 0, 0, 0, 1, 0x86, 0xa0, 0, 0, 0, CHAN_CONFIRM_DEPTH as u8];
+
+       // Send a payment with the right payment hash but the wrong payment secret
+       nodes[0].node.send_payment(&route, our_payment_hash, &Some(random_payment_secret)).unwrap();
+       handle_unknown_invalid_payment_data!();
+       expect_payment_failed!(nodes[0], our_payment_hash, true, expected_error_code, expected_error_data);
+
+       // Send a payment with a random payment hash, but the right payment secret
+       nodes[0].node.send_payment(&route, random_payment_hash, &Some(our_payment_secret)).unwrap();
+       handle_unknown_invalid_payment_data!();
+       expect_payment_failed!(nodes[0], random_payment_hash, true, expected_error_code, expected_error_data);
+
+       // Send a payment with a random payment hash and random payment secret
+       nodes[0].node.send_payment(&route, random_payment_hash, &Some(random_payment_secret)).unwrap();
+       handle_unknown_invalid_payment_data!();
+       expect_payment_failed!(nodes[0], random_payment_hash, true, expected_error_code, expected_error_data);
+}
+
 #[test]
 fn test_update_err_monitor_lockdown() {
        // Our monitor will lock update of local commitment transaction if a broadcastion condition