Skip to content

Commit e23c270

Browse files
authored
Merge pull request #838 from TheBlueMatt/2021-03-skip-blocks
Make `Channel`'s block connection API more electrum-friendly
2 parents de6dded + 47ad3d6 commit e23c270

File tree

9 files changed

+478
-299
lines changed

9 files changed

+478
-299
lines changed

fuzz/src/chanmon_consistency.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -435,11 +435,11 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
435435
let chain_hash = genesis_block(Network::Bitcoin).block_hash();
436436
let mut header = BlockHeader { version: 0x20000000, prev_blockhash: chain_hash, merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
437437
let txdata: Vec<_> = channel_txn.iter().enumerate().map(|(i, tx)| (i + 1, tx)).collect();
438-
$node.block_connected(&header, &txdata, 1);
439-
for i in 2..100 {
438+
$node.transactions_confirmed(&header, 1, &txdata);
439+
for _ in 2..100 {
440440
header = BlockHeader { version: 0x20000000, prev_blockhash: header.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
441-
$node.block_connected(&header, &[], i);
442441
}
442+
$node.update_best_block(&header, 99);
443443
} }
444444
}
445445

fuzz/src/full_stack.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ use bitcoin::hashes::sha256::Hash as Sha256;
2727
use bitcoin::hash_types::{Txid, BlockHash, WPubkeyHash};
2828

2929
use lightning::chain;
30+
use lightning::chain::Listen;
3031
use lightning::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget, FeeEstimator};
3132
use lightning::chain::chainmonitor;
3233
use lightning::chain::transaction::OutPoint;
@@ -202,7 +203,8 @@ impl<'a> MoneyLossDetector<'a> {
202203
self.blocks_connected += 1;
203204
let header = BlockHeader { version: 0x20000000, prev_blockhash: self.header_hashes[self.height].0, merkle_root: Default::default(), time: self.blocks_connected, bits: 42, nonce: 42 };
204205
self.height += 1;
205-
self.manager.block_connected(&header, &txdata, self.height as u32);
206+
self.manager.transactions_confirmed(&header, self.height as u32, &txdata);
207+
self.manager.update_best_block(&header, self.height as u32);
206208
(*self.monitor).block_connected(&header, &txdata, self.height as u32);
207209
if self.header_hashes.len() > self.height {
208210
self.header_hashes[self.height] = (header.block_hash(), self.blocks_connected);
@@ -216,7 +218,7 @@ impl<'a> MoneyLossDetector<'a> {
216218
fn disconnect_block(&mut self) {
217219
if self.height > 0 && (self.max_height < 6 || self.height >= self.max_height - 6) {
218220
let header = BlockHeader { version: 0x20000000, prev_blockhash: self.header_hashes[self.height - 1].0, merkle_root: Default::default(), time: self.header_hashes[self.height].1, bits: 42, nonce: 42 };
219-
self.manager.block_disconnected(&header);
221+
self.manager.block_disconnected(&header, self.height as u32);
220222
self.monitor.block_disconnected(&header, self.height as u32);
221223
self.height -= 1;
222224
let removal_height = self.height;

lightning-persister/src/lib.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -244,15 +244,15 @@ mod tests {
244244
// Force close because cooperative close doesn't result in any persisted
245245
// updates.
246246
nodes[0].node.force_close_channel(&nodes[0].node.list_channels()[0].channel_id).unwrap();
247-
check_closed_broadcast!(nodes[0], false);
247+
check_closed_broadcast!(nodes[0], true);
248248
check_added_monitors!(nodes[0], 1);
249249

250250
let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
251251
assert_eq!(node_txn.len(), 1);
252252

253253
let header = BlockHeader { version: 0x20000000, prev_blockhash: nodes[0].best_block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
254254
connect_block(&nodes[1], &Block { header, txdata: vec![node_txn[0].clone(), node_txn[0].clone()]});
255-
check_closed_broadcast!(nodes[1], false);
255+
check_closed_broadcast!(nodes[1], true);
256256
check_added_monitors!(nodes[1], 1);
257257

258258
// Make sure everything is persisted as expected after close.

lightning/src/ln/chanmon_update_fail_tests.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,7 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool, persister_fail
241241
// ...and make sure we can force-close a frozen channel
242242
nodes[0].node.force_close_channel(&channel_id).unwrap();
243243
check_added_monitors!(nodes[0], 1);
244-
check_closed_broadcast!(nodes[0], false);
244+
check_closed_broadcast!(nodes[0], true);
245245

246246
// TODO: Once we hit the chain with the failure transaction we should check that we get a
247247
// PaymentFailed event

lightning/src/ln/channel.rs

Lines changed: 151 additions & 121 deletions
Large diffs are not rendered by default.

lightning/src/ln/channelmanager.rs

Lines changed: 112 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -1004,16 +1004,14 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
10041004
}
10051005
}
10061006

1007-
fn force_close_channel_with_peer(&self, channel_id: &[u8; 32], peer_node_id: Option<&PublicKey>) -> Result<(), APIError> {
1007+
fn force_close_channel_with_peer(&self, channel_id: &[u8; 32], peer_node_id: Option<&PublicKey>) -> Result<PublicKey, APIError> {
10081008
let mut chan = {
10091009
let mut channel_state_lock = self.channel_state.lock().unwrap();
10101010
let channel_state = &mut *channel_state_lock;
10111011
if let hash_map::Entry::Occupied(chan) = channel_state.by_id.entry(channel_id.clone()) {
10121012
if let Some(node_id) = peer_node_id {
10131013
if chan.get().get_counterparty_node_id() != *node_id {
1014-
// Error or Ok here doesn't matter - the result is only exposed publicly
1015-
// when peer_node_id is None anyway.
1016-
return Ok(());
1014+
return Err(APIError::ChannelUnavailable{err: "No such channel".to_owned()});
10171015
}
10181016
}
10191017
if let Some(short_id) = chan.get().get_short_channel_id() {
@@ -1033,14 +1031,27 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
10331031
});
10341032
}
10351033

1036-
Ok(())
1034+
Ok(chan.get_counterparty_node_id())
10371035
}
10381036

10391037
/// Force closes a channel, immediately broadcasting the latest local commitment transaction to
10401038
/// the chain and rejecting new HTLCs on the given channel. Fails if channel_id is unknown to the manager.
10411039
pub fn force_close_channel(&self, channel_id: &[u8; 32]) -> Result<(), APIError> {
10421040
let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
1043-
self.force_close_channel_with_peer(channel_id, None)
1041+
match self.force_close_channel_with_peer(channel_id, None) {
1042+
Ok(counterparty_node_id) => {
1043+
self.channel_state.lock().unwrap().pending_msg_events.push(
1044+
events::MessageSendEvent::HandleError {
1045+
node_id: counterparty_node_id,
1046+
action: msgs::ErrorAction::SendErrorMessage {
1047+
msg: msgs::ErrorMessage { channel_id: *channel_id, data: "Channel force-closed".to_owned() }
1048+
},
1049+
}
1050+
);
1051+
Ok(())
1052+
},
1053+
Err(e) => Err(e)
1054+
}
10441055
}
10451056

10461057
/// Force close all channels, immediately broadcasting the latest local commitment transaction
@@ -3196,6 +3207,12 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31963207
msg: update
31973208
});
31983209
}
3210+
pending_msg_events.push(events::MessageSendEvent::HandleError {
3211+
node_id: chan.get_counterparty_node_id(),
3212+
action: msgs::ErrorAction::SendErrorMessage {
3213+
msg: msgs::ErrorMessage { channel_id: chan.channel_id(), data: "Channel force-closed".to_owned() }
3214+
},
3215+
});
31993216
}
32003217
},
32013218
}
@@ -3278,12 +3295,26 @@ where
32783295
L::Target: Logger,
32793296
{
32803297
fn block_connected(&self, block: &Block, height: u32) {
3298+
assert_eq!(*self.last_block_hash.read().unwrap(), block.header.prev_blockhash,
3299+
"Blocks must be connected in chain-order - the connected header must build on the last connected header");
3300+
assert_eq!(self.latest_block_height.load(Ordering::Acquire) as u64, height as u64 - 1,
3301+
"Blocks must be connected in chain-order - the connected block height must be one greater than the previous height");
32813302
let txdata: Vec<_> = block.txdata.iter().enumerate().collect();
3282-
ChannelManager::block_connected(self, &block.header, &txdata, height);
3303+
self.transactions_confirmed(&block.header, height, &txdata);
3304+
self.update_best_block(&block.header, height);
32833305
}
32843306

3285-
fn block_disconnected(&self, header: &BlockHeader, _height: u32) {
3286-
ChannelManager::block_disconnected(self, header);
3307+
fn block_disconnected(&self, header: &BlockHeader, height: u32) {
3308+
assert_eq!(*self.last_block_hash.read().unwrap(), header.block_hash(),
3309+
"Blocks must be disconnected in chain-order - the disconnected header must be the last connected header");
3310+
3311+
let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
3312+
let new_height = self.latest_block_height.fetch_sub(1, Ordering::AcqRel) as u32 - 1;
3313+
assert_eq!(new_height, height - 1,
3314+
"Blocks must be disconnected in chain-order - the disconnected block must have the correct height");
3315+
*self.last_block_hash.write().unwrap() = header.prev_blockhash;
3316+
3317+
self.do_chain_event(new_height, |channel| channel.update_best_block(new_height, header.time));
32873318
}
32883319
}
32893320

@@ -3294,22 +3325,11 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
32943325
F::Target: FeeEstimator,
32953326
L::Target: Logger,
32963327
{
3297-
/// Updates channel state based on transactions seen in a connected block.
3298-
pub fn block_connected(&self, header: &BlockHeader, txdata: &TransactionData, height: u32) {
3328+
fn do_chain_event<FN: Fn(&mut Channel<Signer>) -> Result<(Option<msgs::FundingLocked>, Vec<(HTLCSource, PaymentHash)>), msgs::ErrorMessage>>
3329+
(&self, height: u32, f: FN) {
32993330
// Note that we MUST NOT end up calling methods on self.chain_monitor here - we're called
33003331
// during initialization prior to the chain_monitor being fully configured in some cases.
33013332
// See the docs for `ChannelManagerReadArgs` for more.
3302-
let block_hash = header.block_hash();
3303-
log_trace!(self.logger, "Block {} at height {} connected", block_hash, height);
3304-
3305-
let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
3306-
3307-
assert_eq!(*self.last_block_hash.read().unwrap(), header.prev_blockhash,
3308-
"Blocks must be connected in chain-order - the connected header must build on the last connected header");
3309-
assert_eq!(self.latest_block_height.load(Ordering::Acquire) as u64, height as u64 - 1,
3310-
"Blocks must be connected in chain-order - the connected header must build on the last connected header");
3311-
self.latest_block_height.store(height as usize, Ordering::Release);
3312-
*self.last_block_hash.write().unwrap() = block_hash;
33133333

33143334
let mut failed_channels = Vec::new();
33153335
let mut timed_out_htlcs = Vec::new();
@@ -3319,7 +3339,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
33193339
let short_to_id = &mut channel_state.short_to_id;
33203340
let pending_msg_events = &mut channel_state.pending_msg_events;
33213341
channel_state.by_id.retain(|_, channel| {
3322-
let res = channel.block_connected(header, txdata, height);
3342+
let res = f(channel);
33233343
if let Ok((chan_res, mut timed_out_pending_htlcs)) = res {
33243344
for (source, payment_hash) in timed_out_pending_htlcs.drain(..) {
33253345
let chan_update = self.get_channel_update(&channel).map(|u| u.encode_with_len()).unwrap(); // Cannot add/recv HTLCs before we have a short_id so unwrap is safe
@@ -3345,32 +3365,23 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
33453365
short_to_id.insert(channel.get_short_channel_id().unwrap(), channel.channel_id());
33463366
}
33473367
} else if let Err(e) = res {
3368+
if let Some(short_id) = channel.get_short_channel_id() {
3369+
short_to_id.remove(&short_id);
3370+
}
3371+
// It looks like our counterparty went on-chain or funding transaction was
3372+
// reorged out of the main chain. Close the channel.
3373+
failed_channels.push(channel.force_shutdown(true));
3374+
if let Ok(update) = self.get_channel_update(&channel) {
3375+
pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
3376+
msg: update
3377+
});
3378+
}
33483379
pending_msg_events.push(events::MessageSendEvent::HandleError {
33493380
node_id: channel.get_counterparty_node_id(),
33503381
action: msgs::ErrorAction::SendErrorMessage { msg: e },
33513382
});
33523383
return false;
33533384
}
3354-
if let Some(funding_txo) = channel.get_funding_txo() {
3355-
for &(_, tx) in txdata.iter() {
3356-
for inp in tx.input.iter() {
3357-
if inp.previous_output == funding_txo.into_bitcoin_outpoint() {
3358-
log_trace!(self.logger, "Detected channel-closing tx {} spending {}:{}, closing channel {}", tx.txid(), inp.previous_output.txid, inp.previous_output.vout, log_bytes!(channel.channel_id()));
3359-
if let Some(short_id) = channel.get_short_channel_id() {
3360-
short_to_id.remove(&short_id);
3361-
}
3362-
// It looks like our counterparty went on-chain. Close the channel.
3363-
failed_channels.push(channel.force_shutdown(true));
3364-
if let Ok(update) = self.get_channel_update(&channel) {
3365-
pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
3366-
msg: update
3367-
});
3368-
}
3369-
return false;
3370-
}
3371-
}
3372-
}
3373-
}
33743385
true
33753386
});
33763387

@@ -3399,6 +3410,64 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
33993410
for (source, payment_hash, reason) in timed_out_htlcs.drain(..) {
34003411
self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), source, &payment_hash, reason);
34013412
}
3413+
}
3414+
3415+
/// Updates channel state to take note of transactions which were confirmed in the given block
3416+
/// at the given height.
3417+
///
3418+
/// Note that you must still call (or have called) [`update_best_block`] with the block
3419+
/// information which is included here.
3420+
///
3421+
/// This method may be called before or after [`update_best_block`] for a given block's
3422+
/// transaction data and may be called multiple times with additional transaction data for a
3423+
/// given block.
3424+
///
3425+
/// This method may be called for a previous block after an [`update_best_block`] call has
3426+
/// been made for a later block, however it must *not* be called with transaction data from a
3427+
/// block which is no longer in the best chain (ie where [`update_best_block`] has already
3428+
/// been informed about a blockchain reorganization which no longer includes the block which
3429+
/// corresponds to `header`).
3430+
///
3431+
/// [`update_best_block`]: `Self::update_best_block`
3432+
pub fn transactions_confirmed(&self, header: &BlockHeader, height: u32, txdata: &TransactionData) {
3433+
// Note that we MUST NOT end up calling methods on self.chain_monitor here - we're called
3434+
// during initialization prior to the chain_monitor being fully configured in some cases.
3435+
// See the docs for `ChannelManagerReadArgs` for more.
3436+
3437+
let block_hash = header.block_hash();
3438+
log_trace!(self.logger, "{} transactions included in block {} at height {} provided", txdata.len(), block_hash, height);
3439+
3440+
let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
3441+
self.do_chain_event(height, |channel| channel.transactions_confirmed(&block_hash, height, txdata, &self.logger).map(|a| (a, Vec::new())));
3442+
}
3443+
3444+
/// Updates channel state with the current best blockchain tip. You should attempt to call this
3445+
/// quickly after a new block becomes available, however if multiple new blocks become
3446+
/// available at the same time, only a single `update_best_block()` call needs to be made.
3447+
///
3448+
/// This method should also be called immediately after any block disconnections, once at the
3449+
/// reorganization fork point, and once with the new chain tip. Calling this method at the
3450+
/// blockchain reorganization fork point ensures we learn when a funding transaction which was
3451+
/// previously confirmed is reorganized out of the blockchain, ensuring we do not continue to
3452+
/// accept payments which cannot be enforced on-chain.
3453+
///
3454+
/// In both the block-connection and block-disconnection case, this method may be called either
3455+
/// once per block connected or disconnected, or simply at the fork point and new tip(s),
3456+
/// skipping any intermediary blocks.
3457+
pub fn update_best_block(&self, header: &BlockHeader, height: u32) {
3458+
// Note that we MUST NOT end up calling methods on self.chain_monitor here - we're called
3459+
// during initialization prior to the chain_monitor being fully configured in some cases.
3460+
// See the docs for `ChannelManagerReadArgs` for more.
3461+
3462+
let block_hash = header.block_hash();
3463+
log_trace!(self.logger, "New best block: {} at height {}", block_hash, height);
3464+
3465+
let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
3466+
3467+
self.latest_block_height.store(height as usize, Ordering::Release);
3468+
*self.last_block_hash.write().unwrap() = block_hash;
3469+
3470+
self.do_chain_event(height, |channel| channel.update_best_block(height, header.time));
34023471

34033472
loop {
34043473
// Update last_node_announcement_serial to be the max of its current value and the
@@ -3414,48 +3483,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
34143483
}
34153484
}
34163485

3417-
/// Updates channel state based on a disconnected block.
3418-
///
3419-
/// If necessary, the channel may be force-closed without letting the counterparty participate
3420-
/// in the shutdown.
3421-
pub fn block_disconnected(&self, header: &BlockHeader) {
3422-
// Note that we MUST NOT end up calling methods on self.chain_monitor here - we're called
3423-
// during initialization prior to the chain_monitor being fully configured in some cases.
3424-
// See the docs for `ChannelManagerReadArgs` for more.
3425-
let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
3426-
3427-
assert_eq!(*self.last_block_hash.read().unwrap(), header.block_hash(),
3428-
"Blocks must be disconnected in chain-order - the disconnected header must be the last connected header");
3429-
self.latest_block_height.fetch_sub(1, Ordering::AcqRel);
3430-
*self.last_block_hash.write().unwrap() = header.prev_blockhash;
3431-
3432-
let mut failed_channels = Vec::new();
3433-
{
3434-
let mut channel_lock = self.channel_state.lock().unwrap();
3435-
let channel_state = &mut *channel_lock;
3436-
let short_to_id = &mut channel_state.short_to_id;
3437-
let pending_msg_events = &mut channel_state.pending_msg_events;
3438-
channel_state.by_id.retain(|_, v| {
3439-
if v.block_disconnected(header) {
3440-
if let Some(short_id) = v.get_short_channel_id() {
3441-
short_to_id.remove(&short_id);
3442-
}
3443-
failed_channels.push(v.force_shutdown(true));
3444-
if let Ok(update) = self.get_channel_update(&v) {
3445-
pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
3446-
msg: update
3447-
});
3448-
}
3449-
false
3450-
} else {
3451-
true
3452-
}
3453-
});
3454-
}
3455-
3456-
self.handle_init_event_channel_failures(failed_channels);
3457-
}
3458-
34593486
/// Blocks until ChannelManager needs to be persisted or a timeout is reached. It returns a bool
34603487
/// indicating whether persistence is necessary. Only one listener on
34613488
/// `await_persistable_update` or `await_persistable_update_timeout` is guaranteed to be woken

0 commit comments

Comments
 (0)