Add LND Integration Tests in CI #509

moisesPompilio · 2025-03-26T05:36:48Z

This PR introduces integration tests for LND, mirroring the existing CLN tests. Here's what was done:

Dockerized LND Node: Set up a Docker container to run an LND node in CI, configured to work with LDK-Node.
Rust Library Choice: Used lnd-grpc-rust instead of tonic_lnd, as it’s more up-to-date and compatible with recent LND versions.
Test Implementation:
- Opened a channel with LND.
- Generated an invoice on LND and paid it with LDK.
- Generated an invoice on LDK and paid it with LND.
- Closed the channel from both sides.
CI Workflow: Added a workflow to the CI pipeline to run the LND tests.

close #505

tnull

Cool, thank you for looking into this! Already looks pretty good!

.gitignore

Cargo.toml

tests/integration_tests_lnd.rs

tnull · 2025-03-26T08:54:04Z

.github/workflows/lnd-integration.yml

+        run: docker compose -f docker-compose-lnd.yml up -d
+
+      - name: Set permissions for data_lnd folder
+        run: sudo chmod -R 755 ./data_lnd/


Curious to learn why we need this? Doesn't LND set the correct permissions already?

In PR 4622, LND added automatic file creation. However, it sets the file permissions to 0700, which prevents the test code from reading the files. As a result, it's necessary to explicitly modify the permissions for the test code to access them.

I see, makes sense. Mind adding that context as a comment here?

Yes, I added a comment to provide that context.

.github/workflows/lnd-integration.yml

tests/integration_tests_lnd.rs

elnosh

I ran the test locally and passed. Although I had to change the permissions for data_lnd directory. Maybe those could be set in the docker-compose-lnd.yml instead of only CI?

docker-compose-lnd.yml

Sets up a local environment for LND integration testing in CI Closes 505

moisesPompilio · 2025-03-26T20:22:11Z

I ran the test locally and passed. Although I had to change the permissions for data_lnd directory. Maybe those could be set in the docker-compose-lnd.yml instead of only CI?

I think it's better for the user or CI to set the permissions, as modifying file permissions automatically via Docker can be invasive and might require sudo. Now, the user or CI specifies the directory for LND files, allowing them to decide where the files will be stored and handle permission modifications as needed.

tnull

Already looks pretty good, just some minor comments.

tnull · 2025-03-27T08:59:33Z

tests/integration_tests_lnd.rs

+		let cert = cert_bytes.as_hex().to_string();
+		let macaroon = mac_bytes.as_hex().to_string();
+
+		let client = connect(cert, macaroon, socket).await?;


Given this is test code, can we just expect here and drop the Result from the return type?

Yes, I removed the Result and added the expect with indications of where they failed.

tnull · 2025-03-27T09:01:34Z

tests/integration_tests_lnd.rs

+use ldk_node::{Builder, Event};
+
+use lnd_grpc_rust::lnrpc::{
+	invoice::InvoiceState::Settled, GetInfoRequest, GetInfoResponse, Invoice, ListInvoiceRequest,


Can we prefix all these imports with Lnd to make it easier to parse which type comes from 'which side'? I.e., make all of these Invoice as LndInvoice, etc.

Yes, I updated the imports and prefixed them with Lnd

tnull · 2025-03-27T09:03:33Z

.github/workflows/lnd-integration.yml

+        run: docker compose -f docker-compose-lnd.yml up -d
+
+      - name: Set permissions for data_lnd folder
+        run: sudo chmod -R 755 ./data_lnd/


I see, makes sense. Mind adding that context as a comment here?

tnull · 2025-03-27T09:05:04Z

tests/integration_tests_lnd.rs

+	tokio::time::sleep(Duration::from_secs(1)).await;
+
+	// Send a payment to LDK
+	let mut rng = thread_rng();


I think we don't need this to be random at all, no? Let's just make up a fixed string here?

Ok, I removed the RNG and replaced it with a fixed string.

tnull · 2025-03-27T09:07:15Z

tests/integration_tests_lnd.rs

+	assert_eq!(lnd_listed_invoices.first().unwrap().state, Settled as i32);
+
+	// Sleep to process payment in LND
+	tokio::time::sleep(Duration::from_secs(1)).await;


Such sleeps tend to be or get flaky in CI. Rather than just sleeping a fixed amount here, any chance we can poll repeatedly in smaller intervals until we see the payment processed, or reach a max number of tries?

Yes, I replaced the sleep with a retry mechanism (max 25 attempts) since my tests showed variance up to 20. This avoids CI flakiness. I also improved error checking to verify if the invoice was actually paid.

tnull

Thanks for addressing the feedback, one comment.

tnull · 2025-03-28T09:37:12Z

tests/integration_tests_lnd.rs

+		// This retries the payment multiple times to account for that delay.
+		let max_retries = 25;
+
+		for attempt in 1..=max_retries {


Thanks for adding this retry logic, but it's a bit weird to have it be part of pay_invoice, given that we're waiting on the previous operation to complete essentially. Rather than just always retrying here, can we instead retry checking the incoming payment settled at the callsite, and then move on to pay_invoice when we're positive it should succeed?

Yes, after reviewing the process, I realized the payment is registered as paid but not immediately available for routing. To address this, I added a check to ensure the payment can be routed through the channel. Since it takes around 0.6 to 1.1 seconds for this to happen, I added 7 retries with 200ms intervals, totaling a maximum wait time of 1.4 seconds to ensure the balance is available for routing.

tnull · 2025-03-28T09:38:31Z

.github/workflows/lnd-integration.yml

+        env:
+          LND_DATA_DIR: ${{ env.LND_DATA_DIR }}
+
+        # In PR 4622 (https://github.com/lightningnetwork/lnd/pull/4622),


nit: Thanks for adding comment, but let's move it down into the item this is meant for.

Adds tests to: - Open a payment channel with LND. - Request and pay an invoice, verifying receipt. - Generate an invoice for LND to pay, verifying receipt. Uses lnd_grpc_rust for gRPC communication. Closes lightningdevkit#505

Configures CI to run LND tests using the Docker Compose environment. Closes lightningdevkit#505

tnull

LGTM, thank you!

tnull self-requested a review March 26, 2025 08:12

tnull mentioned this pull request Mar 26, 2025

Setup initial integration tests with LND #505

Closed

tnull reviewed Mar 26, 2025

View reviewed changes

elnosh reviewed Mar 26, 2025

View reviewed changes

tests/integration_tests_lnd.rs Outdated Show resolved Hide resolved

elnosh reviewed Mar 26, 2025

View reviewed changes

docker-compose-lnd.yml Outdated Show resolved Hide resolved

moisesPompilio force-pushed the issue_505 branch from b785427 to 9b21153 Compare March 26, 2025 18:03

feat: add Docker Compose for LND, Bitcoind, and Electrs

9a38ab0

Sets up a local environment for LND integration testing in CI Closes 505

moisesPompilio force-pushed the issue_505 branch from 9b21153 to 2abfdcf Compare March 26, 2025 18:28

tnull reviewed Mar 27, 2025

View reviewed changes

moisesPompilio force-pushed the issue_505 branch 2 times, most recently from dd4bb77 to 6f1017c Compare March 27, 2025 19:26

ldk-reviews-bot requested a review from joostjager March 28, 2025 09:08

tnull removed the request for review from joostjager March 28, 2025 09:09

tnull reviewed Mar 28, 2025

View reviewed changes

moisesPompilio force-pushed the issue_505 branch from 6f1017c to 51fe0a3 Compare March 28, 2025 17:33

moisesPompilio added 2 commits March 28, 2025 14:44

feat: implement LND integration tests with lnd_grpc_rust

8d81a4c

Adds tests to: - Open a payment channel with LND. - Request and pay an invoice, verifying receipt. - Generate an invoice for LND to pay, verifying receipt. Uses lnd_grpc_rust for gRPC communication. Closes lightningdevkit#505

ci: add GitHub Actions workflow for LND integration tests

7d592bb

Configures CI to run LND tests using the Docker Compose environment. Closes lightningdevkit#505

moisesPompilio force-pushed the issue_505 branch from 51fe0a3 to 7d592bb Compare March 28, 2025 17:45

tnull approved these changes Mar 31, 2025

View reviewed changes

tnull merged commit a6cb7a3 into lightningdevkit:main Mar 31, 2025
28 of 62 checks passed

tnull mentioned this pull request Apr 2, 2025

Fix broken LND integration test #515

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LND Integration Tests in CI #509

Add LND Integration Tests in CI #509

moisesPompilio commented Mar 26, 2025

tnull left a comment

tnull Mar 26, 2025

moisesPompilio Mar 26, 2025

tnull Mar 27, 2025

moisesPompilio Mar 27, 2025

elnosh left a comment

moisesPompilio commented Mar 26, 2025

tnull left a comment

tnull Mar 27, 2025

moisesPompilio Mar 27, 2025

tnull Mar 27, 2025

moisesPompilio Mar 27, 2025

tnull Mar 27, 2025

tnull Mar 27, 2025

moisesPompilio Mar 27, 2025

tnull Mar 27, 2025

moisesPompilio Mar 27, 2025

tnull left a comment

tnull Mar 28, 2025

moisesPompilio Mar 28, 2025

tnull Mar 28, 2025

tnull left a comment

Add LND Integration Tests in CI #509

Add LND Integration Tests in CI #509

Conversation

moisesPompilio commented Mar 26, 2025

tnull 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

elnosh left a comment

Choose a reason for hiding this comment

moisesPompilio commented Mar 26, 2025

tnull 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

tnull 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

tnull left a comment

Choose a reason for hiding this comment