multi: add local and remote amount fields to revocation log #7379

ellemouton · 2023-02-03T09:33:31Z

This PR re-adds the LocalBalance and RemoteBalance fields to the RevocationLog. It also tweaks the existing channeldb/migration30 to include these fields so that users who have not yet run the optional migration dont have to lose these fields.

The reason for re-adding these fields is for a use cases where we want to be able to construct an lnwallet.BreachRetribution without having access to the actual breach transaction. An example of this use case is with watchtowers where we might want to re-play a backup onto a different tower's session queue which means we will need to re-sign the justice transaction. This is required for this PR in order to solve this issue

By re-adding these two fields to the revocation log, we also get the added bonus of the tower client needing to carry around less info in it's task pipeline which will make the amount of info we need to persist in a disk overflow queue (pr incoming) much more minimal.

positiveblue

Great work @ellemouton.

Thank you for providing enough information in the PR to make it self contained for the reviewer 👌

lnwallet/channel.go

channeldb/migration30/revocation_log.go

ellemouton · 2023-02-03T15:32:20Z

Thanks for the review @positiveblue !

yyforyongyu

Did a quick first round and left some questions. My main comment is about the alternative design, that we save the balance info on the tower side instead of updating revocation logs. Since the current workflow requires the breach info to be created for every state and then fed to the tower, we might as well feed the balance info there.

I may be totally wrong, but I think since the tower is an optional service and may not be universally needed, saving the balance info in a universally needed database just for this case doesn't seem to be optimal.

channeldb/migration30/revocation_log.go

channeldb/revocation_log.go

channeldb/migration30/revocation_log.go

yyforyongyu · 2023-02-03T18:09:20Z

channeldb/revocation_log.go

@@ -204,6 +204,26 @@ type RevocationLog struct {
 	// HTLCEntries is the set of HTLCEntry's that are pending at this
 	// particular commitment height.
 	HTLCEntries []*HTLCEntry
+
+	// LocalBalance is the current available balance within the channel


what happens to channels that are currently open? so they will have mixed revocation logs?

yes they will unfortunately. Callers should be aware of this and always know how to handle the ErrRevLogDataMissing error

yyforyongyu · 2023-02-03T18:11:06Z

lnwallet/channel.go

+				return nil, 0, 0, ErrRevLogDataMissing
+			}
+
+			theirAmt = int64(revokedLog.RemoteBalance.ToSatoshis())


add a unit test for this logical branch?

cool - good catch 👍 was previously covered in TestNewBreachRetribution but now added it to TestCreateBreachRetribution

ellemouton · 2023-02-03T18:29:01Z

My main comment is about the alternative design, that we save the balance info on the tower side instead of updating revocation logs

The issue with that is what if someone decides midway through the lifetime of their node/channels that they want to use towers. If we dont save this info in the revocation log then they will be forced to close and re-open their channels after switching on the tower sub-server.

The reason is we would need to then create a whole new data structure in the wtclient db that essentially looks exactly like the revocation log which would result in some duplication

ellemouton

Thanks for the review @yyforyongyu !

Let me know what you think about my comment on Friday 😊

channeldb/migration30/revocation_log.go

channeldb/revocation_log.go

ellemouton · 2023-02-06T11:26:15Z

lnwallet/channel.go

+				return nil, 0, 0, ErrRevLogDataMissing
+			}
+
+			theirAmt = int64(revokedLog.RemoteBalance.ToSatoshis())


cool - good catch 👍 was previously covered in TestNewBreachRetribution but now added it to TestCreateBreachRetribution

yyforyongyu

Further looked into the case. Assuming the worse case, where 20 bytes are used to encode the balance fields. For a channel with a million updates, this would be 20MB. For a very large node operator with 100 channels, this is roughly 2GB, which doesn't seem bad given it's a large operator.

My question is whether it justifies saving the extra data to a node that doesn't use the watchtower service? cc @guggero @Roasbeef

If we dont save this info in the revocation log then they will be forced to close and re-open their channels after switching on the tower sub-server.

I don't think they need to close the channels. If they decide to use a watchtower halfway through, they will only have history after the watchtower is on, which is the case for the open channels atm facing the migration brought by this PR. This is nice IMO as you'd only have an increased db size after you explicitly decide to use the service.

The reason is we would need to then create a whole new data structure in the wtclient db that essentially looks exactly like the revocation log which would result in some duplication

Indeed. I think to save it at the wtclient db side, we'd at least have to save the identifiers(channelID+commitHeight) to link the balance info. Maybe that's the overhead of using it? Another way is to use a flag to specify if we want to save them or not in the revocation log, similar to how the final htlc settle signal is done.

Speaking of the final htlc signal, does it mean we can already derive the current balances since we know which htlcs are settled vs canceled? Or the reversed, suppose we have the balance info, can we tell which htlcs are settled? If true, it'd be better to have this PR merged and make a new PR to replace the finalHtlcsBucket as this one is structurally nicer and space-wise smaller to achieve the same goal.

I'm being extremely cautious about adding universal data here as ideally, we'd have a bare minimum lnd in the future, and extra data are added only when a feature/service is wanted by the user. This way it can fit many user cases, like a mobile node vs an enterprise node, etc.

ellemouton · 2023-02-06T15:13:13Z

I don't think they need to close the channels.

What I mean is that you would need to reopen channels iff you want to make sure all your states are backed up. Im thinking of a future where someone decides they want to use towers and backup all past states. Doesnt really make sense to use towers but not backup some states.

Another way is to use a flag to specify if we want to save them or not in the revocation log, similar to how the final htlc settle signal is done.

Yeah that is another idea. I think perhaps if we do go down this route, then we should make it opt-out perhaps? ie something that means --i-will-never-run-towers-ever.

I'm being extremely cautious about adding universal data here as ideally, we'd have a bare minimum lnd in the future

Yeah totally appreciate the reason for being cautious 🙏 interested to hear what others think too 👍

This way it can fit many user cases, like a mobile node vs an enterprise node, etc.

The funny thing about these two examples is that the one more likely to want to run towers will be the mobile user case 🙃

So I guess, the main question we should aim to answer is: Do we feel strongly about a future feature that allows users to backup old states. I think this is even more attractive once we have a better paid-tower model too since then it is win-win for both sides.

ellemouton · 2023-02-09T14:08:03Z

Hi @yyforyongyu 😊 I've updated this PR so that there is now an opt-out flag called -- no-rev-log-amt-data.

It was a bit tricky threading this config option to the migration function so im interested to hear what you think of the way I have done it here and if you have any suggestions for doing it a better way. Thanks!

channeldb/migtest/migtest.go

sample-lnd.conf

config.go

yyforyongyu

Almost good to go! Thank you for taking care of the tests, just lovely! No major comments, just the new config flag needs to be put inside db.

yyforyongyu · 2023-02-10T00:10:03Z

channeldb/db.go

+
+	// noRevLogAmtData if true, means that commitment transaction amount
+	// data should not be stored in the revocation log.
+	noRevLogAmtData bool


let's put this field in the same place as keepFailedPaymentAttempts and storeFinalHtlcResolutions? So basically one layer up.

ah yes indeed!

channeldb/options.go

config_builder.go

channeldb/revocation_log_test.go

docs/release-notes/release-notes-0.16.0.md

lnwallet/channel.go

channeldb/migration30/migration.go

channeldb/migration30/migration_test.go

channeldb/db.go

ellemouton

Thanks for the quick review @yyforyongyu 🚀 updated!

channeldb/db.go

channeldb/migtest/migtest.go

channeldb/db.go

channeldb/migration30/migration.go

channeldb/migration30/test_utils.go

config_builder.go

sample-lnd.conf

lnwallet/channel.go

docs/release-notes/release-notes-0.16.0.md

yyforyongyu

Smooth commit flow, LGTM🎉

channeldb/migration30/test_utils.go

channeldb/db.go

channeldb/migration30/migration.go

ellemouton · 2023-02-10T11:17:39Z

Thanks again @yyforyongyu and @positiveblue !

I just did the following tests to ensure things work as expected:

- produce updates on v0.14.5-beta (ie, pre rev log prune migration)
- Do rev log prune migration
- switch to this PR with the db.no-rev-log-amt-data set
- produce a few more state updates
- RESULT: both new and old states have no amount data in the rev log

- produce updates on v0.14.5-beta (ie, pre rev log prune migration)
- Do rev log prune migration
- switch to this PR and dont set db.no-rev-log-amt-data
- produce a few more state updates
- RESULT: new updates have amount data. old states dont

- produce updates on v0.14.5-beta (ie, pre rev log prune migration)
- switch to this PR with the db.no-rev-log-amt-data set
- Do rev log prune migration
- produce a few more state updates
- RESULT: both new and old states have no amount data in the rev log

- produce updates on v0.14.5-beta (ie, pre rev log prune migration)
- switch to this PR and dont set db.no-rev-log-amt-data
- Do rev log prune migration
- produce a few more state updates
- RESULT: both new and old states do have the amount data in the rev log

Roasbeef

Great set of commits, one primary question (why isn't this a new migration), and another comment re future proofing the way we pass a "config" in the migrations now (may never come up in practice, or we can work around, not really fundamental tho).

Roasbeef · 2023-02-15T23:24:17Z

channeldb/migration30/migration.go

@@ -23,12 +23,26 @@ import (
 // indexes.
 const recordsPerTx = 20_000

+// MigrateRevLogConfig can be used to configure the MigrateRevocationLog
+// migration function.
+type MigrateRevLogConfig struct {


This seems pretty rigid, given that afaict we'd pass the same config in for all the migration instances? What about this declaring a specific config interface:

type MigrateRevLogConfig interface { NoAmountData(mgrNumber int32) bool }

Then the "top level" migration config can embed/implement a super set of the config needed by all nodes. The top level type can also stay as any, or we could get more involved and try to bind the type as a compile time type parameter.

Not entiiiiirely sure what you mean here. Especially re the mgrNumber parameter.

I have, however, now updated the PR to have a new superset MigrationConfig struct which houses all the various migration configs (only one for now). I think this makes things slightly nicer. But dont think it is what you meant ..?

might also be worth noting that this is currently only ever passed to 1 migration (since it is only passed to optional migrations)

just pushed an update that makes the config way more generic (kudos to @guggero for the idea!)

Roasbeef · 2023-02-15T23:26:44Z

channeldb/migration30/migration.go

@@ -553,6 +553,11 @@ func convertRevocationLog(commit *mig.ChannelCommitment,
 		HTLCEntries:      make([]*HTLCEntry, 0, len(commit.Htlcs)),
 	}

+	if !noAmtData {


Shouldn't this be a new migration? There're already nodes that have run this prior migration to convert the revocation logs.

there is no actual migration required for this new code. What this is doing is preventing the loss of this data for people who have not yet run this migration. So this is only useful for people who have not run the migration yet.
Basically allows them to get the benefit of the main migration while not giving them a disadvantage when it comes to tower backups.

Since the main migration is optional as well, I think there will be enough users who have not yet run the migration but will still want to one day. So this is to help those users out. This is also the main reason for pushing to get this in to 0.16

(see test matrix above for what this means for various users)

This commit is a pure refactor. It restructures the RevocationLog serialize and deserialize methods so that optional TLV fields can be easily added.

Rename the ApplyMigrationWithDb function to ApplyMigrationWithDB to make the linter happy.

Define a MigrationConfig interface that should be used to pass the config of an optional migration function. An implementation of this interface is added for migration30 (the only current optional migration).

Add a NoAmountData field to the MigrateRevLogConfigImpl struct and set it for tests. This field is still a no-op in the migration.

In this commit, a new `--db.no-rev-log-amt-data` flag is added. The config option is passed though to everywhere that it will be used. Note that it is still a no-op in this commit. An upcoming commit will make use of the flag.

This commit re-adds the LocalBalance and RemoteBalance fields to the RevocationLog. The channeldb/migration30 is also adjusted so that anyone who has not yet run the optional migration will not lose these fields if they run the migration after this commit. The reason for re-adding these fields is that they are needed if we want to reconstruct all the info of the lnwallet.BreachRetribution without having access to the breach spend transaction. In most cases we would have access to the spend tx since we would see it on-chain at which time we would want to reconstruct the retribution info. However, for the watchtower subsystem, we sometimes want to construct the retribution info withouth having access to the spend transaction. A user can use the `--no-rev-log-amt-data` flag to opt-out of storing these amount fields.

In this commit, the NewBreachRetribution function is adjusted so that a caller can optionally set the spendTx parameter to nil. In this case, the function will check the revocation log to see if the local and remote amount fields are available there and use them if they are. If the fields are not present, which they might not be given a previous migration that removed the fields, then an error is returned.

Roasbeef

LGTM 🫀

ellemouton mentioned this pull request Feb 3, 2023

watchtower: replace in-mem task queue with a disk over-flow queue #7380

Merged

ellemouton force-pushed the addFieldsToRevocationLog branch from 1346484 to bb55bb0 Compare February 3, 2023 09:42

positiveblue approved these changes Feb 3, 2023

View reviewed changes

lnwallet/channel.go Show resolved Hide resolved

channeldb/migration30/revocation_log.go Show resolved Hide resolved

ellemouton mentioned this pull request Feb 3, 2023

wtclient: replay pending and unacked updates #6895

Merged

ellemouton force-pushed the addFieldsToRevocationLog branch from bb55bb0 to 5e84fac Compare February 3, 2023 15:31

saubyk assigned ellemouton Feb 3, 2023

saubyk requested a review from yyforyongyu February 3, 2023 16:46

saubyk modified the milestones: v0.16.1, v0.16.0 Feb 3, 2023

yyforyongyu requested changes Feb 3, 2023

View reviewed changes

ellemouton force-pushed the addFieldsToRevocationLog branch from 5e84fac to 8d6e8c6 Compare February 6, 2023 11:33

ellemouton commented Feb 6, 2023

View reviewed changes

ellemouton force-pushed the addFieldsToRevocationLog branch 2 times, most recently from 6cc3b3f to f8ab802 Compare February 6, 2023 12:40

yyforyongyu reviewed Feb 6, 2023

View reviewed changes

ellemouton force-pushed the addFieldsToRevocationLog branch 2 times, most recently from 030dcda to 86d1d09 Compare February 9, 2023 14:05

yyforyongyu reviewed Feb 9, 2023

View reviewed changes

channeldb/migtest/migtest.go Show resolved Hide resolved

yyforyongyu reviewed Feb 9, 2023

View reviewed changes

sample-lnd.conf Outdated Show resolved Hide resolved

yyforyongyu reviewed Feb 9, 2023

View reviewed changes

config.go Outdated Show resolved Hide resolved

yyforyongyu reviewed Feb 10, 2023

View reviewed changes

ellemouton force-pushed the addFieldsToRevocationLog branch from 86d1d09 to 52d0469 Compare February 10, 2023 08:22

ellemouton commented Feb 10, 2023

View reviewed changes

ellemouton requested a review from yyforyongyu February 10, 2023 08:26

yyforyongyu approved these changes Feb 10, 2023

View reviewed changes

channeldb/migration30/test_utils.go Show resolved Hide resolved

channeldb/db.go Outdated Show resolved Hide resolved

channeldb/migration30/migration.go Outdated Show resolved Hide resolved

saubyk requested a review from Roasbeef February 14, 2023 19:45

Roasbeef requested changes Feb 15, 2023

View reviewed changes

ellemouton added 2 commits February 16, 2023 07:51

channeldb: prepare RevocationLog for optional fields

ce8e7ec

This commit is a pure refactor. It restructures the RevocationLog serialize and deserialize methods so that optional TLV fields can be easily added.

channeldb+wtdb: fix migration function name lint

33b7b92

Rename the ApplyMigrationWithDb function to ApplyMigrationWithDB to make the linter happy.

ellemouton force-pushed the addFieldsToRevocationLog branch from 52d0469 to b791fe8 Compare February 16, 2023 06:53

ellemouton requested a review from Roasbeef February 16, 2023 17:25

ellemouton force-pushed the addFieldsToRevocationLog branch from b791fe8 to 421c6a9 Compare February 16, 2023 18:32

ellemouton added 6 commits February 16, 2023 20:45

multi: pass in a MigrationConfig to all optional migrations

70e3f8f

Define a MigrationConfig interface that should be used to pass the config of an optional migration function. An implementation of this interface is added for migration30 (the only current optional migration).

channeldb: add NoAmountData field to MigrateRevLogConfigImpl

9c01916

Add a NoAmountData field to the MigrateRevLogConfigImpl struct and set it for tests. This field is still a no-op in the migration.

multi: add new NoRevLogAmtData config option

0730337

In this commit, a new `--db.no-rev-log-amt-data` flag is added. The config option is passed though to everywhere that it will be used. Note that it is still a no-op in this commit. An upcoming commit will make use of the flag.

docs: update release notes

884cb20

ellemouton force-pushed the addFieldsToRevocationLog branch from 421c6a9 to 884cb20 Compare February 16, 2023 18:48

Roasbeef approved these changes Feb 20, 2023

View reviewed changes

Roasbeef added this pull request to the merge queue Feb 20, 2023

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 20, 2023

Roasbeef merged commit 748461c into lightningnetwork:master Feb 21, 2023

ellemouton deleted the addFieldsToRevocationLog branch February 21, 2023 06:29

ellemouton mentioned this pull request Apr 21, 2023

wtclient: queue backup id only #7623

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: add local and remote amount fields to revocation log #7379

multi: add local and remote amount fields to revocation log #7379

ellemouton commented Feb 3, 2023 •

edited

Loading

positiveblue left a comment

ellemouton commented Feb 3, 2023

yyforyongyu left a comment

yyforyongyu Feb 3, 2023

ellemouton Feb 3, 2023

yyforyongyu Feb 3, 2023

ellemouton Feb 6, 2023

ellemouton commented Feb 3, 2023

ellemouton left a comment

ellemouton Feb 6, 2023

yyforyongyu left a comment

ellemouton commented Feb 6, 2023

ellemouton commented Feb 9, 2023

yyforyongyu left a comment

yyforyongyu Feb 10, 2023

ellemouton Feb 10, 2023

ellemouton left a comment

yyforyongyu left a comment

ellemouton commented Feb 10, 2023 •

edited

Loading

Roasbeef left a comment

Roasbeef Feb 15, 2023

ellemouton Feb 16, 2023

ellemouton Feb 16, 2023

ellemouton Feb 16, 2023

Roasbeef Feb 15, 2023

ellemouton Feb 16, 2023

ellemouton Feb 16, 2023

Roasbeef left a comment

multi: add local and remote amount fields to revocation log #7379

multi: add local and remote amount fields to revocation log #7379

Conversation

ellemouton commented Feb 3, 2023 • edited Loading

positiveblue left a comment

Choose a reason for hiding this comment

ellemouton commented Feb 3, 2023

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton commented Feb 3, 2023

ellemouton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

ellemouton commented Feb 6, 2023

ellemouton commented Feb 9, 2023

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton left a comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

ellemouton commented Feb 10, 2023 • edited Loading

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

ellemouton commented Feb 3, 2023 •

edited

Loading

ellemouton commented Feb 10, 2023 •

edited

Loading