Run rustfmt on payment_tests and functional_tests and split up the latter
#3751
Conversation
|
👋 Thanks for assigning @joostjager as a reviewer! |
TheBlueMatt
force-pushed
the
2025-04-rustfmt-tests-1
branch
from
293c6af to
820cb58
Compare
There was a problem hiding this comment.
First of all, I’m incredibly impressed by your ability to stay focused and produce a change set like this. Maybe you used some refactoring tooling or search/replace, but even so, it's impressive. As a reviewer, I’m quite sure I have done a worse job. This kind of diff is hard to stay attentive to, and I haven’t verified every single repeated change in detail.
Secondly, given the potential for merge conflicts, it's probably best not to leave this PR open longer than necessary.
I understand why you want these refactors before applying rustfmt, but I think it’s important to acknowledge that many of these changes are a matter of personal preference rather than necessity. Some of them might even reduce readability, depending on who you ask.
There’s also some risk involved. As mentioned, this kind of work demands a lot of attention from both the author and reviewers. While the impact is lower for tests, I would be hesitant to apply a similar approach to non-test code. Fixing formatting issues in the normal flow of PRs feels much safer.
All that said, this work is done now, and despite the subjective aspects, I believe this PR is a net positive change. Still, it reinforces my opinion that for the rest of the codebase, we should move forward with #3749.
lightning/src/ln/payment_tests.rs
Outdated
| let params = route.route_params.clone().unwrap(); | ||
| let onion = RecipientOnionFields::spontaneous_empty(); | ||
| let retry = Retry::Attempts(0); | ||
| nodes[0].node.send_spontaneous_payment(preimage, onion, payment_id_0, params, retry).unwrap(); |
There was a problem hiding this comment.
Whether extractions into short var names is an improvement or not, is subjective. Especially with an IDE that whispers argument names, I prefer not to have the indirection through these short vars.
I am totally fine with what it was:
There was a problem hiding this comment.
Would agree with this, in some cases there's a loss of information in the new var name (e.g. removing _msat suffix, secret vs payment_secret) which doesn't seem worth it. It also makes it less clear that the variable isn't used elsewhere.
There was a problem hiding this comment.
There's quite possibly cases where its not worth it, but on the specific example you picked the test_reject_mpp_keysend_htlc_mismatching_secret test happens to fit almost exactly in my monitor (just by chance) whereas taking it from 4 to 10 LoC starts to shift things so that it no longer does (esp once you add the few other places in the test that similar changes happened). That changes it from very easy to read through the test and digest it to very hard (IME any amount of scrolling in a test makes it 2x more difficult to read, and probably another 2x on top because larger tests are also more complicated to begin with). In the specific case here, there's no loss in information as the variables are used one line later (and it arguably substantially improves scan-ability).
I don't really buy that the IDE hints you show add anything to test readability that is important to maintain - yes, we know (roughly) what type node is, and telling us that the parameter we're passing retry to is named retry (or preimage is named payment_preimage) is not exactly useful. The fact that send_spontaneous_payment returns a Result is also pretty obvious from the unwrap, what its failure or success types are is maybe the only useful information the IDE provided here, but I'd argue its not relevant to understanding the test.
Would agree with this, in some cases there's a loss of information in the new var name (e.g. removing _msat suffix, secret vs payment_secret) which doesn't seem worth it. It also makes it less clear that the variable isn't used elsewhere.
I'm definitely happy to discuss individual cases, removing msat is definitely sometimes an information loss, and there's probably some cases where the tradeoff wasn't right. As for removing the payment_ prefix (or our_payment_) in some cases, I don't really buy that it changes readability. If you have a test sending one payment with a hash, preimage, and secret variable, its very obvious what those are in the lightning context, I'm not really sure what else they could be.
There was a problem hiding this comment.
There is a lot of personal preference in this, so I don't think extensive discussion leads to much. If this was completely new code, either format would have been fine. But now that we are touching existing code out of context, I'd lean towards minimal changes.
There was a problem hiding this comment.
I'm not sure that there's a lot of personal preference in this specific instance? One might argue that its too dense at the end, but I don't really buy that (and its less dense than your IDE's hints!) but importantly its less to see if you mostly look at the left-hand-side - there's some variables set and then send_spontaneous_payment with the path its taking set a few lines up. This seems like an incredibly cut-and-dry improvement, though you might reasonably argue its not worth it.
There was a problem hiding this comment.
Indeed, I don't think it is worth it. And I don't like the indirection through the new vars. So not cut-an-dry for, to me at least.
There was a problem hiding this comment.
e.g. removing _msat suffix, secret vs payment_secret
These two cases in particular hurt readability IMO, noticed it independently while going through this commit before reaching this comment.
lightning/src/ln/payment_tests.rs
Outdated
| let third_persister; | ||
| let third_new_chain_monitor; | ||
| let persist_1; | ||
| let chain_1; |
There was a problem hiding this comment.
This might be considered loss of information / readability. It think it is valid to have a preference for more vertical layout instead.
There was a problem hiding this comment.
What specifically are you missing? The variables are just passed into reload_node! and not actually used anywhere in the test directly. persist_1 doesn't really change the amount of information (after all, its an implementation of the Persist trait, not a Persister at all), tho I agree that chain_monitor to chain is somewhat less clear, this particular test uses reload_node! three times, which results in a test that gets very hard to modify as reload_node! has a lot of parameters.
There was a problem hiding this comment.
Indeed, comment about info loss was directed at chain_1.
There was a problem hiding this comment.
Given the variables are totally unused in the test, it seems like a reasonable tradeoff, no?
There was a problem hiding this comment.
I mean, it's not that I can't live with this. Just wanted to note that it is a trade off in the end, with different people assigning different weights to both sides of the scale.
lightning/src/ln/payment_tests.rs
Outdated
| let secret = Some(payment_secret); | ||
| let path = &[&nodes[1]]; | ||
| pass_along_path(&nodes[0], path, amt_msat, payment_hash, secret, event, true, Some(preimage)); | ||
| claim_payment_along_route(ClaimAlongRouteArgs::new(&nodes[0], &[path], preimage)); |
There was a problem hiding this comment.
For this type PR, an alternative commit structure would be to first 'rustfmt' and then fix problems in a later commit. That way as a reviewer it's possible to see how the formatting was improved by looking at the diff.
For this review, I kept a local 'naively' rustfmt'ed copy on the side to glance at occassionally.
There was a problem hiding this comment.
Fair enough, can do it in the next one, but definitely hard to do now on this one :)
There was a problem hiding this comment.
I don't think it is hard to do if you first do a commit that rustfmt all the files that you want to include in this PR, and then perform a filter-branch similar to #3749 (comment).
But not necessary for this PR to avoid extra delay.
|
|
||
| let (temp_chan_id, tx, funding_output) = | ||
| create_funding_transaction(&nodes[0], &nodes[1].node.get_our_node_id(), 1_000_000, 42); | ||
| create_funding_transaction(&nodes[0], &node_b_id, 1_000_000, 42); |
There was a problem hiding this comment.
Mixing node numbers and letters isn't ideal I think. node_0_id etc would have been consistent.
There was a problem hiding this comment.
Hmm, yea, we already have a lot of mixing of the two, quite a few test comments refer to nodes by letters and we already use the node_letter_id form in a few newer tests/files. I'm not sure who started it, but I find node_1_id to be confusing as well because it implies to me that it refers to nodes[0] (ie the first node) whereas letters are clear in the meaning. I guess we could do nodes_1_id but that only somewhat resolves the issue.
There was a problem hiding this comment.
To me, just indexing everything zero-based seems most consistent, but I see your point.
There was a problem hiding this comment.
In going through more tests I noticed that we also very often have variables like as_commitment_signed referring to a CommitmentSigned message generated by nodes[0]. Given that it seems even more consistent, though if we want to go back and change those too we could, I guess.
There was a problem hiding this comment.
Not completely sure what you mean. That adding an index to as_commitment_signed would be more consistent?
I don't think we want to extend this PR further. This comment was just about replacing 0, 1 by 0, b in the same function call, and that it seems a step back in consistency. To make it right, maybe we need a node_a too then. Don't want to linger on this too long either.
There was a problem hiding this comment.
No, I meant just that referring to nodes[0] as "Node A" is consistent not only with the small handful of tests that already do the node_x_id thing (which we could just as easily change) but also with the many tests that store messages in variable names like xs_message. Thus, if we want to avoid the a-z naming and be consistent with the numerical ids it would be a substantial amount of work.
There was a problem hiding this comment.
I got it. I should read "a's commit sig".
In that case, mapping the nodes array into (node_a, node_b, etc) too is probably the way go if one wants consistency? (not now)
There was a problem hiding this comment.
In going through more tests I noticed that we also very often have variables like as_commitment_signed referring to a CommitmentSigned message generated by nodes[0]
I took a look at this reviewing a previous formatting PR and this convention seems to be used in the majority of cases (I specifically looked at all uses of commitment_signed and get_event_msg), so this change makes sense to me.
| assert!(nodes[0].node.create_channel(node_b_id, channel_value_satoshis, push_msat, 42, None, None).is_ok()); //Create a valid channel | ||
| let node0_to_1_send_open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, node_b_id); | ||
| assert!(node0_to_1_send_open_channel.channel_reserve_satoshis>=node0_to_1_send_open_channel.common_fields.dust_limit_satoshis); | ||
| nodes[0].node.create_channel(node_b_id, 100_000, 0, 42, None, None).unwrap(); |
There was a problem hiding this comment.
Explain in the commit message why it is dead code?
There was a problem hiding this comment.
I don't think an explanation was added yet, but still curious to find out. Can maybe piece that together via the spec, but you can probably explain it quickly in a few sentences.
There was a problem hiding this comment.
Oh, sorry, I did rephrase the commit to remove references to the concept of the code being "dead". Its not dead in the sense that the code is unreachable, only dead in the sense that the code isn't usefully testing anything interesting.
There was a problem hiding this comment.
I was after why that code isn't usefully testing anything interesting. For example, why isn't testing 'Sending node must set funding_satoshis to less than 2^24 satoshis' useful?
There was a problem hiding this comment.
FWIW, it looks like we have test coverage of this check elsewhere (I added a panic to the relevant Channel check).
lightning/src/ln/update_fee_tests.rs
Outdated
| @@ -62,7 +62,7 @@ pub fn test_async_inbound_update_fee() { | |||
| *feerate_lock += 20; | |||
| } | |||
| nodes[0].node.timer_tick_occurred(); | |||
| check_added_monitors!(nodes[0], 1); | |||
| check_added_monitors(&nodes[0], 1); | |||
There was a problem hiding this comment.
It's nice to de-macro, but doing it in a separate PR would've been fine too. Just pre-formatting is a lot already.
There was a problem hiding this comment.
I can drop it if you prefer, I was looking at whether it changed compile times (fewer macros to process), but it wasn't a big win. Happy to go either way here, just lmk.
There was a problem hiding this comment.
No need to change anything now. Comment was directed at future work mostly.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
|
✅ Added second reviewer: @valentinewallace |
|
Needs rebase since #3700 landed |
lightning/src/ln/payment_tests.rs
Outdated
| let params = route.route_params.clone().unwrap(); | ||
| let onion = RecipientOnionFields::spontaneous_empty(); | ||
| let retry = Retry::Attempts(0); | ||
| nodes[0].node.send_spontaneous_payment(preimage, onion, payment_id_0, params, retry).unwrap(); |
There was a problem hiding this comment.
Would agree with this, in some cases there's a loss of information in the new var name (e.g. removing _msat suffix, secret vs payment_secret) which doesn't seem worth it. It also makes it less clear that the variable isn't used elsewhere.
TheBlueMatt
force-pushed
the
2025-04-rustfmt-tests-1
branch
from
820cb58 to
7512e48
Compare
|
Rebased and addressed feedback. |
|
FYI, looks like fixup commits weren't added prior to the rebase so it's hard to see what changed. I'm also generally fine with squashing in changes if the rebase is a separate push. |
TheBlueMatt
force-pushed
the
2025-04-rustfmt-tests-1
branch
2 times, most recently
from
ab103e6 to
08f17ab
Compare
| assert!(nodes[0].node.create_channel(node_b_id, channel_value_satoshis, push_msat, 42, None, None).is_ok()); //Create a valid channel | ||
| let node0_to_1_send_open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, node_b_id); | ||
| assert!(node0_to_1_send_open_channel.channel_reserve_satoshis>=node0_to_1_send_open_channel.common_fields.dust_limit_satoshis); | ||
| nodes[0].node.create_channel(node_b_id, 100_000, 0, 42, None, None).unwrap(); |
There was a problem hiding this comment.
I don't think an explanation was added yet, but still curious to find out. Can maybe piece that together via the spec, but you can probably explain it quickly in a few sentences.
`functional_tests.rs` has gotten incredibly huge over the years, so here we move some channel reserve tests into a new file.
`funtional_tests.rs` has gotten incredibly huge over the years, so here we move some further channel reserve tests into a new file. We do so separately from the previous commit to ensure git identifies the changes as move-only.
`funtional_tests.rs` has gotten incredibly huge over the years, so here we move some simple HTLC unit tests into a new file.
`funtional_tests.rs` has gotten incredibly huge over the years, so here we move all the channel fee-related tests out to their own file.
In general we shouldn't have tests lying around that are useless as its not genuinely testing anything and may break due to harmless protocol changes. Here, we drop a few useless tests.
TheBlueMatt
force-pushed
the
2025-04-rustfmt-tests-1
branch
from
08f17ab to
200327d
Compare
|
Rebased.
Apologies not quite sure why I got overly eager and did that, but the full diff from a fresh naive rebase since the first push in this commit to the current contents follows (note that some of the diff is just different variable names picked during the rebase-redo). diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index ab79aa70a..252958035 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -31,9 +31,7 @@ use crate::ln::chan_utils::{
};
use crate::ln::channel::{
- get_holder_selected_channel_reserve_satoshis, Channel, InboundV1Channel, OutboundV1Channel,
- COINBASE_MATURITY,
-};
-use crate::ln::channel::{
- ChannelError, DISCONNECT_PEER_AWAITING_RESPONSE_TICKS, MIN_CHAN_DUST_LIMIT_SATOSHIS,
+ get_holder_selected_channel_reserve_satoshis, Channel, ChannelError, InboundV1Channel,
+ OutboundV1Channel, COINBASE_MATURITY, DISCONNECT_PEER_AWAITING_RESPONSE_TICKS,
+ MIN_CHAN_DUST_LIMIT_SATOSHIS,
};
use crate::ln::channelmanager::{
@@ -775,14 +773,11 @@ fn do_test_fail_back_before_backwards_timeout(post_fail_back_action: PostFailBac
let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
- let node_a_id = nodes[0].node.get_our_node_id();
- let node_b_id = nodes[1].node.get_our_node_id();
- let node_c_id = nodes[2].node.get_our_node_id();
-
for node in nodes.iter() {
*node.fee_estimator.sat_per_kw.lock().unwrap() = 2000;
}
- let node_b_id = node_b_id;
- let node_c_id = node_c_id;
+ let node_a_id = nodes[0].node.get_our_node_id();
+ let node_b_id = nodes[1].node.get_our_node_id();
+ let node_c_id = nodes[2].node.get_our_node_id();
create_announced_chan_between_nodes(&nodes, 0, 1);
@@ -3741,7 +3736,7 @@ fn do_test_holding_cell_htlc_add_timeouts(forwarded_htlc: bool) {
if forwarded_htlc {
- let failure =
+ let fail_type =
HTLCHandlingFailureType::Forward { node_id: Some(node_c_id), channel_id: chan_2.2 };
- expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], vec![failure]);
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], [fail_type]);
check_added_monitors(&nodes[1], 1);
let fail_commit = nodes[1].node.get_and_clear_pending_msg_events();
@@ -9320,12 +9315,10 @@ fn do_test_tx_confirmed_skipping_blocks_immediate_broadcast(test_height_before_t
let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+ *nodes[0].connect_style.borrow_mut() = ConnectStyle::BestBlockFirstSkippingBlocks;
+
let node_a_id = nodes[0].node.get_our_node_id();
let node_b_id = nodes[1].node.get_our_node_id();
let node_c_id = nodes[2].node.get_our_node_id();
- *nodes[0].connect_style.borrow_mut() = ConnectStyle::BestBlockFirstSkippingBlocks;
-
- let node_c_id = node_c_id;
-
create_announced_chan_between_nodes(&nodes, 0, 1);
let (chan_announce, _, channel_id, _) = create_announced_chan_between_nodes(&nodes, 1, 2);
@@ -9647,8 +9640,6 @@ pub fn test_inconsistent_mpp_params() {
expect_pending_htlcs_forwardable_ignore!(nodes[3]);
nodes[3].node.process_pending_htlc_forwards();
- expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!(
- nodes[3],
- vec![HTLCHandlingFailureType::Receive { payment_hash: hash }]
- );
+ let fail_type = HTLCHandlingFailureType::Receive { payment_hash: hash };
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!(nodes[3], [fail_type]);
nodes[3].node.process_pending_htlc_forwards();
@@ -10717,6 +10708,6 @@ fn do_payment_with_custom_min_final_cltv_expiry(valid_delta: bool, use_user_hash
claim_payment(&nodes[0], &[&nodes[1]], payment_preimage);
} else {
- let failure = HTLCHandlingFailureType::Receive { payment_hash: hash };
- expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], vec![failure]);
+ let fail_type = HTLCHandlingFailureType::Receive { payment_hash: hash };
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], [fail_type]);
check_added_monitors(&nodes[1], 1);
diff --git a/lightning/src/ln/htlc_reserve_unit_tests.rs b/lightning/src/ln/htlc_reserve_unit_tests.rs
index 16de71b0e..aee764682 100644
--- a/lightning/src/ln/htlc_reserve_unit_tests.rs
+++ b/lightning/src/ln/htlc_reserve_unit_tests.rs
@@ -1,5 +1,5 @@
//! Various unit tests covering HTLC handling as well as tests covering channel reserve tracking.
-use crate::events::{ClosureReason, Event, HTLCDestination, PaymentPurpose};
+use crate::events::{ClosureReason, Event, HTLCHandlingFailureType, PaymentPurpose};
use crate::ln::chan_utils::{
self, commitment_tx_base_weight, htlc_success_tx_weight, CommitmentTransaction,
@@ -676,7 +676,6 @@ pub fn holding_cell_htlc_counting() {
// fails), the second will process the resulting failure and fail the HTLC backward.
expect_pending_htlcs_forwardable!(nodes[1]);
- let failure =
- HTLCHandlingFailureType::Forward { node_id: Some(node_c_id), channel_id: chan_2.2 };
- expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], vec![failure]);
+ let fail = HTLCHandlingFailureType::Forward { node_id: Some(node_c_id), channel_id: chan_2.2 };
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], vec![fail]);
check_added_monitors(&nodes[1], 1);
diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs
index eb06df55c..b185e7395 100644
--- a/lightning/src/ln/payment_tests.rs
+++ b/lightning/src/ln/payment_tests.rs
@@ -160,7 +160,6 @@ fn mpp_retry() {
// Attempt to forward the payment and complete the 2nd path's failure.
expect_pending_htlcs_forwardable!(&nodes[2]);
- let failure =
- HTLCHandlingFailureType::Forward { node_id: Some(node_d_id), channel_id: chan_4_id };
- expect_pending_htlcs_forwardable_and_htlc_handling_failed!(&nodes[2], vec![failure]);
+ let fail = HTLCHandlingFailureType::Forward { node_id: Some(node_d_id), channel_id: chan_4_id };
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed!(&nodes[2], [fail]);
let htlc_updates = get_htlc_update_msgs!(nodes[2], node_a_id);
assert!(htlc_updates.update_add_htlcs.is_empty());
@@ -283,7 +282,6 @@ fn mpp_retry_overpay() {
// Attempt to forward the payment and complete the 2nd path's failure.
expect_pending_htlcs_forwardable!(&nodes[2]);
- let failure =
- HTLCHandlingFailureType::Forward { node_id: Some(node_d_id), channel_id: chan_4_id };
- expect_pending_htlcs_forwardable_and_htlc_handling_failed!(&nodes[2], vec![failure]);
+ let fail = HTLCHandlingFailureType::Forward { node_id: Some(node_d_id), channel_id: chan_4_id };
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed!(&nodes[2], [fail]);
let htlc_updates = get_htlc_update_msgs!(nodes[2], node_a_id);
@@ -387,8 +385,8 @@ fn do_mpp_receive_timeout(send_partial_mpp: bool) {
// Failed HTLC from node 3 -> 1
- let failure = HTLCHandlingFailureType::Receive { payment_hash };
- expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[3], vec![failure]);
+ let fail = HTLCHandlingFailureType::Receive { payment_hash: hash };
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[3], [fail]);
- let htlc_fail_updates = get_htlc_update_msgs!(nodes[3], nodes[1].node.get_our_node_id());
+ let htlc_fail_updates = get_htlc_update_msgs!(nodes[3], node_b_id);
assert_eq!(htlc_fail_updates.update_fail_htlcs.len(), 1);
nodes[1].node.handle_update_fail_htlc(node_d_id, &htlc_fail_updates.update_fail_htlcs[0]);
@@ -398,7 +396,7 @@ fn do_mpp_receive_timeout(send_partial_mpp: bool) {
// Failed HTLC from node 1 -> 0
- let failure =
+ let fail_type =
HTLCHandlingFailureType::Forward { node_id: Some(node_d_id), channel_id: chan_3_id };
- expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], vec![failure]);
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], [fail_type]);
let htlc_fail_updates = get_htlc_update_msgs!(nodes[1], node_a_id);
@@ -550,5 +548,5 @@ fn test_reject_mpp_keysend_htlc_mismatching_secret() {
let chan_2_id = create_announced_chan_between_nodes(&nodes, 0, 2).0.contents.short_channel_id;
let chan_3_id = create_announced_chan_between_nodes(&nodes, 1, 3).0.contents.short_channel_id;
- let (update_a, _, chan_4_channel_id, _) = create_announced_chan_between_nodes(&nodes, 2, 3);
+ let (update_a, _, chan_4_chan_id, _) = create_announced_chan_between_nodes(&nodes, 2, 3);
let chan_4_id = update_a.contents.short_channel_id;
let amount = 40_000;
@@ -655,6 +653,6 @@ fn test_reject_mpp_keysend_htlc_mismatching_secret() {
}
nodes[3].node.process_pending_htlc_forwards();
- let failure = HTLCHandlingFailureType::Receive { payment_hash };
- expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[3], vec![failure]);
+ let fail_type = HTLCHandlingFailureType::Receive { payment_hash };
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[3], [fail_type]);
check_added_monitors!(nodes[3], 1);
@@ -663,7 +661,8 @@ fn test_reject_mpp_keysend_htlc_mismatching_secret() {
nodes[2].node.handle_update_fail_htlc(node_d_id, &update_fail_0.update_fail_htlcs[0]);
commitment_signed_dance!(nodes[2], nodes[3], update_fail_0.commitment_signed, false);
- let failure =
- HTLCHandlingFailureType::Forward { node_id: Some(node_d_id), channel_id: chan_4_channel_id };
- expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[2], vec![failure]);
+
+ let fail_type =
+ HTLCHandlingFailureType::Forward { node_id: Some(node_d_id), channel_id: chan_4_chan_id };
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[2], [fail_type]);
check_added_monitors!(nodes[2], 1);
@@ -765,7 +764,11 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) {
nodes[1].node.handle_update_add_htlc(node_a_id, &payment_event.msgs[0]);
commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false, true);
+
expect_pending_htlcs_forwardable!(nodes[1]);
- let hop = &HTLCDestination::NextHopChannel { node_id: Some(node_c_id), channel_id: chan_id_2 };
- expect_htlc_handling_failed_destinations!(nodes[1].node.get_and_clear_pending_events(), &[hop]);
+ expect_htlc_handling_failed_destinations!(
+ nodes[1].node.get_and_clear_pending_events(),
+ [HTLCHandlingFailureType::Forward { node_id: Some(node_c_id), channel_id: chan_id_2 }]
+ );
+
check_added_monitors(&nodes[1], 1);
// nodes[1] now immediately fails the HTLC as the next-hop channel is disconnected
@@ -1049,7 +1052,7 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) {
// previous hop channel is already on-chain, but it makes nodes[2] willing to see additional
// incoming HTLCs with the same payment hash later.
- nodes[2].node.fail_htlc_backwards(&payment_hash);
- let failure = HTLCHandlingFailureType::Receive { payment_hash };
- expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[2], [failure]);
+ nodes[2].node.fail_htlc_backwards(&hash);
+ let fail_type = HTLCHandlingFailureType::Receive { payment_hash: hash };
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[2], [fail_type]);
check_added_monitors!(nodes[2], 1);
@@ -1355,7 +1358,8 @@ fn test_fulfill_restart_failure() {
nodes[1].node.fail_htlc_backwards(&payment_hash);
- let failure = HTLCHandlingFailureType::Receive { payment_hash };
- expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], vec![failure]);
+ let fail_type = HTLCHandlingFailureType::Receive { payment_hash };
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], [fail_type]);
check_added_monitors!(nodes[1], 1);
+
let htlc_fail_updates = get_htlc_update_msgs!(nodes[1], node_a_id);
nodes[0].node.handle_update_fail_htlc(node_b_id, &htlc_fail_updates.update_fail_htlcs[0]);
@@ -1863,6 +1867,6 @@ fn abandoned_send_payment_idempotent() {
nodes[1].node.fail_htlc_backwards(&first_payment_hash);
- let failure = HTLCHandlingFailureType::Receive { payment_hash: first_payment_hash };
- expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], [failure]);
+ let fail_type = HTLCHandlingFailureType::Receive { payment_hash: first_payment_hash };
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], [fail_type]);
// Until we abandon the payment upon path failure, no matter how many timer ticks pass, we still cannot reuse the
@@ -2191,8 +2195,7 @@ fn do_test_intercepted_payment(test: InterceptTest) {
// Ensure we can fail the intercepted payment back.
nodes[1].node.fail_intercepted_htlc(intercept_id).unwrap();
- expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!(
- nodes[1],
- vec![HTLCHandlingFailureType::InvalidForward { requested_forward_scid: intercept_scid }]
- );
+ let fail =
+ HTLCHandlingFailureType::InvalidForward { requested_forward_scid: intercept_scid };
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!(nodes[1], [fail]);
nodes[1].node.process_pending_htlc_forwards();
let update_fail = get_htlc_update_msgs!(nodes[1], node_a_id);
@@ -2211,10 +2214,10 @@ fn do_test_intercepted_payment(test: InterceptTest) {
} else if test == InterceptTest::Forward {
// Check that we'll fail as expected when sending to a channel that isn't in `ChannelReady` yet.
- let chan_id = nodes[1].node.create_channel(node_c_id, 100_000, 0, 42, None, None).unwrap();
+ let temp_id = nodes[1].node.create_channel(node_c_id, 100_000, 0, 42, None, None).unwrap();
let unusable_chan_err =
- nodes[1].node.forward_intercepted_htlc(intercept_id, &chan_id, node_c_id, outbound_amt);
+ nodes[1].node.forward_intercepted_htlc(intercept_id, &temp_id, node_c_id, outbound_amt);
let err = format!(
"Channel with id {} for the passed counterparty node_id {} is still opening.",
- chan_id, node_c_id,
+ temp_id, node_c_id,
);
assert_eq!(unusable_chan_err, Err(APIError::ChannelUnavailable { err }));
@@ -2277,7 +2280,7 @@ fn do_test_intercepted_payment(test: InterceptTest) {
connect_block(&nodes[1], &block);
}
- let failure =
+ let fail_type =
HTLCHandlingFailureType::InvalidForward { requested_forward_scid: intercept_scid };
- expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], vec![failure]);
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], [fail_type]);
check_added_monitors!(nodes[1], 1);
@@ -2540,5 +2543,5 @@ fn do_automatic_retries(test: AutoRetry) {
nodes[1].node.process_pending_htlc_forwards();
expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!(nodes[1],
- vec![HTLCHandlingFailureType::Forward {
+ [HTLCHandlingFailureType::Forward {
node_id: Some(node_c_id),
channel_id: $failing_channel_id,
@@ -3095,7 +3098,6 @@ fn fails_paying_after_rejected_by_payee() {
nodes[1].node.fail_htlc_backwards(&payment_hash);
-
- let failure = HTLCHandlingFailureType::Receive { payment_hash };
- expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], [failure]);
+ let fail_type = HTLCHandlingFailureType::Receive { payment_hash };
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], [fail_type]);
let reason = PaymentFailureReason::RecipientRejected;
pass_failed_payment_back(&nodes[0], &[&[&nodes[1]]], false, payment_hash, reason);
@@ -4349,10 +4351,6 @@ fn do_test_custom_tlvs(spontaneous: bool, even_tlvs: bool, known_tlvs: bool) {
(false, true) => {
nodes[1].node.claim_funds(preimage);
- let expected_destinations =
- vec![HTLCHandlingFailureType::Receive { payment_hash: hash }];
- expect_pending_htlcs_forwardable_and_htlc_handling_failed!(
- nodes[1],
- expected_destinations
- );
+ let fail_type = HTLCHandlingFailureType::Receive { payment_hash: hash };
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], [fail_type]);
let reason = PaymentFailureReason::RecipientRejected;
pass_failed_payment_back(&nodes[0], &[&[&nodes[1]]], false, hash, reason);
@@ -4404,7 +4402,6 @@ fn test_retry_custom_tlvs() {
// Attempt to forward the payment and complete the path's failure.
expect_pending_htlcs_forwardable!(&nodes[1]);
- let failure =
- HTLCHandlingFailureType::Forward { node_id: Some(node_c_id), channel_id: chan_2_id };
- expect_pending_htlcs_forwardable_and_htlc_handling_failed!(&nodes[1], vec![failure]);
+ let fail = HTLCHandlingFailureType::Forward { node_id: Some(node_c_id), channel_id: chan_2_id };
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed!(&nodes[1], [fail]);
check_added_monitors!(nodes[1], 1);
@@ -4599,5 +4596,5 @@ fn do_test_custom_tlvs_consistency(
} else {
// Expect fail back
- let expected_destinations = vec![HTLCHandlingFailureType::Receive { payment_hash: hash }];
+ let expected_destinations = [HTLCHandlingFailureType::Receive { payment_hash: hash }];
expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[3], expected_destinations);
check_added_monitors!(nodes[3], 1);
@@ -4607,11 +4604,7 @@ fn do_test_custom_tlvs_consistency(
commitment_signed_dance!(nodes[2], nodes[3], fail_updates_1.commitment_signed, false);
- expect_pending_htlcs_forwardable_and_htlc_handling_failed!(
- nodes[2],
- vec![HTLCHandlingFailureType::Forward {
- node_id: Some(node_d_id),
- channel_id: chan_2_3.2
- }]
- );
+ let fail =
+ HTLCHandlingFailureType::Forward { node_id: Some(node_d_id), channel_id: chan_2_3.2 };
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[2], [fail]);
check_added_monitors!(nodes[2], 1);
@@ -4774,5 +4767,5 @@ fn do_test_payment_metadata_consistency(do_reload: bool, do_modify: bool) {
expect_pending_htlcs_forwardable_conditions(
nodes[3].node.get_and_clear_pending_events(),
- &[HTLCHandlingFailureType::Receive {payment_hash}],
+ &[HTLCHandlingFailureType::Receive { payment_hash }],
);
nodes[3].node.process_pending_htlc_forwards();
@@ -4783,8 +4776,8 @@ fn do_test_payment_metadata_consistency(do_reload: bool, do_modify: bool) {
nodes[2].node.handle_update_fail_htlc(node_d_id, &ds_fail.update_fail_htlcs[0]);
commitment_signed_dance!(nodes[2], nodes[3], ds_fail.commitment_signed, false, true);
- expect_pending_htlcs_forwardable_conditions(
- nodes[2].node.get_and_clear_pending_events(),
- &[HTLCHandlingFailureType::Forward { node_id: Some(node_d_id), channel_id: cd_channel_used }],
- );
+ let events = nodes[2].node.get_and_clear_pending_events();
+ let fail_type =
+ HTLCHandlingFailureType::Forward { node_id: Some(node_d_id), channel_id: cd_chan_id };
+ expect_pending_htlcs_forwardable_conditions(events, &[fail_type]);
} else {
expect_pending_htlcs_forwardable!(nodes[3]);
@@ -4888,7 +4881,6 @@ fn test_htlc_forward_considers_anchor_outputs_value() {
// The forwarding node should reject forwarding it as expected.
expect_pending_htlcs_forwardable!(nodes[1]);
- let failure =
- HTLCHandlingFailureType::Forward { node_id: Some(node_c_id), channel_id: chan_id_2 };
- expect_pending_htlcs_forwardable_and_htlc_handling_failed!(&nodes[1], vec![failure]);
+ let fail = HTLCHandlingFailureType::Forward { node_id: Some(node_c_id), channel_id: chan_id_2 };
+ expect_pending_htlcs_forwardable_and_htlc_handling_failed!(&nodes[1], [fail]);
check_added_monitors(&nodes[1], 1);
@@ -5086,8 +5078,8 @@ fn test_non_strict_forwarding() {
};
// The failure to forward will refer to the channel given in the onion.
- expect_pending_htlcs_forwardable_conditions(
- nodes[1].node.get_and_clear_pending_events(),
- &[HTLCHandlingFailureType::Forward { node_id: Some(node_c_id), channel_id: routed_channel_id }],
- );
+ let events = nodes[1].node.get_and_clear_pending_events();
+ let fail =
+ HTLCHandlingFailureType::Forward { node_id: Some(node_c_id), channel_id: routed_chan_id };
+ expect_pending_htlcs_forwardable_conditions(events, &[fail]);
let updates = get_htlc_update_msgs!(nodes[1], node_a_id);
@@ -5242,7 +5234,6 @@ fn max_out_mpp_path() {
let chanmon_cfgs = create_chanmon_cfgs(3);
let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
- let node_chanmgrs = create_node_chanmgrs(
- 3, &node_cfgs, &[Some(user_cfg.clone()), Some(lsp_cfg.clone()), Some(user_cfg.clone())]
- );
+ let configs = [Some(user_cfg.clone()), Some(lsp_cfg), Some(user_cfg)];
+ let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &configs);
let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
@@ -5259,5 +5250,7 @@ fn max_out_mpp_path() {
let route_params_cfg = crate::routing::router::RouteParametersConfig::default();
- nodes[0].node.pay_for_bolt11_invoice(&invoice, PaymentId([42; 32]), None, route_params_cfg, Retry::Attempts(0)).unwrap();
+ let id = PaymentId([42; 32]);
+ let retry = Retry::Attempts(0);
+ nodes[0].node.pay_for_bolt11_invoice(&invoice, id, None, route_params_cfg, retry).unwrap();
assert!(nodes[0].node.list_recent_payments().len() == 1); |
| assert!(nodes[0].node.create_channel(node_b_id, channel_value_satoshis, push_msat, 42, None, None).is_ok()); //Create a valid channel | ||
| let node0_to_1_send_open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, node_b_id); | ||
| assert!(node0_to_1_send_open_channel.channel_reserve_satoshis>=node0_to_1_send_open_channel.common_fields.dust_limit_satoshis); | ||
| nodes[0].node.create_channel(node_b_id, 100_000, 0, 42, None, None).unwrap(); |
There was a problem hiding this comment.
I was after why that code isn't usefully testing anything interesting. For example, why isn't testing 'Sending node must set funding_satoshis to less than 2^24 satoshis' useful?
lightning/src/ln/payment_tests.rs
Outdated
| @@ -184,14 +193,16 @@ fn mpp_retry_overpay() { | |||
| // in the first attempt. | |||
| let chanmon_cfgs = create_chanmon_cfgs(4); | |||
| let node_cfgs = create_node_cfgs(4, &chanmon_cfgs); | |||
|
|
|||
There was a problem hiding this comment.
nit: unnecessary newline add?
lightning/src/ln/payment_tests.rs
Outdated
| let params = route.route_params.clone().unwrap(); | ||
| let onion = RecipientOnionFields::spontaneous_empty(); | ||
| let retry = Retry::Attempts(0); | ||
| nodes[0].node.send_spontaneous_payment(preimage, onion, payment_id_0, params, retry).unwrap(); |
There was a problem hiding this comment.
e.g. removing _msat suffix, secret vs payment_secret
These two cases in particular hurt readability IMO, noticed it independently while going through this commit before reaching this comment.
lightning/src/ln/payment_tests.rs
Outdated
| @@ -1081,24 +1144,25 @@ fn do_test_dup_htlc_onchain_doesnt_fail_on_reload(persist_manager_post_event: bo | |||
| let chanmon_cfgs = create_chanmon_cfgs(2); | |||
| let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); | |||
| let persister; | |||
| let new_chain_monitor; | |||
| let chain; | |||
There was a problem hiding this comment.
chain_reload to be consistent with other var names used with reload?
Here + a few other instances.
|
|
||
| let (temp_chan_id, tx, funding_output) = | ||
| create_funding_transaction(&nodes[0], &nodes[1].node.get_our_node_id(), 1_000_000, 42); | ||
| create_funding_transaction(&nodes[0], &node_b_id, 1_000_000, 42); |
There was a problem hiding this comment.
In going through more tests I noticed that we also very often have variables like as_commitment_signed referring to a CommitmentSigned message generated by nodes[0]
I took a look at this reviewing a previous formatting PR and this convention seems to be used in the majority of cases (I specifically looked at all uses of commitment_signed and get_event_msg), so this change makes sense to me.
lightning/src/ln/functional_tests.rs
Outdated
| @@ -4667,27 +4667,15 @@ pub fn bolt2_open_channel_sending_node_checks_part2() { | |||
|
|
|||
| let node_b_id = nodes[1].node.get_our_node_id(); | |||
|
|
|||
| // BOLT #2 spec: Sending node must set funding_satoshis to less than 2^24 satoshis | |||
There was a problem hiding this comment.
Why is this test useless? BOLT 02 does require the receiver to reject channels that go over this limit so seems reasonable enough to keep?
| let node_chanmgrs = create_node_chanmgrs(6, &node_cfgs, | ||
| &[Some(config.clone()), Some(config.clone()), Some(config.clone()), Some(config.clone()), Some(config.clone()), Some(config.clone())]); | ||
|
|
||
| let configs = [ |
There was a problem hiding this comment.
let configs = [Some(config.clone()].repeat(6)?
functional_tests' time has come, and there's some easy batches of tests to move into separate files. Even at the end its still 10k LoC, but that's better than the 14K it started as (let alone how long it'd be if it were naively rustfmt'd).