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

multi: clean up forwarding state from closed channels #4364

Merged
merged 14 commits into from
Sep 24, 2021
Merged

multi: clean up forwarding state from closed channels #4364

merged 14 commits into from
Sep 24, 2021

Conversation

yyforyongyu
Copy link
Member

@yyforyongyu yyforyongyu commented Jun 9, 2020 •
edited by Roasbeef
Loading

This PR fixes #3052, which contains mainly two parts,

  • channeldb: clean forwarding packages for closed channels
    The root bucket is “fwd-packages”, and further subdivided based on ShortChanID for each channel. When a channel is closed, its ShortChanID is used to query the nested bucket in "fwd-packages", and the bucket is deleted. As suggested in htlcswitch: clean up forwarding state from closed channels #3052, the Wipe method in ChannelContronller is called inside the CloseChannel method.

  • htlcswitch: clean circuits and keystones for closed channels

Fixes #4712

@yyforyongyu yyforyongyu changed the title Clean forward states multi: clean up forwarding state from closed channels Jun 9, 2020
@yyforyongyu yyforyongyu force-pushed the clean-forward-states branch from fdd7f5d to ace52f9 Compare June 9, 2020 12:37
@Roasbeef Roasbeef added database Related to the database/storage of LND enhancement Improvements to existing features / behaviour htlcswitch labels Jun 23, 2020
@yyforyongyu yyforyongyu force-pushed the clean-forward-states branch from ace52f9 to cf74a41 Compare June 24, 2020 10:53
@yyforyongyu yyforyongyu requested a review from halseth as a code owner June 24, 2020 10:53
@yyforyongyu yyforyongyu force-pushed the clean-forward-states branch 2 times, most recently from 2a4b09a to 7d67b9f Compare June 27, 2020 08:39
@yyforyongyu yyforyongyu force-pushed the clean-forward-states branch from 7d67b9f to 0c5fa37 Compare July 14, 2020 10:43
@halseth halseth removed their request for review July 17, 2020 10:28
@Roasbeef Roasbeef added this to the 0.12.0 milestone Aug 12, 2020
@Roasbeef Roasbeef added the v0.12 label Aug 12, 2020
@yyforyongyu yyforyongyu force-pushed the clean-forward-states branch 3 times, most recently from 2c9881e to a0ce45b Compare August 14, 2020 07:44
@Roasbeef Roasbeef modified the milestones: 0.12.0, 0.13.0 Oct 1, 2020
@Roasbeef Roasbeef added the P2 should be fixed if one has time label Jan 28, 2021
@Roasbeef
Copy link
Member

But fetchOpenChannel also calls the NewChannelPackager. Moreover, fetchOpenChannel calls another method, fetchChanInfo, that also calls the NewChannelPackager. I’m thinking maybe there exists an opportunity for optimization?

IIRC, this is because RefreshShortChanID (which is used to sync the in memory state with any possible on-disk updates) exists, which calls fetchChanInfo.

Roasbeef
Roasbeef requested changes Mar 10, 2021
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

This was buried at the bottom of some notifications, let's see if we can revive it!

One things I'm curious of about is how much state we can clean up for old very active nodes with this PR. To gather a ball park number, we could recruit some testers to run the migration in dry run mode, flagging that it won't commit anything to disk, so it should be safe.

htlcswitch/circuit_map.go Outdated
}

// Delete the keystone using the outgoing key.
if err := keystoneBkt.Delete(k); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Similar comment here re not deleting while iterating over a bucket.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

@Roasbeef Roasbeef modified the milestones: 0.13.0, v0.14.0 Mar 31, 2021
@yyforyongyu yyforyongyu force-pushed the clean-forward-states branch 2 times, most recently from a9540a9 to 0b9f04a Compare September 18, 2021 03:29
@Roasbeef Roasbeef mentioned this pull request Sep 22, 2021
Closed
@bhandras
Copy link
Collaborator

Let me know when ready for another round @yyforyongyu

@yyforyongyu
channeldb: add docs for fwd pkg bucket
0fff613
@yyforyongyu
channeldb: add method to wipe all forwarding packages
3d50edf 
This commit adds a method, Wipe, to remove all forwarding packages on
disk for a given channel.
@yyforyongyu
contractcourt: add docs for state transition paths
91b75f8
@yyforyongyu
channeldb: wipe all forwarding pkgs when close channel
d8c48fa
@yyforyongyu
channeldb/migration: remove old forwarding packages
c74c1d0
@yyforyongyu
htlcswitch: add docs for circuit bucket hierarchy
e513057
@yyforyongyu
channeldb: attach DB to channel in FetchHistoricalChannel
556b038
@yyforyongyu
rpcserver: return total num of fwdPkgs in PendingChannels
16f97e6 
This commit adds a new field, NumForwardingPackages, in the
PendingChannels RPC response to specify how many forwarding packages the
channel has.
@yyforyongyu
itest: test wipe forwarind packages
c525c3d
@yyforyongyu
htlcswitch: fix docs on LookupCircuit
22848df
@yyforyongyu
htlcswitch: remove unused function in switch
1274e31
@yyforyongyu
htlcswitch: clean circuits and keystones for closed channels
3942c7c 
In this commit, a new method `cleanClosedChannels` is added and called
when a circuit map is created. This method will delete the payment
circuits and keystones for closed channels.
@yyforyongyu
trivial: typo fix
995b9a7
@yyforyongyu
docs: add release notes for fwding pkg gc
198dc73
@yyforyongyu
Copy link
Member Author

Let me know when ready for another round @yyforyongyu

It's ready! PTAL.

@bhandras bhandras requested review from bhandras and removed request for cfromknecht September 24, 2021 14:05
bhandras
bhandras approved these changes Sep 24, 2021
View reviewed changes
Copy link
Collaborator

@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.

LGTM 🥇

channeldb/forwarding_package_test.go
db := makeFwdPkgDB(t, "")

shortChanID := lnwire.NewShortChanIDFromInt(1)
packager := channeldb.NewChannelPackager(shortChanID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes indeed, I agree it's not a normal unit test scenario, but in this case we're operating on a shared resource, meaning that accidental bug in the way we create the bucket keys may mean unexpected deletes. I'm ok with the test as is and this is more of an bonus we could add.

Roasbeef
Roasbeef approved these changes Sep 24, 2021
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM ⚒

@Roasbeef Roasbeef merged commit fbb1d15 into lightningnetwork:master Sep 24, 2021
@yyforyongyu yyforyongyu deleted the clean-forward-states branch September 25, 2021 05:57
@carlaKC carlaKC mentioned this pull request Oct 5, 2021
Closed
@C-Otto C-Otto mentioned this pull request Nov 6, 2021
Closed
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

@Roasbeef Roasbeef Roasbeef approved these changes

Assignees
No one assigned
Labels
database Related to the database/storage of LND enhancement Improvements to existing features / behaviour htlcswitch P2 should be fixed if one has time v0.12
Projects
None yet
Milestone
v0.14.0
4 participants
@yyforyongyu @Roasbeef @orijbot @bhandras