Uh oh!
There was an error while loading. Please reload this page.
Few more ChannelMonitor Cleanups - #597
Conversation
ariard
left a comment
There was a problem hiding this comment.
Mostly good, just some clarifications. You can take to cleanup a bit more ariard@3bb1afe
| DecodeError::UnknownVersion => f.write_str("Unknown realm byte in Onion packet"), | ||
| DecodeError::UnknownRequiredFeature => f.write_str("Unknown required feature preventing decode"), | ||
| DecodeError::InvalidValue => f.write_str("Nonsense bytes didn't map to the type they were interpreted as"), | ||
| DecodeError::ShortRead => f.write_str("Packet extended beyond the provided bytes"), |
There was a problem hiding this comment.
Error description doesn't make sense with field comment "Buffer too short" IMO
There was a problem hiding this comment.
This was in 594, needs to be in a followup PR.
| /// tells you the last block hash which was block_connect()ed. You MUST rescan any blocks along | ||
| /// the "reorg path" (ie not just starting at the same height but starting at the highest | ||
| /// common block that appears on your best chain as well as on the chain which contains the | ||
| /// last block hash returned) upon deserializing the object! |
There was a problem hiding this comment.
I think this comment may be wrong and lead to bug. You must not only rescan from common ancestor but even before disconnect any block from reorg'ed branch. Not doing so may lead to false onchain_events_waiting_threshold_conf confirmations and therefore returning earlier than expected. Or not rebroadcasting a transaction for a reor'ed out outpoint ?
Should we be more severe in block_connected method and assert than any connection is parent of previous last_block_hash or last_block_hash for in-place rescan ?
There was a problem hiding this comment.
Hmm, right, its intended to mean that but the parenthetical isn't clear. I'll change it but note that this isn't new documentation. Yes, we should probably be much stricter in block_connected+disconnected methods in a few ways, that included.
| @@ -1843,6 +1843,9 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> { | |||
| return Err((None, ChannelError::Close("Got wrong number of HTLC signatures from remote"))); | |||
| } | |||
| // TODO: Merge these two, sadly they are currently both required to be passed separately to | |||
There was a problem hiding this comment.
I'm not sure about merging both, they aren't targeted to the same layer, HTLCOutputInCommitment is addressed to OnchainTxHandler and HTLCSource for ChannelManager even if we use for both index to take decisions.
First one is to generate correct redeemScript and commitment transaction, second one is to link HTLCs through a payment path.
Even in ChannelMonitor, IIRC we don't use both at same location.
There was a problem hiding this comment.
Sure, but just because they're intended for different uses doesn't mean we should be passing two separate vectors over with redundant data. We can pass one Vec and let ChannelMonitor figure out how to put what where.
There was a problem hiding this comment.
Okay but that means ChannelMonitor doing more repacking stuff on sensible code paths, which has been error-prone in the past (something we can improve with well-documented named structures). There is also few intersection between them beyond index
There was a problem hiding this comment.
Right, but its even easier to get confused passing things twice than it is to get confused passing things once and having to think about a well-documented value :).
| feerate_per_kw: initial_local_commitment_tx.feerate_per_kw, | ||
| htlc_outputs: Vec::new(), // There are never any HTLCs in the initial commitment transactions | ||
| }; | ||
| // Returning a monitor error before updating tracking points means in case of using |
There was a problem hiding this comment.
Was this my comment or yours ? Because if watchtower doesn't reject update, it means they have the latest local valid commitment, not us. But if we reject update, because of a block triggering broadcast, our tip local commitment is a valid one too. It's just the prev one?
I think we are robust against this scenario right now or what failure do you consider ? If we reject update (quorum or single-failure only) revocation secret shouldn't have been released offchain?
There was a problem hiding this comment.
Definitely not mine. It is in provide_latest_local_commitment_tx_info on master with a blame of e46e183.
There was a problem hiding this comment.
I think your point was that if we update one monitor but another refuses the update, then the first monitor may broadcast a new local state, which the second monitor won't have stored anywhere. I believe this is still the case after this PR.
There was a problem hiding this comment.
Yes, right my comment is confusing as I read it, what I mean here is by latest local valid is a in fact the prev one from the other watchtower viewpoint. Anyway we aren't robust against this, and more I'm thinking there is also dirty reorg-case where one monitor lockdown itself, broadcast commitment, this one get reorg'ed out... Other monitors have always been on the main chain, what happens ?
There was a problem hiding this comment.
Yea, lets discuss it on the issue. Its def confusing.
06fc27d to
d0b6b72CompareNot only was watchtower mode never implemented, but the bits that we had were removed some time ago. It doesn't seem likely we'll move forward with a "watchtower-mode" ChannelMonitor, instead we'll likely have some other, separate struct for this.
ariard
commented
Apr 23, 2020
Code Review ACK d0b6b72 |
3d640da looped over a new HashMap new_claims, clone()ing entries out of it right before droppng the whole thing. This is an obvious candidate for drain(..).
The ChanKeys is created with knowledge of the Channel's value and funding redeemscript up-front, so we should not be providing it when making signing requests.
1107ab0 introduced some additional metadata, including per-HTLC data in LocalCommitmentTransaction. To keep diff reasonable it did so in ChannelMonitor after the LocalCommitmentTransaction had been constructed and passed over the wall, but there's little reason to do so - we should just be constructing them with the data from the start, filled in by Channel. This cleans up some internal interfaces a bit, slightly reduces some data duplication and moves us one step forward to exposing the guts of LocalCommitmentTransaction publicly in a sensible way.
Previously, we created the initial ChannelMonitor on outbound channels when we generated the funding_created message. This was somewhat unnecessary as, at that time, we hadn't yet received clearance to broadcast our initial funding transaction, and thus there should never be any use for a ChannelMonitor. It also complicated ChannelMonitor a bit as, at this point, we didn't have an initial local commitment transaction. By moving the creation of the initial ChannelMonitor to when we receive our counterparty's funding_signed, we can ensure that any ChannelMonitor will always have both a latest remote commitment tx and a latest local commitment tx for broadcast. This also fixes a strange API where we would close a channel unceremoniously on peer-disconnection if we hadn't yet received the funding_signed, but we'd already have a ChannelMonitor for that channel. While it isn't strictly a bug (some potential DoS issues aside), it is strange that these two definitions of a channel being open were not in sync.
Since we now are always initialised with an initial local commitment transaction available now, we might as well take advantage of it and stop using an Option<> where we don't need to.
d0b6b72 to
80055d4CompareTheBlueMatt
commented
Apr 23, 2020
Only change is drop the "NEW: " from the new commit, so gonna merge as-is once travis passes. |
Based on #594, this cleans up ChannelMonitor a bit more. Still playing with this, so its a draft, most especially not sure if I'm gonna want to do "De-Option<> current_local_signed_commitment_tx in ChannelMonitor" yet, depends on how things shake out at the end.