Skip to content

Electrum interface for ChannelMonitor #858

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 16 commits into from
Apr 15, 2021
Merged

Electrum interface for ChannelMonitor #858

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
a899965
Flatten onchain_events_waiting_threshold_conf
jkczyz Mar 31, 2021
561ddc0
Check for duplicate HTLC events having matured
jkczyz Apr 10, 2021
24351f5
Add txid to on-chain event tracking
jkczyz Mar 31, 2021
5610ca1
Combine ChannelManager's block hash and height
jkczyz Apr 9, 2021
2db1f1f
Track block height in ChannelMonitor
jkczyz Apr 6, 2021
13a3340
Add ChannelMonitor::transactions_confirmed
jkczyz Mar 24, 2021
8e37448
Add ChannelMonitor::update_best_block
jkczyz Apr 2, 2021
65e588f
Add ChannelMonitor::transaction_unconfirmed
jkczyz Mar 27, 2021
c57bf73
Add ChannelMonitor::get_relevant_txids
jkczyz Apr 5, 2021
f57c885
Electrum interface for ChainMonitor
jkczyz Apr 2, 2021
34792d0
Remove unnecessary script_pubkey clones
jkczyz Apr 2, 2021
1c72489
Add an internal typedef for transaction outputs
jkczyz Apr 2, 2021
87f74fd
Reuse txdata in functional_test_utils.rs
jkczyz Apr 7, 2021
d45b38f
Test ChainMonitor's Electrum interface
jkczyz Apr 7, 2021
5e8b683
Parameterize test_htlc_on_chain_timeout
jkczyz Apr 10, 2021
524c532
Rename onchain_events_waiting_threshold_conf
jkczyz Apr 12, 2021
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
8 changes: 3 additions & 5 deletions background-processor/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ mod tests {
use lightning::chain::keysinterface::{Sign, InMemorySigner, KeysInterface, KeysManager};
use lightning::chain::transaction::OutPoint;
use lightning::get_event_msg;
use lightning::ln::channelmanager::{ChainParameters, ChannelManager, SimpleArcChannelManager};
use lightning::ln::channelmanager::{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 @@ -192,14 +192,12 @@ mod tests {
let persister = Arc::new(FilesystemPersister::new(format!("{}_persister_{}", persist_dir, i)));
let seed = [i as u8; 32];
let network = Network::Testnet;
let genesis_block = genesis_block(network);
let now = Duration::from_secs(genesis_block.header.time as u64);
let now = Duration::from_secs(genesis_block(network).header.time as u64);
let keys_manager = Arc::new(KeysManager::new(&seed, now.as_secs(), now.subsec_nanos()));
let chain_monitor = Arc::new(chainmonitor::ChainMonitor::new(Some(chain_source.clone()), tx_broadcaster.clone(), logger.clone(), fee_estimator.clone(), persister.clone()));
let params = ChainParameters {
network,
latest_hash: genesis_block.block_hash(),
latest_height: 0,
best_block: BestBlock::from_genesis(network),
};
let manager = Arc::new(ChannelManager::new(fee_estimator.clone(), chain_monitor.clone(), tx_broadcaster, logger.clone(), keys_manager.clone(), UserConfig::default(), params));
let msg_handler = MessageHandler { chan_handler: Arc::new(test_utils::TestChannelMessageHandler::new()), route_handler: Arc::new(test_utils::TestRoutingMessageHandler::new() )};
Expand Down
5 changes: 2 additions & 3 deletions fuzz/src/chanmon_consistency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ use lightning::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdateErr,
use lightning::chain::transaction::OutPoint;
use lightning::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget, FeeEstimator};
use lightning::chain::keysinterface::{KeysInterface, InMemorySigner};
use lightning::ln::channelmanager::{ChainParameters, ChannelManager, PaymentHash, PaymentPreimage, PaymentSecret, PaymentSendFailure, ChannelManagerReadArgs};
use lightning::ln::channelmanager::{BestBlock, ChainParameters, ChannelManager, PaymentHash, PaymentPreimage, PaymentSecret, PaymentSendFailure, ChannelManagerReadArgs};
use lightning::ln::features::{ChannelFeatures, InitFeatures, NodeFeatures};
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, DecodeError, ErrorAction, UpdateAddHTLC, Init};
use lightning::util::enforcing_trait_impls::{EnforcingSigner, INITIAL_REVOKED_COMMITMENT_NUMBER};
Expand Down Expand Up @@ -322,8 +322,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
let network = Network::Bitcoin;
let params = ChainParameters {
network,
latest_hash: genesis_block(network).block_hash(),
latest_height: 0,
best_block: BestBlock::from_genesis(network),
};
(ChannelManager::new(fee_est.clone(), monitor.clone(), broadcast.clone(), Arc::clone(&logger), keys_manager.clone(), config, params),
monitor, keys_manager)
Expand Down
8 changes: 3 additions & 5 deletions fuzz/src/full_stack.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ use lightning::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget,
use lightning::chain::chainmonitor;
use lightning::chain::transaction::OutPoint;
use lightning::chain::keysinterface::{InMemorySigner, KeysInterface};
use lightning::ln::channelmanager::{ChainParameters, ChannelManager, PaymentHash, PaymentPreimage, PaymentSecret};
use lightning::ln::channelmanager::{BestBlock, ChainParameters, ChannelManager, PaymentHash, PaymentPreimage, PaymentSecret};
use lightning::ln::peer_handler::{MessageHandler,PeerManager,SocketDescriptor};
use lightning::ln::msgs::DecodeError;
use lightning::routing::router::get_route;
Expand Down Expand Up @@ -355,15 +355,13 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
config.channel_options.announced_channel = get_slice!(1)[0] != 0;
config.peer_channel_config_limits.min_dust_limit_satoshis = 0;
let network = Network::Bitcoin;
let genesis_hash = genesis_block(network).block_hash();
let params = ChainParameters {
network,
latest_hash: genesis_hash,
latest_height: 0,
best_block: BestBlock::from_genesis(network),
};
let channelmanager = Arc::new(ChannelManager::new(fee_est.clone(), monitor.clone(), broadcast.clone(), Arc::clone(&logger), keys_manager.clone(), config, params));
let our_id = PublicKey::from_secret_key(&Secp256k1::signing_only(), &keys_manager.get_node_secret());
let net_graph_msg_handler = Arc::new(NetGraphMsgHandler::new(genesis_hash, None, Arc::clone(&logger)));
let net_graph_msg_handler = Arc::new(NetGraphMsgHandler::new(genesis_block(network).block_hash(), None, Arc::clone(&logger)));

let peers = RefCell::new([false; 256]);
let mut loss_detector = MoneyLossDetector::new(&peers, channelmanager.clone(), monitor.clone(), PeerManager::new(MessageHandler {
Expand Down
98 changes: 91 additions & 7 deletions lightning/src/chain/chainmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,13 @@
//! servicing [`ChannelMonitor`] updates from the client.

use bitcoin::blockdata::block::{Block, BlockHeader};
use bitcoin::hash_types::Txid;

use chain;
use chain::{Filter, WatchedOutput};
use chain::chaininterface::{BroadcasterInterface, FeeEstimator};
use chain::channelmonitor;
use chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateErr, MonitorEvent, Persist};
use chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateErr, MonitorEvent, Persist, TransactionOutputs};
use chain::transaction::{OutPoint, TransactionData};
use chain::keysinterface::Sign;
use util::logger::Logger;
Expand Down Expand Up @@ -82,24 +83,77 @@ where C::Target: chain::Filter,
/// descendants of such transactions. It is not necessary to re-fetch the block to obtain
/// updated `txdata`.
pub fn block_connected(&self, header: &BlockHeader, txdata: &TransactionData, height: u32) {
self.process_chain_data(header, txdata, |monitor, txdata| {
monitor.block_connected(
header, txdata, height, &*self.broadcaster, &*self.fee_estimator, &*self.logger)
});
}

/// Dispatches to per-channel monitors, which are responsible for updating their on-chain view
/// of a channel and reacting accordingly to newly confirmed transactions. For details, see
/// [`ChannelMonitor::transactions_confirmed`].
///
/// Used instead of [`block_connected`] by clients that are notified of transactions rather than
/// blocks. May be called before or after [`update_best_block`] for transactions in the
/// corresponding block. See [`update_best_block`] for further calling expectations.
///
/// [`block_connected`]: Self::block_connected
/// [`update_best_block`]: Self::update_best_block
pub fn transactions_confirmed(&self, header: &BlockHeader, txdata: &TransactionData, height: u32) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be a bit ambiguous whether it means are_transactions_confirmed or transations_got_confirmed. Not sure I have a good idea for a better name, but food for thought?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think the intended meaning for this and block_connected is that they are named as events with an implicit "handle" prefix. Let's punt a naming discussion to the follow-up PR.

self.process_chain_data(header, txdata, |monitor, txdata| {
monitor.transactions_confirmed(
header, txdata, height, &*self.broadcaster, &*self.fee_estimator, &*self.logger)
});
}

/// Dispatches to per-channel monitors, which are responsible for updating their on-chain view
/// of a channel and reacting accordingly based on the new chain tip. For details, see
/// [`ChannelMonitor::update_best_block`].
///
/// Used instead of [`block_connected`] by clients that are notified of transactions rather than
/// blocks. May be called before or after [`transactions_confirmed`] for the corresponding
/// block.
///
/// Must be called after new blocks become available for the most recent block. Intermediary
/// blocks, however, may be safely skipped. In the event of a chain re-organization, this only
/// needs to be called for the most recent block assuming `transaction_unconfirmed` is called
/// for any affected transactions.
///
/// [`block_connected`]: Self::block_connected
/// [`transactions_confirmed`]: Self::transactions_confirmed
/// [`transaction_unconfirmed`]: Self::transaction_unconfirmed
pub fn update_best_block(&self, header: &BlockHeader, height: u32) {
self.process_chain_data(header, &[], |monitor, txdata| {
// While in practice there shouldn't be any recursive calls when given empty txdata,
// it's still possible if a chain::Filter implementation returns a transaction.
debug_assert!(txdata.is_empty());
monitor.update_best_block(
header, height, &*self.broadcaster, &*self.fee_estimator, &*self.logger)
});
}

fn process_chain_data<FN>(&self, header: &BlockHeader, txdata: &TransactionData, process: FN)
where
FN: Fn(&ChannelMonitor<ChannelSigner>, &TransactionData) -> Vec<TransactionOutputs>
{
let mut dependent_txdata = Vec::new();
let monitors = self.monitors.read().unwrap();
for monitor in monitors.values() {
let mut txn_outputs = monitor.block_connected(header, txdata, height, &*self.broadcaster, &*self.fee_estimator, &*self.logger);
let mut txn_outputs = process(monitor, txdata);

// Register any new outputs with the chain source for filtering, storing any dependent
// transactions from within the block that previously had not been included in txdata.
if let Some(ref chain_source) = self.chain_source {
let block_hash = header.block_hash();
for (txid, outputs) in txn_outputs.drain(..) {
for (idx, output) in outputs.iter() {
for (txid, mut outputs) in txn_outputs.drain(..) {
for (idx, output) in outputs.drain(..) {
// Register any new outputs with the chain source for filtering and recurse
// if it indicates that there are dependent transactions within the block
// that had not been previously included in txdata.
let output = WatchedOutput {
block_hash: Some(block_hash),
outpoint: OutPoint { txid, index: *idx as u16 },
script_pubkey: output.script_pubkey.clone(),
outpoint: OutPoint { txid, index: idx as u16 },
script_pubkey: output.script_pubkey,
};
if let Some(tx) = chain_source.register_output(output) {
dependent_txdata.push(tx);
Expand All @@ -114,7 +168,7 @@ where C::Target: chain::Filter,
dependent_txdata.sort_unstable_by_key(|(index, _tx)| *index);
dependent_txdata.dedup_by_key(|(index, _tx)| *index);
let txdata: Vec<_> = dependent_txdata.iter().map(|(index, tx)| (*index, tx)).collect();
self.block_connected(header, &txdata, height);
self.process_chain_data(header, &txdata, process);
}
}

Expand All @@ -128,6 +182,36 @@ where C::Target: chain::Filter,
}
}

/// Dispatches to per-channel monitors, which are responsible for updating their on-chain view
/// of a channel based on transactions unconfirmed as a result of a chain reorganization. See
/// [`ChannelMonitor::transaction_unconfirmed`] for details.
///
/// Used instead of [`block_disconnected`] by clients that are notified of transactions rather
/// than blocks. May be called before or after [`update_best_block`] for transactions in the
/// corresponding block. See [`update_best_block`] for further calling expectations.
///
/// [`block_disconnected`]: Self::block_disconnected
/// [`update_best_block`]: Self::update_best_block
pub fn transaction_unconfirmed(&self, txid: &Txid) {
let monitors = self.monitors.read().unwrap();
for monitor in monitors.values() {
monitor.transaction_unconfirmed(txid, &*self.broadcaster, &*self.fee_estimator, &*self.logger);
}
}

/// Returns the set of txids that should be monitored for re-organization out of the chain.
pub fn get_relevant_txids(&self) -> Vec<Txid> {
let mut txids = Vec::new();
let monitors = self.monitors.read().unwrap();
for monitor in monitors.values() {
txids.append(&mut monitor.get_relevant_txids());
}

txids.sort_unstable();
txids.dedup();
Copy link

Choose a reason for hiding this comment

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

I think here we're fetching from ChannelMonitor.onchain_events_waiting_threshold_conf and OnchainTxHandler.onchain_events_waiting_threshold_conf, both of them should already discard duplicated txids ? And we don't do transaction across monitors ? I think we already speak about it in a previous refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And we don't do transaction across monitors ? I think we already speak about it in a previous refactor.

Yes, I think that I recall a similar conversation. When you say, "we" do you mean in rust-lightning or more broadly as part of a spec requirement? I ask because we may need to handle an atypical transaction.

Copy link

@ariard ariard Apr 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The spec doesn't forbid aggregating transactions from different channels for example penalty ones or second-stage HTLCs. And we don't implement it though it could be a potential fee optimization in the future, though a bit tricky to get right.

IIRC last time we concluded we don't do it for now, but we're likely going to require dedup in the future, e.g dual-funding transaction initiating many channels. So sorry overriding first comment, let's just keep it imo.

What do you mean by an atypical transaction ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spec doesn't forbid aggregating transactions from different channels for example penalty ones or second-stage HTLCs. And we don't implement it though it could be a potential fee optimization in the future, though a bit tricky to get right.

Ah, I may have had a bit of confusion around who is generating the transactions. If we are always the one, then we won't need to dedup if we don't produce such transactions yet. But if our counterparty can also produces the transactions, then we may have to dedup if their implementation optimizes in this way. I think that's what I was trying to get at before.

IIRC last time we concluded we don't do it for now, but we're likely going to require dedup in the future, e.g dual-funding transaction initiating many channels.

Seems like it is fine to keep the dedup if we'll eventually need it. Let me know if you prefer to drop it. It should mostly be linear in the number of channels and only involves scanning (not shifting) if we don't produce dups.

What do you mean by an atypical transaction ?

Just as what you described above.

txids
}

/// Creates a new `ChainMonitor` used to watch on-chain activity pertaining to channels.
///
/// When an optional chain source implementing [`chain::Filter`] is provided, the chain monitor
Expand Down
Loading