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.

Sign up for GitHub

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

Jump to bottom

[4/?] Instant loop out: Add instant loop outs #651

Merged
merged 10 commits into from
Feb 6, 2024
Merged

[4/?] Instant loop out: Add instant loop outs #651

merged 10 commits into from
Feb 6, 2024

Conversation

sputn1ck
Copy link
Member

@sputn1ck sputn1ck commented Oct 25, 2023 •
edited
Loading

This PR focuses on the new instant loop out flow.

The instantout statemachine looks as follows

stateDiagram-v2
[*] --> Init: OnStart
BuildHtlc
BuildHtlc --> PushPreimage: OnHtlcSigReceived
BuildHtlc --> InstantFailedOutFailed: OnError
BuildHtlc --> InstantFailedOutFailed: OnRecover
FailedHtlcSweep
FinishedCoop
Init
Init --> InstantFailedOutFailed: OnRecover
Init --> SendPaymentAndPollAccepted: OnInit
Init --> InstantFailedOutFailed: OnError
InstantFailedOutFailed
PublishHtlc
PublishHtlc --> WaitForHtlcSweepConfirmed: OnHtlcBroadcasted
PublishHtlc --> FailedHtlcSweep: OnError
PublishHtlc --> PublishHtlc: OnRecover
PushPreimage
PushPreimage --> WaitForSweeplessSweepConfirmed: OnSweeplessSweepBroadcasted
PushPreimage --> InstantFailedOutFailed: OnError
PushPreimage --> PublishHtlc: OnErrorPublishHtlc
PushPreimage --> PushPreimage: OnRecover
SendPaymentAndPollAccepted
SendPaymentAndPollAccepted --> BuildHtlc: OnPaymentAccepted
SendPaymentAndPollAccepted --> InstantFailedOutFailed: OnError
SendPaymentAndPollAccepted --> InstantFailedOutFailed: OnRecover
WaitForHtlcSweepConfirmed
WaitForHtlcSweepConfirmed --> FinishedHtlcPreimageSweep: OnHtlcSwept
WaitForHtlcSweepConfirmed --> WaitForHtlcSweepConfirmed: OnRecover
WaitForHtlcSweepConfirmed --> FailedHtlcSweep: OnError
WaitForSweeplessSweepConfirmed
WaitForSweeplessSweepConfirmed --> FinishedCoop: OnSweeplessSweepConfirmed
WaitForSweeplessSweepConfirmed --> WaitForSweeplessSweepConfirmed: OnRecover
WaitForSweeplessSweepConfirmed --> PublishHtlc: OnError
Loading

TODO:

  • SQL store
  • recovery
  • loopd stuff
  • loopcli commands
  • unit tests

@sputn1ck sputn1ck force-pushed the instantloopout_4 branch 2 times, most recently from 958c349 to 7e8962a Compare November 21, 2023 14:57
@sputn1ck sputn1ck marked this pull request as ready for review November 21, 2023 15:00
@sputn1ck sputn1ck changed the title [3/?] Instant loop out: Add instant loop outs [4/?] Instant loop out: Add instant loop outs Nov 21, 2023
@sputn1ck sputn1ck force-pushed the instantloopout_4 branch 4 times, most recently from c6352eb to 18a50e2 Compare November 28, 2023 17:05
@sputn1ck sputn1ck force-pushed the instantloopout_4 branch 6 times, most recently from 8424734 to 4a36985 Compare December 18, 2023 13:18
@sputn1ck sputn1ck force-pushed the instantloopout_4 branch 2 times, most recently from 90fa073 to a8c2402 Compare December 19, 2023 11:57
hieblmi
hieblmi reviewed Jan 4, 2024
View reviewed changes
Copy link
Collaborator

@hieblmi hieblmi left a comment

Choose a reason for hiding this comment

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

Once again nice work @sputn1ck. I've done a first pass and flagged what I noted. I will do a test run next and check the UX.

@sputn1ck sputn1ck force-pushed the instantloopout_4 branch 4 times, most recently from 8796363 to 6c8a4c3 Compare January 12, 2024 11:24
bhandras
bhandras reviewed Jan 12, 2024
View reviewed changes
Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

Looks ripe! I think the main issue remaining is fixing the case when the htlc is going on chain but for some reason the daemon restarts before the sweep also is on-chain. Other than that mosly nits! 🎉 🎉

instantout/reservation/actions.go Outdated
f.Debugf("Reservation expired")
err := f.SendEvent(OnTimedOut, nil)
if err != nil {
f.Errorf("Error sending event:"+
Copy link
Member

Choose a reason for hiding this comment

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

nit: could extracted into a member function for handling event sending errors.

instantout/manager.go Outdated
runCtx, cancel := context.WithCancel(ctx)
defer cancel()

i.Lock()
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to lock here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we need to

instantout/manager.go Outdated
i.Unlock()
return nil, err
}
i.Unlock()
Copy link
Member

Choose a reason for hiding this comment

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

I think we also want to add the new instantOut to the activeInstantOuts.

instantout/manager.go Outdated
i.Lock()
defer i.Unlock()

reservation, ok := i.activeInstantOuts[swapHash]
Copy link
Member

Choose a reason for hiding this comment

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

Should we clean out the finished ones here?

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea!

@sputn1ck sputn1ck requested a review from hieblmi January 16, 2024 16:30
hieblmi
hieblmi reviewed Jan 21, 2024
View reviewed changes
Copy link
Collaborator

@hieblmi hieblmi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Amazing work @sputn1ck, this looks very close. I found some minor issues while testing the instant out flow which I communicated offline.
There are also some minor issues mentioned in my latest review, including some code and format nits. This is as good as done 🚀

test/lnd_services_mock.go Outdated
for _, listener := range s.blockHeightListeners {
lis := listener
go func() {
lis <- height
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't we need the timeout case here?

instantout/store.go
// byteSliceToReservationIds converts a byte slice to a slice of reservation
// ids.
func byteSliceToReservationIds(byteSlice []byte) ([]reservation.ID, error) {
if len(byteSlice)%32 != 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can do %reservation.IdLength instead.

@hieblmi
Copy link
Collaborator

hieblmi commented Jan 22, 2024

Minor cosmetic issue:
In

Select reservations for instantout (e.g. '1,2,3')
Type 'ALL' to use all available reservations.

Hitting enter causes:

[loop] strconv.Atoi: parsing "": invalid syntax

@lightninglabs-deploy
Copy link

@GeorgeTsagk: review reminder
@sputn1ck, remember to re-request review from reviewers when ready

@sputn1ck sputn1ck requested review from bhandras and hieblmi January 31, 2024 15:27
@sputn1ck sputn1ck force-pushed the instantloopout_4 branch 9 times, most recently from 8002d08 to 3ce3b9a Compare February 6, 2024 10:46
hieblmi
hieblmi approved these changes Feb 6, 2024
View reviewed changes
Copy link
Collaborator

@hieblmi hieblmi left a comment

Choose a reason for hiding this comment

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

tACK 💾

Fantastic effort @sputn1ck, can't wait to give this a go on testnet.

bhandras
bhandras approved these changes Feb 6, 2024
View reviewed changes
Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

tACK, LGTM 🎉 (pending a few nits)
Awesome work on the whole instant loopout feature 🥇 🥇

instantout/actions.go
},
)
if err != nil {
pollPaymentTries++
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'm not sure that 50 seconds will always be enough for the payment to be accepted.

Copy link
Member Author

Choose a reason for hiding this comment

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

polling every 15 seconds now for 20 tries, which results in 5 mins (the same as the sendpayment timeout)

cmd/loop/instantout.go
"github.com/urfave/cli"
)

var instantOutCommand = cli.Command{
Copy link
Member

Choose a reason for hiding this comment

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

Would be cool to have a cli command to also list available reservations. (can be a follow up).

Copy link
Member Author

Choose a reason for hiding this comment

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

we have that loop r l

Copy link
Member Author

Choose a reason for hiding this comment

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

Or do you mean list instantouts? (we should have that as well)

@sputn1ck sputn1ck merged commit c6e8664 into lightninglabs:master Feb 6, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bhandras bhandras bhandras approved these changes

@hieblmi hieblmi hieblmi approved these changes

@GeorgeTsagk GeorgeTsagk Awaiting requested review from GeorgeTsagk

Assignees

@sputn1ck sputn1ck

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sputn1ck @hieblmi @lightninglabs-deploy @bhandras