Skip to content

Drop the need for fork headers when calling Listen's disconnect #3876

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Drop the need for fork headers when calling Listen's disconnect #3876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7ae4f68
Drop the need for fork headers when calling `Listen`'s disconnect
TheBlueMatt Jun 18, 2025
3b99b55
f correct panic message on blocks_disconnected
TheBlueMatt Jul 1, 2025
7ff5313
Use similar `blocks_disconnected` semantics in `OnchainTxHandler`
TheBlueMatt Jul 1, 2025
9fc599f
Add more robust functional test of `Listen::blocks_disconnected`
TheBlueMatt Jun 18, 2025
9385161
Don't pass a latest-block-time to `Channel` unless we have one
TheBlueMatt Jun 19, 2025
4ae8fcc
Add further additional documentation to `Listen`
TheBlueMatt Jun 25, 2025
a6985eb
f clean up docs
TheBlueMatt Jun 27, 2025
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
5 changes: 3 additions & 2 deletions fuzz/src/full_stack.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,8 +344,9 @@ impl<'a> MoneyLossDetector<'a> {
self.header_hashes[self.height - 1].0,
self.header_hashes[self.height].1,
);
self.manager.block_disconnected(&header, self.height as u32);
self.monitor.block_disconnected(&header, self.height as u32);
let best_block = BestBlock::new(header.prev_blockhash, self.height as u32 - 1);
self.manager.blocks_disconnected(best_block);
self.monitor.blocks_disconnected(best_block);
self.height -= 1;
let removal_height = self.height;
self.txids_confirmed.retain(|_, height| removal_height != *height);
Expand Down
30 changes: 11 additions & 19 deletions lightning-block-sync/src/init.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use bitcoin::hash_types::BlockHash;
use bitcoin::network::Network;

use lightning::chain;
use lightning::chain::BestBlock;

use std::ops::Deref;

Expand Down Expand Up @@ -230,8 +231,8 @@ impl<'a, L: chain::Listen + ?Sized> chain::Listen for DynamicChainListener<'a, L
unreachable!()
}

fn block_disconnected(&self, header: &Header, height: u32) {
self.0.block_disconnected(header, height)
fn blocks_disconnected(&self, new_best_block: BestBlock) {
self.0.blocks_disconnected(new_best_block)
}
}

Expand All @@ -257,7 +258,7 @@ impl<'a, L: chain::Listen + ?Sized> chain::Listen for ChainListenerSet<'a, L> {
}
}

fn block_disconnected(&self, _header: &Header, _height: u32) {
fn blocks_disconnected(&self, _new_best_block: BestBlock) {
unreachable!()
}
}
Expand Down Expand Up @@ -300,19 +301,16 @@ mod tests {
let fork_chain_3 = main_chain.fork_at_height(3);

let listener_1 = MockChainListener::new()
.expect_block_disconnected(*fork_chain_1.at_height(4))
.expect_block_disconnected(*fork_chain_1.at_height(3))
.expect_block_disconnected(*fork_chain_1.at_height(2))
.expect_blocks_disconnected(*fork_chain_1.at_height(2))
.expect_block_connected(*main_chain.at_height(2))
.expect_block_connected(*main_chain.at_height(3))
.expect_block_connected(*main_chain.at_height(4));
let listener_2 = MockChainListener::new()
.expect_block_disconnected(*fork_chain_2.at_height(4))
.expect_block_disconnected(*fork_chain_2.at_height(3))
.expect_blocks_disconnected(*fork_chain_2.at_height(3))
.expect_block_connected(*main_chain.at_height(3))
.expect_block_connected(*main_chain.at_height(4));
let listener_3 = MockChainListener::new()
.expect_block_disconnected(*fork_chain_3.at_height(4))
.expect_blocks_disconnected(*fork_chain_3.at_height(4))
.expect_block_connected(*main_chain.at_height(4));

let listeners = vec![
Expand All @@ -337,23 +335,17 @@ mod tests {
let fork_chain_3 = fork_chain_2.fork_at_height(3);

let listener_1 = MockChainListener::new()
.expect_block_disconnected(*fork_chain_1.at_height(4))
.expect_block_disconnected(*fork_chain_1.at_height(3))
.expect_block_disconnected(*fork_chain_1.at_height(2))
.expect_blocks_disconnected(*fork_chain_1.at_height(2))
.expect_block_connected(*main_chain.at_height(2))
.expect_block_connected(*main_chain.at_height(3))
.expect_block_connected(*main_chain.at_height(4));
let listener_2 = MockChainListener::new()
.expect_block_disconnected(*fork_chain_2.at_height(4))
.expect_block_disconnected(*fork_chain_2.at_height(3))
.expect_block_disconnected(*fork_chain_2.at_height(2))
.expect_blocks_disconnected(*fork_chain_2.at_height(2))
.expect_block_connected(*main_chain.at_height(2))
.expect_block_connected(*main_chain.at_height(3))
.expect_block_connected(*main_chain.at_height(4));
let listener_3 = MockChainListener::new()
.expect_block_disconnected(*fork_chain_3.at_height(4))
.expect_block_disconnected(*fork_chain_3.at_height(3))
.expect_block_disconnected(*fork_chain_3.at_height(2))
.expect_blocks_disconnected(*fork_chain_3.at_height(2))
.expect_block_connected(*main_chain.at_height(2))
.expect_block_connected(*main_chain.at_height(3))
.expect_block_connected(*main_chain.at_height(4));
Expand All @@ -380,7 +372,7 @@ mod tests {
let old_tip = fork_chain.tip();

let listener = MockChainListener::new()
.expect_block_disconnected(*old_tip)
.expect_blocks_disconnected(*old_tip)
.expect_block_connected(*new_tip);

let listeners = vec![(old_tip.block_hash, &listener as &dyn chain::Listen)];
Expand Down
20 changes: 11 additions & 9 deletions lightning-block-sync/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ use bitcoin::hash_types::BlockHash;
use bitcoin::pow::Work;

use lightning::chain;
use lightning::chain::Listen;
use lightning::chain::{BestBlock, Listen};

use std::future::Future;
use std::ops::Deref;
Expand Down Expand Up @@ -398,12 +398,15 @@ where
}

/// Notifies the chain listeners of disconnected blocks.
fn disconnect_blocks(&mut self, mut disconnected_blocks: Vec<ValidatedBlockHeader>) {
for header in disconnected_blocks.drain(..) {
fn disconnect_blocks(&mut self, disconnected_blocks: Vec<ValidatedBlockHeader>) {
for header in disconnected_blocks.iter() {
if let Some(cached_header) = self.header_cache.block_disconnected(&header.block_hash) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change the Cache API accordingly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could, but it seems somewhat disjoint from this PR, given there's further work to do in lightning-block-sync anyway, so I kinda wanted to keep it as small as can be.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mhh, might be good to document somewhere what steps you deem left before you'd consider the transition to this new approach complete. I guess all of them should land before the next release then, to not end up with a half-migrated codebase in the release?

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 the API/stuff in lightning in in a perfectly fine state after this PR, and lightning-block-sync just deserves a bit of efficiency in dropping a vec and improving the cache afterwards. This PR obviously doesn't accomplish the goals that #3600 set out to, but is just a step towards it (the "complicated" step that requires making sure all the lightning-internal stuff isn't broken by changing the API).

AFAIU, to get to the point that bitcoind (and all) syncs support swapping to a new chain during a reorg and don't fetch a pile of redundant blocks we need to:

  • make BestBlock contain a list of 6(ish?) block hashes, not one
  • use that list in lightning-block-sync (and eventually lightning-transaction-sync) to do the sync without leaning on the cache
  • expand the runtime caching to cache partial chains across clients in the init sync logic
  • (eventually) ensure lightning-block-sync doesn't have any spare vecs or whatever lying around.

Copy link
Contributor

@tnull tnull Jul 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I still think it would make sense to open an issue for this larger refactoring you're planning, as there are connected aspects to discuss that don't really belong in this PR.

make BestBlock contain a list of 6(ish?) block hashes, not one

For example, how do we imagine this to work exactly, especially for the Confirm/lightning-transaction-sync case?

For Esplora, we currently retrieve the tip hash, retrieve the header and the block status (including the hight), before we call best_block_updated, which is already 3 costly RTTs. If we now would require to track the 6 most-recent hashes, we'd need to make at least 5 more subsequent calls (bringing this it at least 8 RTTs per sync round) to retrieve the hashes at [(height-5)..(height-1)]. But, given that these are individual calls, there is no way to tell if any reorg happened during some of these calls, so they are inherently race-y.

For Electrum, the results would be very similar for the polling client in its current form. Note we eventually want to switch the client to a streaming version making use of Electum's subscription model though, and requiring 'last 6 blocks' would probably require us to resort to costly polling again.

If we'd otherwise extend the client to start tracking more of the chain state (i.e., actually tracking a local chain 6-suffix) across rounds this would complicate the client logic quite a bit and make it even more susceptible to race conditions.

TLDR: I still maintain all of this would be/is resulting in a major refactor across our chain syncing crates and logic, and it would be great to discuss what we imagine to be involved and the corresponding trade-offs before we just jump into it heads first.

assert_eq!(cached_header, header);
assert_eq!(cached_header, *header);
}
self.chain_listener.block_disconnected(&header.header, header.height);
}
if let Some(block) = disconnected_blocks.last() {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're now happy to use the fork point, why are we keeping a Vec of all disconnected blocks around at all? Shouldn't we now be able to convert this to the new behavior entirely? Seems ChainDifference::common_ancestor would already provide what we need, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Its needed by the cache, which I didn't want to change in this PR - #3876 (comment)

let best_block = BestBlock::new(block.header.prev_blockhash, block.height - 1);
self.chain_listener.blocks_disconnected(best_block);
}
}

Expand Down Expand Up @@ -615,7 +618,7 @@ mod chain_notifier_tests {
let new_tip = fork_chain.tip();
let old_tip = main_chain.tip();
let chain_listener = &MockChainListener::new()
.expect_block_disconnected(*old_tip)
.expect_blocks_disconnected(*old_tip)
.expect_block_connected(*new_tip);
let mut notifier =
ChainNotifier { header_cache: &mut main_chain.header_cache(0..=2), chain_listener };
Expand All @@ -635,8 +638,7 @@ mod chain_notifier_tests {
let new_tip = fork_chain.tip();
let old_tip = main_chain.tip();
let chain_listener = &MockChainListener::new()
.expect_block_disconnected(*old_tip)
.expect_block_disconnected(*main_chain.at_height(2))
.expect_blocks_disconnected(*main_chain.at_height(2))
.expect_block_connected(*new_tip);
let mut notifier =
ChainNotifier { header_cache: &mut main_chain.header_cache(0..=3), chain_listener };
Expand All @@ -656,7 +658,7 @@ mod chain_notifier_tests {
let new_tip = fork_chain.tip();
let old_tip = main_chain.tip();
let chain_listener = &MockChainListener::new()
.expect_block_disconnected(*old_tip)
.expect_blocks_disconnected(*old_tip)
.expect_block_connected(*fork_chain.at_height(2))
.expect_block_connected(*new_tip);
let mut notifier =
Expand Down
16 changes: 10 additions & 6 deletions lightning-block-sync/src/test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use bitcoin::transaction;
use bitcoin::Transaction;

use lightning::chain;
use lightning::chain::BestBlock;

use std::cell::RefCell;
use std::collections::VecDeque;
Expand Down Expand Up @@ -203,7 +204,7 @@ impl chain::Listen for NullChainListener {
&self, _header: &Header, _txdata: &chain::transaction::TransactionData, _height: u32,
) {
}
fn block_disconnected(&self, _header: &Header, _height: u32) {}
fn blocks_disconnected(&self, _new_best_block: BestBlock) {}
}

pub struct MockChainListener {
Expand Down Expand Up @@ -231,7 +232,7 @@ impl MockChainListener {
self
}

pub fn expect_block_disconnected(self, block: BlockHeaderData) -> Self {
pub fn expect_blocks_disconnected(self, block: BlockHeaderData) -> Self {
self.expected_blocks_disconnected.borrow_mut().push_back(block);
self
}
Expand Down Expand Up @@ -264,14 +265,17 @@ impl chain::Listen for MockChainListener {
}
}

fn block_disconnected(&self, header: &Header, height: u32) {
fn blocks_disconnected(&self, new_best_block: BestBlock) {
match self.expected_blocks_disconnected.borrow_mut().pop_front() {
None => {
panic!("Unexpected block disconnected: {:?}", header.block_hash());
panic!(
"Unexpected block(s) disconnect to {} at height {}",
new_best_block.block_hash, new_best_block.height,
);
},
Some(expected_block) => {
assert_eq!(header.block_hash(), expected_block.header.block_hash());
assert_eq!(height, expected_block.height);
assert_eq!(new_best_block.block_hash, expected_block.header.prev_blockhash);
assert_eq!(new_best_block.height, expected_block.height - 1);
},
}
}
Expand Down
9 changes: 2 additions & 7 deletions lightning-liquidity/src/manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,14 +583,9 @@ where
self.best_block_updated(header, height);
}

fn block_disconnected(&self, header: &bitcoin::block::Header, height: u32) {
let new_height = height - 1;
fn blocks_disconnected(&self, new_best_block: BestBlock) {
if let Some(best_block) = self.best_block.write().unwrap().as_mut() {
assert_eq!(best_block.block_hash, header.block_hash(),
"Blocks must be disconnected in chain-order - the disconnected header must be the last connected header");
assert_eq!(best_block.height, height,
"Blocks must be disconnected in chain-order - the disconnected block must have the correct height");
*best_block = BestBlock::new(header.prev_blockhash, new_height)
*best_block = new_best_block;
}

// TODO: Call block_disconnected on all sub-modules that require it, e.g., LSPS1MessageHandler.
Expand Down
15 changes: 7 additions & 8 deletions lightning/src/chain/chainmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use crate::chain::channelmonitor::{
WithChannelMonitor,
};
use crate::chain::transaction::{OutPoint, TransactionData};
use crate::chain::{ChannelMonitorUpdateStatus, Filter, WatchedOutput};
use crate::chain::{BestBlock, ChannelMonitorUpdateStatus, Filter, WatchedOutput};
use crate::events::{self, Event, EventHandler, ReplayEvent};
use crate::ln::channel_state::ChannelDetails;
use crate::ln::msgs::{self, BaseMessageHandler, Init, MessageSendEvent};
Expand Down Expand Up @@ -910,18 +910,17 @@ where
self.event_notifier.notify();
}

fn block_disconnected(&self, header: &Header, height: u32) {
fn blocks_disconnected(&self, new_best_block: BestBlock) {
let monitor_states = self.monitors.read().unwrap();
log_debug!(
self.logger,
"Latest block {} at height {} removed via block_disconnected",
header.block_hash(),
height
"Block(s) removed to height {} via blocks_disconnected. New best block is {}",
new_best_block.height,
new_best_block.block_hash,
);
for monitor_state in monitor_states.values() {
monitor_state.monitor.block_disconnected(
header,
height,
monitor_state.monitor.blocks_disconnected(
new_best_block,
&*self.broadcaster,
&*self.fee_estimator,
&self.logger,
Expand Down
44 changes: 19 additions & 25 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2099,23 +2099,16 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {

/// Determines if the disconnected block contained any transactions of interest and updates
/// appropriately.
#[rustfmt::skip]
pub fn block_disconnected<B: Deref, F: Deref, L: Deref>(
&self,
header: &Header,
height: u32,
broadcaster: B,
fee_estimator: F,
logger: &L,
pub fn blocks_disconnected<B: Deref, F: Deref, L: Deref>(
&self, new_best_block: BestBlock, broadcaster: B, fee_estimator: F, logger: &L,
) where
B::Target: BroadcasterInterface,
F::Target: FeeEstimator,
L::Target: Logger,
{
let mut inner = self.inner.lock().unwrap();
let logger = WithChannelMonitor::from_impl(logger, &*inner, None);
inner.block_disconnected(
header, height, broadcaster, fee_estimator, &logger)
inner.blocks_disconnected(new_best_block, broadcaster, fee_estimator, &logger)
}

/// Processes transactions confirmed in a block with the given header and height, returning new
Expand Down Expand Up @@ -2149,10 +2142,10 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {

/// Processes a transaction that was reorganized out of the chain.
///
/// Used instead of [`block_disconnected`] by clients that are notified of transactions rather
/// Used instead of [`blocks_disconnected`] by clients that are notified of transactions rather
/// than blocks. See [`chain::Confirm`] for calling expectations.
///
/// [`block_disconnected`]: Self::block_disconnected
/// [`blocks_disconnected`]: Self::blocks_disconnected
#[rustfmt::skip]
pub fn transaction_unconfirmed<B: Deref, F: Deref, L: Deref>(
&self,
Expand Down Expand Up @@ -4374,8 +4367,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
log_trace!(logger, "Best block re-orged, replaced with new block {} at height {}", block_hash, height);
self.onchain_events_awaiting_threshold_conf.retain(|ref entry| entry.height <= height);
let conf_target = self.closure_conf_target();
self.onchain_tx_handler.block_disconnected(
height + 1, broadcaster, conf_target, &self.destination_script, fee_estimator, logger,
self.onchain_tx_handler.blocks_disconnected(
height, broadcaster, conf_target, &self.destination_script, fee_estimator, logger,
);
Vec::new()
} else { Vec::new() }
Expand Down Expand Up @@ -4591,12 +4584,12 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
!unmatured_htlcs.contains(&&source),
"An unmature HTLC transaction conflicts with a maturing one; failed to \
call either transaction_unconfirmed for the conflicting transaction \
or block_disconnected for a block containing it.");
or blocks_disconnected for a before below it.");
debug_assert!(
!matured_htlcs.contains(&source),
"A matured HTLC transaction conflicts with a maturing one; failed to \
call either transaction_unconfirmed for the conflicting transaction \
or block_disconnected for a block containing it.");
or blocks_disconnected for a block before it.");
matured_htlcs.push(source.clone());
}

Expand Down Expand Up @@ -4734,26 +4727,27 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
}

#[rustfmt::skip]
fn block_disconnected<B: Deref, F: Deref, L: Deref>(
&mut self, header: &Header, height: u32, broadcaster: B, fee_estimator: F, logger: &WithChannelMonitor<L>
fn blocks_disconnected<B: Deref, F: Deref, L: Deref>(
&mut self, new_best_block: BestBlock, broadcaster: B, fee_estimator: F, logger: &WithChannelMonitor<L>
) where B::Target: BroadcasterInterface,
F::Target: FeeEstimator,
L::Target: Logger,
{
log_trace!(logger, "Block {} at height {} disconnected", header.block_hash(), height);
let new_height = new_best_block.height;
log_trace!(logger, "Block(s) disconnected to height {}", new_height);

//We may discard:
//- htlc update there as failure-trigger tx (revoked commitment tx, non-revoked commitment tx, HTLC-timeout tx) has been disconnected
//- maturing spendable output has transaction paying us has been disconnected
self.onchain_events_awaiting_threshold_conf.retain(|ref entry| entry.height < height);
self.onchain_events_awaiting_threshold_conf.retain(|ref entry| entry.height <= new_height);

let bounded_fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator);
let conf_target = self.closure_conf_target();
self.onchain_tx_handler.block_disconnected(
height, broadcaster, conf_target, &self.destination_script, &bounded_fee_estimator, logger
self.onchain_tx_handler.blocks_disconnected(
new_height, broadcaster, conf_target, &self.destination_script, &bounded_fee_estimator, logger
);

self.best_block = BestBlock::new(header.prev_blockhash, height - 1);
self.best_block = new_best_block;
}

#[rustfmt::skip]
Expand Down Expand Up @@ -5198,8 +5192,8 @@ where
self.0.block_connected(header, txdata, height, &*self.1, &*self.2, &self.3);
}

fn block_disconnected(&self, header: &Header, height: u32) {
self.0.block_disconnected(header, height, &*self.1, &*self.2, &self.3);
fn blocks_disconnected(&self, new_best_block: BestBlock) {
self.0.blocks_disconnected(new_best_block, &*self.1, &*self.2, &self.3);
}
}

Expand Down
Loading
Loading