Add registration of commitment tx's outputs from check_spend_remote_transaction #159
Conversation
src/ln/channelmonitor.rs
Outdated
| pub(super) fn set_funding_info(&mut self, funding_info: (OutPoint, Script)) { | ||
| //TODO: Need to register the given script here with a chain_monitor |
There was a problem hiding this comment.
Seems to me that registration of given script is already handled in internal_funding_created or funding_transaction_generated by call to add_update_monitor. If self.funding_txo is set, a call to add_update_monitor_by_key is enough.
There was a problem hiding this comment.
Hmm, yea, I think thats fine, but the comment on the ManyChannelMonitor trait should be updated to indicate that this doesn't happen for them!
|
"channel_monitor_network_test has been running for over 60 seconds" will investigate on tomorrow (and maybe add test) |
There was a problem hiding this comment.
You hit an infinite loop cause the ChainWatchUtil crap doesn't check if a registration is duplicative before calling back into block_connected.
I went ahead and clarified the intended purpose and gave users a bit more flexibility, as well as fixed the infinite loop here at https://github.com/TheBlueMatt/rust-lightning/commits/2018-09-159-clarification-cleanup.
src/ln/channelmonitor.rs
Outdated
| monitor.block_connected(txn_matched, height, &*self.broadcaster); | ||
| let outputs = monitor.block_connected(txn_matched, height, &*self.broadcaster); | ||
| for output in outputs { | ||
| self.chain_monitor.install_watch_script(&output.script_pubkey); |
There was a problem hiding this comment.
I think this needs to be install_watch_outpoint not script, as script was meant to monitor a specific tx by a random script, not meant to monitor outpoints which is all we need here (the commitment txn are already being monitored). Sorry for the confusion here, I had to go remind myself of what this stuff was for, heh.
There was a problem hiding this comment.
Ah yeah didn't get plainly the distinction! Thanks
ariard
force-pushed
the
channel_monitor
branch
from
b7c869e to
0bafe5c
Compare
|
I'd a review on your refactoring, I find it far more clearer this way with ChainWatchedUtil ! Just rebased your commits. |
|
Responded, thanks! Just squash down the fixup "f" commit and looks good! |
Previously we'd hit an infinite loop if a block_connected call always resulted in the same ChainWatchInterface registrations. While we're at it, we also split ChainWatchUtil in two to make things a bit more flexible for users, though not sure if that actually matters, and make the matching more aggressive in testing, even if we pick the more performant option at runtime.
check_spend_remote_transaction Fixup more descriptive var names by Matt Corallo <[email protected]>
ariard
force-pushed
the
channel_monitor
branch
from
0bafe5c to
4b9adea
Compare
|
Rebased, updated with I hope explicitly-enough comments! |
Partial fulfilling of #137, simplest design of second suggestion ? Need to be tested when ChannelMonitor logic'll be completed.