Skip to content

Move fail-backwards up for no to-remote output claims #280

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 5 commits into from
Jan 5, 2019
Merged

Move fail-backwards up for no to-remote output claims #280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2457734
Move fail-backwards up for no to-remote output claims
TheBlueMatt Dec 21, 2018
dbbf491
Add dust mode to test_commitment_revoked_fail_backward_exhaustive
TheBlueMatt Dec 27, 2018
e344b0a
Add test_commitment_revoked_fail_backward_exhaustive no_to_remote
TheBlueMatt Dec 27, 2018
913d56e
Macro-DRY HTLC back-fails for revoked remote transaction broadcast
TheBlueMatt Dec 27, 2018
e662fa1
Fix stale comment in functional_tests after failing back async
TheBlueMatt Jan 5, 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
48 changes: 24 additions & 24 deletions src/ln/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1179,11 +1179,34 @@ impl ChannelMonitor {
}
}

if !inputs.is_empty() || !txn_to_broadcast.is_empty() { // ie we're confident this is actually ours
if !inputs.is_empty() || !txn_to_broadcast.is_empty() || per_commitment_option.is_some() { // ie we're confident this is actually ours
// We're definitely a remote commitment transaction!
log_trace!(self, "Got broadcast of revoked remote commitment transaction, generating general spend tx with {} inputs and {} other txn to broadcast", inputs.len(), txn_to_broadcast.len());
watch_outputs.append(&mut tx.output.clone());
self.remote_commitment_txn_on_chain.insert(commitment_txid, (commitment_number, tx.output.iter().map(|output| { output.script_pubkey.clone() }).collect()));

// TODO: We really should only fail backwards after our revocation claims have been
// confirmed, but we also need to do more other tracking of in-flight pre-confirm
// on-chain claims, so we can do that at the same time.
macro_rules! check_htlc_fails {
($txid: expr, $commitment_tx: expr) => {
if let Some(&(_, ref outpoints)) = self.remote_claimable_outpoints.get(&$txid) {
for &(ref payment_hash, ref source, _) in outpoints.iter() {
log_trace!(self, "Failing HTLC with payment_hash {} from {} remote commitment tx due to broadcast of revoked remote commitment transaction", log_bytes!(payment_hash.0), $commitment_tx);
htlc_updated.push(((*source).clone(), None, payment_hash.clone()));
}
}
}
}
if let Storage::Local { ref current_remote_commitment_txid, ref prev_remote_commitment_txid, .. } = self.key_storage {
if let &Some(ref txid) = current_remote_commitment_txid {
check_htlc_fails!(txid, "current");
}
if let &Some(ref txid) = prev_remote_commitment_txid {
check_htlc_fails!(txid, "remote");
}
}
// No need to check local commitment txn, symmetric HTLCSource must be present as per-htlc data on remote commitment tx
}
if inputs.is_empty() { return (txn_to_broadcast, (commitment_txid, watch_outputs), spendable_outputs, htlc_updated); } // Nothing to be done...probably a false positive/local tx

Expand Down Expand Up @@ -1211,29 +1234,6 @@ impl ChannelMonitor {
output: spend_tx.output[0].clone(),
});
txn_to_broadcast.push(spend_tx);

// TODO: We really should only fail backwards after our revocation claims have been
// confirmed, but we also need to do more other tracking of in-flight pre-confirm
// on-chain claims, so we can do that at the same time.
if let Storage::Local { ref current_remote_commitment_txid, ref prev_remote_commitment_txid, .. } = self.key_storage {
if let &Some(ref txid) = current_remote_commitment_txid {
if let Some(&(_, ref latest_outpoints)) = self.remote_claimable_outpoints.get(&txid) {
for &(ref payment_hash, ref source, _) in latest_outpoints.iter() {
log_trace!(self, "Failing HTLC with payment_hash {} from current remote commitment tx due to broadcast of revoked remote commitment transaction", log_bytes!(payment_hash.0));
htlc_updated.push(((*source).clone(), None, payment_hash.clone()));
}
}
}
if let &Some(ref txid) = prev_remote_commitment_txid {
if let Some(&(_, ref prev_outpoint)) = self.remote_claimable_outpoints.get(&txid) {
for &(ref payment_hash, ref source, _) in prev_outpoint.iter() {
log_trace!(self, "Failing HTLC with payment_hash {} from previous remote commitment tx due to broadcast of revoked remote commitment transaction", log_bytes!(payment_hash.0));
htlc_updated.push(((*source).clone(), None, payment_hash.clone()));
}
}
}
}
// No need to check local commitment txn, symmetric HTLCSource must be present as per-htlc data on remote commitment tx
} else if let Some(per_commitment_data) = per_commitment_option {
// While this isn't useful yet, there is a potential race where if a counterparty
// revokes a state at the same time as the commitment transaction for that state is
Expand Down
37 changes: 27 additions & 10 deletions src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2951,7 +2951,7 @@ fn test_simple_commitment_revoked_fail_backward() {
}
}

fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool) {
fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use_dust: bool, no_to_remote: bool) {
// Test that if our counterparty broadcasts a revoked commitment transaction we fail all
// pending HTLCs on that channel backwards even if the HTLCs aren't present in our latest
// commitment transaction anymore.
Expand All @@ -2973,15 +2973,22 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool) {
create_announced_chan_between_nodes(&nodes, 0, 1);
let chan_2 = create_announced_chan_between_nodes(&nodes, 1, 2);

let (payment_preimage, _payment_hash) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 3000000);
let (payment_preimage, _payment_hash) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], if no_to_remote { 10_000 } else { 3_000_000 });
// Get the will-be-revoked local txn from nodes[2]
let revoked_local_txn = nodes[2].node.channel_state.lock().unwrap().by_id.get(&chan_2.2).unwrap().last_local_commitment_txn.clone();
assert_eq!(revoked_local_txn[0].output.len(), if no_to_remote { 1 } else { 2 });
// Revoke the old state
claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage);

let (_, first_payment_hash) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 3000000);
let (_, second_payment_hash) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 3000000);
let (_, third_payment_hash) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 3000000);
let value = if use_dust {
// The dust limit applied to HTLC outputs considers the fee of the HTLC transaction as
// well, so HTLCs at exactly the dust limit will not be included in commitment txn.
nodes[2].node.channel_state.lock().unwrap().by_id.get(&chan_2.2).unwrap().our_dust_limit_satoshis * 1000
} else { 3000000 };

let (_, first_payment_hash) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], value);
let (_, second_payment_hash) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], value);
let (_, third_payment_hash) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], value);

assert!(nodes[2].node.fail_htlc_backwards(&first_payment_hash, 0));
expect_pending_htlcs_forwardable!(nodes[2]);
Expand Down Expand Up @@ -3043,8 +3050,8 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool) {

if deliver_bs_raa {
nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &bs_raa).unwrap();
// One monitor for the new revocation preimage, one as we generate a commitment for
// nodes[0] to fail first_payment_hash backwards.
// One monitor for the new revocation preimage, no second on as we won't generate a new
// commitment transaction for nodes[0] until process_pending_htlc_forwards().
check_added_monitors!(nodes[1], 1);
let events = nodes[1].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
Expand Down Expand Up @@ -3152,9 +3159,19 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool) {
}

#[test]
fn test_commitment_revoked_fail_backward_exhaustive() {
do_test_commitment_revoked_fail_backward_exhaustive(false);
do_test_commitment_revoked_fail_backward_exhaustive(true);
fn test_commitment_revoked_fail_backward_exhaustive_a() {
do_test_commitment_revoked_fail_backward_exhaustive(false, true, false);
do_test_commitment_revoked_fail_backward_exhaustive(true, true, false);
do_test_commitment_revoked_fail_backward_exhaustive(false, false, false);
do_test_commitment_revoked_fail_backward_exhaustive(true, false, false);
}

#[test]
fn test_commitment_revoked_fail_backward_exhaustive_b() {
do_test_commitment_revoked_fail_backward_exhaustive(false, true, true);
do_test_commitment_revoked_fail_backward_exhaustive(true, true, true);
do_test_commitment_revoked_fail_backward_exhaustive(false, false, true);
do_test_commitment_revoked_fail_backward_exhaustive(true, false, true);
}

#[test]
Expand Down