ChannelKeys - provide to_self_delay alongside the remote channel pubkeys #658
Conversation
Codecov Report
@@ Coverage Diff @@
## master #658 +/- ##
=======================================
Coverage 91.29% 91.30%
=======================================
Files 35 35
Lines 21388 21297 -91
=======================================
- Hits 19527 19446 -81
+ Misses 1861 1851 -10
Continue to review full report at Codecov.
|
lightning/src/chain/keysinterface.rs
Outdated
| /// Holds late-bound channel data. | ||
| /// This data is available after the channel is known to be accepted, either | ||
| /// when receiving an open_channel for an inbound channel or when | ||
| /// receiving accept_channel for an outbound channel. | ||
| pub struct AcceptedChannelData { | ||
| /// Remote public keys and base points | ||
| pub(crate) remote_channel_pubkeys: ChannelPublicKeys, | ||
| /// Remote to_self_delay | ||
| pub(crate) remote_to_self_delay: u16, | ||
| /// Local to_self_delay | ||
| pub(crate) local_to_self_delay: u16, | ||
| } |
There was a problem hiding this comment.
Is there any reason why Channel cannot have its local_keys be Option<ChanSigner> and only initialize it with a Some value when this data is available?
Then the set_remote_channel_pubkeys / on_accepted method could be dispensed with altogether and you wouldn't need this struct. Instead, KeysInterface::get_channel_keys would be called with this data to construct a ChannelKeys implementation rather than require that the ChannelKeys implementation maintain what state the Channel is in.
Essentially, move the "accepted" state to Channel rather than have it leak into ChannelKeys. This also defines errors in ChannelKeys out of existence.
There was a problem hiding this comment.
We need the local_keys populated in Channel in order to create an open_channel message for outbound channels. They are used to generate the first per_commitment_point and the local pubkeys for insertion into that message.
The first two fields in this additional struct will then be available later, when the peer responds with an accept_channel.
So I don't think there's a way to create the local keys in one place without breaking some abstraction.
There was a problem hiding this comment.
Also, a more general point, an external signer does need to be stateful if it wants to keep funds secure against node compromise. Keeping track of channel state and commitment transaction revocation are part of that needed state.
There was a problem hiding this comment.
I see your point. Perhaps a better articulation my concern is with objects in partially initialized states. If this can't be avoided, then it may be a sign that the abstraction is not appropriate or is growing beyond its original purpose. This likely happened when set_remote_channel_pubkeys (or set_remote_funding_pubkey before it) was added.
I see that Channel already has both a ChannelKeys implementation for the signer/local keys and an Option<ChannelPublicKeys> for the remote public keys. What isn't clear to me is why the latter needs to be explicitly set on the former using set_remote_channel_pubkeys rather than have Channel pass in the appropriate basepoint of ChannelPublicKeys as needed. Then a ChannelKeys implementation would never be partially initialized and would be less error prone since it could be given exactly the data it needs when Channel uses its interface.
Could you help me understand the use case that would necessitate passing all the data upfront rather than the pertinent data as needed?
There was a problem hiding this comment.
Hum, right, I mean there may be a refactor here that makes sense given non-external-signer users won't do enforcement, but it is probably a bigger deal and unrelated to this PR.
There was a problem hiding this comment.
The intent of ChannelKeys in this context is that an external signer API (ie API to hardware wallet) needs to initialize the context for a channel (and derive keys for said channel) first, then we can send the OpenChannel message, and then once we receive AcceptChannel we can tell the hardware wallet what our counterparty's public keys are for the channel. We could try to split it up and have a "get the context for the keys you just gave me, and here's my counterparty's keys" API with the local public keys provided via something separate, but that's also somewhat awkward.
There was a problem hiding this comment.
That doesn't quite help me understand why we need to tell the external signer of all the counterparty's public keys upfront rather than just supplying the appropriate public keys as needed when making calls to it.
There was a problem hiding this comment.
I started a thread about this in Slack (#hsm channel).
A fully validating external signer would have to check that the node isn't supplying it with remote public keys that vary over time for a channel. So it seems repetitive to supply data that is checked to be the same and then effectively thrown away. It would also be less performant if the signer is across a network connection.
There was a problem hiding this comment.
Performance is one issue, but, at least from my perspective, anything we can do to reduce the number of things an external signer needs to check is one less possible critical vulnerability, so this kind of movement is important. Of course I'm not sure that the to_self_delays could be used for such an attack, having one fewer things to puzzle over is welcome to me.
There was a problem hiding this comment.
Gotcha. Responded on Slack but may need to wait for @devrandom to come back online before continuing the conversation. I'm largely indifferent as to how the signer gets the necessary data (i.e., upfront vs per call). My concern here is having a signer in a partially initialized state.
To state my concern slightly differently now that I have a better understanding from Slack: "keys" and "signer" are really two different abstractions but are currently conflated into one abstraction. Thus, a signer isn't functional until all the keys are present and so shouldn't be constructed until then, in my view.
@TheBlueMatt You're right that this sort of refactor may possible but unrelated to this PR. I don't mean to unnecessarily hold it up. But I do think we should strive to move to cleaner abstractions as we move forward. It's possible this series of refactorings will get us there. But I have don't have a way of comparing the intended end result against a hypothetical different approach. Maybe @devrandom can open an issue describing the "phase 2" signer that these refactorings are leading to? This would allow for early feedback on the overall design / approach.
lightning/src/chain/keysinterface.rs
Outdated
| pub struct AcceptedChannelData { | ||
| /// Remote public keys and base points | ||
| pub(crate) remote_channel_pubkeys: ChannelPublicKeys, | ||
| /// Remote to_self_delay |
There was a problem hiding this comment.
Irrespective of name changes, can you expand the comments to describe what the value really means - eg "The to_self_delay value specified by our counterparty and applied on locally-broadcastable transactions, ie the amount of time that we have to wait to recover our funds if we broadcast a transaction. You'll likely want to pass this to the ln::chan_utils::build*_transaction functions when signing local transactions."
|
Rebased and squashed |
In the phase 2 signer, we will construct the commitment transaction inside the signer. In preparation, provide needed channel related data.
| /// This data is available after the channel is known to be accepted, either | ||
| /// when receiving an open_channel for an inbound channel or when | ||
| /// receiving accept_channel for an outbound channel. | ||
| struct AcceptedChannelData { |
There was a problem hiding this comment.
Ah, yes. Made this and its fields private, as well as the relevant field in the containing structure.
In the "phase 2" signer, we will construct the commitment transaction inside the signer from primary data (such as
to_self_delay, amounts, etc.). That way it doesn't have to decode the transaction to figure out what the intention is.In preparation for that goal, provide needed channel related data to
ChannelKeysincluding local and remoteto_self_delay.