Custom Keysmanager and openchannel_without_peer_addr added #138
Conversation
MyKeysManager is a custom keysmanager that can be used to set custom channel secrets. Setting custom channel is essential for interacting with Lnprototest. A method called openchannel_without_peer_addr is also added. This allows creating a channel with a peer that is already connected. This is also needed for Lnprototest tests regarding open channel.
| @@ -74,6 +76,53 @@ pub(crate) fn poll_for_user_input( | |||
| if let Some(word) = words.next() { | |||
| match word { | |||
| "help" => help(), | |||
| "openchannel_without_peer_addr" => { | |||
There was a problem hiding this comment.
I am not sure that we need a new cli command like openchannel_without_peer_addr, but probably it is required just to allow a less restrictive parser in the ldk-sample by assuming that if the address is not specified the peer is already connected (or exist inside the ldk gossip map already)
What do you think about it?
There was a problem hiding this comment.
Yeah, that should be better.
There was a problem hiding this comment.
Yea, lets definitely just update the existing openchannel.
There was a problem hiding this comment.
Regarding this, openchannel command should be followed by either pubkey@host:port or pubkey.
- If it is just
pubkey,- if that peer is already connected, open the fund channel
- otherwise return error
- if it is
pubkey@host:port, proceed with the old behavior
Does this sound good?
| @@ -74,6 +76,53 @@ pub(crate) fn poll_for_user_input( | |||
| if let Some(word) = words.next() { | |||
| match word { | |||
| "help" => help(), | |||
| "openchannel_without_peer_addr" => { | |||
There was a problem hiding this comment.
Yea, lets definitely just update the existing openchannel.
I agree on having a seperate branch for Lnprototest. I would also like to add the an Lnprototest folder inside LDK-Sample, so that we can have the entire testing suite along with the runner script I made inside this new branch. For reference rustyrussell/lnprototest#131 |
|
Sure, makes sense. We could even do it as a separate repo. Might be easier that way vs always referencing a branch? |
|
A seperate repo would be even better! |
|
If you're considering a separate repo, does that mean you don't intend to update this PR? |
If a seperate repo is created, I will make a new PR to that repo. I will still make changes to the openchannel as discussed above in the main ldk-sample branch |
|
in that case, pending the separate repo, do you think we should still add the cfg-gating here as @tnull suggested? |
Sorry if I jump it there. IMHO this should always be under a The only thing that lnprototest needs are compatible API to interact with the node - e.g: while connecting with a peer without specifying the address, lnprototest will expect that the implementation will consider the peers that are already connected. What the separate repo should contains IMHO is just a runner for ldk that is what you need to write tests, and at the same time include these runners inside the lnprototest repository to be able to run the common tests that all the implementation have to pass (we are still thinking about the 'common tests') |
|
Alright, so it sounds like (a) this PR should be updated to only include the changes to openchannel (which are of general interest), (b) we should make a new repo in the ldk org which is just a copy of this one which you'll open a new PR against to include the rest of the changes (as well as a README update, presumably). If that sounds good I'll go ahead and create the repo. |
Sounds good to me! I will add Regarding openchannel, should I make changes to this PR or make another PR to LDK-Sample? |
|
(a) Sounds good to me. (b) The second repository IMHO should include the Python project to implement the runner, similar to https://github.com/vincenzopalazzo/lampo.rs/tree/main/tests/lnprototest. (@Psycho-Pirate has already done something similar that just needs to be refactored, as seen in rustyrussell/lnprototest#131). This separate repository should contain all the prototest tests that LDK wants to have inside their repo. With this setup, it’s possible to deploy the lnprototest-ldk-runner to PyPI, which we can also use in the main lnprototest repo for basic testing. I’m still not sure where the channel key change will be implemented, but this is another modification that lnprototest requires. P.S.: It is also possible that @Psycho-Pirate could create the repository initially, and then we can take it over when the code is ready to be integrated into LDK. |
Yep, sounds great to me.
Sure, also sounds good to me. Feel free to just create a new repo and we'll import it once its ready. |
|
As per our discussion, I have made a new openchannel PR I will be closing this PR and making the new repo as well. |
MyKeysManager is a custom keysmanager that can be used to set custom channel secrets. Setting custom channel is essential for interacting with Lnprototest.
A cli command
openchannel_without_peer_addris also added. This allows creating a channel with a peer that is already connected. This is also needed for Lnprototest tests regarding open channel.A test run with Lnprototest can be found here https://github.com/Psycho-Pirate/lnprototest/actions/runs/12806099333/job/35703810589