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

Update to LDK 0.0.121 #126

Merged
merged 1 commit into from
Feb 1, 2024
Merged

Update to LDK 0.0.121 #126

merged 1 commit into from
Feb 1, 2024

Conversation

optout21
Copy link
Contributor

@optout21 optout21 commented Dec 5, 2023

Updated to LDK 0.0.121 and bitcoin library 0.30.2.
Also updated MSRV to 1.63, as LDK requires that (since 0.0.119).

Fixes #125 .

TODO:

  • Fix issue of circular type dep SimpleArcPeerManager <-> GossipVerifier
  • squash commits

Relevant changes in LDK:

  • Added temporary_channel_id to create_channel. #2699
  • Replace maze of BOLT11 payment utilities with parameter generators #2727
  • Drop non-anchor channel fee upper bound limit entirely #2696
  • Update to rust-bitcoin v0.30.2 #2740
  • Add peer_id and channel_id explicitly to log records #2314
  • Pass Record by value to Logger
  • GossipVerifier now takes a PeerManager type (leads to circular type deps)
  • Onion routing: signature of send_onion_message() has changed, path is not needed
  • New ConectionNeeded event, ignored
  • DefaultMessageRouter takes a network graph
  • In WalletSource, sign_tx() has been changed to sign_psbt()
  • Event processor process_events_async() has additional argument for fetching duration, constant 0 used here
  • and some others

@orbitalturtle
Copy link

Just curious if this is still in the works :)

@optout21
Copy link
Contributor Author

optout21 commented Jan 3, 2024

LDK 0.0.119 is available for some time now -- unless at the time of creation of this PR. However, more adaption is needed, TODO.

@tnull
Copy link
Contributor

tnull commented Jan 3, 2024

LDK 0.0.119 is available for some time now -- unless at the time of creation of this PR. However, more adaption is needed, TODO.

Right, but do you intend to go ahead with this PR? If not, I'd be happy to take it over and upgrade ldk-sample.

@optout21
Copy link
Contributor Author

optout21 commented Jan 4, 2024

Yes, I plan to do this in next 1-2 days, but no hard commitment :D

@optout21
Copy link
Contributor Author

optout21 commented Jan 4, 2024

I run into a small problem here:
I'm up to date till LDK Dec 5 cbde4a7
PR #2773 simplified GossipVerifier, which takes now a PeerManager type parameter, but our PeerManager takes a GossipVerifier, which creates a circular dependency in the types.
@tnull @TheBlueMatt

@tnull
Copy link
Contributor

tnull commented Jan 4, 2024

I run into a small problem here: I'm up to date till LDK Dec 5 cbde4a7 PR #2773 simplified GossipVerifier, which takes now a PeerManager type parameter, but our PeerManager takes a GossipVerifier, which creates a circular dependency in the types. @tnull @TheBlueMatt

Ugh, that might indeed be an issue we should fix upstream. Not sure if we need to find a (likely ugly) workaround here and roll it back, or wait for the upstream change though. @TheBlueMatt Any opinion on this?

@optout21
Copy link
Contributor Author

optout21 commented Jan 4, 2024

I did a workaround, by creating a version of GossipVerifier without a PeerManager, called SimpleGossipVerifier, in a separate file. This workaround probably BREAKS functionality, and is ugly.
Rest of the adaptations are done, but the GossipVerifier issue has to be finalized.

@optout21
Copy link
Contributor Author

optout21 commented Jan 4, 2024

Also mentioning @johncantrell97 , the author of #2773 .

@TheBlueMatt
Copy link
Contributor

Ugh, thanks for doing this! Sorry it took a while to get back to this. We'll want to (partially) revert [#2773.](lightningdevkit/rust-lightning#2773.

@optout21
Copy link
Contributor Author

Update: PR #2773 was partially reverted by PR #2822 .
It looks like LDK v0.0.119 will have to be skipped, and wait for v0.0.120.
I plan to check into the changes as required by current LDK main, post-2822.

@optout21 optout21 changed the title [WIP] Update to LDK 0.0.119 [WIP] Update to LDK 0.0.120 Jan 18, 2024
@optout21
Copy link
Contributor Author

LDK v0.0.120 has been released, includes #2822.
These changes should bring ldk-sample up to date with LDK v0.0.120 (and bitcoin 0.30.2)

@optout21 optout21 changed the title [WIP] Update to LDK 0.0.120 Update to LDK 0.0.120 Jan 18, 2024
@optout21 optout21 force-pushed the ldk-119 branch 2 times, most recently from 8ac6a80 to d33c07e Compare January 18, 2024 21:19
@optout21 optout21 marked this pull request as ready for review January 18, 2024 21:25
@optout21
Copy link
Contributor Author

There's one CI test failure, one dependency lib was not found, object 0.32.2, seems unrelated to changes here, in stable build there was no failure.

@optout21 optout21 changed the title Update to LDK 0.0.120 Update to LDK 0.0.121 Jan 24, 2024
@optout21
Copy link
Contributor Author

Bumped from 120 to 121.

@optout21 optout21 marked this pull request as draft January 24, 2024 21:34
@optout21
Copy link
Contributor Author

Also updated minimal tested Rust version to 1.63, as LDK requires that now (since 0.0.119), and previously tested 1.48 caused some dependency issues (tokio 1.35.1, backtrace 0.3.69, object 0.32.2 does not work for 1.48).

@optout21 optout21 marked this pull request as ready for review January 24, 2024 21:51
src/bitcoind_client.rs Outdated Show resolved Hide resolved
src/cli.rs Outdated Show resolved Hide resolved
src/cli.rs Outdated Show resolved Hide resolved
src/convert.rs Outdated Show resolved Hide resolved
@optout21 optout21 force-pushed the ldk-119 branch 2 times, most recently from b70d39c to 37d9df6 Compare January 25, 2024 11:45
src/main.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, mod one unwrap I missed before (sorry for that).

src/cli.rs Outdated Show resolved Hide resolved
src/cli.rs Outdated Show resolved Hide resolved
@tnull
Copy link
Contributor

tnull commented Feb 1, 2024

@optout21 I think this should be very close to landing, mod the nits above.

@optout21
Copy link
Contributor Author

optout21 commented Feb 1, 2024

@optout21 I think this should be very close to landing, mod the nits above.

Done now. Thanks for reminding, it slid lower in my to-do.

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@tnull tnull merged commit 3bb8258 into lightningdevkit:main Feb 1, 2024
4 checks passed
@domZippilli
Copy link
Contributor

domZippilli commented Feb 6, 2024

Hey, dunno if anyone will see this, but I was wondering if the team considered using payment_parameters_from_invoice for cli::send_payment implementation. That was introduced by @TheBlueMatt in #2727, and we're considering using it for our upgrade, but noticed it wasn't used here. The implementation of payment_parameters_from_invoice does seem to have some key differences from what ended up here, like unwraps and not as much specificity to errors, so I'm wondering if it was a conscious choice not to use it, or it was just overlooked.

@jkczyz
Copy link
Collaborator

jkczyz commented Feb 6, 2024

Hey, dunno if anyone will see this, but I was wondering if the team considered using payment_parameters_from_invoice for cli::send_payment implementation. That was introduced by @TheBlueMatt in #2727, and we're considering using it for our upgrade, but noticed it wasn't used here. The implementation of payment_parameters_from_invoice does seem to have some key differences from what ended up here, like unwraps and not as much specificity to errors, so I'm wondering if it was a conscious choice not to use it, or it was just overlooked.

payment_parameters_from_invoice would be preferable. The error checking here is probably overkill. The unwraps in payment_parameters_from_invoice are fine because PaymentParameters has a Payee, and some methods are only applicable to certain types of Payees.

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.

Update bitcoin library
6 participants