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.123 #133

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

Conversation

valentinewallace
Copy link
Contributor

No description provided.

Copy link
Contributor

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

LGTM, lets land once we have an RC.

@TheBlueMatt
Copy link
Contributor

Oh, wait, no, we need to replace our sweeper with the upstream one now.

@valentinewallace
Copy link
Contributor Author

Oh, wait, no, we need to replace our sweeper with the upstream one now.

Oops! Will push an update for this.

@valentinewallace valentinewallace changed the title WIP: Update to LDK 0.0.123 Update to LDK 0.0.123-rc1 May 7, 2024
@valentinewallace valentinewallace marked this pull request as ready for review May 7, 2024 20:26
src/main.rs Outdated
@@ -1022,6 +1076,7 @@ async fn start_ldk() {
}
});

// TODO: remove after a few months since the new `OutputSweeper` was added in LDK v0.0.123.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... actually I think we'll need migration code since outputs may remain spendable even if the user shuts down their node for a year. Can you confirm @TheBlueMatt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, do we want to bother with migration for the sample? Does the sample exist as a rolling sample or as a codebase for people to use? I guess we could leave the code here as-is and do the transition separately after we ask folks in the meeting on monday and on discord if anyone cares about migration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sgtm. Thinking on it, seems more aligned with our goals to prioritize readability.

@valentinewallace
Copy link
Contributor Author

valentinewallace commented May 8, 2024

CI failed due to rust-lang/rust#63033, pushed a fixup. Not sure our stance on raising the MSRV of the sample but it seems the bug is fixed in 1.69.

@TheBlueMatt
Copy link
Contributor

I feel fine with the fix, its not wildly intrusive, as long as we leave a comment linking to the rustc issue.

@valentinewallace
Copy link
Contributor Author

Added a comment for the rust bug and tweaked the TODO regarding periodic_sweep.

@valentinewallace valentinewallace changed the title Update to LDK 0.0.123-rc1 Update to LDK 0.0.123 May 9, 2024
@valentinewallace
Copy link
Contributor Author

Updated from using the RC to the release.

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, just a few nits.

src/cli.rs Outdated Show resolved Hide resolved
src/cli.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@@ -1022,6 +1079,7 @@ async fn start_ldk() {
}
});

// TODO: remove this, since the new `OutputSweeper` was added in LDK v0.0.123.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could also modify the sweeper to read the stored SpendableOutputDescriptors, have the OutputSweeper track them and delete the entries. The corresponding migration logic is pretty trivial in LDK Node (see https://github.com/lightningdevkit/ldk-node/blob/640a1fdb7833ad9c74ede0926f990d85ac1b3bca/src/io/utils.rs#L240-L314), most of it is really error handling which we could just unwrap here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, thanks for pointing me to that code. I'm working on this, will open a follow-up based on this PR.

@valentinewallace
Copy link
Contributor Author

Apologies @tnull I totally missed that review. Updated!

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.

None yet

3 participants