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: queue backup id only #7623

Merged
merged 12 commits into from
Apr 24, 2023
Merged

wtclient: queue backup id only #7623

merged 12 commits into from
Apr 24, 2023

Conversation

ellemouton
Copy link
Collaborator

@ellemouton ellemouton commented Apr 21, 2023 •
edited
Loading

Currently, the entire breach retribution struct is passed to the watchtower client
and queued in its task pipeline. This is no longer necessary with the changes made
in #7379 since it is now possible for the
tower client to build the breach retribution info on the fly.

So what this PR does is to now only queue the wtdb.BackupID of the update in the
task pipeline and reconstruct the breach retribution only at the time that it will be
required.

This PR sets up all the ground work that will be required for #7380

Flow of the PR

  1. A new FetchChannelByID method is added to ChannelStateDB to make it
    possible to later on fetch a channel by its lnwire.ChannelID
    representation. The reason for this is that currently there is only a
    FetchChannel method which allows you to fetch a channel by its wire.Outpoint
    but the tower client uses lnwire.ChannelID to keep track of channels so we
    need a new lookup method here.
  2. To prepare for the later on when we want to be able to construct the
    BreachRetribution at the time that it is needed, we create and pass through a
    buildBreachRetribution callback to the tower client.
  3. Since the tower client now has the ability to build the breach retribution by
    itself, we let it do that instead of having the channellink do that. This
    is to prepare for later on when we construct the breach info JIT.
  4. Move the construction of the breach retribution to the time of binding it to
    a session.
  5. At this point, we can now let the (still in-memory) pipeline task queue only
    carry the BackupID.

@ellemouton ellemouton mentioned this pull request Apr 21, 2023
Merged
@ellemouton
Copy link
Collaborator Author

Hey @guggero & @bitromortac :)

This is a split off from #7380 so I think mostly reviewed already :)

@ellemouton ellemouton added this to the v0.17.0 milestone Apr 21, 2023
bitromortac
bitromortac approved these changes Apr 24, 2023
View reviewed changes
Copy link
Collaborator

@bitromortac bitromortac left a comment

Choose a reason for hiding this comment

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

tested, LGTM (I reviewed this in #7380) 💪. Do you have an estimate how much compression we get by this per backup @ellemouton?

@ellemouton
Copy link
Collaborator Author

Thanks for the review @bitromortac 🙏

Do you have an estimate how much compression we get by this per backup

The wtdb.BackupID is a fixed 40 bytes while the lnwallet.BreachRetribution is muuuch bigger: a quick calculation estimates that the lower bound of bytes it holds is: 510 + 94(numHTLCs). And this is without taking into account any []byte fields.

guggero
guggero reviewed Apr 24, 2023
View reviewed changes
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Thank you for extracting those commits out. Almost ready, just a few small nits.

ellemouton added 12 commits April 24, 2023 13:10
@ellemouton
wtclient: cleanup the test file
cab0560 
This commit just does some linting of the client_test.go file so that
future commits are easier to parse.
@ellemouton
channeldb: lint FetchChannel method
aebdd23 
A few lint fixes of the FetchChannel method.
@ellemouton
channeldb: optimise FetchChannel method
908cb60 
This commit adds a small optimisation to the FetchChannel method.
Instead of iterating over each channel bucket, an identifiable error is
thrown once the wanted channel is found so that the iteration can stop
early.
@ellemouton
channeldb: factor out generic FetchChannel logic
63442cb 
This commit introduces a new `channelSelector` method and moves all
generic logic from `FetchChannel` to it. This refactor will make it
easier to add new methods that require the same open-channel db
traversal with slightly different channel selection logic.
@ellemouton
channeldb: add a FetchChannelByID method
fe2304e 
Add a FetchChannelByID method that allows a caller to fetch an
OpenChannel using an lnwire.ChannelID.
@ellemouton
multi: pass BuildBreachRetribution callback to tower client
85ec38f 
In this commit, a new BuildBreachRetribution callback is added to the
tower client's Config struct. The main LND server provides the client
with an implementation of the callback.
@ellemouton
wtclient: lint the package
530a8ca 
This commit fixes some lints in the wtclient package. This is done so
that upcoming logic changes are easier to parse.
@ellemouton
multi: build retribution info in TowerClient
458ac32 
Since the TowerClient now has a callback that it can use to retrieve the
retribution for a certain channel and commit height, let it use this
call back instead of requiring the info to be passed to it through
BackupState.
@ellemouton
wtclient: only fetch retribution info when needed.
2371bbf 
Only construct the retribution info at the time that the backup task is
being bound to a session.
@ellemouton
wtclient: let task pipeline only carry wtdb.BackupID
65dc20f 
Since the retrubution info of a backup task is now only constructed at
the time that the task is being bound to a session, the in-memory queue
only needs to carry the BackupID of the task.
@ellemouton
wtclient: add mutex locking in perUpdate
08cde98 
Lock the `backupMu` when accessing `c.chanCommitHeights` in the `New`
function. It is not strictly necessary right now but good to add it so
that there is no accidental oversight if the `perUpdate` method is ever
extracted and reused in future.
@ellemouton
docs: update release notes
9a1ed8c
guggero
guggero approved these changes Apr 24, 2023
View reviewed changes
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Nice 🔥 LGTM 🎉

@guggero guggero merged commit 4355ce6 into lightningnetwork:master Apr 24, 2023
@ellemouton ellemouton deleted the queueBackupIDOnly branch April 24, 2023 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guggero guggero guggero approved these changes

@bitromortac bitromortac bitromortac approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.17.0
Development

Successfully merging this pull request may close these issues.
3 participants
@ellemouton @guggero @bitromortac