Test for BOLT 2 requirements for update_add_htlc message #291

philipr-za · 2019-01-15T10:15:11Z

Related to issue #129 and #240

This PR contributes a test to explicitly test each requirement for the sender and receiver for the update_add_htlc message as defined in the BOLT 2 spec.

Some notes:
I found that it did not appear that the following requirement was validated in the send_htlc method:

- MUST NOT offer amount_msat it cannot pay for in the remote commitment transaction at the current feerate_per_kw (see "Updating Fees") while maintaining its channel reserve.

but because the Fee aspect of the BOLT specs is being updated I left it as a TODO.

I also did not specifically test the following requirement:

- for channels with chain_hash identifying the Bitcoin blockchain:
    * MUST set the four most significant bytes of amount_msat to 0.

I could not find a sensible way to get around the many layers of checks that prevent amount_msat from being set to close to a value that violates this requirement.

Finally, I found that the following requirement on the Sender was not explicitly tested in the various methods involved in handling an update_add_htlc message:

Sender MUST set cltv_expiry less than 500000000.

It exists as a TODO in the send_htlc() but it is enforced when constructing a route.

TheBlueMatt

Awesome! Thanks! Would like a few cleanups but looks good.

src/ln/functional_tests.rs

TheBlueMatt · 2019-01-15T15:59:19Z

src/ln/functional_tests.rs

+	}, onion_packet);
+
+	if let Err(ChannelError::Ignore(_)) = err {
+		assert!(true);


Can you check the error message is the expected one?

Done in all the tests

TheBlueMatt · 2019-01-15T15:59:59Z

src/ln/functional_tests.rs

+	let (onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads(&route, cur_height).unwrap();
+	let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, &our_payment_hash);
+
+	let err = nodes[0].node.channel_state.lock().unwrap().by_id.get_mut(&chan.2).unwrap().send_htlc(0, our_payment_hash, TEST_FINAL_CLTV, HTLCSource::OutboundRoute {


Why call through to the channel directly? Can't you test the same using nodes[0].node.send_payment and check the error message without stepping over a layer? (also again further down).

The reason is that from what I can see when using send_payment(...) the payment parameters are all set up when you call get_route(...) and most of these test cases cause various failures when trying to construct the route. I was trying to test all the validation that is done when adding the HTLC to be sent rather than constructing the route so to get into the middle of that process I constructed a valid route and then implemented the essential stuff that is happening inside nodes[0].node.send_payment(...) in the test and changed the payment parameters for the test cases.

Oh, I think we're out of sync on which send_payment. I dont mean the functional_tests::send_payment function that steps through everything, I meant the ChannelManager::send_payment, which calls Channel::send_htlc pretty directly and takes the Route as a parameter.

Ok, I figured out how to effect these tests without skipping that layer. I was originally trying to check for the correct error types which was tricky because the various error handling macro's changed the types into more generic versions as they were handled but by just looking for the error message strings it is possible to do it as you suggest.

TheBlueMatt

Awesome! I have a number of comments regarding making sure the tests don't break when we change minor implementation details and making sure we get the most out of the tests, but this is great work.

TheBlueMatt · 2019-01-18T17:45:46Z

src/ln/channel.rs

@@ -382,7 +394,7 @@ macro_rules! secp_check {

 impl Channel {
 	// Convert constants + channel value to limits:
-	fn get_our_max_htlc_value_in_flight_msat(channel_value_satoshis: u64) -> u64 {
+	pub fn get_our_max_htlc_value_in_flight_msat(channel_value_satoshis: u64) -> u64 {


I think this is no longer necessary?

changed back to be not private, I do need to query this value but do it via get_value_stat(...) now.

TheBlueMatt · 2019-01-18T18:24:20Z

src/ln/functional_tests.rs

+	let max_accepted_htlcs = nodes[1].node.channel_state.lock().unwrap().by_id.get(&chan.2).unwrap().their_max_accepted_htlcs as u64;
+
+	//Confirm the first HTLC ID is zero
+	assert_eq!(nodes[0].node.channel_state.lock().unwrap().by_id.get(&chan.2).unwrap().next_local_htlc_id, 0);


Why not just check this based on the field in the update_add_htlc message? Then you dont have to expose more internal state (and check that the internal state is converted to messages correctly).

Done, took out making the internal state public for the test.

TheBlueMatt · 2019-01-18T18:28:02Z

src/ln/functional_tests.rs

+		commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false);
+
+		expect_pending_htlcs_forwardable!(nodes[1]);
+		let _ = nodes[1].node.get_and_clear_pending_events();


expect_payment_received!().

Done, had to pull the definition of the macro out of the test it was in to make it available to the other tests.

TheBlueMatt · 2019-01-18T18:31:02Z

src/ln/functional_tests.rs

+	let mut nodes = create_network(2);
+	let _chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 0);
+
+	let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &[], 100000000, TEST_FINAL_CLTV).unwrap();


Can you test the boundary? ie test that we will send exactly max_htlc_value_in_flight and then test that sending on more fails? This also means if we change the order of checks to check HTLCs dont add total_value - reserve value this test wont spuriously break.

TheBlueMatt · 2019-01-18T18:36:05Z

src/ln/functional_tests.rs

+		let channel = chan_lock.by_id.get(&chan.2).unwrap();
+		htlc_minimum_msat = channel.get_our_htlc_minimum_msat();
+	}
+	let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &[], htlc_minimum_msat+1, TEST_FINAL_CLTV).unwrap();


Shouldnt need the +1 here.

If you don't the get_route(...) call fails saying it cannot find the route. You can indeed go in and change the message manually after constructing the route to have 1 less and it works but the route creation process seems to need that +1.

Ahh, I'll drop it in #203, then!

TheBlueMatt · 2019-01-18T19:10:58Z

src/ln/functional_tests.rs

+	check_added_monitors!(nodes[0], 1);
+	let mut updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
+
+	updates.update_add_htlcs[0].amount_msat = 4000001;


Where did this come from/wont this break if/when we change our reserve values?

Updated to be based on the queried channel_reserve value.

TheBlueMatt · 2019-01-18T19:12:00Z

src/ln/functional_tests.rs

+		msg.htlc_id = i as u64;
+		nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &msg).unwrap();
+	}
+	msg.htlc_id = (super::channel::OUR_MAX_HTLCS + 1) as u64;


The +1 is extraneous, no?

true, fixed

TheBlueMatt · 2019-01-18T19:12:11Z

src/ln/functional_tests.rs

+	let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1000000, 1000000);
+	let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &[], 1000000, TEST_FINAL_CLTV).unwrap();
+	let (_, our_payment_hash) = get_payment_preimage_hash!(nodes[0]);
+    nodes[0].node.send_payment(route, our_payment_hash).unwrap();


nit: bad indentation.

TheBlueMatt · 2019-01-18T19:15:43Z

src/ln/functional_tests.rs

+	{
+		assert_eq!(nodes[1].node.channel_state.lock().unwrap().by_id.len(), 0);
+	}
+	//Clear unhandled msg events.


I'm really not a fan of doing this, generally. If we bothered to write out a test for a given behavior, we should check that the user-facing result of it is consistent with what we expect. If there are a bunch of things that happen, just add a macro that checks all of them then you can reuse the code.

I found the check_closed_broadcast!(...) macro that seems to check that the final channel close broadcast message is sent and I replaced these calls with that.

TheBlueMatt · 2019-01-18T19:16:19Z

src/ln/functional_tests.rs

+	let _ = handle_chan_reestablish_msgs!(nodes[1], nodes[0]);
+	let _ = nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &updates.update_add_htlcs[0]);
+	//Confirm the HTLC was ignored
+	assert_eq!(nodes[1].node.channel_state.lock().unwrap().by_id.get(&chan.2).unwrap().next_remote_htlc_id, 1);


Maybe also check the commitment_signed only has one HTLC in it?

So I was implementing your suggestion but while I was looking around I realized that I am not sure if this requirement is actually being handled as written.

The requirement for the receiver says: BOLT 2 requirement: if the sender did not previously acknowledge the commitment of that HTLC: MUST ignore a repeated id value after a reconnection.

However, from what I can see is that if an HTLC is sent and the disconnect/reconnect happens before the commitment_dance is done and then a new HTLC message is sent with the same ID it is added rather than ignored and the original HTLC is discarded during the disconnect/reconnect process. This results in the same state I was testing for of there only being a single HTLC in the queue but it doesn't seem to be what the spec intends?

I would have thought that the first HTLC is retained and the repeated HTLC message should be ignored? Thoughts?

I think the spec needs changed there, then - the point of the disconnect/reconnect dance is we assume messages may have been lost in flight, so we definitely should be dropping the old ones and replacing.

Awesome, in that case we are good to go.

TheBlueMatt · 2019-01-22T17:17:58Z

Will take as #296, you can see the list of changes at https://github.com/TheBlueMatt/rust-lightning/commits/2019-01-291-cleanups

TheBlueMatt reviewed Jan 15, 2019

View reviewed changes

TheBlueMatt reviewed Jan 18, 2019

View reviewed changes

philipr-za added 4 commits January 21, 2019 14:48

Wrote test to explicitly test BOLT 2 requirements for update_add_htlc

e44a360

Addressed PR comments

568e9d0

Address PR comments

cac0355

Addressing PR comments

5473bd4

philipr-za force-pushed the philip-#129-tests-for-update-add-htlc branch from 54ed39d to 5473bd4 Compare January 21, 2019 12:49

Addressing PR comments

7219b7d

TheBlueMatt mentioned this pull request Jan 22, 2019

Wrote test to explicitly test BOLT 2 requirements for update_add_htlc #296

Merged

TheBlueMatt closed this Jan 22, 2019

philipr-za mentioned this pull request Jan 23, 2019

Check Bolt 2 spec #240

Closed

26 tasks

Test for BOLT 2 requirements for update_add_htlc message #291

Test for BOLT 2 requirements for update_add_htlc message #291

Uh oh!

Conversation

philipr-za commented Jan 15, 2019

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

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!

philipr-za Jan 16, 2019 • 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!

philipr-za Jan 18, 2019 • 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!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philipr-za Jan 21, 2019 • 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!

philipr-za Jan 21, 2019 • 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!

philipr-za Jan 21, 2019 • 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!

philipr-za Jan 21, 2019 • 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!

philipr-za Jan 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

philipr-za Jan 16, 2019 •

edited

Loading

philipr-za Jan 18, 2019 •

edited

Loading

philipr-za Jan 21, 2019 •

edited

Loading

philipr-za Jan 21, 2019 •

edited

Loading

philipr-za Jan 21, 2019 •

edited

Loading

philipr-za Jan 21, 2019 •

edited

Loading

philipr-za Jan 21, 2019 •

edited

Loading