Merge pull request #253 from TheBlueMatt/2018-11-misc-tweaks

Misc Tweaks

diff --combined src/ln/channel.rs
index 207eda4183f7a8993c5ff0278df605637f59c4a0,e56fbd53228be08a73b86a596571ffde0435a703..63d8754d903e9c79412b8b993fd3645b3fb68cb7
--- 1/src/ln/channel.rs
--- 2/src/ln/channel.rs
+++ b/src/ln/channel.rs
@@@ -2,9 -2,11 +2,9 @@@ use bitcoin::blockdata::block::BlockHea
  use bitcoin::blockdata::script::{Script,Builder};
  use bitcoin::blockdata::transaction::{TxIn, TxOut, Transaction, SigHashType};
  use bitcoin::blockdata::opcodes;
 -use bitcoin::util::hash::{Sha256dHash, Hash160};
 +use bitcoin::util::hash::{BitcoinHash, Sha256dHash, Hash160};
  use bitcoin::util::bip143;
 -use bitcoin::network;
 -use bitcoin::network::serialize::{BitcoinHash, RawDecoder, RawEncoder};
 -use bitcoin::network::encodable::{ConsensusEncodable, ConsensusDecodable};
 +use bitcoin::consensus::encode::{self, Encodable, Decodable};
  
  use secp256k1::key::{PublicKey,SecretKey};
  use secp256k1::{Secp256k1,Message,Signature};
@@@ -1120,8 -1122,9 +1120,9 @@@ impl Channel 
                Ok(our_sig)
        }
  
-       /// May return an IgnoreError, but should not, and will always return Ok(_) when
-       /// debug_assertions are turned on
+       /// Per HTLC, only one get_update_fail_htlc or get_update_fulfill_htlc call may be made.
+       /// In such cases we debug_assert!(false) and return an IgnoreError. Thus, will always return
+       /// Ok(_) if debug assertions are turned on and preconditions are met.
        fn get_update_fulfill_htlc(&mut self, htlc_id_arg: u64, payment_preimage_arg: [u8; 32]) -> Result<(Option<msgs::UpdateFulfillHTLC>, Option<ChannelMonitor>), HandleError> {
                // Either ChannelFunded got set (which means it wont bet unset) or there is no way any
                // caller thought we could have something claimed (cause we wouldn't have accepted in an
@@@ -1173,7 -1176,9 +1174,9 @@@
                                        &HTLCUpdateAwaitingACK::FailHTLC { htlc_id, .. } => {
                                                if htlc_id_arg == htlc_id {
                                                        debug_assert!(false, "Tried to fulfill an HTLC we already had a holding-cell failure on");
-                                                       return Err(HandleError{err: "Unable to find a pending HTLC which matched the given HTLC ID", action: Some(msgs::ErrorAction::IgnoreError)});
+                                                       // Return the new channel monitor in a last-ditch effort to hit the
+                                                       // chain and claim the funds
+                                                       return Ok((None, Some(self.channel_monitor.clone())));
                                                }
                                        },
                                        _ => {}
@@@ -1213,8 -1218,9 +1216,9 @@@
                }
        }
  
-       /// May return an IgnoreError, but should not, and will always return Ok(_) when
-       /// debug_assertions are turned on
+       /// Per HTLC, only one get_update_fail_htlc or get_update_fulfill_htlc call may be made.
+       /// In such cases we debug_assert!(false) and return an IgnoreError. Thus, will always return
+       /// Ok(_) if debug assertions are turned on and preconditions are met.
        pub fn get_update_fail_htlc(&mut self, htlc_id_arg: u64, err_packet: msgs::OnionErrorPacket) -> Result<Option<msgs::UpdateFailHTLC>, HandleError> {
                if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) {
                        panic!("Was asked to fail an HTLC when channel was not in an operational state");
@@@ -2821,6 -2827,16 +2825,16 @@@
                                                self.channel_update_count += 1;
                                                return Err(HandleError{err: "funding tx had wrong script/value", action: Some(ErrorAction::DisconnectPeer{msg: None})});
                                        } else {
+                                               if self.channel_outbound {
+                                                       for input in tx.input.iter() {
+                                                               if input.witness.is_empty() {
+                                                                       // We generated a malleable funding transaction, implying we've
+                                                                       // just exposed ourselves to funds loss to our counterparty.
+                                                                       #[cfg(not(feature = "fuzztarget"))]
+                                                                       panic!("Client called ChannelManager::funding_transaction_generated with bogus transaction!");
+                                                               }
+                                                       }
+                                               }
                                                self.funding_tx_confirmations = 1;
                                                self.short_channel_id = Some(((height as u64)          << (5*8)) |
                                                                             ((*index_in_block as u64) << (2*8)) |
@@@ -3509,9 -3525,9 +3523,9 @@@ impl Writeable for Channel 
  
                (self.last_local_commitment_txn.len() as u64).write(writer)?;
                for tx in self.last_local_commitment_txn.iter() {
 -                      if let Err(e) = tx.consensus_encode(&mut RawEncoder::new(WriterWriteAdaptor(writer))) {
 +                      if let Err(e) = tx.consensus_encode(&mut WriterWriteAdaptor(writer)) {
                                match e {
 -                                      network::serialize::Error::Io(e) => return Err(e),
 +                                      encode::Error::Io(e) => return Err(e),
                                        _ => panic!("last_local_commitment_txn must have been well-formed!"),
                                }
                        }
@@@ -3688,7 -3704,7 +3702,7 @@@ impl<R : ::std::io::Read> ReadableArgs<
                let last_local_commitment_txn_count: u64 = Readable::read(reader)?;
                let mut last_local_commitment_txn = Vec::with_capacity(cmp::min(last_local_commitment_txn_count as usize, OUR_MAX_HTLCS as usize*2 + 1));
                for _ in 0..last_local_commitment_txn_count {
 -                      last_local_commitment_txn.push(match Transaction::consensus_decode(&mut RawDecoder::new(reader.by_ref())) {
 +                      last_local_commitment_txn.push(match Transaction::consensus_decode(reader.by_ref()) {
                                Ok(tx) => tx,
                                Err(_) => return Err(DecodeError::InvalidValue),
                        });
@@@ -3816,7 -3832,7 +3830,7 @@@
  mod tests {
        use bitcoin::util::hash::{Sha256dHash, Hash160};
        use bitcoin::util::bip143;
 -      use bitcoin::network::serialize::serialize;
 +      use bitcoin::consensus::encode::serialize;
        use bitcoin::blockdata::script::{Script, Builder};
        use bitcoin::blockdata::transaction::Transaction;
        use bitcoin::blockdata::opcodes;
@@@ -3938,7 -3954,7 +3952,7 @@@
  
                                chan.sign_commitment_transaction(&mut unsigned_tx.0, &their_signature);
  
 -                              assert_eq!(serialize(&unsigned_tx.0).unwrap()[..],
 +                              assert_eq!(serialize(&unsigned_tx.0)[..],
                                                hex::decode($tx_hex).unwrap()[..]);
                        };
                }
@@@ -3971,7 -3987,7 +3985,7 @@@
                                }
  
                                chan.sign_htlc_transaction(&mut htlc_tx, &remote_signature, &preimage, &htlc, &keys).unwrap();
 -                              assert_eq!(serialize(&htlc_tx).unwrap()[..],
 +                              assert_eq!(serialize(&htlc_tx)[..],
                                                hex::decode($tx_hex).unwrap()[..]);
                        };
                }

diff --combined src/ln/channelmanager.rs
index 1f765eb197f5f218582af1dfdf39ee43a3e1926e,e1975c08d6712ea5a1e358a581bc5f4a69bab6dc..dab22d286cc5581422f861e91c97f6b2a4b96e40
--- 1/src/ln/channelmanager.rs
--- 2/src/ln/channelmanager.rs
+++ b/src/ln/channelmanager.rs
@@@ -12,7 -12,8 +12,7 @@@ use bitcoin::blockdata::block::BlockHea
  use bitcoin::blockdata::transaction::Transaction;
  use bitcoin::blockdata::constants::genesis_block;
  use bitcoin::network::constants::Network;
 -use bitcoin::network::serialize::BitcoinHash;
 -use bitcoin::util::hash::Sha256dHash;
 +use bitcoin::util::hash::{BitcoinHash, Sha256dHash};
  
  use secp256k1::key::{SecretKey,PublicKey};
  use secp256k1::{Secp256k1,Message};
@@@ -3223,11 -3224,13 +3223,11 @@@ mod tests 
        use util::ser::{Writeable, Writer, ReadableArgs};
        use util::config::UserConfig;
  
 -      use bitcoin::util::hash::Sha256dHash;
 +      use bitcoin::util::hash::{BitcoinHash, Sha256dHash};
        use bitcoin::blockdata::block::{Block, BlockHeader};
        use bitcoin::blockdata::transaction::{Transaction, TxOut};
        use bitcoin::blockdata::constants::genesis_block;
        use bitcoin::network::constants::Network;
 -      use bitcoin::network::serialize::serialize;
 -      use bitcoin::network::serialize::BitcoinHash;
  
        use hex;
  
@@@ -3517,7 -3520,7 +3517,7 @@@
                                tx = Transaction { version: chan_id as u32, lock_time: 0, input: Vec::new(), output: vec![TxOut {
                                        value: *channel_value_satoshis, script_pubkey: output_script.clone(),
                                }]};
 -                              funding_output = OutPoint::new(Sha256dHash::from_data(&serialize(&tx).unwrap()[..]), 0);
 +                              funding_output = OutPoint::new(tx.txid(), 0);
  
                                node_a.node.funding_transaction_generated(&temporary_channel_id, funding_output);
                                let mut added_monitors = node_a.chan_monitor.added_monitors.lock().unwrap();
@@@ -6209,7 -6212,7 +6209,7 @@@
  
        /// pending_htlc_adds includes both the holding cell and in-flight update_add_htlcs, whereas
        /// for claims/fails they are separated out.
-       fn reconnect_nodes(node_a: &Node, node_b: &Node, pre_all_htlcs: bool, pending_htlc_adds: (i64, i64), pending_htlc_claims: (usize, usize), pending_cell_htlc_claims: (usize, usize), pending_cell_htlc_fails: (usize, usize), pending_raa: (bool, bool)) {
+       fn reconnect_nodes(node_a: &Node, node_b: &Node, send_funding_locked: (bool, bool), pending_htlc_adds: (i64, i64), pending_htlc_claims: (usize, usize), pending_cell_htlc_claims: (usize, usize), pending_cell_htlc_fails: (usize, usize), pending_raa: (bool, bool)) {
                node_a.node.peer_connected(&node_b.node.get_our_node_id());
                let reestablish_1 = get_chan_reestablish_msgs!(node_a, node_b);
                node_b.node.peer_connected(&node_a.node.get_our_node_id());
@@@ -6242,7 -6245,7 +6242,7 @@@
                        (pending_htlc_adds.1 == 0 && pending_htlc_claims.1 == 0 && pending_cell_htlc_claims.1 == 0 && pending_cell_htlc_fails.1 == 0));
  
                for chan_msgs in resp_1.drain(..) {
-                       if pre_all_htlcs {
+                       if send_funding_locked.0 {
                                node_a.node.handle_funding_locked(&node_b.node.get_our_node_id(), &chan_msgs.0.unwrap()).unwrap();
                                let announcement_event = node_a.node.get_and_clear_pending_msg_events();
                                if !announcement_event.is_empty() {
@@@ -6299,7 -6302,7 +6299,7 @@@
                }
  
                for chan_msgs in resp_2.drain(..) {
-                       if pre_all_htlcs {
+                       if send_funding_locked.1 {
                                node_b.node.handle_funding_locked(&node_a.node.get_our_node_id(), &chan_msgs.0.unwrap()).unwrap();
                                let announcement_event = node_b.node.get_and_clear_pending_msg_events();
                                if !announcement_event.is_empty() {
@@@ -6363,7 -6366,7 +6363,7 @@@
  
                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], true, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
+               reconnect_nodes(&nodes[0], &nodes[1], (true, true), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
  
                let payment_preimage_1 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 1000000).0;
                let payment_hash_2 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 1000000).1;
@@@ -6372,7 -6375,7 +6372,7 @@@
  
                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, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
+               reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
  
                let payment_preimage_3 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 1000000).0;
                let payment_preimage_4 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 1000000).0;
@@@ -6385,7 -6388,7 +6385,7 @@@
                claim_payment_along_route(&nodes[0], &vec!(&nodes[1], &nodes[2]), true, payment_preimage_3);
                fail_payment_along_route(&nodes[0], &[&nodes[1], &nodes[2]], true, payment_hash_5);
  
-               reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (1, 0), (1, 0), (false, false));
+               reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (1, 0), (1, 0), (false, false));
                {
                        let events = nodes[0].node.get_and_clear_pending_events();
                        assert_eq!(events.len(), 2);
@@@ -6466,19 -6469,19 +6466,19 @@@
                if messages_delivered < 3 {
                        // Even if the funding_locked messages get exchanged, as long as nothing further was
                        // received on either side, both sides will need to resend them.
-                       reconnect_nodes(&nodes[0], &nodes[1], true, (0, 1), (0, 0), (0, 0), (0, 0), (false, false));
+                       reconnect_nodes(&nodes[0], &nodes[1], (true, true), (0, 1), (0, 0), (0, 0), (0, 0), (false, false));
                } else if messages_delivered == 3 {
                        // nodes[0] still wants its RAA + commitment_signed
-                       reconnect_nodes(&nodes[0], &nodes[1], false, (-1, 0), (0, 0), (0, 0), (0, 0), (true, false));
+                       reconnect_nodes(&nodes[0], &nodes[1], (false, false), (-1, 0), (0, 0), (0, 0), (0, 0), (true, false));
                } else if messages_delivered == 4 {
                        // nodes[0] still wants its commitment_signed
-                       reconnect_nodes(&nodes[0], &nodes[1], false, (-1, 0), (0, 0), (0, 0), (0, 0), (false, false));
+                       reconnect_nodes(&nodes[0], &nodes[1], (false, false), (-1, 0), (0, 0), (0, 0), (0, 0), (false, false));
                } else if messages_delivered == 5 {
                        // nodes[1] still wants its final RAA
-                       reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (0, 0), (0, 0), (false, true));
+                       reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, true));
                } else if messages_delivered == 6 {
                        // Everything was delivered...
-                       reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
+                       reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
                }
  
                let events_1 = nodes[1].node.get_and_clear_pending_events();
@@@ -6490,7 -6493,7 +6490,7 @@@
  
                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, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
+               reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
  
                nodes[1].node.channel_state.lock().unwrap().next_forward = Instant::now();
                nodes[1].node.process_pending_htlc_forwards();
@@@ -6564,7 -6567,7 +6564,7 @@@
                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);
                if messages_delivered < 2 {
-                       reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (1, 0), (0, 0), (0, 0), (false, false));
+                       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 {
                                let events_4 = nodes[0].node.get_and_clear_pending_events();
@@@ -6578,21 -6581,21 +6578,21 @@@
                        //}
                } else if messages_delivered == 2 {
                        // nodes[0] still wants its RAA + commitment_signed
-                       reconnect_nodes(&nodes[0], &nodes[1], false, (0, -1), (0, 0), (0, 0), (0, 0), (false, true));
+                       reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, -1), (0, 0), (0, 0), (0, 0), (false, true));
                } else if messages_delivered == 3 {
                        // nodes[0] still wants its commitment_signed
-                       reconnect_nodes(&nodes[0], &nodes[1], false, (0, -1), (0, 0), (0, 0), (0, 0), (false, false));
+                       reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, -1), (0, 0), (0, 0), (0, 0), (false, false));
                } else if messages_delivered == 4 {
                        // nodes[1] still wants its final RAA
-                       reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (0, 0), (0, 0), (true, false));
+                       reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (true, false));
                } else if messages_delivered == 5 {
                        // Everything was delivered...
-                       reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
+                       reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
                }
  
                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, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
+               reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
  
                // Channel should still work fine...
                let payment_preimage_2 = send_along_route(&nodes[0], route, &[&nodes[1]], 1000000).0;
@@@ -6633,20 -6636,28 +6633,28 @@@
                        _ => panic!("Unexpected event"),
                }
  
+               reconnect_nodes(&nodes[0], &nodes[1], (false, true), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
+ 
+               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);
+ 
                confirm_transaction(&nodes[1].chain_monitor, &tx, tx.version);
                let events_2 = nodes[1].node.get_and_clear_pending_msg_events();
-               assert_eq!(events_2.len(), 1);
+               assert_eq!(events_2.len(), 2);
                match events_2[0] {
                        MessageSendEvent::SendFundingLocked { ref node_id, msg: _ } => {
                                assert_eq!(*node_id, nodes[0].node.get_our_node_id());
                        },
                        _ => panic!("Unexpected event"),
                }
+               match events_2[1] {
+                       MessageSendEvent::SendAnnouncementSignatures { ref node_id, msg: _ } => {
+                               assert_eq!(*node_id, nodes[0].node.get_our_node_id());
+                       },
+                       _ => panic!("Unexpected event"),
+               }
  
-               reconnect_nodes(&nodes[0], &nodes[1], true, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
-               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], true, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
+               reconnect_nodes(&nodes[0], &nodes[1], (true, true), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
  
                // TODO: We shouldn't need to manually pass list_usable_chanels here once we support
                // rebroadcasting announcement_signatures upon reconnect.
@@@ -6849,7 -6860,7 +6857,7 @@@
                if disconnect {
                        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], true, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
+                       reconnect_nodes(&nodes[0], &nodes[1], (true, true), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
                }
  
                *nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
@@@ -6890,7 -6901,7 +6898,7 @@@
                if disconnect {
                        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, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
+                       reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
                }
  
                // ...and make sure we can force-close a TemporaryFailure channel with a PermanentFailure
@@@ -7450,7 -7461,7 +7458,7 @@@
                nodes[0].node = Arc::new(nodes_0_deserialized);
                check_added_monitors!(nodes[0], 1);
  
-               reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
+               reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
  
                fail_payment(&nodes[0], &[&nodes[1]], our_payment_hash);
                claim_payment(&nodes[0], &[&nodes[1]], our_payment_preimage);
@@@ -7520,8 -7531,8 +7528,8 @@@
                nodes[0].node = Arc::new(nodes_0_deserialized);
  
                // nodes[1] and nodes[2] have no lost state with nodes[0]...
-               reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
-               reconnect_nodes(&nodes[0], &nodes[2], false, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
+               reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
+               reconnect_nodes(&nodes[0], &nodes[2], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
                //... and we can even still claim the payment!
                claim_payment(&nodes[2], &[&nodes[0], &nodes[1]], our_payment_preimage);