Skip to content
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

Fix to_remote SpendableOutputs generation in rare reorg cases #1022

Merged
merged 1 commit into from
Aug 4, 2021
Merged

Fix to_remote SpendableOutputs generation in rare reorg cases #1022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 7 additions & 3 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2451,7 +2451,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
output: outp.clone(),
});
break;
} else if let Some(ref broadcasted_holder_revokable_script) = self.broadcasted_holder_revokable_script {
}
if let Some(ref broadcasted_holder_revokable_script) = self.broadcasted_holder_revokable_script {
Copy link

Choose a reason for hiding this comment

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

One step further to minimize unseen interactions like this could be to unset self.broadcasted_holder_revokeable_script in block_disconnected/transaction_unconfirmed's onchain_events_awaiting_threshold_conf. If the removed event is of type MaturingOutput and SpendableOutputDescriptor == DelayedPaymentOutput, self.broadcasted_holder_revokeable_script==None I think ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean with this no longer being an if else branch list it should "always work", and unsettnig scripts to match seems risky.

if broadcasted_holder_revokable_script.0 == outp.script_pubkey {
spendable_output = Some(SpendableOutputDescriptor::DelayedPaymentOutput(DelayedPaymentOutputDescriptor {
Comment on lines +2455 to 2457
Copy link
Contributor

Choose a reason for hiding this comment

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

Added some print statements and it seems like the issue was: it'd enter this else if statement, but wouldn't pass the check on the second highlighted line, and then naturally fail to even consider the next else if check

Copy link
Contributor

Choose a reason for hiding this comment

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

Given the second if clause overwrites spendable_output, it seems like essentially a reordering of the clauses?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That might have worked too, but it's really the if else blocks miss that one in the middle isn't fully captured by the check. Because the check is split across the if let and the inner if, we sometimes incorrectly short-circuit the if else tree.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I was wondering if they could have possibly been merge-able into one condition.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, rather obnoxiously Rust doesn't let you merge if let into any additional constraints.

Copy link

@ariard ariard Aug 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Here my understanding of the fixed issue.

If a holder commitment is broadcast, we set ChannelMonitor::broadcasted_holder_revokable_script to Some in check_spend_holder_transaction, from then always marking the spendable output as a DelayedPaymentOutput in is_paying_spendable_output.

After this PR, the spendable output type detection is moved from a "else if" control flow to a "if" one, thus preventing the accidental jump over StaticPaymentOutput detection.

outpoint: OutPoint { txid: tx.txid(), index: i as u16 },
Expand All @@ -2464,19 +2465,22 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
}));
break;
}
} else if self.counterparty_payment_script == outp.script_pubkey {
}
if self.counterparty_payment_script == outp.script_pubkey {
spendable_output = Some(SpendableOutputDescriptor::StaticPaymentOutput(StaticPaymentOutputDescriptor {
outpoint: OutPoint { txid: tx.txid(), index: i as u16 },
output: outp.clone(),
channel_keys_id: self.channel_keys_id,
channel_value_satoshis: self.channel_value_satoshis,
}));
break;
} else if outp.script_pubkey == self.shutdown_script {
}
if outp.script_pubkey == self.shutdown_script {
spendable_output = Some(SpendableOutputDescriptor::StaticOutput {
outpoint: OutPoint { txid: tx.txid(), index: i as u16 },
output: outp.clone(),
});
break;
}
}
if let Some(spendable_output) = spendable_output {
Expand Down
113 changes: 113 additions & 0 deletions lightning/src/ln/reorg_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
//! Further functional tests which test blockchain reorganizations.

use chain::channelmonitor::{ANTI_REORG_DELAY, ChannelMonitor};
use chain::transaction::OutPoint;
use chain::{Confirm, Watch};
use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs};
use ln::features::InitFeatures;
Expand All @@ -20,7 +21,10 @@ use util::test_utils;
use util::ser::{ReadableArgs, Writeable};

use bitcoin::blockdata::block::{Block, BlockHeader};
use bitcoin::blockdata::script::Builder;
use bitcoin::blockdata::opcodes;
use bitcoin::hash_types::BlockHash;
use bitcoin::secp256k1::Secp256k1;

use prelude::*;
use core::mem;
Expand Down Expand Up @@ -427,3 +431,112 @@ fn test_set_outpoints_partial_claiming() {
node_txn.clear();
}
}

fn do_test_to_remote_after_local_detection(style: ConnectStyle) {
// In previous code, detection of to_remote outputs in a counterparty commitment transaction
// was dependent on whether a local commitment transaction had been seen on-chain previously.
// This resulted in some edge cases around not being able to generate a SpendableOutput event
// after a reorg.
//
// Here, we test this by first confirming one set of commitment transactions, then
// disconnecting them and reconnecting another. We then confirm them and check that the correct
// SpendableOutput event is generated.
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 mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);

*nodes[0].connect_style.borrow_mut() = style;
*nodes[1].connect_style.borrow_mut() = style;

let (_, _, chan_id, funding_tx) =
create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 100_000_000, InitFeatures::known(), InitFeatures::known());
let funding_outpoint = OutPoint { txid: funding_tx.txid(), index: 0 };
assert_eq!(funding_outpoint.to_channel_id(), chan_id);

let remote_txn_a = get_local_commitment_txn!(nodes[0], chan_id);
let remote_txn_b = get_local_commitment_txn!(nodes[1], chan_id);

mine_transaction(&nodes[0], &remote_txn_a[0]);
mine_transaction(&nodes[1], &remote_txn_a[0]);

assert!(nodes[0].node.list_channels().is_empty());
check_closed_broadcast!(nodes[0], true);
check_added_monitors!(nodes[0], 1);
assert!(nodes[1].node.list_channels().is_empty());
check_closed_broadcast!(nodes[1], true);
check_added_monitors!(nodes[1], 1);

// Drop transactions broadcasted in response to the first commitment transaction (we have good
// test coverage of these things already elsewhere).
assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0).len(), 1);
assert_eq!(nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0).len(), 1);

assert!(nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());
assert!(nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());

disconnect_blocks(&nodes[0], 1);
disconnect_blocks(&nodes[1], 1);

assert!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty());
assert!(nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty());
assert!(nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());
assert!(nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());

connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);

assert!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty());
assert!(nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty());
assert!(nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());
assert!(nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());

mine_transaction(&nodes[0], &remote_txn_b[0]);
mine_transaction(&nodes[1], &remote_txn_b[0]);

assert!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty());
assert!(nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty());
assert!(nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());
assert!(nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());

connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);

let mut node_a_spendable = nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events();
assert_eq!(node_a_spendable.len(), 1);
if let Event::SpendableOutputs { outputs } = node_a_spendable.pop().unwrap() {
assert_eq!(outputs.len(), 1);
let spend_tx = nodes[0].keys_manager.backing.spend_spendable_outputs(&[&outputs[0]], Vec::new(),
Builder::new().push_opcode(opcodes::all::OP_RETURN).into_script(), 253, &Secp256k1::new()).unwrap();
check_spends!(spend_tx, remote_txn_b[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

admittedly not really the place for this remark, but check_spends sounds awfully vague. Especially with the macro not being defined in this file, a more descriptive name might be helpful for readers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can't say I disagree. There's a lot of open PRs right now, however, so if we want to change it maybe we should wait until things are a bit calmer in a month or two.

}

// nodes[1] is waiting for the to_self_delay to expire, which is many more than
// ANTI_REORG_DELAY. Instead, walk it back and confirm the original remote_txn_a commitment
// again and check that nodes[1] generates a similar spendable output.
// Technically a reorg of ANTI_REORG_DELAY violates our assumptions, so this is undefined by
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a list for which events we wouldn't know how to handle correctly based on max possible reorg size assumptions? Might be useful to link from this comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think probably roughly all of them. In general we don't bother to correctly handle anything that includes a reorg of ANTI_REORG_DELAY. Many things are likely to work just fine, probably, but it's outside our security model entirely. This could maybe be better documented.

// our API spec, but we currently handle this correctly and there's little reason we shouldn't
// in the future.
assert!(nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());
disconnect_blocks(&nodes[1], ANTI_REORG_DELAY);
mine_transaction(&nodes[1], &remote_txn_a[0]);
connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);

let mut node_b_spendable = nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events();
assert_eq!(node_b_spendable.len(), 1);
if let Event::SpendableOutputs { outputs } = node_b_spendable.pop().unwrap() {
assert_eq!(outputs.len(), 1);
let spend_tx = nodes[1].keys_manager.backing.spend_spendable_outputs(&[&outputs[0]], Vec::new(),
Builder::new().push_opcode(opcodes::all::OP_RETURN).into_script(), 253, &Secp256k1::new()).unwrap();
check_spends!(spend_tx, remote_txn_a[0]);
}
}

#[test]
fn test_to_remote_after_local_detection() {
do_test_to_remote_after_local_detection(ConnectStyle::BestBlockFirst);
do_test_to_remote_after_local_detection(ConnectStyle::BestBlockFirstSkippingBlocks);
do_test_to_remote_after_local_detection(ConnectStyle::TransactionsFirst);
do_test_to_remote_after_local_detection(ConnectStyle::TransactionsFirstSkippingBlocks);
do_test_to_remote_after_local_detection(ConnectStyle::FullBlockViaListen);
}