From 5c370232eb48e5ddbd3c06cfc241dd726d4329a8 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 12 Jun 2020 21:01:32 -0400 Subject: [PATCH] Use usize for transaction-position-in-block values We use them largely as indexes into a Vec so there's little reason for them to be u32s. Instead, use them as usize everywhere. We also take this opportunity to add range checks before short_channel_id calculation, as we could otherwise end up with a bogus short_channel_id due to an output index out of range. --- fuzz/src/chanmon_consistency.rs | 2 +- fuzz/src/full_stack.rs | 2 +- fuzz/src/router.rs | 2 +- lightning/src/chain/chaininterface.rs | 12 ++++++------ lightning/src/ln/channel.rs | 6 +++++- lightning/src/ln/channelmanager.rs | 2 +- lightning/src/ln/channelmonitor.rs | 2 +- lightning/src/ln/functional_test_utils.rs | 2 +- lightning/src/ln/functional_tests.rs | 4 ++-- lightning/src/util/test_utils.rs | 2 +- 10 files changed, 20 insertions(+), 16 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 8be994bd..eba0cb9e 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -311,7 +311,7 @@ pub fn do_test(data: &[u8], out: Out) { let mut posn = Vec::with_capacity(channel_txn.len()); for i in 0..channel_txn.len() { txn.push(&channel_txn[i]); - posn.push(i as u32 + 1); + posn.push(i + 1); } $node.block_connected(&header, 1, &txn, &posn); for i in 2..100 { diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs index 61699136..18e31fc9 100644 --- a/fuzz/src/full_stack.rs +++ b/fuzz/src/full_stack.rs @@ -183,7 +183,7 @@ impl<'a> MoneyLossDetector<'a> { hash_map::Entry::Vacant(e) => { e.insert(self.height); txn.push(tx); - txn_idxs.push(idx as u32 + 1); + txn_idxs.push(idx + 1); }, _ => {}, } diff --git a/fuzz/src/router.rs b/fuzz/src/router.rs index c7abd152..3a6b5bdf 100644 --- a/fuzz/src/router.rs +++ b/fuzz/src/router.rs @@ -75,7 +75,7 @@ impl ChainWatchInterface for DummyChainWatcher { fn install_watch_tx(&self, _txid: &Txid, _script_pub_key: &Script) { } fn install_watch_outpoint(&self, _outpoint: (Txid, u32), _out_script: &Script) { } fn watch_all_txn(&self) { } - fn filter_block(&self, _block: &Block) -> Vec { + fn filter_block(&self, _block: &Block) -> Vec { Vec::new() } fn reentered(&self) -> usize { 0 } diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs index 7c215f36..2e874296 100644 --- a/lightning/src/chain/chaininterface.rs +++ b/lightning/src/chain/chaininterface.rs @@ -55,7 +55,7 @@ pub trait ChainWatchInterface: Sync + Send { /// Gets the list of transaction indices within a given block that the ChainWatchInterface is /// watching for. - fn filter_block(&self, block: &Block) -> Vec; + fn filter_block(&self, block: &Block) -> Vec; /// Returns a usize that changes when the ChainWatchInterface's watched data is modified. /// Users of `filter_block` should pre-save a copy of `reentered`'s return value and use it to @@ -86,7 +86,7 @@ pub trait ChainListener: Sync + Send { /// /// This also means those counting confirmations using block_connected callbacks should watch /// for duplicate headers and not count them towards confirmations! - fn block_connected(&self, header: &BlockHeader, height: u32, txn_matched: &[&Transaction], indexes_of_txn_matched: &[u32]); + fn block_connected(&self, header: &BlockHeader, height: u32, txn_matched: &[&Transaction], indexes_of_txn_matched: &[usize]); /// Notifies a listener that a block was disconnected. /// Unlike block_connected, this *must* never be called twice for the same disconnect event. /// Height must be the one of the block which was disconnected (not new height of the best chain) @@ -280,7 +280,7 @@ impl<'a, CL: Deref + 'a, C: Deref> BlockNotifier<'a let matched_indexes = self.chain_monitor.filter_block(block); let mut matched_txn = Vec::new(); for index in matched_indexes.iter() { - matched_txn.push(&block.txdata[*index as usize]); + matched_txn.push(&block.txdata[*index]); } reentered = self.block_connected_checked(&block.header, height, matched_txn.as_slice(), matched_indexes.as_slice()); } @@ -292,7 +292,7 @@ impl<'a, CL: Deref + 'a, C: Deref> BlockNotifier<'a /// Returns true if notified listeners registered additional watch data (implying that the /// block must be re-scanned and this function called again prior to further block_connected /// calls, see ChainListener::block_connected for more info). - pub fn block_connected_checked(&self, header: &BlockHeader, height: u32, txn_matched: &[&Transaction], indexes_of_txn_matched: &[u32]) -> bool { + pub fn block_connected_checked(&self, header: &BlockHeader, height: u32, txn_matched: &[&Transaction], indexes_of_txn_matched: &[usize]) -> bool { let last_seen = self.chain_monitor.reentered(); let listeners = self.listeners.lock().unwrap(); @@ -361,13 +361,13 @@ impl ChainWatchInterface for ChainWatchInterfaceUtil { Err(ChainError::NotSupported) } - fn filter_block(&self, block: &Block) -> Vec { + fn filter_block(&self, block: &Block) -> Vec { let mut matched_index = Vec::new(); { let watched = self.watched.lock().unwrap(); for (index, transaction) in block.txdata.iter().enumerate() { if self.does_match_tx_unguarded(transaction, &watched) { - matched_index.push(index as u32); + matched_index.push(index); } } } diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index b4a24833..706acfa2 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3299,7 +3299,7 @@ impl Channel { /// /// May return some HTLCs (and their payment_hash) which have timed out and should be failed /// back. - pub fn block_connected(&mut self, header: &BlockHeader, height: u32, txn_matched: &[&Transaction], indexes_of_txn_matched: &[u32]) -> Result<(Option, Vec<(HTLCSource, PaymentHash)>), msgs::ErrorMessage> { + pub fn block_connected(&mut self, header: &BlockHeader, height: u32, txn_matched: &[&Transaction], indexes_of_txn_matched: &[usize]) -> Result<(Option, Vec<(HTLCSource, PaymentHash)>), msgs::ErrorMessage> { let mut timed_out_htlcs = Vec::new(); self.holding_cell_htlc_updates.retain(|htlc_update| { match htlc_update { @@ -3350,6 +3350,10 @@ impl Channel { } } } + if height > 0xff_ff_ff || (*index_in_block) > 0xff_ff_ff { + panic!("Block was bogus - either height 16 million or had > 16 million transactions"); + } + assert!(txo_idx <= 0xffff); // txo_idx is a (u16 as usize), so this is just listed here for completeness self.funding_tx_confirmations = 1; self.short_channel_id = Some(((height as u64) << (5*8)) | ((*index_in_block as u64) << (2*8)) | diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index f453ffc0..a8a05298 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2963,7 +2963,7 @@ impl(notifier: &'a chaininterface::BlockNotifierRef<'b, &chaininterface::ChainWatchInterfaceUtil>, chain: &chaininterface::ChainWatchInterfaceUtil, tx: &Transaction, chan_id: u32) { assert!(chain.does_match_tx(tx)); let mut header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 }; - notifier.block_connected_checked(&header, 1, &[tx; 1], &[chan_id; 1]); + notifier.block_connected_checked(&header, 1, &[tx; 1], &[chan_id as usize; 1]); for i in 2..CHAN_CONFIRM_DEPTH { header = BlockHeader { version: 0x20000000, prev_blockhash: header.bitcoin_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 }; notifier.block_connected_checked(&header, i, &vec![], &[0; 0]); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 0a058152..c0851e1d 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -408,10 +408,10 @@ fn test_1_conf_open() { assert!(nodes[1].chain_monitor.does_match_tx(&tx)); let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 }; - nodes[1].block_notifier.block_connected_checked(&header, 1, &[&tx; 1], &[tx.version; 1]); + nodes[1].block_notifier.block_connected_checked(&header, 1, &[&tx; 1], &[tx.version as usize; 1]); nodes[0].node.handle_funding_locked(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingLocked, nodes[0].node.get_our_node_id())); - nodes[0].block_notifier.block_connected_checked(&header, 1, &[&tx; 1], &[tx.version; 1]); + nodes[0].block_notifier.block_connected_checked(&header, 1, &[&tx; 1], &[tx.version as usize; 1]); let (funding_locked, _) = create_chan_between_nodes_with_value_confirm_second(&nodes[1], &nodes[0]); let (announcement, as_update, bs_update) = create_chan_between_nodes_with_value_b(&nodes[0], &nodes[1], &funding_locked); diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 145465ef..2134c26f 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -371,7 +371,7 @@ impl ChainWatchInterface for TestChainWatcher { fn install_watch_tx(&self, _txid: &Txid, _script_pub_key: &Script) { } fn install_watch_outpoint(&self, _outpoint: (Txid, u32), _out_script: &Script) { } fn watch_all_txn(&self) { } - fn filter_block<'a>(&self, _block: &'a Block) -> Vec { + fn filter_block<'a>(&self, _block: &'a Block) -> Vec { Vec::new() } fn reentered(&self) -> usize { 0 } -- 2.30.2