Update to `rust-lightning:0.0.117` #176

luckysori · 2023-11-22T06:13:12Z

The highlight in terms of code changes in rust-dlc is:

Use new LDK Channel ID struct

Start to use newly introduced ChannelId struct. With a stronger type we are less likely to mix up LN ChannelIds, DLC channel IDs and contract IDs. To that end, we've renamed the old ChannelId type alias to DlcChannelId since it is now only supposed to be used for (temporary or final) DLC channel IDs. We should consider introducing a struct at some point.

To ensure backwards-compatibility we're still serialising the LN channel IDs into bytes, but we now have to use the serialize_with attribute since LDK's ChannelId doesn't implement serde itself. The same applies to deserialisation.

Tibo-lg · 2023-11-24T01:25:58Z

Use new LDK Channel ID struct

Start to use newly introduced ChannelId struct. With a stronger type we are less likely to mix up LN ChannelIds, DLC channel IDs and contract IDs. To that end, we've renamed the old ChannelId type alias to DlcChannelId since it is now only supposed to be used for (temporary or final) DLC channel IDs. We should consider introducing a struct at some point.

To ensure backwards-compatibility we're still serialising the LN channel IDs into bytes, but we now have to use the serialize_with attribute since LDK's ChannelId doesn't implement serde itself. The same applies to deserialisation.

I feel it'd be cleaner to just define our own structs, and implement From<ldk::ChannelId> for the SubChannelId one. Then we could derive the serde::Serialize, and also have a more consistent naming. I feel it can be confusing that Channel structs use DlcChannelId while SubChannel use ChannelId. Unless there is some advantage that I'm not seeing?

luckysori · 2023-11-28T21:10:41Z

Use new LDK Channel ID struct

Start to use newly introduced ChannelId struct. With a stronger type we are less likely to mix up LN ChannelIds, DLC channel IDs and contract IDs. To that end, we've renamed the old ChannelId type alias to DlcChannelId since it is now only supposed to be used for (temporary or final) DLC channel IDs. We should consider introducing a struct at some point.
To ensure backwards-compatibility we're still serialising the LN channel IDs into bytes, but we now have to use the serialize_with attribute since LDK's ChannelId doesn't implement serde itself. The same applies to deserialisation.

I feel it'd be cleaner to just define our own structs, and implement From<ldk::ChannelId> for the SubChannelId one. Then we could derive the serde::Serialize, and also have a more consistent naming. I feel it can be confusing that Channel structs use DlcChannelId while SubChannel use ChannelId. Unless there is some advantage that I'm not seeing?

I think what you propose is even better, but I wasn't sure if you wanted to go that far since before this PR we were just using type aliases everywhere. I can work on adding a couple of newtypes like you suggest.

luckysori · 2023-12-20T06:37:48Z

@Tibo-lg: I've tried to address your comment.

It's probably not perfect because it was a pretty painful process, so let me know if I can make it better.

I had to move some stuff around so that the new structs could be used in all the necessary places. I also left a couple of TODOs and a NOTE. Let me know what you think about those too.

Still fixing some tests

As part of this update `rust-lightning` introduced a `ChannelId` struct, which motivated us to also introduce the `DlcChannelId` and `SubChannelId` structs.

Tibo-lg

Overall LGTM, still a couple of things that need fixing.

Tibo-lg · 2023-12-21T04:44:56Z

dlc-manager/src/manager.rs

@@ -130,8 +131,8 @@ macro_rules! check_for_timed_out_channels {
                if is_timed_out {
                    let sub_channel = if channel.is_sub_channel() {
                        log::info!(
-                            "Skipping force-closure of subchannel {}: not supported",
-                            bitcoin::hashes::hex::ToHex::to_hex(&channel.channel_id[..])
+                            "Skipping force-closure of subchannel {:?}: not supported",


Does this mean it will be printed as an array rather than a hex string? It'd be nice to have an easy way to print these ids.

I agree, we need to print this as hex. I'll fix it.

Tibo-lg · 2023-12-21T04:45:34Z

dlc-manager/src/manager.rs

+        let channel_id = crate::utils::get_new_temporary_id();
+        let channel_id = DlcChannelId::from_bytes(channel_id);


It'd be nice to have some wrapper within the type for this (can do it later as well).

Yeah, I agree. I will do that.

Tibo-lg · 2023-12-21T04:49:40Z

dlc-manager/src/manager.rs

@@ -1756,7 +1763,7 @@ where
        self.chain_monitor.lock().unwrap().add_tx(
            prev_tx_id,
            ChannelInfo {
-                channel_id: signed_channel.channel_id,
+                channel_id: signed_channel.channel_id.inner(),


Would it make sense to implement Deref or into (really just a suggestion, not sure it'd be much better).

Yep. I think From/Into makes sense. Can do that.

Tibo-lg · 2023-12-21T05:05:37Z

dlc-manager/src/sub_channel_manager.rs

@@ -228,10 +231,11 @@ where
            ln_channel_manager,
            dlc_channel_manager,
            actions: Mutex::new(actions),
-            phantom: PhantomData,
+            phantom_cs: PhantomData,


I think you don't need the CS generic parameter anymore.

Tibo-lg · 2023-12-21T05:06:20Z

dlc-manager/src/sub_channel_manager.rs

@@ -359,8 +363,7 @@ where
        let per_split_seed_pk =
            PublicKey::from_secret_key(self.dlc_channel_manager.get_secp(), &per_split_seed);

-        let temporary_channel_id: ContractId =
-            subchannel::generate_temporary_channel_id(*channel_id, update_idx, 0);
+        let temporary_channel_id = generate_temporary_dlc_channel_id(channel_id, update_idx, 0);


Feels like this would be better as a method on the type.

Tibo-lg · 2023-12-21T05:20:03Z

dlc-manager/tests/ln_dlc_channel_execution_tests.rs

+        _: u64,
+        _: &NodeId,
+        _: &NodeId,
+        _: ChannelUsage,


not sure why you changed these and not the last one :D (I feel not changing them is also fine)

Yeah, that's weird. Definitely not intentional. I'll revert it.

Tibo-lg · 2023-12-21T05:23:14Z

dlc-manager/tests/test_utils.rs

@@ -714,3 +715,59 @@ pub fn refresh_wallet<B: Deref, W: Deref>(
        retry += 1;
    }
 }
+
+/// Serialize an hexadecimal value.
+pub fn serialize_hex<S>(hex: &[u8], s: S) -> Result<S::Ok, S::Error>


I think there are the same methods in dlc::serde_utils, maybe easier to just reference that?

Yep, will do that!

Tibo-lg · 2023-12-21T05:24:22Z

dlc-messages/Cargo.toml

@@ -12,8 +12,8 @@ use-serde = ["serde", "secp256k1-zkp/use-serde"]

 [dependencies]
 bitcoin = {version = "0.29.2"}
-dlc = {version = "0.4.0", path = "../dlc"}
-lightning = {version = "0.0.116"}
+dlc = {version = "0.4.0", path = "../dlc", features = ["serde"] }


I think serde is supposed to be feature gated (not 100% sure it's correctly setup but at least that was the goal)

You are right. We want to only enable serde for the dlc crate if the serde feature flag is passed. I'll fix that.

Tibo-lg · 2023-12-21T05:27:23Z

dlc/Cargo.toml

@@ -9,10 +9,11 @@ version = "0.4.0"

 [dependencies]
 bitcoin = {version = "0.29.2"}
+lightning = {version = "0.0.117"}


I would rather this crate to stay very simple and not import lightning unless necessary. Actually is there a reason that you put ids.rs here and not in dlc-manager? It doesn't seem to be used within this crate.

Actually is there a reason that you put ids.rs here and not in dlc-manager? It doesn't seem to be used within this crate.

I was also hesitant about this. But it seemed to me like IDs belong in a core crate like dlc (seems to be).

The reason why I didn't want to put them in dlc-manager was because IDs are used in dlc-messages, meaning that dlc-messages would have to depend on dlc-manager which seems weird. And, more importantly, that would create a circular dependency.

I would rather this crate to stay very simple and not import lightning unless necessary.

I considered adding a feature flag to the dlc crate, but it was getting a bit tedious so I dropped it. Should I try that again?

Yeah a circular dependency wouldn't be good. What do you think about just not using these types for the dlc-messages crate? I guess it's not that a big deal to have it in dlc though, so I'll leave it up to you.

Tibo-lg · 2023-12-21T05:28:26Z

mocks/src/memory_storage_provider.rs

 use secp256k1_zkp::{PublicKey, SecretKey};
 use simple_wallet::WalletStorage;
 use std::collections::HashMap;
 use std::sync::{Mutex, RwLock};

 pub struct MemoryStorage {
    contracts: RwLock<HashMap<ContractId, Contract>>,
-    channels: RwLock<HashMap<ChannelId, Channel>>,
-    sub_channels: RwLock<HashMap<ChannelId, SubChannel>>,
+    channels: RwLock<HashMap<[u8; 32], Channel>>,


Is there a reason for not using the types here?

Hehe, I had the types here until the very last second, when the compiler started giving me a hard time about the types not implementing certain traits. I can give it a go again.

luckysori added the ln label Nov 22, 2023

luckysori requested review from holzeis and Tibo-lg November 22, 2023 06:13

luckysori self-assigned this Nov 22, 2023

luckysori force-pushed the chore/upgrade-ldk-117 branch 2 times, most recently from 65b6bce to 61e33c8 Compare November 28, 2023 20:57

luckysori force-pushed the chore/upgrade-ldk-117 branch 2 times, most recently from cb6964f to 1fe6ab5 Compare November 29, 2023 10:13

holzeis force-pushed the chore/upgrade-ldk-117 branch 3 times, most recently from 07d1fe3 to b7721f9 Compare November 29, 2023 14:38

luckysori force-pushed the chore/upgrade-ldk-117 branch 2 times, most recently from 1fe6ab5 to 745717d Compare December 1, 2023 05:01

luckysori force-pushed the chore/upgrade-ldk-117 branch from 745717d to de1af07 Compare December 20, 2023 06:35

luckysori force-pushed the chore/upgrade-ldk-117 branch from de1af07 to 7fdc4db Compare December 20, 2023 06:38

Update to rust-lightning:0.0.117

86e2000

As part of this update `rust-lightning` introduced a `ChannelId` struct, which motivated us to also introduce the `DlcChannelId` and `SubChannelId` structs.

luckysori force-pushed the chore/upgrade-ldk-117 branch from 7fdc4db to 86e2000 Compare December 20, 2023 06:47

Tibo-lg reviewed Dec 21, 2023

View reviewed changes

luckysori mentioned this pull request Jan 2, 2024

One ChannelMonitor per ChannelManager #193

Open

luckysori removed their assignment Feb 9, 2024

luckysori removed the request for review from holzeis February 9, 2024 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to `rust-lightning:0.0.117` #176

Update to `rust-lightning:0.0.117` #176

luckysori commented Nov 22, 2023 •

edited

Loading

Tibo-lg commented Nov 24, 2023 •

edited by luckysori

Loading

Use new LDK Channel ID struct

luckysori commented Nov 28, 2023

Use new LDK Channel ID struct

luckysori commented Dec 20, 2023 •

edited

Loading

Tibo-lg left a comment

Tibo-lg Dec 21, 2023

luckysori Dec 21, 2023

Tibo-lg Dec 21, 2023

luckysori Dec 21, 2023

Tibo-lg Dec 21, 2023

luckysori Dec 21, 2023

Tibo-lg Dec 21, 2023

Tibo-lg Dec 21, 2023

Tibo-lg Dec 21, 2023

luckysori Dec 21, 2023

Tibo-lg Dec 21, 2023

luckysori Dec 21, 2023

Tibo-lg Dec 21, 2023

luckysori Dec 21, 2023

Tibo-lg Dec 21, 2023

luckysori Dec 21, 2023 •

edited

Loading

Tibo-lg Dec 21, 2023

Tibo-lg Dec 21, 2023

luckysori Dec 21, 2023

		let channel_id = crate::utils::get_new_temporary_id();
		let channel_id = DlcChannelId::from_bytes(channel_id);

Update to rust-lightning:0.0.117 #176

Are you sure you want to change the base?

Update to rust-lightning:0.0.117 #176

Conversation

luckysori commented Nov 22, 2023 • edited Loading

Use new LDK Channel ID struct

Tibo-lg commented Nov 24, 2023 • edited by luckysori Loading

Use new LDK Channel ID struct

luckysori commented Nov 28, 2023

Use new LDK Channel ID struct

luckysori commented Dec 20, 2023 • edited Loading

Tibo-lg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luckysori Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Update to `rust-lightning:0.0.117` #176

Update to `rust-lightning:0.0.117` #176

luckysori commented Nov 22, 2023 •

edited

Loading

Tibo-lg commented Nov 24, 2023 •

edited by luckysori

Loading

luckysori commented Dec 20, 2023 •

edited

Loading

luckysori Dec 21, 2023 •

edited

Loading