Add interop integration test harness for LND, CLN, and Eclair#839
Add interop integration test harness for LND, CLN, and Eclair#839febyeji wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
|
🎉 This PR is now ready for review! |
8c1e94f to
a50c9d0
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
a50c9d0 to
433515c
Compare
tnull
left a comment
There was a problem hiding this comment.
Thanks, this already looks great on the first pass! I think we should get some minor things out of the way and then move forward with this so we can take advantage of the additional test coverage for our upcoming release already.
Some comments def. can be addressed in a follow-up (e.g. adding BOLT12 test coverage would be great).
…setup Address review feedback on PR lightningdevkit#839: - Replace reqwest dependency with bitreq for Eclair REST client - Replace curl shell calls with bitreq async HTTP requests - Remove per-test docker container recreation (reuse single Eclair instance, unlock UTXOs between tests instead) - Fix chmod -R 755 to u+rwX,go+rX in CLN/LND CI workflows - Add --fail flag to curl readiness check in Eclair CI
433515c to
2613a3f
Compare
…setup Address review feedback on PR lightningdevkit#839: - Replace reqwest dependency with bitreq for Eclair REST client - Replace curl shell calls with bitreq async HTTP requests - Remove per-test docker container recreation (reuse single Eclair instance, unlock UTXOs between tests instead) - Fix chmod -R 755 to u+rwX,go+rX in CLN/LND CI workflows - Add --fail flag to curl readiness check in Eclair CI
2613a3f to
9c2fbdc
Compare
…setup Address review feedback on PR lightningdevkit#839: - Replace reqwest dependency with bitreq for Eclair REST client - Replace curl shell calls with bitreq async HTTP requests - Remove per-test docker container recreation (reuse single Eclair instance, unlock UTXOs between tests instead) - Fix chmod -R 755 to u+rwX,go+rX in CLN/LND CI workflows - Add --fail flag to curl readiness check in Eclair CI
9c2fbdc to
7420893
Compare
|
Thanks for the review! Pushed an update addressing the feedback. Will address in follow-ups:
|
Thanks! Mind opening tracking issues for these so we don't forget? |
tnull
left a comment
There was a problem hiding this comment.
CI is failing right now:
thread 'test_eclair_cooperative_close_after_fee_change' (16127) panicked at tests/common/scenarios/channel.rs:275:55:
called `Result::unwrap()` on an `Err` value: ChannelClosingFailed
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
test_eclair_cooperative_close_after_fee_change
|
@tnull Fixed the |
tnull
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback! Here are some more comments.
What stands out to me is that:
- This PR/the code is very verbose. It would be good to simplify and DRY up where possible to improve maintainability going forward.
- We currently ignore a lot of test cases, often for reasons that aren't conclusive to me (or seem outdated). Can we double-check whether we really can't make them work?
tests/integration_tests_cln.rs
Outdated
| } | ||
|
|
||
| /// CLN v24.08+ includes a `payment_secret` in outbound keysend HTLCs. | ||
| /// LDK treats any inbound HTLC with `payment_secret` as a BOLT11 payment and |
There was a problem hiding this comment.
Hmm, I'm not positive this is true? Could you un-ignore this so we can see what the error actually is? Is it related to the checks here?: https://github.com/lightningdevkit/rust-lightning/blob/db42ad6ba053d54f98f360f05afad5fee896ed69/lightning/src/ln/channelmanager.rs#L8479
There was a problem hiding this comment.
This is the keysend test, not force close. CLN's keysend plugin hardcodes min_final_cltv_expiry=22 (plugins/keysend.c:230), but LDK requires at least 42 (HTLC_FAIL_BACK_BUFFER(39) + 3). CLN's keysend RPC has no parameter to override this. Keeping ignored until CLN adds a final_cltv parameter.
There was a problem hiding this comment.
This is the keysend test, not force close.
I'm confused what you mean by that?
CLN's keysend plugin hardcodes
min_final_cltv_expiry=22(plugins/keysend.c:230), but LDK requires at least 42 (HTLC_FAIL_BACK_BUFFER(39) + 3). CLN's keysend RPC has no parameter to override this. Keeping ignored until CLN adds afinal_cltvparameter.
Okay, interesting, but that seems an entirely different issue than originally described in the comment / PR description? Or am I confused? The comment now says "needs CLN upstream fix", but are they even aware of the issue and agreed it should be fixed?
There was a problem hiding this comment.
This is the keysend test, not force close.
Sorry for the confusing wording, I think I misread your original comment as asking about a different test.
You're right that there's no upstream CLN issue filed for this.
The min_final_cltv_expiry=22 hardcoding in plugins/keysend.c is something I found by reading CLN source, but I haven't confirmed whether the CLN team considers it a bug. will un-ignored the test so we can see the actual error in CI.
…setup Address review feedback on PR lightningdevkit#839: - Replace reqwest dependency with bitreq for Eclair REST client - Replace curl shell calls with bitreq async HTTP requests - Remove per-test docker container recreation (reuse single Eclair instance, unlock UTXOs between tests instead) - Fix chmod -R 755 to u+rwX,go+rX in CLN/LND CI workflows - Add --fail flag to curl readiness check in Eclair CI
Update test infrastructure to Bitcoin Core 29.0: - corepc-node feature: 27_2 → 29_0 - electrsd feature: corepc-node_27_2 → corepc-node_29_0 - download script: bitcoind 27.2 → 29.0 with updated SHA256 hashes Prerequisite for lightningdevkit#839 (interop test harness) which needs bitcoind 29.0 for compatibility with updated Docker images.
Update test infrastructure to Bitcoin Core 29.0: - corepc-node: 0.10.0 → 0.10.1, feature 27_2 → 29_0 - electrsd feature: corepc-node_27_2 → corepc-node_29_0 - download script: bitcoind 27.2 → 29.0 with updated SHA256 hashes Prerequisite for lightningdevkit#839 (interop test harness).
…setup Address review feedback on PR lightningdevkit#839: - Replace reqwest dependency with bitreq for Eclair REST client - Replace curl shell calls with bitreq async HTTP requests - Remove per-test docker container recreation (reuse single Eclair instance, unlock UTXOs between tests instead) - Fix chmod -R 755 to u+rwX,go+rX in CLN/LND CI workflows - Add --fail flag to curl readiness check in Eclair CI
f039418 to
96517d1
Compare
|
Pushed an update addressing all review feedback. I've replied with details on each change. While I was writing comments, GitHub seems to have posted some duplicate ones. I've since deleted them, so please ignore any notifications from those. |
1bc1de3 to
54fc858
Compare
Bump test dependencies, reorganize Docker files, and add interop integration test infrastructure with shared scenario runner for CLN, LND, and Eclair.
54fc858 to
10d4cd8
Compare
Summary
Closes #766
Add a interop test harness for testing LDK-Node against external Lightning implementations (LND, CLN, Eclair).
What changed
ExternalNodetrait (tests/common/external_node.rs): common interface for all external Lightning implementations (get_node_id,open_channel,create_invoice,close_channel,force_close,send_to_route, etc.)tests/common/scenarios/): channel lifecycle, payments (bolt11, keysend, bidirectional, concurrent), disconnect/reconnect, expired invoice, fee-change close — plus a combo orchestrator that runs 16 disconnect×payment×close permutationsinterop_tests!(setup_clients)macro that runs the full shared suiteTestEclairNodeimpl, Dockerfile, docker-compose, CI workflowexpect_*_event!macros now timeout after 60s with a message identifying which event was expected, instead of hanging indefinitelyelementsproject/lightningdlightninglabs/lndacinq/eclairKnown issues
test_cln_splice_inis#[ignore]: rust-lightning panics duringtx_signaturesexchange (channel.rsdebug_assert), fix merged in rust-lightning#4472 but not yet in the pinned rev (see Assertion on!is_monitor_update_in_progress()is hit during splicing attempt on VSS #857)test_cln_receive_keysendis#[ignore]: CLN keysend setsfinal_cltv=22which is below LDK's minimum of 42