Add Test for same-payment_hash-different-CLTV HTLC sorting

[TheBlueMatt]

This was fixed in #319 but no test was added.

[TheBlueMatt]

If there arent some already, this should probably also come with BOLT test vectors.

[ariard]

@rrybarczyk, this one should be a good first issue if you're looking for something to hack on :)

[varunsrin]

i can pick this up, if its still relevant

[ariard]

@varunsrin Yes this one is still relevant. You should check duplicate_htlc_test, test_duplicate_htlc_different_direction_onchain, test_duplicate_payment_hash_one_failure_one_success but IIRC they don't test this general case.

Also you may have a look on https://github.com/rust-bitcoin/rust-lightning/issues/240, going through bolt 2 and checking we implement everything right you will surely find nice tests to write. And beyond learn about the core operations of LN.

(going to label "good first issue" more this week, also you can grep really easy TODOs like L773 in peer_handler or others :) )

[Harshdev098]

Hey @TheBlueMatt is this issue still exist, I want to contribute to it!

[Harshdev098]

@TheBlueMatt In case if not present, can you please assign me some good first issue to understand more about the project as a beginner. Although I have contributed to many web2 organizations, but its my first time to contribute to such blockchain based project!

[TheBlueMatt]

Looks like we indeed do not have any test coverage directly for this (the following patch does not result in any test failures in general channel logic).

diff --git a/lightning/src/util/transaction_utils.rs b/lightning/src/util/transaction_utils.rs
index 5a77fbf06..a0517cecc 100644
--- a/lightning/src/util/transaction_utils.rs
+++ b/lightning/src/util/transaction_utils.rs
@@ -22,7 +22,7 @@ use core::cmp::Ordering;
 pub fn sort_outputs<T, C: Fn(&T, &T) -> Ordering>(outputs: &mut Vec<(TxOut, T)>, tie_breaker: C) {
        outputs.sort_unstable_by(|a, b| {
                a.0.value.cmp(&b.0.value).then_with(|| {
-                       a.0.script_pubkey[..].cmp(&b.0.script_pubkey[..]).then_with(|| tie_breaker(&a.1, &b.1))
+                       a.0.script_pubkey[..].cmp(&b.0.script_pubkey[..]).then_with(|| tie_breaker(&a.1, &b.1).reverse())
                })
        });
 }

[Harshdev098]

Hey @TheBlueMatt , Can you please give some context or reference how I can get cltv from commitment tx

[TheBlueMatt]

The CLTV should be calculable from the route.