Check channel_announcement is on Bitcoin Mainnet or Testnet + test_invalid_channel_announcement #134

ariard · 2018-08-29T00:20:23Z

Also remove_channel_info stuff

ariard · 2018-08-29T00:24:07Z

src/ln/router.rs

@@ -624,6 +624,17 @@ impl Router {

 		Err(HandleError{err: "Failed to find a path to the given destination", action: None})
 	}
+
+	/// Drop entry from network map
+	pub fn remove_channel_info(&self, short_channel_id: u64, chain_hash: Sha256dHash) -> Result<bool, HandleError> {


Implemented for the need of plumbing in test_invalid_channel_announcement but I think we're gonna to need it in future, specially if we follow BOLT 7 on "once its funding output has been spent OR reorganized out : SHOULD forget a channel", not implemented yet but I think we need to implement a ChainListener for Router for this one.

Why cant this be handled via HTLCFailChannelUpdate::ChannelClosed?

ariard · 2018-08-29T00:26:54Z

src/ln/channel.rs

@@ -2000,6 +2000,16 @@ impl Channel {
 		self.channel_value_satoshis
 	}

+	#[cfg(test)]
+	pub fn get_local_keys(&self) -> &ChannelKeys {


Well, I found this utilities useful if we want to test messages field by field without exposing field in API but maybe we don't want a proliferation of these ones..

I think its fine for test-only for now, the need should go away once we have a more robust solution for #81 (ie where users get to specify how to generate the keys, meaning a ChannelManager-driver can easily re-derive the same keys). Maybe leave a TODO to that effect?

ariard · 2018-08-29T00:30:01Z

src/ln/channelmanager.rs

@@ -3239,4 +3240,90 @@ mod tests {
 		assert_eq!(channel_state.by_id.len(), 0);
 		assert_eq!(channel_state.short_to_id.len(), 0);
 	}
+
+	#[test]
+	fn test_invalid_channel_announcement() {


It's a little bit plumbing but I needed to separate msg content from its signatures to tweak fields freely so can't reuse create_announced_chan_between_nodes here

ariard · 2018-08-29T00:33:22Z

src/ln/router.rs

 		if msg.contents.features.requires_unknown_bits() {
 			return Err(HandleError{err: "Channel announcement required unknown feature flags", action: None});
 		}

+		//TODO: Call blockchain thing to ask if the short_channel_id is valid


Try to get this one done as described in the rfc at first but I think we need rust-blockchain-store API before. We're gonna need to access to UTXO set, built with rust-blockchain-store.

TheBlueMatt · 2018-08-29T00:28:44Z

src/ln/channel.rs

+
+	#[cfg(test)]
+	pub fn get_secp_ctx(&self) -> &Secp256k1<secp256k1::All> {
+		&self.secp_ctx


If its just for testing why not just create a new context?

TheBlueMatt · 2018-08-29T00:30:08Z

src/ln/msgs.rs

@@ -120,6 +120,13 @@ impl GlobalFeatures {
 		}
 	}

+	#[cfg(test)]
+	pub fn dummy_new(v: Vec<u8>) -> GlobalFeatures {


Maybe new_with_flags or something?

TheBlueMatt · 2018-08-29T00:30:54Z

src/ln/router.rs

 		if msg.contents.features.requires_unknown_bits() {
 			return Err(HandleError{err: "Channel announcement required unknown feature flags", action: None});
 		}

+		//TODO: Call blockchain thing to ask if the short_channel_id is valid
+		if msg.contents.chain_hash != genesis_block(Network::Bitcoin).header.bitcoin_hash() && msg.contents.chain_hash != genesis_block(Network::Testnet).header.bitcoin_hash() {


Should probably let the client specify which network we're working on, maybe via ChainInterface or just as an arg like in ChannelManager.

Add ChainWatchInterface in Router to get access to network config.

TheBlueMatt · 2018-08-29T15:49:51Z

src/ln/channel.rs

@@ -2000,6 +2000,16 @@ impl Channel {
 		self.channel_value_satoshis
 	}

+	#[cfg(test)]
+	pub fn get_local_keys(&self) -> &ChannelKeys {


I think its fine for test-only for now, the need should go away once we have a more robust solution for #81 (ie where users get to specify how to generate the keys, meaning a ChannelManager-driver can easily re-derive the same keys). Maybe leave a TODO to that effect?

TheBlueMatt · 2018-08-29T15:55:53Z

src/ln/router.rs

@@ -624,6 +624,17 @@ impl Router {

 		Err(HandleError{err: "Failed to find a path to the given destination", action: None})
 	}
+
+	/// Drop entry from network map
+	pub fn remove_channel_info(&self, short_channel_id: u64, chain_hash: Sha256dHash) -> Result<bool, HandleError> {


Why cant this be handled via HTLCFailChannelUpdate::ChannelClosed?

ariard · 2018-08-29T23:16:02Z

updated at 103d62d without effectively useless remove_channel_info stuff

ariard · 2018-08-29T23:38:17Z

updated at 19e02c4, delete few things to match with #136

TheBlueMatt · 2018-08-29T23:42:12Z

src/chain/chaininterface.rs

@@ -24,6 +25,9 @@ pub trait ChainWatchInterface: Sync + Send {

 	fn register_listener(&self, listener: Weak<ChainListener>);
 	//TODO: unregister
+
+	/// Gets the chain currently watched
+	fn get_network(&self) -> Network;


Actually while you're at it, why not just impelement the other TODO and make this
fn get_chain_txo(genesis_hash, txid, output_index) -> Result<(Script, Value), enum { IDontImplementThat, ItsNotThere, DontKnowThatChain })>

ariard · 2018-08-30T01:51:25Z

src/chain/chaininterface.rs

+	fn get_chain_txo(&self, genesis_hash: Sha256dHash, txid: Sha256dHash, output_index: u16) -> Result<(Script, u64), ChainError>;
+
+	/// Gets if outpoint is among UTXO
+	fn get_spendable_outpoint(&self, genesis_hash: Sha256dHash, txid: Sha256dHash, output_index: u16) -> Result<bool, ChainError>;


Not sure about the name, what is the quality of an outpoint in the set of UTXO?

ariard · 2018-08-30T01:53:36Z

src/chain/chaininterface.rs

+	fn get_chain_txo(&self, genesis_hash: Sha256dHash, _txid: Sha256dHash, _output_index: u16) -> Result<(Script, u64), ChainError> {
+		watched_chain!(self, genesis_hash);
+
+		//TODO: self.BlockchainStore.get_txo(txid, output_index)


Add chain_store: Arc in ChainWatchInterface? Need to have a look on rust-blockchain-store

TheBlueMatt · 2018-08-30T19:04:47Z

Hmm, I had a somewhat simpler API in mind - see https://github.com/TheBlueMatt/rust-lightning/commits/2018-08-134-proposed-changes

ariard · 2018-08-30T22:30:05Z

src/chain/chaininterface.rs

@@ -11,14 +11,12 @@ use std::sync::atomic::{AtomicUsize, Ordering};

 /// Used to give chain error details upstream
 pub enum ChainError {
-	/// Chain isn't supported
+	/// Client doesn't support UTXO lookup (but the chain hash matches our genesis block hash)


Hmm, I think we can have cases where the chain hash is really unknown and don't match our genesis block hash. Or maybe we want to filter channel_MSG to router by chain downstream ?

I'm not entirely sure what you're suggesting here? If a client is eg an SPV client they may not want to support arbitrary UTXO lookup, but we still want to check the genesis block hash is correct before we handle the message, hence the distinction here.

ariard · 2018-08-31T01:49:08Z

rebased at 28d025a

TheBlueMatt

Looks good. If you dont feel like refactoring the router_target I can do that post-merge. Either way please squash the fixup! commit and then looks good.

TheBlueMatt · 2018-08-31T03:39:54Z

fuzz/fuzz_targets/router_target.rs

@@ -107,9 +110,10 @@ pub fn do_test(data: &[u8]) {
 	}

 	let logger: Arc<Logger> = Arc::new(test_logger::TestLogger{});
+	let chain_monitor = Arc::new(chaininterface::ChainWatchInterfaceUtil::new(Network::Bitcoin, Arc::clone(&logger)));


Hmm, to keep the fuzzer from choking on the complication can you create a dummy chain watcher that reads from a byte from the input and just returns something based on that?

I've it written but can't pass InputData to chain_monitor due to lifetime conflicting requirements, hmmm

You have to do something like the InputData struct in full_stack_target, but should be able to do so without the Vec input copy.

Done, fixup squash at 2d87bad

TheBlueMatt · 2018-08-31T03:42:17Z

src/ln/channelmanager.rs

+		dummy_unsigned_msg!();
+		unsigned_msg.chain_hash = genesis_block(Network::Bitcoin).header.bitcoin_hash();
+		sign_msg!();
+		match nodes[0].router.handle_channel_announcement(&chan_announcement) {


Can assert!(.....is_ok()).

TheBlueMatt · 2018-08-31T03:42:39Z

src/ln/channelmanager.rs

+		let mut unsigned_msg;
+		let mut chan_announcement;
+
+		macro_rules! dummy_unsigned_msg {


This can just return an UnsignedChannelAnnouncement instead of assigning to a local variable? That could be a bit cleaner, no?

ariard · 2018-08-31T17:02:22Z

updated with comments at 86b8596

ariard · 2018-08-31T17:07:45Z

last commit doesn't compile, working on avoiding lifetime requirements without performance hit

ChainWatchInterface to Router check on channel_announcement Needed for BOLT 7

Fix typo

TheBlueMatt

Will take as #145 with these few nits fixed.

TheBlueMatt · 2018-09-03T21:11:48Z

src/ln/channelmanager.rs

-			let router = Router::new(PublicKey::from_secret_key(&secp_ctx, &node_id), Arc::clone(&logger));
-			nodes.push(Node { chain_monitor, tx_broadcaster, chan_monitor, node, router });
+			let router = Router::new(PublicKey::from_secret_key(&secp_ctx, &node_id), chain_monitor.clone(), Arc::clone(&logger));
+			nodes.push(Node { feeest, chain_monitor, tx_broadcaster, chan_monitor, node, router });


Note that you have a rebase error here - you re-add the feest to the Node obejct (making the first commit not compile), but then you remove it again in "Add test_invalid_channel_announcemnt + test utilities".

TheBlueMatt · 2018-09-03T21:41:32Z

fuzz/fuzz_targets/router_target.rs

+	}
+
+	fn get_chain_utxo(&self, _genesis_hash: Sha256dHash, _unspent_tx_output_identifier: u64) -> Result<(Script, u64), ChainError> {
+		match self.input.get_slice(1) {


This doesn't generate full coverage of possible return values, best to expand the return set.

TheBlueMatt · 2018-09-03T21:43:08Z

fuzz/fuzz_targets/router_target.rs

@@ -110,7 +153,13 @@ pub fn do_test(data: &[u8]) {
 	}

 	let logger: Arc<Logger> = Arc::new(test_logger::TestLogger{});
-	let chain_monitor = Arc::new(chaininterface::ChainWatchInterfaceUtil::new(Network::Bitcoin, Arc::clone(&logger)));
+	let input = Arc::new(InputData {
+		data: data.to_vec(),


Since you don't change the existing calls you end up using elements in data twice, potentially reducing coverage of get_chain_utxo even more.

#134 rebased

ariard commented Aug 29, 2018

View reviewed changes

TheBlueMatt reviewed Aug 29, 2018

View reviewed changes

ariard force-pushed the route_check branch from 0d27b72 to 103d62d Compare August 29, 2018 23:13

ariard force-pushed the route_check branch 2 times, most recently from 19cc555 to 19e02c4 Compare August 29, 2018 23:37

TheBlueMatt reviewed Aug 29, 2018

View reviewed changes

ariard force-pushed the route_check branch 2 times, most recently from 9d7b961 to f3c4e4a Compare August 30, 2018 01:45

ariard commented Aug 30, 2018

View reviewed changes

ariard force-pushed the route_check branch from f3c4e4a to a534e9d Compare August 30, 2018 22:25

ariard commented Aug 30, 2018

View reviewed changes

Add ChainWatchInterface in Router

fa2d2bd

ariard force-pushed the route_check branch from a534e9d to 28d025a Compare August 31, 2018 01:48

TheBlueMatt reviewed Aug 31, 2018

View reviewed changes

ariard force-pushed the route_check branch from 28d025a to 86b8596 Compare August 31, 2018 17:01

TheBlueMatt mentioned this pull request Aug 31, 2018

Ensure handle_announcement_signatures always has a ErrorAction #143

Merged

ariard force-pushed the route_check branch from 5136906 to f861906 Compare August 31, 2018 23:30

Antoine Riard and others added 3 commits August 31, 2018 23:40

Implement get_chain_utxo and ChainError in

6a12ff7

ChainWatchInterface to Router check on channel_announcement Needed for BOLT 7

Check script_pubkey against expected channel_announcement

267a412

Add test_invalid_channel_announcemnt + test utilities

56571ae

Fix typo

Add DummyChainWatcher in route_target

2d87bad

ariard force-pushed the route_check branch from f861906 to 2d87bad Compare August 31, 2018 23:40

TheBlueMatt reviewed Sep 3, 2018

View reviewed changes

TheBlueMatt closed this Sep 3, 2018

TheBlueMatt added a commit that referenced this pull request Sep 3, 2018

Merge pull request #145 from TheBlueMatt/2018-09-134-rebased

cd9d680

#134 rebased

Check channel_announcement is on Bitcoin Mainnet or Testnet + test_invalid_channel_announcement #134

Check channel_announcement is on Bitcoin Mainnet or Testnet + test_invalid_channel_announcement #134

Uh oh!

Conversation

ariard commented Aug 29, 2018

Uh oh!

ariard Aug 29, 2018 • 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!

ariard Aug 29, 2018 • 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!

ariard Aug 29, 2018 • 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!

ariard commented Aug 29, 2018

Uh oh!

ariard commented Aug 29, 2018

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 Aug 30, 2018

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!

ariard commented Aug 31, 2018

Uh oh!

TheBlueMatt left a comment

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!

ariard commented Aug 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariard commented Aug 31, 2018

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

ariard Aug 29, 2018 •

edited

Loading

ariard Aug 29, 2018 •

edited

Loading

ariard Aug 29, 2018 •

edited

Loading

ariard commented Aug 31, 2018 •

edited

Loading