Use the `TxOutRef` type for the `ChannelMonitor`'s funding_txo field. #33

jeandudey · 2018-06-27T13:17:59Z

No description provided.

TheBlueMatt

I'm really not sure how I feel about this generally. While using a struct is definitely nicer, using the usize-index TxOutRef from rust-bitcoin makes for a bunch of platform-dependant conversions, and since lightning restricts us to u16-indexes we end up down-converting from internal datastructures to lightning-specific stuff in a few places that worry me. Specifically, if an attacker were to eg provide a channelmonitor to a watchtower with a value too large it could make the watchtower panic. Maybe use our own with u16 indexes instead?

TheBlueMatt · 2018-06-27T21:06:16Z

src/ln/channel.rs

@@ -1930,7 +1930,7 @@ impl Channel {
 						self.funding_tx_confirmations = 1;
 						self.short_channel_id = Some(((height as u64)          << (5*8)) |
 						                             ((*index_in_block as u64) << (2*8)) |
-						                             ((self.channel_monitor.get_funding_txo().unwrap().1 as u64) << (2*8)));
+						                             ((self.channel_monitor.get_funding_txo().unwrap().index as u64) << (2*8)));


Should just use txo_idx here now, then.

TheBlueMatt · 2018-06-27T21:07:10Z

src/ln/channelmanager.rs

-				funding_key:               SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]).unwrap(),
-				revocation_base_key:       SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]).unwrap(),
-				payment_base_key:          SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]).unwrap(),
+				funding_key:			   SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]).unwrap(),


You broken alignment here by converting spaces to tabs mid-line.

Sorry for this I'm going to fix it up, Vim is messing with the alignment.

TheBlueMatt · 2018-06-27T21:10:17Z

src/ln/channelmanager.rs

@@ -661,12 +661,12 @@ impl ChannelManager {

 	/// Call this upon creation of a funding transaction for the given channel.
 	/// Panics if a funding transaction has already been provided for this channel.
-	pub fn funding_transaction_generated(&self, temporary_channel_id: &Uint256, funding_txo: (Sha256dHash, u16)) {
+	pub fn funding_transaction_generated(&self, temporary_channel_id: &Uint256, funding_txo: TxOutRef) {


Ugh, I dont like this. Practically no one should ever create a funding transaction (or any transaction) with more than 2**16 outputs, but its not technically impossible, so letting the user provide a usize instead of a u16 feels icky cause we're going a down-convert without making the user aware of it.

TheBlueMatt · 2018-06-27T21:10:26Z

src/ln/channelmanager.rs

-				funding_key:               SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]).unwrap(),
-				revocation_base_key:       SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]).unwrap(),
-				payment_base_key:          SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]).unwrap(),
+				funding_key:			   SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]).unwrap(),


Broke alignment here, too.

TheBlueMatt · 2018-06-27T21:10:48Z

src/ln/channelmanager.rs

@@ -1578,7 +1578,7 @@ impl ChannelMessageHandler for ChannelManager {

 								let mut hmac = Hmac::new(Sha256::new(), &um);
 								hmac.input(&err_packet.encode()[32..]);
-								let mut calc_tag =  [0u8; 32];
+								let mut calc_tag =	[0u8; 32];


No idea what this is (you can remove the extra space, though, if you want).

TheBlueMatt · 2018-06-27T21:11:26Z

src/ln/channelmanager.rs

-				funding_output = (Sha256dHash::from_data(&serialize(&tx).unwrap()[..]), 0);
+				funding_output = TxOutRef {
+                    txid: Sha256dHash::from_data(&serialize(&tx).unwrap()[..]),
+                    index: 0


Bad alignment here.

TheBlueMatt · 2018-06-27T21:12:37Z

src/ln/channelmonitor.rs

@@ -370,7 +370,7 @@ impl ChannelMonitor {
 	/// avoid this (or call unset_funding_info) on a monitor you wish to send to a watchtower as it
 	/// provides slightly better privacy.
 	pub fn set_funding_info(&mut self, funding_txid: Sha256dHash, funding_output_index: u16) {
-		self.funding_txo = Some((funding_txid, funding_output_index));
+		self.funding_txo = Some(TxOutRef { txid: funding_txid, index: funding_output_index as usize });


Why not change the function's signature, too?

TheBlueMatt · 2018-06-27T21:14:04Z

src/ln/channelmonitor.rs

@@ -748,7 +748,7 @@ impl ChannelMonitor {
 	fn block_connected(&self, txn_matched: &[&Transaction], height: u32, broadcaster: &BroadcasterInterface) {
 		for tx in txn_matched {
 			for txin in tx.input.iter() {
-				if self.funding_txo.is_none() || (txin.prev_hash == self.funding_txo.unwrap().0 && txin.prev_index == self.funding_txo.unwrap().1 as u32) {
+				if self.funding_txo.is_none() || (txin.prev_hash == self.funding_txo.unwrap().txid && txin.prev_index == self.funding_txo.unwrap().index as u32) {


Please make a note somewhere (maybe just file an issue when this is merged?) that whoever writes the serializer/deserializer for ChannelMonitor must use u32s or u16s for indexes (though I'm kinda not a fan of either, given the usize is platform-dependant).

TheBlueMatt · 2018-06-28T15:58:07Z

Failed travis as fuzz/ compilation looks broken.

This structure replaces the (Sha256dHash, u16) tuple that was being used for the funding output. Signed-off-by: Jean Pierre Dudey <[email protected]>

lightningdevkit#33 review

Cleanups after #33, plus one unrelated bugfix spotted in #33 review

Fixes lightningdevkit#33.

jeandudey force-pushed the funding_txo branch 2 times, most recently from fece28a to b2934c0 Compare June 27, 2018 17:43

TheBlueMatt reviewed Jun 27, 2018

View reviewed changes

jeandudey force-pushed the funding_txo branch 2 times, most recently from 248a312 to e80932d Compare June 28, 2018 13:59

jeandudey force-pushed the funding_txo branch 3 times, most recently from 82b9bc7 to 940babe Compare June 29, 2018 02:50

Add the OutPoint type for the ChannelMonitor's funding_txo field.

d0e9137

This structure replaces the (Sha256dHash, u16) tuple that was being used for the funding output. Signed-off-by: Jean Pierre Dudey <[email protected]>

jeandudey force-pushed the funding_txo branch from 940babe to d0e9137 Compare June 29, 2018 16:12

TheBlueMatt merged commit b65aa86 into lightningdevkit:master Jun 29, 2018

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Jun 29, 2018

Cleanups after lightningdevkit#33, plus one unrelated bugfix spotted in

56842c2

lightningdevkit#33 review

TheBlueMatt added a commit that referenced this pull request Jun 29, 2018

Merge pull request #36 from TheBlueMatt/2018-08-33-cleanups

5918df8

Cleanups after #33, plus one unrelated bugfix spotted in #33 review

jeandudey deleted the funding_txo branch June 30, 2018 14:42

carlaKC pushed a commit to carlaKC/rust-lightning that referenced this pull request Aug 9, 2023

Correctly parse args without port but with net

e87a9e1

Fixes lightningdevkit#33.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use the `TxOutRef` type for the `ChannelMonitor`'s funding_txo field. #33

Use the `TxOutRef` type for the `ChannelMonitor`'s funding_txo field. #33

Uh oh!

jeandudey commented Jun 27, 2018

Uh oh!

TheBlueMatt left a comment •

edited

Loading

Uh oh!

TheBlueMatt Jun 27, 2018

Uh oh!

TheBlueMatt Jun 27, 2018

Uh oh!

jeandudey Jun 28, 2018

Uh oh!

TheBlueMatt Jun 27, 2018

Uh oh!

TheBlueMatt Jun 27, 2018

Uh oh!

TheBlueMatt Jun 27, 2018

Uh oh!

TheBlueMatt Jun 27, 2018

Uh oh!

TheBlueMatt Jun 27, 2018

Uh oh!

TheBlueMatt Jun 27, 2018

Uh oh!

TheBlueMatt commented Jun 28, 2018

Uh oh!

Uh oh!

Use the TxOutRef type for the ChannelMonitor's funding_txo field. #33

Use the TxOutRef type for the ChannelMonitor's funding_txo field. #33

Uh oh!

Conversation

jeandudey commented Jun 27, 2018

Uh oh!

TheBlueMatt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Jun 28, 2018

Uh oh!

Uh oh!

Use the `TxOutRef` type for the `ChannelMonitor`'s funding_txo field. #33

Use the `TxOutRef` type for the `ChannelMonitor`'s funding_txo field. #33

TheBlueMatt left a comment •

edited

Loading