Skip to content

Fix HTLC-output-in-commitment sorting for duplicate-HTLCs #319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 25, 2019
Merged

Fix HTLC-output-in-commitment sorting for duplicate-HTLCs #319

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d990f72
Fix HTLC-output-in-commitment sorting for duplicate-HTLCs
TheBlueMatt Mar 7, 2019
a085488
Remove unused BIP69 input sorting
TheBlueMatt Mar 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -934,7 +934,19 @@ impl Channel {
}, None));
}

transaction_utils::sort_outputs(&mut txouts);
transaction_utils::sort_outputs(&mut txouts, |a, b| {
if let &Some(ref a_htlc) = a {
if let &Some(ref b_htlc) = b {
a_htlc.0.cltv_expiry.cmp(&b_htlc.0.cltv_expiry)
// Note that due to hash collisions, we have to have a fallback comparison
// here for fuzztarget mode (otherwise at least chanmon_fail_consistency
// may fail)!
.then(a_htlc.0.payment_hash.0.cmp(&b_htlc.0.payment_hash.0))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm didn't get you there, if offered HTLC A and offered HTLC B have same pubkey script, isn't it implied that they have same payment hash, as per BOLT-3, hash is part of script and it's the only diff for HTLCs with same per commitment point ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in fuzztarget, in fuzztarget hashes are just (roughly) the whole input xor'd together, with 31 bytes being set to 0s. Thus, its rather trivial to have different payment hashes have the same script_pubkey.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay make sense

// For non-HTLC outputs, if they're copying our SPK we don't really care if we
// close the channel due to mismatches - they're doing something dumb:
} else { cmp::Ordering::Equal }
} else { cmp::Ordering::Equal }
});

let mut outputs: Vec<TxOut> = Vec::with_capacity(txouts.len());
let mut htlcs_included: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(txouts.len() + included_dust_htlcs.len());
Expand Down Expand Up @@ -1010,7 +1022,7 @@ impl Channel {
}, ()));
}

transaction_utils::sort_outputs(&mut txouts);
transaction_utils::sort_outputs(&mut txouts, |_, _| { cmp::Ordering::Equal }); // Ordering doesnt matter if they used our pubkey...

let mut outputs: Vec<TxOut> = Vec::new();
for out in txouts.drain(..) {
Expand Down
18 changes: 10 additions & 8 deletions src/util/transaction_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@ use bitcoin_hashes::sha256d::Hash as Sha256dHash;

use std::cmp::Ordering;

pub fn sort_outputs<T>(outputs: &mut Vec<(TxOut, T)>) {
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(
a.0.script_pubkey[..].cmp(&b.0.script_pubkey[..])
)
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)
})
})
});
}

Expand Down Expand Up @@ -58,7 +60,7 @@ mod tests {
let txout2_ = txout2.clone();

let mut outputs = vec![(txout1, "ignore"), (txout2, "ignore")];
sort_outputs(&mut outputs);
sort_outputs(&mut outputs, |_, _| { unreachable!(); });

assert_eq!(
&outputs,
Expand All @@ -81,7 +83,7 @@ mod tests {
let txout2_ = txout2.clone();

let mut outputs = vec![(txout1, "ignore"), (txout2, "ignore")];
sort_outputs(&mut outputs);
sort_outputs(&mut outputs, |_, _| { unreachable!(); });

assert_eq!(
&outputs,
Expand All @@ -105,7 +107,7 @@ mod tests {
let txout2_ = txout2.clone();

let mut outputs = vec![(txout1, "ignore"), (txout2, "ignore")];
sort_outputs(&mut outputs);
sort_outputs(&mut outputs, |_, _| { unreachable!(); });

assert_eq!(&outputs, &vec![(txout1_, "ignore"), (txout2_, "ignore")]);
}
Expand All @@ -131,7 +133,7 @@ mod tests {
outputs.reverse(); // prep it

// actually do the work!
sort_outputs(&mut outputs);
sort_outputs(&mut outputs, |_, _| { unreachable!(); });

assert_eq!(outputs, expected);
}
Expand Down