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

Use static key for ChannelPackager #4459

Closed

Conversation

roeierez
Copy link
Contributor

This PR follows the suggestion here #4424 to make the ChannelPackager use lnwire.ChannelD as key which doesn't change on confirmation.
A migration is needed to handle existing keys by renaming existing keys (copy buckets) to the new format.

@roeierez roeierez force-pushed the fwd_package_static_key branch 3 times, most recently from a50a077 to 187a6dc Compare July 12, 2020 20:26
@roeierez
Copy link
Contributor Author

Although the existing tests pass the case where the migration is done when a pending payment exists, on channel re-establishment the payment is cancelled by the receiver with UpdateFailMalformedHTLC.

I believe it is related to the following log message I see:
ChannelLink(20800:1:0): unable to decode onion hop iterator: TemporaryChannelFailure

So this PR still a work in progress.

@roeierez
Copy link
Contributor Author

I found the root cause of the earlier problem. The Decay log which uses the sphinxreplay.db also has a mapping of batch identifiers to replay set where the batch identifier uses the old key (short channel id + height).
To address the above case I need to rename the keys in the batch-replay bucket the same way I did for the fwd-packages bucket.
@cfromknecht the problem is that the migration process do not take into consideration the sphinxreplay.db atm. I can change that if needed but would like to hear if there is a better way.

@Crypt-iQ
Copy link
Collaborator

Crypt-iQ commented Jul 16, 2020

I have a turbo branch that I'd be willing to share based on 0.9, it's a bit old but the code may help you choose an approach.
There are basically two approaches to take:

  • store everything under the "real shortchannelid" that is assigned during funding confirmation. This is complicated as you may have payments during this time before the short channel id is assigned.
  • store everything under the agreed upon fake short channel id. This is far easier, but the data is sort of incorrect as you are not storing the real short channel id. When the real short channel is finally known, you just route with that one and the database layer is unchanged. Least intrusive change in the short-term.

The second approach will work, but is a little clunky and may introduce code debt.

@roeierez
Copy link
Contributor Author

I have a turbo branch that I'd be willing to share based on 0.9, it's a bit old but the code may help you choose an approach.
There are basically two approaches to take:

  • store everything under the "real shortchannelid" that is assigned during funding confirmation. This is complicated as you may have payments during this time before the short channel id is assigned.
  • store everything under the agreed upon fake short channel id. This is far easier, but the data is sort of incorrect as you are not storing the real short channel id. When the real short channel is finally known, you just route with that one and the database layer is unchanged. Least intrusive change in the short-term.

The second approach will work, but is a little clunky and may introduce code debt.

@Crypt-iQ thanks for looking at this. I submitted another PR that depends on this one and deals with skipping funding confirmation (#4424). See this #4424 (comment) as I have made some major changes on the initial version.
Your feedback is very welcome there.

@Roasbeef Roasbeef added database Related to the database/storage of LND funding Related to the opening of new channels with funding transactions on the blockchain migration labels Jul 20, 2020
@halseth halseth requested review from Crypt-iQ and removed request for Roasbeef and halseth September 11, 2020 11:34
@Crypt-iQ
Copy link
Collaborator

I'd like to avoid a migration and I think we don't need this to implement turbo channels - one less step.

Use lnwire.ChannelD as ChannelPackager key instead of short channel id.
This makes it safe to use the ChannelPackager when the short channel
id changes.
This adds a migration step necessary to change the keys used in
ChannelPackager from lnwire.ShortChannelId to lnwire.ChannelID.
@roeierez
Copy link
Contributor Author

Rebased

@lightninglabs-deploy
Copy link

@cfromknecht: review reminder
@Crypt-iQ: review reminder

@Crypt-iQ
Copy link
Collaborator

Crypt-iQ commented Jan 3, 2022

I'm going to close this pull since #5955 gets around this. Also this isn't enough - the circuits in the circuitmap are based on ShortChannelID and if the ID changes, you risk a force close since packets won't be routed properly during the ID upgrade.

@Crypt-iQ Crypt-iQ closed this Jan 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database Related to the database/storage of LND funding Related to the opening of new channels with funding transactions on the blockchain migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants