Add channel_monitor_claim_revoked_commitment_tx test #184

ariard · 2018-09-16T02:18:18Z

Test htlc outputs shared claim case

Following discussion in #137 on testing htlc outputs claiming.
Note that I didn't clean the TODO in channel_monitor, I'm also gonna add this specific case here.

ariard · 2018-09-16T02:21:22Z

src/ln/channelmanager.rs

-	/// Tests that the given node has broadcast a claim transaction against the provided revoked
-	/// HTLC transaction.
+	/// Tests that the given node has broadcast a claim transaction against the provided
+	/// HTLC transaction issued from a revoked commitment tx


Properly speaking, does a HTLC transaction is revoked ? It's more the broadcasting of a revoked commitment tx which "nullify" the chain of following transactions, seems to me

ariard · 2018-09-16T02:23:34Z

src/ln/channelmanager.rs

+
+			nodes[1].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![revoked_local_txn[0].clone()] }, 1);
+			let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
+			assert_eq!(node_txn.len(), 4); // ChannelManager is broadcasting last_local_commitment_txn.. and revocation tx is broacasted twice as block has been re-scanned.


As described in force_close, ChannelManager is also broadcasting the last_local_commitment_txn, which is normal. But revocation tx which is generated and broadcasted again due to block re-scanning seems really a performance bottleneck?

ariard · 2018-09-16T02:26:15Z

src/ln/channelmanager.rs

+			nodes[1].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![revoked_local_txn[0].clone()] }, 1);
+			let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
+			assert_eq!(node_txn.len(), 4); // ChannelManager is broadcasting last_local_commitment_txn.. and revocation tx is broacasted twice as block has been re-scanned.
+			assert_eq!(node_txn[0].input.len(), 2); //TODO: really should be 4 when we implement to_local and to_remote via revocation key claiming


Still thinking to more assert, but how to identify specifically htlc input scripts?

ariard · 2018-09-18T00:04:43Z

This 2 tests are covering claiming of htlc outputs from a revoked commitment tx, both case where they can shared a tx and case where they can't due to timeout.

TheBlueMatt

Nice! I think we should be a bit more aggressive with these tests. Better to be aggressive (with lots of comments) and change the tests later if we tweak how txn get created than have a bug in on-chain claiming.

TheBlueMatt · 2018-09-18T16:24:32Z

src/ln/channelmanager.rs

+		let _payment_preimage_2 = route_payment(&nodes[1], &vec!(&nodes[0])[..], 3000000).0;
+
+		// Get the will-be-revoked local txn from node[0]
+		let revoked_local_txn = nodes[0].node.channel_state.lock().unwrap().by_id.iter().next().unwrap().1.last_local_commitment_txn.clone();


Note that chan_1.3 is the channel_id, so you can do a get(chan_1.3).unwrap() here for a bit more readability.

Thanks, updated

TheBlueMatt · 2018-09-18T16:27:30Z

src/ln/channelmanager.rs

+			let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
+
+			nodes[0].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![revoked_local_txn[0].clone()] }, 1);
+			test_txn_broadcast(&nodes[0], &chan_1, Some(revoked_local_txn[0].clone()), HTLCType::TIMEOUT);


Do we really care what nodes[0] did here? It misbehaved (broadcasted an old local commitment state, so is probably gonna lose its money on revocation claims).

Was thinking of the comment, check_spend_revoked_transaction channel_monitor l 1338, maybe we want to implement it and substitute it to check_spend_local_transaction internally for specially handling revoked tx from us.
Seems check_spend_revoked_transaction is no longer news ?

Deleted the test_txn_broadcast

TheBlueMatt · 2018-09-18T16:30:58Z

src/ln/channelmanager.rs

+
+			nodes[1].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![revoked_local_txn[0].clone()] }, 1);
+			let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
+			assert_eq!(node_txn.len(), 4); // ChannelManager is broadcasting 2 last_local_commitment_txn..and revocation tx is broacasted twice as block has been re-scanned.


Hmm, can we test what the 4 txn look like, then to be much tighter?

Yeah, add asserts based on witness script, txid, number of inputs.

TheBlueMatt · 2018-09-18T16:32:00Z

src/ln/channelmanager.rs

+		assert_eq!(nodes[0].node.list_channels().len(), 0);
+		assert_eq!(nodes[1].node.list_channels().len(), 0);
+
+		let chan_2 = create_announced_chan_between_nodes(&nodes, 2, 3);


Hmm, if the tests are completely separate I'd prefer we make a new test. This means fewer channel_update/channel_announcement handles and also lets rust run them in paralell. I previously didn't but going forward would be nice to keep tests smaller.

Splitted as of 080d5df

TheBlueMatt · 2018-09-18T16:35:08Z

src/ln/channelmanager.rs

+		// Rebalance the network to generate htlc in the two directions
+		send_payment(&nodes[2], &vec!(&nodes[3])[..], 8000000);
+		// node[2] is gonna to revoke an old state thus node[3] should be able to claim both offered/received HTLC outputs on top of commitment tx, but this
+		// time as two different claim transactions as we're gonna to timeout htlc with given a high current heigh


nit: heigh -> height.

TheBlueMatt · 2018-09-18T16:37:19Z

src/ln/channelmanager.rs

+			assert_eq!(node_txn[1].input.len(), 1);
+
+			let mut funding_tx_map = HashMap::new();
+			funding_tx_map.insert(revoked_local_txn[0].txid(), revoked_local_txn[0].clone());


Hmm, this test looks like it would pass whether the tx here were claiming the revoked normal output or the revoked HTLC output. We should test that later txn claim a different output in the revoked_local_txn[0], at least, if not be more aggressive with what all of the txn here look like.

Yes add assert based on witness_script. Note: we can't assume which one is the htlc offered/received first as of using HashMap internally.

ariard · 2018-09-20T01:02:27Z

src/ln/channelmanager.rs

+			let witness_script_1 = node_txn[0].clone().input[0].witness.pop().unwrap();
+			let witness_script_2 = node_txn[0].clone().input[1].witness.pop().unwrap();
+			if witness_script_1.len() > 133 {
+				assert_eq!(witness_script_1.len(), 138);


Is there any reason to get accepted HTLC script of 138 bytes instead of BOLT 3 139 bytes ?

I think it should be 139 in prod but because the block height being used as locktime here is < 2^15 (but more than 2^7) its 138.

TheBlueMatt

Just re-reviewed first test for now.

TheBlueMatt · 2018-09-20T02:05:02Z

src/ln/channelmanager.rs

+			assert_eq!(node_txn[3].input.len(), 2);
+			node_txn[3].verify(&revoked_tx_map).unwrap();
+
+			assert_eq!(node_txn[0].txid(), node_txn[3].txid()); // justice tx is duplicated due to block re-scanning


Should just be able to assert node_txn[0], node_txn[3], no? The witnesses should also be identical.

TheBlueMatt · 2018-09-20T02:16:53Z

src/ln/channelmanager.rs

+			let witness_script_1 = node_txn[0].clone().input[0].witness.pop().unwrap();
+			let witness_script_2 = node_txn[0].clone().input[1].witness.pop().unwrap();
+			if witness_script_1.len() > 133 {
+				assert_eq!(witness_script_1.len(), 138);


I think it should be 139 in prod but because the block height being used as locktime here is < 2^15 (but more than 2^7) its 138.

TheBlueMatt · 2018-09-20T02:18:26Z

src/ln/channelmanager.rs

+			}
+
+			assert_eq!(node_txn[1].input.len(), 1);
+			let witness_script = node_txn[1].clone().input[0].witness.pop().unwrap();


Why not just assert the input prevout is the funding tx?

TheBlueMatt · 2018-09-20T02:19:48Z

src/ln/channelmanager.rs

+			assert_eq!(witness_script.len(), 71); // Spending funding tx unique txoutput, tx broadcasted by ChannelManager
+
+			assert_eq!(node_txn[2].input.len(), 1);
+			let witness_script = node_txn[2].clone().input[0].witness.pop().unwrap();


Can't we check that its spending the same as revoked_tx_map (and has a unique outpoint that wasnt in one of the others)?

ariard · 2018-09-21T00:31:21Z

Rebased, updated with comments, also add more asserts in remaining test based on last review pass.

TheBlueMatt · 2018-09-29T19:02:39Z

New tests look good, but found a bug that needs to be fixed first. Can you review #199 (which includes this rebased)?

Fix simple to_local revoked output claim and rebase #184

ariard commented Sep 16, 2018

View reviewed changes

ariard force-pushed the test_claim_revoked_htlc_outputs branch from 94235ea to 7e2fd5a Compare September 18, 2018 00:03

TheBlueMatt reviewed Sep 18, 2018

View reviewed changes

ariard force-pushed the test_claim_revoked_htlc_outputs branch from 7e2fd5a to 2f41481 Compare September 20, 2018 00:56

ariard commented Sep 20, 2018

View reviewed changes

ariard force-pushed the test_claim_revoked_htlc_outputs branch from 2f41481 to 080d5df Compare September 20, 2018 01:54

TheBlueMatt reviewed Sep 20, 2018

View reviewed changes

TheBlueMatt mentioned this pull request Sep 20, 2018

BOLT 3 : accepted_htlc_script expected weight shouldn't be 138 instead of 139 ? lightning/bolts#483

Closed

ariard force-pushed the test_claim_revoked_htlc_outputs branch from 080d5df to 622c958 Compare September 21, 2018 00:21

Antoine Riard added 2 commits September 21, 2018 00:22

Test htlc outputs shared tx claim case

9db22d0

Test htlc outputs single tx claim due to timeout case

f1f9ead

ariard force-pushed the test_claim_revoked_htlc_outputs branch from 622c958 to f1f9ead Compare September 21, 2018 00:22

TheBlueMatt mentioned this pull request Sep 29, 2018

Fix simple to_local revoked output claim and rebase #184 #199

Merged

TheBlueMatt closed this Sep 29, 2018

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

Merge pull request #199 from TheBlueMatt/2018-09-184-fixed-monitor

6c1123c

Fix simple to_local revoked output claim and rebase #184

Add channel_monitor_claim_revoked_commitment_tx test #184

Add channel_monitor_claim_revoked_commitment_tx test #184

Uh oh!

Conversation

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

Uh oh!

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 Sep 18, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard Sep 20, 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 Sep 20, 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!

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!

ariard commented Sep 21, 2018

Uh oh!

TheBlueMatt commented Sep 29, 2018

Uh oh!

Uh oh!

ariard commented Sep 16, 2018 •

edited

Loading

ariard Sep 20, 2018 •

edited

Loading

ariard Sep 20, 2018 •

edited

Loading