Skip to content

Delay DelayedPaymentOutput spendable events until the CSV delay #936

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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 8 additions & 5 deletions lightning-background-processor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ mod tests {
use lightning::chain::keysinterface::{InMemorySigner, KeysInterface, KeysManager};
use lightning::chain::transaction::OutPoint;
use lightning::get_event_msg;
use lightning::ln::channelmanager::{BestBlock, ChainParameters, ChannelManager, SimpleArcChannelManager};
use lightning::ln::channelmanager::{BREAKDOWN_TIMEOUT, BestBlock, ChainParameters, ChannelManager, SimpleArcChannelManager};
use lightning::ln::features::InitFeatures;
use lightning::ln::msgs::ChannelMessageHandler;
use lightning::ln::peer_handler::{PeerManager, MessageHandler, SocketDescriptor};
Expand Down Expand Up @@ -303,8 +303,8 @@ mod tests {
}}
}

fn confirm_transaction(node: &mut Node, tx: &Transaction) {
for i in 1..=ANTI_REORG_DELAY {
fn confirm_transaction_depth(node: &mut Node, tx: &Transaction, depth: u32) {
for i in 1..=depth {
let prev_blockhash = node.best_block.block_hash();
let height = node.best_block.height() + 1;
let header = BlockHeader { version: 0x20000000, prev_blockhash, merkle_root: Default::default(), time: height, bits: 42, nonce: 42 };
Expand All @@ -315,14 +315,17 @@ mod tests {
node.node.transactions_confirmed(&header, &txdata, height);
node.chain_monitor.transactions_confirmed(&header, &txdata, height);
},
ANTI_REORG_DELAY => {
x if x == depth => {
node.node.best_block_updated(&header, height);
node.chain_monitor.best_block_updated(&header, height);
},
_ => {},
}
}
}
fn confirm_transaction(node: &mut Node, tx: &Transaction) {
confirm_transaction_depth(node, tx, ANTI_REORG_DELAY);
}

#[test]
fn test_background_processor() {
Expand Down Expand Up @@ -454,7 +457,7 @@ mod tests {
// Force close the channel and check that the SpendableOutputs event was handled.
nodes[0].node.force_close_channel(&nodes[0].node.list_channels()[0].channel_id).unwrap();
let commitment_tx = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().pop().unwrap();
confirm_transaction(&mut nodes[0], &commitment_tx);
confirm_transaction_depth(&mut nodes[0], &commitment_tx, BREAKDOWN_TIMEOUT as u32);
let event = receiver
.recv_timeout(Duration::from_secs(EVENT_DEADLINE))
.expect("SpendableOutputs not handled within deadline");
Expand Down
10 changes: 9 additions & 1 deletion lightning/src/chain/channelmonitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,15 @@ struct OnchainEventEntry {

impl OnchainEventEntry {
fn confirmation_threshold(&self) -> u32 {
self.height + ANTI_REORG_DELAY - 1
let mut conf_threshold = self.height + ANTI_REORG_DELAY - 1;
if let OnchainEvent::MaturingOutput {
descriptor: SpendableOutputDescriptor::DelayedPaymentOutput(ref descriptor)
} = self.event {
// A CSV'd transaction is confirmable in block (input height) + CSV delay, which means
// it's broadcastable when we see the previous block.
conf_threshold = cmp::max(conf_threshold, self.height + descriptor.to_self_delay as u32 - 1);
}
conf_threshold
}

fn has_reached_confirmation_threshold(&self, height: u32) -> bool {
Expand Down
22 changes: 18 additions & 4 deletions lightning/src/ln/functional_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4875,11 +4875,13 @@ fn test_claim_sizeable_push_msat() {
assert_eq!(node_txn[0].output.len(), 2); // We can't force trimming of to_remote output as channel_reserve_satoshis block us to do so at channel opening

mine_transaction(&nodes[1], &node_txn[0]);
connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
connect_blocks(&nodes[1], BREAKDOWN_TIMEOUT as u32 - 1);

let spend_txn = check_spendable_outputs!(nodes[1], 1, node_cfgs[1].keys_manager, 100000);
assert_eq!(spend_txn.len(), 1);
assert_eq!(spend_txn[0].input.len(), 1);
check_spends!(spend_txn[0], node_txn[0]);
assert_eq!(spend_txn[0].input[0].sequence, BREAKDOWN_TIMEOUT as u32);
}

#[test]
Expand Down Expand Up @@ -5507,12 +5509,14 @@ fn test_dynamic_spendable_outputs_local_htlc_success_tx() {
};

mine_transaction(&nodes[1], &node_tx);
connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
connect_blocks(&nodes[1], BREAKDOWN_TIMEOUT as u32 - 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably outside the scope of the PR, but I feel like BREAKDOWN_TIMEOUT could use a better name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably, the fact that its public with such an awkward name kinda sucks.


// Verify that B is able to spend its own HTLC-Success tx thanks to spendable output event given back by its ChannelMonitor
let spend_txn = check_spendable_outputs!(nodes[1], 1, node_cfgs[1].keys_manager, 100000);
assert_eq!(spend_txn.len(), 1);
assert_eq!(spend_txn[0].input.len(), 1);
check_spends!(spend_txn[0], node_tx);
assert_eq!(spend_txn[0].input[0].sequence, BREAKDOWN_TIMEOUT as u32);
}

fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, announce_latest: bool) {
Expand Down Expand Up @@ -5802,15 +5806,20 @@ fn test_dynamic_spendable_outputs_local_htlc_timeout_tx() {
};

mine_transaction(&nodes[0], &htlc_timeout);
connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
connect_blocks(&nodes[0], BREAKDOWN_TIMEOUT as u32 - 1);
expect_payment_failed!(nodes[0], our_payment_hash, true);

// Verify that A is able to spend its own HTLC-Timeout tx thanks to spendable output event given back by its ChannelMonitor
let spend_txn = check_spendable_outputs!(nodes[0], 1, node_cfgs[0].keys_manager, 100000);
assert_eq!(spend_txn.len(), 3);
check_spends!(spend_txn[0], local_txn[0]);
assert_eq!(spend_txn[1].input.len(), 1);
check_spends!(spend_txn[1], htlc_timeout);
assert_eq!(spend_txn[1].input[0].sequence, BREAKDOWN_TIMEOUT as u32);
assert_eq!(spend_txn[2].input.len(), 2);
check_spends!(spend_txn[2], local_txn[0], htlc_timeout);
assert!(spend_txn[2].input[0].sequence == BREAKDOWN_TIMEOUT as u32 ||
spend_txn[2].input[1].sequence == BREAKDOWN_TIMEOUT as u32);
}

#[test]
Expand Down Expand Up @@ -5877,16 +5886,21 @@ fn test_key_derivation_params() {
};

mine_transaction(&nodes[0], &htlc_timeout);
connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
connect_blocks(&nodes[0], BREAKDOWN_TIMEOUT as u32 - 1);
expect_payment_failed!(nodes[0], our_payment_hash, true);

// Verify that A is able to spend its own HTLC-Timeout tx thanks to spendable output event given back by its ChannelMonitor
let new_keys_manager = test_utils::TestKeysInterface::new(&seed, Network::Testnet);
let spend_txn = check_spendable_outputs!(nodes[0], 1, new_keys_manager, 100000);
assert_eq!(spend_txn.len(), 3);
check_spends!(spend_txn[0], local_txn_1[0]);
assert_eq!(spend_txn[1].input.len(), 1);
check_spends!(spend_txn[1], htlc_timeout);
assert_eq!(spend_txn[1].input[0].sequence, BREAKDOWN_TIMEOUT as u32);
assert_eq!(spend_txn[2].input.len(), 2);
check_spends!(spend_txn[2], local_txn_1[0], htlc_timeout);
assert!(spend_txn[2].input[0].sequence == BREAKDOWN_TIMEOUT as u32 ||
spend_txn[2].input[1].sequence == BREAKDOWN_TIMEOUT as u32);
}

#[test]
Expand Down
3 changes: 2 additions & 1 deletion lightning/src/util/events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,8 @@ pub enum Event {
/// now + 5*time_forwardable).
time_forwardable: Duration,
},
/// Used to indicate that an output was generated on-chain which you should know how to spend.
/// Used to indicate that an output which you should know how to spend was confirmed on chain
/// and is now spendable.
/// Such an output will *not* ever be spent by rust-lightning, and are not at risk of your
/// counterparty spending them due to some kind of timeout. Thus, you need to store them
/// somewhere and spend them when you create on-chain transactions.
Expand Down