Skip to content
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

Make payment_key derivation deterministic #3391

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Oct 31, 2024

Previously, KeysManager::derive_channel_keys would derive the channel's payment_key uniquely on a per-channel basis which would disallow users losing their channel_keys_id to recover funds. As there is no necessity to have payment_key derivation depend on channel_keys_id we can allow for easier recovery of any non-HTLC encumbered funds if we make payment_key derivation deterministic.

Here we do just this but also include a larger refactor introducing a ChannelKeysDerivationParameters struct holding a versioning field to be able to express this change in the derivation scheme. To this end we ensure that legacy channels will continue to use the old derivation scheme after upgrade, while new channels will derive the payment_key
deterministicly.

To this end, we use the first byte of channel_keys_id as a versioning byte indicating the version of the used channel keys derivation scheme. Note that Previously KeysManager::generate_channel_keys_id would with
very high likelyhood never have generated a channel_keys_id with a non-null first byte, which makes this a backwards-compatible change for any users that didn't run custom implementations of
SignerProvider::generate_channel_keys_id conflicting with this assumption.

Apart from this, we add some refactoring commits that drop unused logic (Writeable for InMemorySigner) and rename fields to align them with the BOLTs for clarity.

We previously also discussed abusing the first byte of channel_keys_id as a version byte, which wouldn't require the overhead of introducing ChannelKeysDerivationParameters and serializing the channel_keys_derivation_version field independently everywhere. However, as discussed I now chose to go with the latter approach as it seems cleaner and more future-proof.

That said, looking for an approach ACK here before continuing. Will be draft until then.

Should be good for review

TODO:

  • Fix one remaining failing test case.
  • Add test case for (partial) recovery from seed only.
  • Include commit adding a channel_keys_id argument to get_shutdown_scriptpubkey, just as for the destination case. (cc @alecchendev)

@tnull tnull marked this pull request as draft October 31, 2024 10:33
@tnull tnull force-pushed the 2024-09-deterministic-payment-point branch 10 times, most recently from b8c5f77 to 5b17762 Compare October 31, 2024 14:28
... which we haven't been using since 0.0.119 / commit
7a951b1.
.. as it doesn't use the actual signer's `payment_key`, but the
associated public key.
.. to make the name more clear and since before we `clone` it in
`derive_channel_keys` anyways.
@tnull tnull force-pushed the 2024-09-deterministic-payment-point branch from 5b17762 to a7db955 Compare November 6, 2024 11:14
@tnull tnull marked this pull request as ready for review November 6, 2024 11:14
@tnull
Copy link
Contributor Author

tnull commented Nov 6, 2024

After discussion this offline I now reverted this to simply use the first byte of channel_keys_id as a versioning field which considerably reduces the diff size. Note however that this approach might have backwards compatibility issues if users would have custom implementations of SignerProvider::generate_channel_keys_id for which the always-0-first-byte assumption doesn't hold.

Furthermore, this change will of course prohibit downgrading channel monitors to use the older derivation version. This is also the reason why currently monitor_tests::test_restored_packages_retry was failing before. Now added a workaround to use the legacy keys derivation for this test.

Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 94.95798% with 6 lines in your changes missing coverage. Please review.

Project coverage is 89.72%. Comparing base (8c086c7) to head (8991d74).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/sign/mod.rs 94.82% 2 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs 81.81% 2 Missing ⚠️
lightning/src/util/test_utils.rs 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3391      +/-   ##
==========================================
+ Coverage   89.66%   89.72%   +0.05%     
==========================================
  Files         128      128              
  Lines      104887   105577     +690     
  Branches   104887   105577     +690     
==========================================
+ Hits        94052    94734     +682     
- Misses       8126     8140      +14     
+ Partials     2709     2703       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tnull tnull force-pushed the 2024-09-deterministic-payment-point branch 2 times, most recently from f9619b8 to 0c26baf Compare November 6, 2024 11:48
.. to align the field everywhere.
.. to align the field naming with the spec for clarity.
Previously, `KeysManager::derive_channel_keys` would derive the
channel's `payment_key` uniquely on a per-channel basis which would
disallow users losing their `channel_keys_id` to recover funds. As it's
no real necessity to have `payment_key` derivation depend on
`channel_keys_id` we can allow for easier recovery of any non-HTLC
encumbered funds if we make `payment_key` derivation deterministic.

To this end, we use the first byte of `channel_keys_id` as a versioning
byte indicating the version of the used channel keys derivation scheme.
Note that Previously `KeysManager::generate_channel_keys_id` would with
very high likelyhood never have generated a `channel_keys_id` with a
non-null first byte, which makes this a backwards-compatible change for
any users that didn't run custom implementations of
`SignerProvider::generate_channel_keys_id` conflicting with this
assumption.
Some test cases have hard-coded values which we change here (to be
squashed in after review).
@tnull tnull force-pushed the 2024-09-deterministic-payment-point branch from 0c26baf to 57cadf2 Compare November 6, 2024 12:56
@tnull tnull force-pushed the 2024-09-deterministic-payment-point branch from 57cadf2 to c70f7ec Compare November 6, 2024 13:26
In 6a55dcc we fixed an issue where
we'd just `unwrap` `InMemorySigner`'s `channel_parameters` field and
added a comment explaining why we can't do that.

However, in particular
as related APIs were also changed in follow-up commits to return
`Option`s, the introduced comment is now outdated and doesn't really
make sense anymore without that historical context present.

As it's more or less misleading by now, we just drop it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant