Initial implementation of LDK Node #11

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.

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 2 times, most recently from abce308 to 83e3325 Compare March 6, 2023 09:41

jkczyz reviewed Mar 6, 2023

View reviewed changes

tnull force-pushed the 2022-09-initial-setup-and-api branch 7 times, most recently from b52e1fb to 2e0ec29 Compare March 10, 2023 11:29

tnull requested a review from jkczyz March 10, 2023 11:29

tnull force-pushed the 2022-09-initial-setup-and-api branch 2 times, most recently from 6e14b5c to 8314dbb Compare March 12, 2023 00:20

tnull mentioned this pull request Mar 13, 2023

LDK Node v0.1 Tracking Issue #45

Closed

47 tasks

tnull added this to the 0.1 milestone Mar 13, 2023

jkczyz reviewed Mar 13, 2023

View reviewed changes

tnull force-pushed the 2022-09-initial-setup-and-api branch from 8314dbb to 9fa0d70 Compare March 14, 2023 12:43

tnull requested a review from jkczyz March 14, 2023 13:38

jkczyz reviewed Mar 15, 2023

View reviewed changes

tnull requested a review from jkczyz March 15, 2023 20:49

tnull force-pushed the 2022-09-initial-setup-and-api branch from 72f9eb3 to 39d33f1 Compare March 15, 2023 20:54

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

tnull force-pushed the 2022-09-initial-setup-and-api branch from 25f4106 to f896eef Compare March 16, 2023 17:41

jkczyz approved these changes Mar 16, 2023

View reviewed changes

Initial implementation of LDK Node

3dee645

tnull force-pushed the 2022-09-initial-setup-and-api branch from f896eef to 3dee645 Compare March 16, 2023 17:47

tnull merged commit 8bc5c40 into lightningdevkit:main Mar 16, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of LDK Node #11

Initial implementation of LDK Node #11

tnull commented Sep 1, 2022 •

edited

Loading

tnull commented Sep 19, 2022

jkczyz left a comment

jkczyz Mar 13, 2023

tnull Mar 14, 2023

jkczyz Mar 14, 2023

tnull Mar 15, 2023

jkczyz Mar 15, 2023

tnull Mar 15, 2023 •

edited

Loading

jkczyz Mar 13, 2023

tnull Mar 14, 2023 •

edited

Loading

jkczyz left a comment

jkczyz Mar 15, 2023

tnull Mar 15, 2023

jkczyz Mar 15, 2023

tnull Mar 15, 2023

jkczyz Mar 15, 2023

tnull Mar 15, 2023

jkczyz Mar 15, 2023

tnull Mar 15, 2023

Initial implementation of LDK Node #11

Initial implementation of LDK Node #11

Conversation

tnull commented Sep 1, 2022 • edited Loading

tnull commented Sep 19, 2022

jkczyz 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

tnull Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

jkczyz 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

tnull commented Sep 1, 2022 •

edited

Loading

tnull Mar 15, 2023 •

edited

Loading

tnull Mar 14, 2023 •

edited

Loading