Refactor channelmanager tests into more appropriate submodule tests #2977

srikanth-iyengar · 2024-03-29T14:30:46Z

Previously channelmanager had a single test module withint channelmanager.rs
This changes introduces moving refactoring 23 tests of channelmanager into following test groups

Group 1: Key Send and Payment Verification

test_keysend_dup_hash_partial_mpp
test_keysend_dup_payment_hash
test_keysend_hash_mismatch
test_keysend_msg_with_secret_err
bad_inbound_payment_hash
reject_excessively_underpaying_htlcs
test_final_incorrect_cltv
test_payment_display
test_malformed_forward_htlcs_ser

Group 3: Anchor Channel and Configuration Tests

test_inbound_anchors_manual_acceptance
test_anchors_zero_fee_htlc_tx_fallback
test_update_channel_config

codecov-commenter · 2024-03-29T14:39:53Z

Codecov Report

Attention: Patch coverage is 97.88315% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 89.77%. Comparing base (1890e80) to head (2e4f552).
Report is 11 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channelmanager_limits_tests.rs	97.63%	9 Missing and 4 partials ⚠️
...tning/src/ln/anchor_channel_configuration_tests.rs	93.49%	8 Missing ⚠️
lightning/src/ln/keysend_payments_tests.rs	99.21%	4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2977      +/-   ##
==========================================
- Coverage   89.82%   89.77%   -0.06%     
==========================================
  Files         116      119       +3     
  Lines       96466    96480      +14     
  Branches    96466    96480      +14     
==========================================
- Hits        86655    86615      -40     
- Misses       7264     7304      +40     
- Partials     2547     2561      +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt

Note that you'll probably have to rebase this after #2731 adds a new test.

lightning/src/ln/anchor_channel_configuration_tests.rs

srikanth-iyengar · 2024-04-02T14:00:50Z

Note that you'll probably have to rebase this after #2731 adds a new test.

Will take care of that for sure 👍

srikanth-iyengar · 2024-04-05T12:44:59Z

@TheBlueMatt updated changes from main after the merge of #2731 Took care of added test

lightning/src/ln/channelmanager.rs

lightning/src/ln/channelmanager_limits_tests.rs

lightning/src/ln/keysend_payments_tests.rs

lightning/src/ln/channelmanager_limits_tests.rs

srikanth-iyengar · 2024-04-20T11:45:11Z

lightning/src/ln/keysend_payments_tests.rs

+#[test]
+fn bad_inbound_payment_hash() {
+	// Add coverage for checking that a user-provided payment hash matches the payment secret.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+	node_cfgs[0].keys_manager.get_inbound_payment_key_material();
+
+	let highest_seen_timestamp = bitcoin::blockdata::constants::genesis_block(bitcoin::Network::Testnet).header.time;
+	let node_signer = node_cfgs[0].keys_manager;
+	let inbound_pmt_key_material = node_signer.get_inbound_payment_key_material();
+	let expanded_inbound_key = inbound_payment::ExpandedKey::new(&inbound_pmt_key_material);
+
+	let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(&nodes[0]);
+	let payment_data = msgs::FinalOnionHopData {
+		payment_secret,
+		total_msat: 100_000,
+	};
+
+


@TheBlueMatt I have refactored this test, can you go through this whether this behaves as expected ?

Basically I tried to untie the dependency between the channelmanagers internal state, let me know if there is better way

srikanth-iyengar · 2024-04-20T11:49:07Z

lightning/src/ln/channelmanager_limits_tests.rs

+		// Should we drop this check ?
+		// Assert that ChannelUpdate message has been added to node[0] pending broadcast messages
+		// let pending_broadcast_messages = nodes[0].node.get_and_clear_pending_msg_events();
+		// assert_eq!(pending_broadcast_messages.len(), 2);
+	}


@TheBlueMatt I had to remove this check, because this effects the at the 703-704 and fails
I am not able to figure out the reason why this is happening

tnull · 2024-05-14T08:21:42Z

This might need a rebase by now - sorry!

srikanth-iyengar · 2024-05-14T12:25:53Z

This might need a rebase by now - sorry!

Thanks, will rebase with main.
do we have some new tests in channelmanager ? Asking so that I don't accidentally remove the tests

TheBlueMatt · 2024-05-14T13:44:27Z

Asking so that I don't accidentally remove the tests

If you do the move-only parts in a separate move-only commit it'll be pretty easy to see with git show --color-move --color-move-ws=ignore-space-change :)

srikanth-iyengar · 2024-05-14T16:24:48Z

Rebased with main

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2024-05-17T23:52:23Z

lightning/src/ln/channelmanager_limits_tests.rs

-		let pending_broadcast_messages= nodes[0].node.pending_broadcast_messages.lock().unwrap();
-		assert_eq!(pending_broadcast_messages.len(), 1);
+		// let pending_broadcast_messages= nodes[0].node.get_and_clear_pending_msg_events();
+		// assert_eq!(pending_broadcast_messages.len(), 1);


Why'd you remove these checks?

Still figuring out a way to rewrite this test

It looks like if you added a test-only getter that clones the pending_broadcast_messages, that would fix it. I don't immediately see a way to replicate this part of the test using the public API (since get_and_clear.. clears, which breaks later parts of the test).

It looks like if you added a test-only getter that clones the pending_broadcast_messages, that would fix it. I don't immediately see a way to replicate this part of the test using the public API (since get_and_clear.. clears, which breaks later parts of the test).

Thanks, I was stuck on this test for a while

* Few tests were dependent on channelmanagers internal state, refactored that them to publicly visible apis

valentinewallace

This seems close, thanks for tackling this!

valentinewallace · 2024-05-22T16:03:32Z

lightning/src/ln/anchor_channel_configuration_tests.rs

-use crate::ln::msgs::ChannelMessageHandler;
+use crate::ln::types::ChannelId;
+use crate::ln::functional_test_utils::*;
 use crate::ln::msgs::ErrorAction;
-use crate::ln::{functional_test_utils::*, ChannelId};
+use crate::ln::msgs::ChannelMessageHandler;
 use crate::prelude::*;
-use crate::util::config::ChannelConfigUpdate;
 use crate::util::errors::APIError;
+use crate::util::config::ChannelConfigUpdate;


I think this diff belongs in the previous commit?

valentinewallace · 2024-05-22T16:19:52Z

lightning/src/ln/channelmanager_limits_tests.rs

@@ -663,7 +663,7 @@ fn test_channel_update_cached() {

 	let chan = create_announced_chan_between_nodes(&nodes, 0, 1);

-	nodes[0].node.force_close_channel_with_peer(&chan.2, &nodes[1].node.get_our_node_id(), None, true).unwrap();
+	let _ = nodes[0].node.force_close_broadcasting_latest_txn(&chan.2, &nodes[1].node.get_our_node_id());


Let's keep the unwrap, we still want to make sure this call doesn't error.

valentinewallace · 2024-05-22T16:44:31Z

lightning/src/ln/channelmanager_limits_tests.rs

-		let pending_broadcast_messages= nodes[0].node.pending_broadcast_messages.lock().unwrap();
-		assert_eq!(pending_broadcast_messages.len(), 1);
+		// let pending_broadcast_messages= nodes[0].node.get_and_clear_pending_msg_events();
+		// assert_eq!(pending_broadcast_messages.len(), 1);


It looks like if you added a test-only getter that clones the pending_broadcast_messages, that would fix it. I don't immediately see a way to replicate this part of the test using the public API (since get_and_clear.. clears, which breaks later parts of the test).

valentinewallace · 2024-05-22T16:46:59Z

lightning/src/ln/anchor_channel_configuration_tests.rs

+use crate::ln::msgs::ChannelMessageHandler;
+use crate::ln::msgs::ErrorAction;


nit: combine these imports

valentinewallace · 2024-05-22T16:47:11Z

lightning/src/ln/anchor_channel_configuration_tests.rs

+use crate::events::{Event, MessageSendEvent, MessageSendEventsProvider};
+use crate::ln::msgs::ChannelMessageHandler;
+use crate::ln::msgs::ErrorAction;
+use crate::ln::{functional_test_utils::*, ChannelId};


nit: split this into two imports and alphabetize them. This should remove some unnecessary diff in the next commit too

valentinewallace · 2024-05-22T16:47:46Z

lightning/src/ln/channelmanager_limits_tests.rs

+use bitcoin::hashes::Hash;
+use crate::ln::channelmanager::{PaymentId, PaymentSendFailure, RecipientOnionFields};
+use crate::ln::functional_test_utils::*;
+use crate::util::errors::APIError;
+use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey};
+use crate::events::{Event, MessageSendEvent, MessageSendEventsProvider, ClosureReason};
+use crate::ln::channelmanager;
+use crate::ln::ChannelId;
+use crate::ln::msgs::{self};
+use crate::ln::msgs::ChannelMessageHandler;
+use crate::prelude::*;
+use crate::util::config::ChannelConfig;
+use crate::sign::EntropySource;


nit: alphabetize these imports. I think if you copy this into the rust playground and run rustfmt it will do it for you. Please check the other new test files in this commit to see if they need it too.

srikanth-iyengar force-pushed the refactor/2958 branch from 8eda436 to 388e6bc Compare March 29, 2024 14:45

TheBlueMatt mentioned this pull request Apr 2, 2024

Delay broadcasting Channel Updates until connected to peers #2731

Merged

TheBlueMatt reviewed Apr 2, 2024

View reviewed changes

lightning/src/ln/anchor_channel_configuration_tests.rs Outdated Show resolved Hide resolved

srikanth-iyengar force-pushed the refactor/2958 branch from 388e6bc to e75b405 Compare April 2, 2024 13:42

srikanth-iyengar force-pushed the refactor/2958 branch from e75b405 to 30dcf01 Compare April 5, 2024 12:42

TheBlueMatt reviewed Apr 9, 2024

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager_limits_tests.rs Outdated Show resolved Hide resolved

srikanth-iyengar commented Apr 19, 2024

View reviewed changes

lightning/src/ln/keysend_payments_tests.rs Outdated Show resolved Hide resolved

srikanth-iyengar force-pushed the refactor/2958 branch from 30dcf01 to f585d35 Compare April 19, 2024 12:55

srikanth-iyengar commented Apr 19, 2024

View reviewed changes

lightning/src/ln/channelmanager_limits_tests.rs Outdated Show resolved Hide resolved

srikanth-iyengar force-pushed the refactor/2958 branch 2 times, most recently from e50313f to 77fc367 Compare April 20, 2024 11:42

srikanth-iyengar commented Apr 20, 2024

View reviewed changes

Move out channelmanager test into different module

093edef

srikanth-iyengar force-pushed the refactor/2958 branch from 77fc367 to e4dfd72 Compare May 14, 2024 16:22

srikanth-iyengar force-pushed the refactor/2958 branch from e4dfd72 to 162ddea Compare May 14, 2024 16:32

TheBlueMatt reviewed May 17, 2024

View reviewed changes

Refactor channelmanager tests to test publicly visible apis

2e4f552

* Few tests were dependent on channelmanagers internal state, refactored that them to publicly visible apis

srikanth-iyengar force-pushed the refactor/2958 branch from 162ddea to 2e4f552 Compare May 18, 2024 06:59

valentinewallace reviewed May 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor channelmanager tests into more appropriate submodule tests #2977

Refactor channelmanager tests into more appropriate submodule tests #2977

srikanth-iyengar commented Mar 29, 2024 •

edited

codecov-commenter commented Mar 29, 2024 •

edited

TheBlueMatt left a comment

srikanth-iyengar commented Apr 2, 2024

srikanth-iyengar commented Apr 5, 2024

srikanth-iyengar Apr 20, 2024 •

edited

srikanth-iyengar Apr 20, 2024 •

edited

srikanth-iyengar Apr 20, 2024 •

edited

tnull commented May 14, 2024

srikanth-iyengar commented May 14, 2024 •

edited

TheBlueMatt commented May 14, 2024

srikanth-iyengar commented May 14, 2024

TheBlueMatt May 17, 2024

srikanth-iyengar May 18, 2024 •

edited

valentinewallace May 22, 2024

srikanth-iyengar May 23, 2024

valentinewallace left a comment

valentinewallace May 22, 2024

valentinewallace May 22, 2024

valentinewallace May 22, 2024

valentinewallace May 22, 2024

valentinewallace May 22, 2024

valentinewallace May 22, 2024

		use crate::ln::msgs::ChannelMessageHandler;
		use crate::ln::msgs::ErrorAction;

Refactor channelmanager tests into more appropriate submodule tests #2977

Are you sure you want to change the base?

Refactor channelmanager tests into more appropriate submodule tests #2977

Conversation

srikanth-iyengar commented Mar 29, 2024 • edited

Group 1: Key Send and Payment Verification

Group 2: Channel Management and Limits

codecov-commenter commented Mar 29, 2024 • edited

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

srikanth-iyengar commented Apr 2, 2024

srikanth-iyengar commented Apr 5, 2024

srikanth-iyengar Apr 20, 2024 • edited

Choose a reason for hiding this comment

srikanth-iyengar Apr 20, 2024 • edited

Choose a reason for hiding this comment

srikanth-iyengar Apr 20, 2024 • edited

Choose a reason for hiding this comment

tnull commented May 14, 2024

srikanth-iyengar commented May 14, 2024 • edited

TheBlueMatt commented May 14, 2024

srikanth-iyengar commented May 14, 2024

Choose a reason for hiding this comment

srikanth-iyengar May 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikanth-iyengar commented Mar 29, 2024 •

edited

codecov-commenter commented Mar 29, 2024 •

edited

srikanth-iyengar Apr 20, 2024 •

edited

srikanth-iyengar Apr 20, 2024 •

edited

srikanth-iyengar Apr 20, 2024 •

edited

srikanth-iyengar commented May 14, 2024 •

edited

srikanth-iyengar May 18, 2024 •

edited