Uh oh!
There was an error while loading. Please reload this page.
[Key derivation V2] Attach a version byte to channel_keys_id - #3887
[Key derivation V2] Attach a version byte to channel_keys_id#3887tankyleo wants to merge 1 commit into
channel_keys_id#3887Conversation
👋 Thanks for assigning @tnull as a reviewer! |
tnull
left a comment
There was a problem hiding this comment.
Seems this already needs a rebase :)
tnull
left a comment
There was a problem hiding this comment.
Concept ACK.
This is basically 1:1 my preferred approach from #3391, which @TheBlueMatt wasn't the biggest fan of though.
It might be good to include the actual v2 derivation in this PR, too, as it would be critical to see whether we can do the derivation in a backwards compatible manner everywhere. Also, would we necessarily break forwards compat everywhere with this?
Speaking off, please note that newly introduced fields need to have odd numbers to not automatically break forwards compatibility, as LDK follows the 'it's okay to be odd' rule for its serialization. This means that nodes would panic on downgrade if they encountered an even field that they aren't expecting.
| (0, outpoint, required), | ||
| (1, channel_keys_id, option), | ||
| (2, output, required), | ||
| (4, channel_keys_version, (default_value, 0)), |
There was a problem hiding this comment.
This needs to be odd, as otherwise we'd break downgrades.
| (15, counterparty_fulfilled_htlcs, option), | ||
| (17, initial_counterparty_commitment_info, option), | ||
| (19, channel_id, option), | ||
| (20, channel_keys_version, (default_value, 0)), |
| (10, self.channel_keys_id.id, required), | ||
| (12, self.channel_value_satoshis, required), | ||
| (13, self.channel_transaction_parameters, (option: ReadableArgs, Some(channel_value_satoshis.0.unwrap()))), | ||
| (14, self.channel_keys_id.version, (no_write_default, 0)), |
There was a problem hiding this comment.
Same, here, new field needs to be odd to not break downgrades.
There was a problem hiding this comment.
I was hoping with 'no_write_default' to allow backwards compatibility iff the version is set to 0. I may be missing something let me know.
There was a problem hiding this comment.
Hmm, ah, not sure if we'd want to add another serialization path just for this. Maybe it would be easier to make the version an Option<u8>? This would also make the behavior explicit and you could actually do manual migration steps depending on whether it's set (without leaning on a magic '0' value).
There was a problem hiding this comment.
If I recall correctly, for a None we would still write the even type, and make it impossible to downgrade ? Will check tomorrow :)
Or actually yes we could make it Option together with the option field type (not required)
There was a problem hiding this comment.
It does seem like its a nicer API if we always have the version, vs having an Optional version, no? I guess as a new field it is kinda an Option... FWIW, I don't think we have to break the reads out nor add a new write type here, eg the following. I guess I don't have a strong opinion on option-or-not.
+impl_writeable_tlv_based!(DelayedPaymentOutputDescriptor, {
+ (0, outpoint, required),
+ (2, per_commitment_point, required),
+ (4, to_self_delay, required),
+ (6, output, required),
+ (8, revocation_pubkey, required),
+ (10, channel_keys_id_id, (legacy, [u8; 32], |obj: &DelayedPaymentOutputDescriptor| Some(obj.channel_keys_id.id))),
+ (12, channel_value_satoshis, required),
+ (13, channel_transaction_parameters, (option: ReadableArgs, Some(channel_value_satoshis.0.unwrap()))),
+ (14, channel_keys_id_vers, (legacy, u8, |obj: &DelayedPaymentOutputDescriptor| Some(obj.channel_keys_id.version))),
+ (x, channel_keys_id, (static_value, ChannelKeysId {
+ id: channel_keys_id_id.ok_or(DecodeError::InvalidValue)?,
+ version: channel_keys_id_vers.ok_or(DecodeError::InvalidValue)?,
+ })),
+});
| (4, self.channel_keys_id.id, required), | ||
| (6, self.channel_value_satoshis, required), | ||
| (7, self.channel_transaction_parameters, (option: ReadableArgs, Some(channel_value_satoshis.0.unwrap()))), | ||
| (8, self.channel_keys_id.version, (no_write_default, 0)), |
| (0, self.value_satoshis, required), | ||
| (2, self.keys_id.id, required), | ||
| (4, self.transaction_parameters, (required: ReadableArgs, Some(value_satoshis.0.unwrap()))), | ||
| (6, self.keys_id.version, (no_write_default, 0)), |
tankyleo
commented
Jun 24, 2025
Overall the idea is to allow backwards compat for version 0 and no backwards compat for anything else. For forward compat, when the field is not set, we assume it is 0. |
ldk-reviews-bot
commented
Jun 26, 2025
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
tnull
left a comment
There was a problem hiding this comment.
This unfortunately already needs a rebase.
TheBlueMatt
left a comment
There was a problem hiding this comment.
This is basically 1:1 my preferred approach from #3391, which @TheBlueMatt wasn't the biggest fan of though.
Indeed, I remain not a huge fan of the version approach - the channel keys id is a public thing that isn't restricted to just our KeysManager (even if most nodes probably do use our KeysManager), so changing the public API because of a limitation of our KeysManager feels incredibly weird - what is the "version" for a SignerProvider that doesn't need it?
In our KeysManager, on the other hand, we do have room for a few extra bytes...currently the first 4 bytes are a channel-open counter since start. Defining the top byte being 0xff as "version two derivation" seems perfectly reasonable to me, I'm really quite confident no one has opened 4 billion channels in a single LDK instance without restarting :).
Although note we previously established that we can’t go that way either as some of our users deployed a custom |
Ah, I'd forgotten that (also, why on earth did anyone do that?!). So, then, yea, I guess a version byte is the only real option? Was there any other option? (discussion on the previous PR seems to hint that we had some other path to explore) |
tnull
commented
Jul 3, 2025
Well, one third option we discussed was to always derive both variants and always just check which one to use, i.e., never set an explicit version anywhere. I do however dislike that approach, if we can avoid it, also since introducing a version does allow us to change the derivation scheme in the future without facing the same challenges again. |
TheBlueMatt
commented
Jul 3, 2025
Ah, I mean hopefully we don't have to do that if we are more careful about the derivation scheme now? There's pretty little overhead to the extra derivation on construction and equality checks, so it shouldn't be too bad... |
ldk-reviews-bot
commented
Jul 3, 2026
Hi @tankyleo, Thanks for your contributions to After too many struggles with bugs, outages, contributor bans, and, finally, a multi-week CI ban, the rust-lightning project is moving off of GitHub for day-to-day development. You can still file issues and access the git tree here, but PRs will now take place exclusively at https://git.rust-bitcoin.org/. As such, this PR has been migrated to https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/3887 If you log in using GitHub (or otherwise link your GitHub account from https://git.rust-bitcoin.org/user/settings/security), ownership of your PRs, issues, and comments will automatically transfer. To push updates to this PR, you'll need to use |
Early, broken draft of a PR that would allow us to understand which key derivation we used for a particular
channel_keys_id.After this PR, we would ship a new derivation for
to_remoteoutputs such that they may be recovered solely from the seed in case of loss of all channel state (including thechannel_keys_idof the channel - which currently is required to derive theto_remotekey).An alternative approach could assume a certain byte in
channel_keys_idhas never been used, and we would rely on that byte to understand which key derivation to use. I've heard this assumption would break some downstream users, so pushed this approach instead.