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

wtclient+migration: start storing chan max height in channel details bucket #8222

Merged
merged 4 commits into from
Nov 28, 2023
Merged

wtclient+migration: start storing chan max height in channel details bucket #8222

merged 4 commits into from
Nov 28, 2023

Conversation

ellemouton
Copy link
Collaborator

@ellemouton ellemouton commented Nov 23, 2023 •
edited
Loading

The problem

See the problem described in detail here: #8221

  • the client keeps an in-mem map of map[channelID] max-commit-height. This map represents the maximum commitment height of a backup for a channel that the client has received. It uses this map in various places.
  • the issue is the population of this map (which is done at startup) : it will currently range over every session (which could be in the ten thousands) and for every session, it goes through each of its range indices to get the max height per channel backed up with the session.

The solution:

This PR implements the first solution step defined in the issue. I'll repeat it here:

This PR removes the need to iterate through all the sessions & range indices on each start-up.
It does this by:

  1. adding a new max height field to the channel details bucket & updating that as new updates are added.
  2. doing a migration to back-fill this new field.
  3. using the new field to populate the in-mem map on startup

@ellemouton ellemouton self-assigned this Nov 23, 2023
@saubyk saubyk linked an issue Nov 23, 2023 that may be closed by this pull request
wtclient: improve startup performance #8221
Closed
2 tasks
@ellemouton ellemouton mentioned this pull request Nov 23, 2023
Closed
2 tasks
@ellemouton ellemouton force-pushed the wtclientStartupPerf branch 2 times, most recently from 96f20e3 to 2683940 Compare November 24, 2023 09:48
@bhandras bhandras self-requested a review November 24, 2023 09:56
bhandras
bhandras approved these changes Nov 24, 2023
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.

Excellent work 🍫

watchtower/wtdb/queue.go Show resolved Hide resolved
watchtower/wtdb/client_chan_summary.go Show resolved Hide resolved
watchtower/wtdb/client_db_test.go Outdated Show resolved Hide resolved
watchtower/wtdb/migration8/migration.go Outdated Show resolved Hide resolved
yyforyongyu
yyforyongyu reviewed Nov 27, 2023
View reviewed changes
Copy link
Member

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

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

Very nice and clean PR👍 Have minor questions, otherwise LGTM🙏

watchtower/wtdb/migration8/migration.go Show resolved Hide resolved
watchtower/wtdb/client_db.go Outdated Show resolved Hide resolved
@ellemouton
watchtower/wtdb: start populating channel max commitment
01ba266 
In this commit, a new key, cChanMaxCommitmentHeight, is added to the
channel details bucket. This key will hold the highest commitment number
that the tower has been handed for this channel. In this commit, we
start writing to it in the two places where a backup is first persisted
in the tower client db: 1) CommitUpdate and 2) in the Queue's `addItem`
method. The logic for both 1 & 2 is tested in the next commit which adds
a DB helper that allows us to read the new field.

A follow up commit will do a migration to back-fill the new field.
@ellemouton
watchtower: start using the new channel max heights
fee94ae 
This commit also adds tests for the DB changes made in the previous
commit since we can now read the new field with the FetchChanInfos
method.

The commit following this one does the backfill migration.
@ellemouton
wtdb/migration8: migrate channel max heights
4131ced
@ellemouton
docs: update release notes
3642cb6
yyforyongyu
yyforyongyu approved these changes Nov 28, 2023
View reviewed changes
Copy link
Member

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

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

🏖

@yyforyongyu yyforyongyu merged commit 80684ec into lightningnetwork:master Nov 28, 2023
24 of 25 checks passed
@ellemouton ellemouton deleted the wtclientStartupPerf branch November 28, 2023 07:39
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

@yyforyongyu yyforyongyu yyforyongyu approved these changes

Assignees

@ellemouton ellemouton

Labels
performance watchtower
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

wtclient: improve startup performance
3 participants
@ellemouton @bhandras @yyforyongyu