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

Lnd plan of attack #50

Merged
merged 9 commits into from Feb 3, 2020
Merged

Lnd plan of attack #50

merged 9 commits into from Feb 3, 2020

Conversation

@thomaseizinger
Copy link
Member

thomaseizinger commented Jan 24, 2020

No description provided.

Copy link
Member

bonomat left a comment

Great write-up. I'm already super hyped about getting this implemented.

You have it probably in your head already: can we have a few more details on in which order you see this being implemented please?

I could think of something like this (if we follow your proposal of having most LND interactions outside of CND):

  1. Phase 0: e2e test lib support:
    • Create and redeem a hold invoice using JS in our e2e tests. We can start this in parallel of Phase 1:
  2. Phase 1: HALight-HAN - quick prototype:
    • Implement LND client in CND.
    • Reuse messaging protocol
    • Add new actions to cnd's Rest API (lnd-send-payment, lnd-add-hold-invoice, lnd-settle-invoice).
    • Create e2e tests for HALight-HAN
  3. Phase 2: Add new communication protocol ...
  4. Phase 3:
    • Extract things in libraries, etc.
    • Add LND support in sdk (?we probably want this?)
    • Add example in create-comit-app (?we probably want this?)

== Context

We want to allow users to use Lightning for swapping assets.

This comment has been minimized.

Copy link
@bonomat

bonomat Jan 27, 2020

Member

I think we can mention that this spike only covers LND.

HoldInvoice is also supported by clightning but I we really can ignore this for now and focus only on LND.

See BitcoinOptech Newsletter: https://bitcoinops.org/en/newsletters/2019/04/16/#c-lightning-2540

0022-lnd-plan-of-attack.adoc Show resolved Hide resolved

The proposal is roughly outlined here: https://github.com/comit-network/comit-rs/issues/1326.

To start with, I would add the following routes: (bitcoin to LN seems pointless but of course we can still add it)

This comment has been minimized.

Copy link
@bonomat

bonomat Jan 27, 2020

Member

That sounds a bit harsh :D Imho bitcoin to lightning will be a crucial feature in the future. There are already business offering these things as a service such as https://submarineswaps.org/

Suggested change
To start with, I would add the following routes: (bitcoin to LN seems pointless but of course we can still add it)
To start with, I would add the following routes:

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Jan 29, 2020

Author Member

Just because there are business offerings doesn't mean it is not pointless :)

Just trying to reduce the scope here but we can ofc add any combination.

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Jan 29, 2020

Author Member

BTC to LN is also not cross-chain. We have this discussion at some point and I remember we agreed that it is not a priority for us?

This comment has been minimized.

Copy link
@bonomat

bonomat Feb 3, 2020

Member

No worries, just thought it is not pointless :)

I'm thinking about various use cases for some time now and how to get COMIT known in a community we feel comfortable with. That's why I'd consider BTC<->LN and USDT<->LN.
... Ithought I add some more insights in my thinking. Please feel free to leave it as is.


end

a->al: Add hold invoice

This comment has been minimized.

Copy link
@bonomat

bonomat Jan 27, 2020

Member

What's the reasoning of having Alice to add the hold invoice and not Alice's CND?

The downside of this is that some RPC commands such as add addholdinvoice are only available through the RPC interface and if you think back, it was really hard to get this working from the browser.

Wouldn't it be easier if CND would just do this?
I think letting CND doing this would cope with the race-condition issue you mention in HALight-HAN swap as the invoice can already added to LND by CND before the users received the invoice data.

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Jan 29, 2020

Author Member

What's the reasoning of having Alice to add the hold invoice and not Alice's CND?

Simplicity for now. See last section I added.

The downside of this is that some RPC commands such as add addholdinvoice are only available through the RPC interface and if you think back, it was really hard to get this working from the browser.

If we want it from the browser I suggest we wait until it is stabilized and an HTTP interface is provided.
I am not aware of any browser-based COMIT projects in the near future.
Given that this problem will solve itself over time, I opted for the easiest solution for now.

This comment has been minimized.

Copy link
@bonomat

bonomat Feb 3, 2020

Member

That's fine. We should keep this justification in mind next time when we start discussing: "why isn't cnd doing this for me".

assets/0022-halight-eth-ln-swap.puml Show resolved Hide resolved
0022-lnd-plan-of-attack.adoc Outdated Show resolved Hide resolved
0022-lnd-plan-of-attack.adoc Outdated Show resolved Hide resolved

== Recommendation

- should we try and integrate lightning without changing our messaging??

This comment has been minimized.

Copy link
@bonomat

bonomat Jan 27, 2020

Member

Imho yes. This would allow us to have a working feature soon on which we can then iterate.
I know the downside of this is that more code is harder to refactor.

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Jan 29, 2020

Author Member

I thought about this quite a bit but decided against it. See the last section for details.

assets/0022-halight-ln-eth-swap.puml Outdated Show resolved Hide resolved
0022-lnd-plan-of-attack.adoc Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger force-pushed the lnd-plan-of-attack branch from 455c74f to 5c9e9ff Jan 28, 2020
@thomaseizinger thomaseizinger requested a review from D4nte Jan 28, 2020
@thomaseizinger thomaseizinger force-pushed the lnd-plan-of-attack branch from 9e2ae87 to fa9897e Jan 28, 2020
@thomaseizinger thomaseizinger force-pushed the lnd-plan-of-attack branch from fa9897e to 235eaec Jan 28, 2020
Copy link
Member

D4nte left a comment

Overall comment:

  • What changes on the SDK? e.g. Adding a hold invoice is for now only possible via the gRPC API. which means we need to bring gRPC to the SDK?
0022-lnd-plan-of-attack.adoc Outdated Show resolved Hide resolved
0022-lnd-plan-of-attack.adoc Outdated Show resolved Hide resolved
Hence, the integration of Lightning will likely require adding an "Events" traits, conceptually similar to `HtlcEvents`.
`HtlcEvents` is already specific to a single ledger, so we can simply add a trait `halight::Events` that describes all the events that can happen during the execution of the HALight protocol.

Note: `HtlcEvents` will be decomposed into single function traits with https://github.com/comit-network/comit-rs/issues/1779[#1779].

This comment has been minimized.

Copy link
@D4nte

D4nte Jan 28, 2020

Member

I feel that with the introduction of HALight then the name HtlcEvents for HAN does not fit anymore.
Even with this new issue, I think the terminology Htlc is going to be confusing.
Maybe similarly to what you propose for HALight, we should update comit-network/comit-rs#1779 to create new traits which are han::Funded instead of HtlcFunded.

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Jan 29, 2020

Author Member

It really depends on which issue happens first.
I'd expect everyone to follow along here good enough to get this :)

0022-lnd-plan-of-attack.adoc Outdated Show resolved Hide resolved
0022-lnd-plan-of-attack.adoc Show resolved Hide resolved
0022-lnd-plan-of-attack.adoc Show resolved Hide resolved
0022-lnd-plan-of-attack.adoc Outdated Show resolved Hide resolved
0022-lnd-plan-of-attack.adoc Show resolved Hide resolved
Our e2e test suite is in a limbo state at the moment, hence the first step is to get this sorted.

. Make the e2e tests run in parallel: https://github.com/comit-network/comit-rs/issues/1927 (including the dependencies)
. Setup up the test infrastructure to start an LND instance per actor and open a channel between them

This comment has been minimized.

Copy link
@D4nte

D4nte Jan 28, 2020

Member

It would be interesting to add an edge case with Alice -- Charlie -- Bob channel topography.

This comment has been minimized.

Copy link
@bonomat

bonomat Feb 3, 2020

Member

Good point. Maybe don't even call it edge case but default. At the end I think it is more likely that there is someone between Alice and Bob than that there is a direct connection between the two.

0022-lnd-plan-of-attack.adoc Show resolved Hide resolved
@thomaseizinger thomaseizinger marked this pull request as ready for review Jan 30, 2020
Copy link
Collaborator

luckysori left a comment

Excellent work. Nothing to report, it looks good to me.

@D4nte
D4nte approved these changes Jan 31, 2020
@tcharding

This comment has been minimized.

Copy link
Contributor

tcharding commented Jan 31, 2020

Pushed two minor patches, one grammar, one rendering.

@bonomat
bonomat approved these changes Feb 3, 2020
Copy link
Member

bonomat left a comment

Thanks for writing this up. I added a few minor comments but overall good work.

For the next spike/document, can you use 1-sentence-per-line approach again please? I find this easier to read/review.

Note: I ignored grammar/typos completely :)


The proposal is roughly outlined here: https://github.com/comit-network/comit-rs/issues/1326.

To start with, I would add the following routes: (bitcoin to LN seems pointless but of course we can still add it)

This comment has been minimized.

Copy link
@bonomat

bonomat Feb 3, 2020

Member

No worries, just thought it is not pointless :)

I'm thinking about various use cases for some time now and how to get COMIT known in a community we feel comfortable with. That's why I'd consider BTC<->LN and USDT<->LN.
... Ithought I add some more insights in my thinking. Please feel free to leave it as is.

Our e2e test suite is in a limbo state at the moment, hence the first step is to get this sorted.

. Make the e2e tests run in parallel: https://github.com/comit-network/comit-rs/issues/1927 (including the dependencies)
. Setup up the test infrastructure to start an LND instance per actor and open a channel between them

This comment has been minimized.

Copy link
@bonomat

bonomat Feb 3, 2020

Member

Good point. Maybe don't even call it edge case but default. At the end I think it is more likely that there is someone between Alice and Bob than that there is a direct connection between the two.

0022-lnd-plan-of-attack.adoc Show resolved Hide resolved

end

a->al: Add hold invoice

This comment has been minimized.

Copy link
@bonomat

bonomat Feb 3, 2020

Member

That's fine. We should keep this justification in mind next time when we start discussing: "why isn't cnd doing this for me".

assets/0022-halight-eth-ln-swap.puml Show resolved Hide resolved
The answer to this is "try again". We already make an assumption that cnd's
clients can successfully make transactions.
Failing to make a payment because the invoice does not yet exist is similar
to failing to make a Bitcoin transaction because the fee was to low.

This comment has been minimized.

Copy link
@bonomat

bonomat Feb 3, 2020

Member

If the fee was too low the transaction would remain pending for a long time (some nodes never clear the mempool).

Suggested change
to failing to make a Bitcoin transaction because the fee was to low.
to failing to make a Bitcoin transaction because it was invalid.
@tcharding tcharding merged commit 32f44c9 into master Feb 3, 2020
1 check passed
1 check passed
license/cla Contributor License Agreement is signed.
Details
@tcharding tcharding deleted the lnd-plan-of-attack branch Feb 3, 2020
@tcharding

This comment has been minimized.

Copy link
Contributor

tcharding commented Feb 3, 2020

I merged this @thomaseizinger, please yell if this messes with your flow.

@thomaseizinger

This comment has been minimized.

Copy link
Member Author

thomaseizinger commented Feb 4, 2020

@bonomat, I've actually tried to follow the one sentence per line thingy. Apologies if I've missed it somewhere :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.