Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setup ldk node for integration tests #70

Merged
merged 4 commits into from
Sep 20, 2023
Merged

Conversation

orbitalturtle
Copy link
Collaborator

@orbitalturtle orbitalturtle commented Sep 7, 2023

This PR sets up the ldk node needed to run our integration tests:

  • Runs bitcoind so we can run an ldk node on top of it.
  • Runs two ldk nodes on top and makes sure they can successfully connect and pass an onion message between them.
  • Makes sure we save the ldk logs to a spot in /tmp so we can take a look at them after the tests run for debugging.

Copy link
Collaborator

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is as good as we're going to get if we want nodes run "as code" in the itests / all the things that we need to test onion functionality.

This is a lot of code to include in the repo just for tests, and I'm worried about the maintainability of cherry-picking it in. How about:

  1. We fork ldk-sample to the lndk org
  2. Create a new branch that has the functionality we need
  3. Add it as a dependency for our tests

I know we've gone in circles a bit about how best to handle this, but I think we'll be able to save quite a few LOC by just rebasing a branch?

.cargo/audit.toml Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@orbitalturtle
Copy link
Collaborator Author

@carlaKC Yeah I'm a bit frustrated because this feels like sort of where we started. But I think overall we're getting to a better place. Actually this triggered an idea -- which might be what you're describing above but I'm not sure.

Initially when starting this project I had created a fork of ldk-node (here: https://github.com/orbitalturtle/ldk-node/pull/3/files) because ldk-node is a library that we can import while ldk-sample is not.

But now having worked with both ldk-sample and ldk-node... ldk-node is far more complicated than we need for our tests and I think it would be far more confusing to maintain a fork of. And another annoying point of ldk-node, at least for now, is that we have to set up a separate electrs process to interact with it, rather than just use the simple bitcoind process we use with lnd.

I think the best middle ground would be to fork ldk-sample with changes in commits 5-7 of this PR to make it importable/usable for us. It's a fair amount of code but I think using ldk-sample will save us some headaches in the future.

If that sounds good, I'll port over this code to aldk-sample fork for us to work with.

@carlaKC
Copy link
Collaborator

carlaKC commented Sep 12, 2023

I think the best middle ground would be to fork ldk-sample with changes in commits 5-7 of this PR to make it importable/usable for us.

Yeah, that's what I was thinking.

It's a fair amount of code but I think using ldk-sample will save us some headaches in the future.

+1, seems like the right one to use.

I think what we have here is good - and can pretty much just be copied as-is onto a new branch in a fork. So not a change of approach, just moving around where we keep things - rationale being that it'll be easier to maintain than copying things over when they change. I know it's frustrating switching paths all the time, but it is a once-off cost to getting something that's long term sustainable.

@orbitalturtle
Copy link
Collaborator Author

@carlaKC Awesome, glad we're on the same page! And yeah good point -- best to spend some time on this for long-term sustainability

@carlaKC
Copy link
Collaborator

carlaKC commented Sep 12, 2023

@carlaKC Awesome, glad we're on the same page! And yeah good point -- best to spend some time on this for long-term sustainability

Sorry if it came across as "let's start from scratch for the 1000th time" ☠️ I think we're getting there!

The bitcoind package introduces a security vulnerability in the time
dependency. But since bitcoind is only used in the integration tests,
we can safely ignore this advisory.
@orbitalturtle
Copy link
Collaborator Author

@carlaKC Yeah I think I just misunderstood at first! Addressed your two comments above and updated this PR with the ldk-sample as a separate fork.

Not sure if you want to take another look at the ldk-sample portion as well: lndk-org/ldk-sample#1 I already merged because I was having trouble importing it here, but didn't need to in hindsight

Copy link
Collaborator

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test test_ldk_send_onion_message ... ok lfgggg 🚀

@carlaKC
Copy link
Collaborator

carlaKC commented Sep 14, 2023

Not sure if you want to take another look at the ldk-sample portion as well: lndk-org/ldk-sample#1 I already merged

Took a look at this post-merge and it looks good to me 🙌

@orbitalturtle
Copy link
Collaborator Author

orbitalturtle commented Sep 14, 2023

@carlaKC Yay! @dunxen Would you like to take a look before merge? (No big rush)

@carlaKC
Copy link
Collaborator

carlaKC commented Sep 20, 2023

@orbitalturtle I'm happy to go ahead and merge this if you are!

@orbitalturtle orbitalturtle merged commit edc906a into master Sep 20, 2023
3 checks passed
@orbitalturtle orbitalturtle deleted the ldk-test-setup branch October 3, 2023 04:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants