Check LND has required version and build tags on start LNDK.

[kannapoix]

This PR resolves #30
Check version and build tags of connecting LND at the start of LNDK.

We could make the has_version function smaller, but I chose clarity over shotness.
The has_version and has_tags functions have an optional requirement argument, so we can test these functions with constants dedicated to the unit testing.

I have found that LND does not strictly follow semantic versioning.
LND always release with beta pre-release version. Release candidate versions are tagged with something like beta.rc1. If I understand correctly, 0.18.0-beta < 0.18.0-beta.rc right?

[orbitalturtle]

Thanks for the PR @kannapoix! This is really useful. I'll try to give it a look this week.

[orbitalturtle]

Thanks again for the PR @kannapoix! 🚀

It's looking good to me so far, though I still need to test it out. Just a few nits and the following things:

• Re: the rc candidates. Actually, 0.18.0-beta > 0.18.0-beta.rc, the rcs are just test versions ahead of the official release, so I think we can assume our users won't be running those. Looks like 0.18.0-beta officially just came out today :)
• The integration tests are failing because they're running lnd v17 (which seems to show that your code is working correctly!). If you can, in a separate commit just bump the lnd submodule to v18. I think you can do that by: cd-ing into lnd (in the root lndk dir) and switching to the v18 branch.
• Feel free to ignore the codecov error, that's something I need to look into separately...

[kannapoix]

Thank you for your feedback. I will address the issue.
I have been unwell recently and unable to work on it.
I hope I can push fixes this weekend.

[orbitalturtle]

Hey thanks for making those changes @kannapoix! I did some tests on some LND nodes of different versions and the changes are working well 🎊

I think we're pretty close. The main thing is make itest fails because the skip_version_check field is added to Cfg, which needs to be updated in src/cli.rs and tests/integration_tests.rs. If you could rebase over the most recent master that'd be great too.

[orbitalturtle]

I'm thinking let's move this below the if !args.skip_version_check scope because even if skip_version_check is set, we can check if the right build tags are set

[kannapoix]

Oh, I see. I misunderstood. I'll fix it.

[orbitalturtle]

nit: update* typo

[kannapoix]

I think we're pretty close. The main thing is make itest fails because the skip_version_check field is added to Cfg, which needs to be updated in src/cli.rs and tests/integration_tests.rs.

Sorry, I overlooked it.
I'll add skip version check option to CLI and fix tests.
Also rebase to latest master branch.

[orbitalturtle]

@kannapoix Thank you, appreciate it!

[kannapoix]

I believe the skip version check option for the CLI is no longer necessary, as #104 has changed the CLI implementation so that it now simply connects to an already running LNDK server.

I can't run integration tests on my ARM Mac, because bitcoind crate brings binary for Intel Mac.
So, please let me know if integration tests failed.

[mrfelton]

I believe the skip version check option for the CLI is no longer necessary, as #104 has changed the CLI implementation so that it now simply connects to an already running LNDK server.

I believe it would still be needed to enable support for older patched LND versions, like the one we are running. Without it, it would be impossible to start LNDK.

EDIT: If you're just talking about the cli, yes that probably makes sense.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (302fc05) to head (21dfc92).

Files	Patch %	Lines
src/main.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##           master    #111   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           1       1           
  Lines          96      97    +1     
======================================
- Misses         96      97    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[orbitalturtle]

@kannapoix Looks like the integration tests are passing. 🎊 And the code looks good. It's super close to merge-ready.

Just one final comment below and it would be great if you could squash the commits into two: 1) the main changes (LND version and build tag checks), 2) the skip_version_check option.

[kannapoix]

Commits squashed into 'main changes', 'skip version check option', and 'bump lnd'.
Also updated the description of the skip_version_check option. Thanks for the comment! 🙇