Initial implementation of LDK Node by tnull · Pull Request #11 · lightningdevkit/ldk-node

tnull · 2022-09-01T07:22:12Z

Based on #8, #9, ~~#10~~, ~~#33~~.

This PR wraps the previously mentioned PRs and provides initial implementations of Node, Builder, as well as basic API methods.

tnull · 2022-09-19T16:54:14Z

Rebased on #9/#10.

tnull · 2022-12-15T11:37:49Z

Rebased after now using a custom KeysInterface.

Cargo.toml

src/wallet.rs

tnull · 2023-01-27T15:07:41Z

Squashed fixup commits and rebased on main after merge of #33.

jkczyz

Only partway through.

src/io_utils.rs

src/lib.rs

src/error.rs

src/peer_store.rs

src/lib.rs

src/tests/functional_tests.rs

jkczyz

Feel fee to squash fixups.

src/peer_store.rs

Cargo.toml

jkczyz · 2023-03-13T21:47:47Z

src/io_utils.rs

+	let ldk_data_dir = format!("{}/ldk", config.storage_dir_path);
+	let network_graph_path = format!("{}/network_graph", ldk_data_dir);
+
+	if let Ok(file) = fs::File::open(network_graph_path) {


What if we don't yet have a NetworkGraph?

Not sure I'm understanding the question? In case we don't have a persisted network graph at the given location, the results won't be Ok and hence we'll return an empty one via NetworkGraph::new below?

Ah, I missed that. Why do we need to use a Result then?

Good point, removed the Result, will likely need to touch this anyways again when integrating RGS though.

Is this logic robust enough? For instance, if we get PermissionDenied, we'll create a new NetworkGraph and then presumably fail to write it later.

Yes, was thinking about this, too, but I'll have to rework all this anyways in the upcoming switch to SQLite and trait-based persistence remodel.

Currently all of this follows the assumption is just lying around somewhere on disk where we have access to it and we can read out-of-bounds it before we feed it into LDK. After the remodel all of these operations need to go through a trait-based interface that probably will extend KVStorePersister (already implementing a KVStoreUnpersister in #13), a KVStoreAccess trait or similar is still missing.

Also, even asserting that we can open the file in write mode at this point doesn't ensure we won't fail persistence to it in the future, e.g., because permissions changed.

src/lib.rs

jkczyz · 2023-03-13T21:55:59Z

src/lib.rs

+		Ok(())
+	}
+
+	/// Disconnects all peers, stops all running background tasks, and shuts down [`Node`].


Perhaps specify if operations are no longer valid once called, to reflect the start docs.

Now included a corresponding note in the docs.

I'm however a bit split on the API design here for the exposed methods that don't require the runtime loaded. Previously all methods returned Error::NotRunning when no runtime was loaded which gave a clear API-level distinction between started and stopped states. Currently I however opted to not do this for any methods that don't require the runtime, e.g., node_id() or new_funding_address() would work, whether the Node is started or not.

On the one hand I think being able to access some of Node's methods in a stopped state could come in handy (e.g., if users want to keep many nodes in a Vec and could therefore query which node is which without the need to start them), on the other drawing a clear line could be beneficial too in order to avoid confusion, and it would allow us to change to some runtime-dependant behavior under the hood without breaking API going forward.

src/lib.rs

jkczyz

Feel free to punt on anything that requires a large change.

src/lib.rs

jkczyz · 2023-03-15T18:03:48Z

src/lib.rs

+			running,
+			config,
+			wallet,
+			tx_sync,
+			event_queue,
+			channel_manager,
+			chain_monitor,
+			peer_manager,
+			keys_manager,
+			network_graph,
+			gossip_sync,
+			persister,
+			logger,
+			scorer,
+			inbound_payments,
+			outbound_payments,
+			peer_store,
+		}


Consider packing these on a few lines if rustfmt supports that. Probably worth doing in a follow-up later if it will result in a lot of churn.

No, rustfmt doesn't allow manually changing this. I can experiment and see if any config knob allows to change this, but I doubt it, as this is one of the behaviors why Matt is NACK on introducing rustfmt it in LDK?

jkczyz · 2023-03-15T18:05:53Z

src/lib.rs

+		let _background_processor = BackgroundProcessor::start(
+			Arc::clone(&self.persister),
+			Arc::clone(&event_handler),
+			Arc::clone(&self.chain_monitor),
+			Arc::clone(&self.channel_manager),
+			BPGossipSync::p2p(Arc::clone(&self.gossip_sync)),
+			Arc::clone(&self.peer_manager),
+			Arc::clone(&self.logger),
+			Some(Arc::clone(&self.scorer)),
+		);


Should we use the async interface now?

We can't unfortunately due to lightningdevkit/rust-lightning#2003. Will hopefully include a fix in 0.0.115.

src/lib.rs

jkczyz · 2023-03-15T18:24:21Z

src/peer_store.rs

+	pub(crate) fn add_peer(&self, peer_info: PeerInfo) -> Result<(), Error> {
+		let mut locked_peers = self.peers.write().unwrap();
+
+		locked_peers.insert(peer_info.pubkey, peer_info);


Is it intended to override an existing peer if present?

Yes, we may want to do that a bit more fine-grained if we'd ever add additional data, but currently we just override the whole struct to allow for address changes.

jkczyz · 2023-03-15T18:25:55Z

src/peer_store.rs

+	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), lightning::io::Error> {
+		self.pubkey.write(writer)?;
+
+		match self.address.ip() {
+			IpAddr::V4(v4addr) => {
+				0u8.write(writer)?;
+				u32::from(v4addr).write(writer)?;
+			}
+			IpAddr::V6(v6addr) => {
+				1u8.write(writer)?;
+				u128::from(v6addr).write(writer)?;
+			}
+		}
+
+		self.address.port().write(writer)?;
+
+		Ok(())
+	}


Should we use TLV encoding in case we add more fields?

Generally yes, currently holding off since I still hope to simply be able to reuse NetAddress here (cf. lightningdevkit/rust-lightning#2056). If said issue doesn't land I'll probably just newtype it and impl the FromStr here.

src/tests/functional_tests.rs

src/lib.rs

src/tests/functional_tests.rs

tnull force-pushed the 2022-09-initial-setup-and-api branch 6 times, most recently from fe1e326 to 55966e7 Compare September 6, 2022 10:27

This was referenced Sep 6, 2022

Initial payment store implementation #13

Merged

Make wallet entropy source configurable #14

Closed

tnull force-pushed the 2022-09-initial-setup-and-api branch 7 times, most recently from fdabd21 to c0425dd Compare September 8, 2022 12:30

tnull mentioned this pull request Sep 9, 2022

Implement chain::Access trait #21

Closed

tnull force-pushed the 2022-09-initial-setup-and-api branch 6 times, most recently from 4c62cd8 to 9f7085d Compare September 14, 2022 16:52

tnull force-pushed the 2022-09-initial-setup-and-api branch from 9f7085d to 37bf53b Compare September 19, 2022 16:52

tnull force-pushed the 2022-09-initial-setup-and-api branch from 37bf53b to 8d8955d Compare September 22, 2022 13:53

tnull mentioned this pull request Sep 28, 2022

Initial event handling implementation #10

Merged

tnull force-pushed the 2022-09-initial-setup-and-api branch 4 times, most recently from f52524e to 54a25b7 Compare September 30, 2022 08:25

tnull force-pushed the 2022-09-initial-setup-and-api branch 6 times, most recently from a68fa60 to 20f8f47 Compare December 15, 2022 11:32

tnull force-pushed the 2022-09-initial-setup-and-api branch 6 times, most recently from 3d770e0 to 36e59e4 Compare December 19, 2022 10:57

tnull changed the title ~~Initial implementation of LdkLite~~ Initial implementation of LDK Node Jan 4, 2023

MaxFangX mentioned this pull request Jan 12, 2023

Add transaction sync crate lightningdevkit/rust-lightning#1870

Merged

MaxFangX reviewed Jan 12, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

MaxFangX reviewed Jan 17, 2023

View reviewed changes

src/wallet.rs Outdated Show resolved Hide resolved

tnull mentioned this pull request Jan 27, 2023

Initial implementation of wallet functionality #33

Merged

jkczyz reviewed Jan 31, 2023

View reviewed changes

jkczyz reviewed Mar 6, 2023

View reviewed changes

tnull mentioned this pull request Mar 13, 2023

LDK Node v0.1 Tracking Issue #45

Closed

47 tasks

jkczyz reviewed Mar 13, 2023

View reviewed changes

jkczyz reviewed Mar 15, 2023

View reviewed changes

tnull mentioned this pull request Mar 16, 2023

Allow to read Bip39 mnemonic #47

Closed

jkczyz reviewed Mar 16, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/tests/functional_tests.rs Show resolved Hide resolved

jkczyz approved these changes Mar 16, 2023

View reviewed changes

Initial implementation of LDK Node

3dee645

tnull mentioned this pull request Mar 24, 2023

Allow for wallet import #12

Closed

tnull mentioned this pull request May 3, 2023

Switch to use NetAddress for peer addresses #85

Closed

Conversation

tnull commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull commented Sep 19, 2022

Uh oh!

tnull commented Dec 15, 2022

Uh oh!

Uh oh!

Uh oh!

tnull commented Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

tnull commented Sep 1, 2022 •

edited

Loading

tnull commented Jan 27, 2023 •

edited

Loading

tnull Mar 15, 2023 •

edited

Loading

tnull Mar 14, 2023 •

edited

Loading