-
Notifications
You must be signed in to change notification settings - Fork 407
Track the full list of outpoints a chanmon wants monitoring for #455
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -117,9 +117,16 @@ pub struct HTLCUpdate { | |
pub trait ManyChannelMonitor<ChanSigner: ChannelKeys>: Send + Sync { | ||
/// Adds or updates a monitor for the given `funding_txo`. | ||
/// | ||
/// Implementor must also ensure that the funding_txo outpoint is registered with any relevant | ||
/// ChainWatchInterfaces such that the provided monitor receives block_connected callbacks with | ||
/// any spends of it. | ||
/// Implementer must also ensure that the funding_txo txid *and* outpoint are registered with | ||
/// any relevant ChainWatchInterfaces such that the provided monitor receives block_connected | ||
/// callbacks with the funding transaction, or any spends of it. | ||
/// | ||
/// Further, the implementer must also ensure that each output returned in | ||
/// monitor.get_outputs_to_watch() is registered to ensure that the provided monitor learns about | ||
/// any spends of any of the outputs. | ||
/// | ||
/// Any spends of outputs which should have been registered which aren't passed to | ||
/// ChannelMonitors via block_connected may result in funds loss. | ||
fn add_update_monitor(&self, funding_txo: OutPoint, monitor: ChannelMonitor<ChanSigner>) -> Result<(), ChannelMonitorUpdateErr>; | ||
|
||
/// Used by ChannelManager to get list of HTLC resolved onchain and which needed to be updated | ||
|
@@ -259,6 +266,11 @@ impl<Key : Send + cmp::Eq + hash::Hash + 'static, ChanSigner: ChannelKeys> Simpl | |
self.chain_monitor.watch_all_txn(); | ||
} | ||
} | ||
for (txid, outputs) in monitor.get_outputs_to_watch().iter() { | ||
for (idx, script) in outputs.iter().enumerate() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, it would need to change the same as the walk of the return value of block_connected would. |
||
self.chain_monitor.install_watch_outpoint((*txid, idx as u32), script); | ||
} | ||
} | ||
monitors.insert(key, monitor); | ||
Ok(()) | ||
} | ||
|
@@ -666,6 +678,12 @@ pub struct ChannelMonitor<ChanSigner: ChannelKeys> { | |
// actions when we receive a block with given height. Actions depend on OnchainEvent type. | ||
onchain_events_waiting_threshold_conf: HashMap<u32, Vec<OnchainEvent>>, | ||
|
||
// If we get serialized out and re-read, we need to make sure that the chain monitoring | ||
// interface knows about the TXOs that we want to be notified of spends of. We could probably | ||
// be smart and derive them from the above storage fields, but its much simpler and more | ||
// Obviously Correct (tm) if we just keep track of them explicitly. | ||
outputs_to_watch: HashMap<Sha256dHash, Vec<Script>>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you try to implement this with just returning back Script from remote_commitment_txn_on_chain+prev_local_commitment_tx+current_local_signed_commitment_tx? I think it covers all scripts we care about (well there is also outputs on revoked HTLC-txb but that's a bug we don't track them yet) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have not. I was aiming for "quick and easy" given we have a ton of ongoing larger patches and refactors in that area coming up, so landing something complicated would be a waste of time, and a bunch of time. Up to you if you want me to try another way, though. |
||
|
||
// We simply modify last_block_hash in Channel's block_connected so that serialization is | ||
// consistent but hopefully the users' copy handles block_connected in a consistent way. | ||
// (we do *not*, however, update them in insert_combine to ensure any local user copies keep | ||
|
@@ -736,7 +754,8 @@ impl<ChanSigner: ChannelKeys> PartialEq for ChannelMonitor<ChanSigner> { | |
self.to_remote_rescue != other.to_remote_rescue || | ||
self.pending_claim_requests != other.pending_claim_requests || | ||
self.claimable_outpoints != other.claimable_outpoints || | ||
self.onchain_events_waiting_threshold_conf != other.onchain_events_waiting_threshold_conf | ||
self.onchain_events_waiting_threshold_conf != other.onchain_events_waiting_threshold_conf || | ||
self.outputs_to_watch != other.outputs_to_watch | ||
{ | ||
false | ||
} else { | ||
|
@@ -966,6 +985,15 @@ impl<ChanSigner: ChannelKeys + Writeable> ChannelMonitor<ChanSigner> { | |
} | ||
} | ||
|
||
(self.outputs_to_watch.len() as u64).write(writer)?; | ||
for (txid, output_scripts) in self.outputs_to_watch.iter() { | ||
txid.write(writer)?; | ||
(output_scripts.len() as u64).write(writer)?; | ||
for script in output_scripts.iter() { | ||
script.write(writer)?; | ||
} | ||
} | ||
|
||
Ok(()) | ||
} | ||
|
||
|
@@ -1036,6 +1064,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> { | |
claimable_outpoints: HashMap::new(), | ||
|
||
onchain_events_waiting_threshold_conf: HashMap::new(), | ||
outputs_to_watch: HashMap::new(), | ||
|
||
last_block_hash: Default::default(), | ||
secp_ctx: Secp256k1::new(), | ||
|
@@ -1370,6 +1399,12 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> { | |
} | ||
} | ||
|
||
/// Gets a list of txids, with their output scripts (in the order they appear in the | ||
/// transaction), which we must learn about spends of via block_connected(). | ||
pub fn get_outputs_to_watch(&self) -> &HashMap<Sha256dHash, Vec<Script>> { | ||
&self.outputs_to_watch | ||
} | ||
|
||
/// Gets the sets of all outpoints which this ChannelMonitor expects to hear about spends of. | ||
/// Generally useful when deserializing as during normal operation the return values of | ||
/// block_connected are sufficient to ensure all relevant outpoints are being monitored (note | ||
|
@@ -2362,6 +2397,11 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> { | |
} | ||
} | ||
|
||
/// Called by SimpleManyChannelMonitor::block_connected, which implements | ||
/// ChainListener::block_connected. | ||
/// Eventually this should be pub and, roughly, implement ChainListener, however this requires | ||
/// &mut self, as well as returns new spendable outputs and outpoints to watch for spending of | ||
/// on-chain. | ||
fn block_connected(&mut self, txn_matched: &[&Transaction], height: u32, block_hash: &Sha256dHash, broadcaster: &BroadcasterInterface, fee_estimator: &FeeEstimator)-> (Vec<(Sha256dHash, Vec<TxOut>)>, Vec<SpendableOutputDescriptor>, Vec<(HTLCSource, Option<PaymentPreimage>, PaymentHash)>) { | ||
for tx in txn_matched { | ||
let mut output_val = 0; | ||
|
@@ -2589,6 +2629,9 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> { | |
} | ||
} | ||
self.last_block_hash = block_hash.clone(); | ||
for &(ref txid, ref output_scripts) in watch_outputs.iter() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. block_connected could definitely use some comments. Is this a function that's supposed to be called on block connection? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its called by ManyChannelMonitor::block_connected, which is a https://docs.rs/lightning/0.0.10/lightning/chain/chaininterface/trait.ChainListener.html#tymethod.block_connected . Ideally ChannelMonitor would implement ChainListener too, but we're a ways away from that - #474 implements one step, though it still returns two other values (unlike ChainListener). I'll add a commit that comments this state. |
||
self.outputs_to_watch.insert(txid.clone(), output_scripts.iter().map(|o| o.script_pubkey.clone()).collect()); | ||
} | ||
(watch_outputs, spendable_outputs, htlc_updated) | ||
} | ||
|
||
|
@@ -3241,6 +3284,20 @@ impl<R: ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArgs<R, | |
onchain_events_waiting_threshold_conf.insert(height_target, events); | ||
} | ||
|
||
let outputs_to_watch_len: u64 = Readable::read(reader)?; | ||
let mut outputs_to_watch = HashMap::with_capacity(cmp::min(outputs_to_watch_len as usize, MAX_ALLOC_SIZE / (mem::size_of::<Sha256dHash>() + mem::size_of::<Vec<Script>>()))); | ||
for _ in 0..outputs_to_watch_len { | ||
let txid = Readable::read(reader)?; | ||
let outputs_len: u64 = Readable::read(reader)?; | ||
let mut outputs = Vec::with_capacity(cmp::min(outputs_len as usize, MAX_ALLOC_SIZE / mem::size_of::<Script>())); | ||
for _ in 0..outputs_len { | ||
outputs.push(Readable::read(reader)?); | ||
} | ||
if let Some(_) = outputs_to_watch.insert(txid, outputs) { | ||
return Err(DecodeError::InvalidValue); | ||
} | ||
} | ||
|
||
Ok((last_block_hash.clone(), ChannelMonitor { | ||
commitment_transaction_number_obscure_factor, | ||
|
||
|
@@ -3273,6 +3330,7 @@ impl<R: ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArgs<R, | |
claimable_outpoints, | ||
|
||
onchain_events_waiting_threshold_conf, | ||
outputs_to_watch, | ||
|
||
last_block_hash, | ||
secp_ctx, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"This set of outputs correspond to in-channel outputs, i.e any output for which LN-specific state is needed to solve them and faithfully execute the protocol. Any miss in registering them may entrain a fund loss"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it matters what the outputs are...maybe in the future we'll register revokeable outputs (which we don't currently do and just send a SpendableOutput event to the user) so that we can identify if we went backwards. I added a note that you may lose funds if you fail just emphasize that it matters, though.