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

Upgrade to LDK 0.0.117 #151

Merged
merged 2 commits into from
Oct 24, 2023

Conversation

tnull
Copy link
Collaborator

@tnull tnull commented Aug 8, 2023

Based on #105.

This is a draft PR which will be regularly rebased on upstream HEAD to incorporate and test the made changes.

@tnull tnull marked this pull request as draft August 8, 2023 11:34
@tnull tnull force-pushed the 2023-08-upgrade-to-LDK-0.0.117 branch 3 times, most recently from d61ad2e to d72896e Compare August 22, 2023 08:04
@tnull tnull force-pushed the 2023-08-upgrade-to-LDK-0.0.117 branch 4 times, most recently from f1db58d to 1fc87a1 Compare September 11, 2023 18:34
@tnull tnull force-pushed the 2023-08-upgrade-to-LDK-0.0.117 branch 6 times, most recently from 40c5bdb to c8f06de Compare September 18, 2023 12:48
Copy link
Contributor

@G8XSU G8XSU left a comment

Choose a reason for hiding this comment

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

Reviewed code apart from sql migrations.
LGTM overall.

src/payment_store.rs Outdated Show resolved Hide resolved
src/test/functional_tests.rs Show resolved Hide resolved
src/test/utils.rs Outdated Show resolved Hide resolved
@tnull tnull force-pushed the 2023-08-upgrade-to-LDK-0.0.117 branch 9 times, most recently from 6fb34d6 to 4854931 Compare September 21, 2023 09:36
use std::path::PathBuf;
use std::sync::RwLock;

pub(crate) fn do_read_write_remove_list_persist<K: KVStore + RefUnwindSafe>(kv_store: &K) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there another way to test this ?
I am unable to use this, if vssStore owns a runtime, it is not RefUnwindSafe somehow.

also why did we remote propTest?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is there another way to test this ? I am unable to use this, if vssStore owns a runtime, it is not RefUnwindSafe somehow.

Hm, what is the exact error message? A quick fix may be to just impl RefUnwindSafe for VssStore {} in test.

also why did we remote propTest?

It didn't add much in this case hence I removed it when upstreaming where we probably won't add the dependency just for this. And I'd now like to keep this codebase and the one of the SqliteStore upstream as close as possible to avoid introducing any bugs when transitioning back-and-forth.

@tnull tnull force-pushed the 2023-08-upgrade-to-LDK-0.0.117 branch from 4854931 to e5650db Compare September 22, 2023 09:00
@tnull tnull force-pushed the 2023-08-upgrade-to-LDK-0.0.117 branch from 0901c19 to e42e9c1 Compare September 27, 2023 11:57
@tnull
Copy link
Collaborator Author

tnull commented Sep 27, 2023

Updated to 0.0.117-alpha2.

@tnull tnull mentioned this pull request Aug 15, 2023
9 tasks
@tnull tnull force-pushed the 2023-08-upgrade-to-LDK-0.0.117 branch from e42e9c1 to 2522c8c Compare September 29, 2023 09:30
@tnull
Copy link
Collaborator Author

tnull commented Sep 29, 2023

Updated to current upstream main, now accounting for the renamed namespaces.

@tnull tnull force-pushed the 2023-08-upgrade-to-LDK-0.0.117 branch 3 times, most recently from e2d5dd8 to 52f52e6 Compare October 5, 2023 01:33
@tnull
Copy link
Collaborator Author

tnull commented Oct 5, 2023

Switched to released 0.0.117 crate and squashed commits.

@tnull tnull marked this pull request as ready for review October 5, 2023 01:34
@tnull tnull changed the title WIP Upgrade to LDK 0.0.117 Upgrade to LDK 0.0.117 Oct 5, 2023
This was referenced Oct 5, 2023
@tnull tnull force-pushed the 2023-08-upgrade-to-LDK-0.0.117 branch from ca4e0da to 46c4249 Compare October 12, 2023 09:19
@tnull
Copy link
Collaborator Author

tnull commented Oct 12, 2023

Now re-added the balance_msat field as it had been re-added last minute upstream:

diff --git a/src/types.rs b/src/types.rs
index d7aae5d..8bcd9e0 100644
--- a/src/types.rs
+++ b/src/types.rs
@@ -162,4 +162,10 @@ pub struct ChannelDetails {
        /// which is applied to commitment and HTLC transactions.
        pub feerate_sat_per_1000_weight: u32,
+       /// The total balance of the channel. This is the amount that will be returned to
+       /// the user if the channel is closed.
+       ///
+       /// The value is not exact, due to potential in-flight and fee-rate changes. Therefore, exactly
+       /// this amount is likely irrecoverable on close.
+       pub balance_msat: u64,
        /// The available outbound capacity for sending HTLCs to the remote peer.
        ///
@@ -211,4 +217,5 @@ impl From<LdkChannelDetails> for ChannelDetails {
                        user_channel_id: UserChannelId(value.user_channel_id),
                        feerate_sat_per_1000_weight: value.feerate_sat_per_1000_weight.unwrap(),
+                       balance_msat: value.balance_msat,
                        outbound_capacity_msat: value.outbound_capacity_msat,
                        inbound_capacity_msat: value.inbound_capacity_msat,

@tnull tnull force-pushed the 2023-08-upgrade-to-LDK-0.0.117 branch from 46c4249 to 102dc61 Compare October 12, 2023 10:30
Unfortunately LDK had a regression that broke keysend/spontaneous
payments. While this bug and corresponding tests are fixed upstream with
0.0.117, we also introduce test coverage for spontaneous payments here.
@tnull tnull force-pushed the 2023-08-upgrade-to-LDK-0.0.117 branch from 102dc61 to acf76e3 Compare October 12, 2023 10:49
@tnull
Copy link
Collaborator Author

tnull commented Oct 12, 2023

Dropped the two pin commits since they have landed in #174 and re-added balance_msat in the UDL file, which I previously forgot:

diff --git a/bindings/ldk_node.udl b/bindings/ldk_node.udl
index c5b832d..d924e08 100644
--- a/bindings/ldk_node.udl
+++ b/bindings/ldk_node.udl
@@ -192,4 +192,5 @@ dictionary ChannelDetails {
        UserChannelId user_channel_id;
        u32 feerate_sat_per_1000_weight;
+       u64 balance_msat;
        u64 outbound_capacity_msat;
        u64 inbound_capacity_msat;

@tnull tnull merged commit 160347c into lightningdevkit:main Oct 24, 2023
9 checks passed
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.

2 participants